From: Panagiotis P. <pap...@cs...> - 2007-02-01 17:10:15
Attachments:
patch.diff
|
Checking with valgrind I got some invalid reads. The message was like the following: ==6988== Invalid read of size 4 ==6988== at 0x4B3C7FD: intersect_rect (radeon_state.c:61) ==6988== by 0x4B3C9DA: radeonRecalcScissorRects (radeon_state.c:108) ==6988== by 0x4B3CAEC: radeonUpdateScissor (radeon_state.c:131) ==6988== by 0x4B3CD04: radeonEnable (radeon_state.c:205) ==6988== by 0x4B4B1C1: r300Enable (r300_state.c:542) ==6988== by 0x4D13827: _mesa_set_enable (enable.c:956) ==6988== by 0x4D138A6: _mesa_Enable (enable.c:971) ==6988== by 0x4769879: glEnable (glapitemp.h:1160) ==6988== by 0x4613A5F: osgUtil::RenderStage::drawImplementation(osg::RenderInfo&, osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== by 0x4607658: osgUtil::RenderBin::draw(osg::RenderInfo&, osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== by 0x46133BC: osgUtil::RenderStage::drawInner(osg::RenderInfo&, osgUtil::RenderLeaf*&, bool&) (in /usr/lib/libosgUtil.so) ==6988== by 0x4612E6C: osgUtil::RenderStage::draw(osg::RenderInfo&, osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== Address 0x4AF585C is 4 bytes inside a block of size 8 free'd ==6988== at 0x402303F: free (vg_replace_malloc.c:233) ==6988== by 0x4BAF503: _mesa_free (imports.c:93) ==6988== by 0x4B2FF84: __driUtilUpdateDrawableInfo (dri_util.c:430) ==6988== by 0x4B2FD46: DoBindContext (dri_util.c:339) ==6988== by 0x4B2FF00: driBindContext (dri_util.c:383) ==6988== by 0x4735921: BindContextWrapper (glxext.c:1620) ==6988== by 0x4735A53: MakeContextCurrent (glxext.c:1674) ==6988== by 0x4735D7C: glXMakeCurrent (glxext.c:1796) ==6988== by 0x47D8BB3: Producer::RenderSurface::makeCurrent(bool) (in /usr/lib/libProducer.so) ==6988== by 0x47DEEC6: Producer::Camera::_frame(bool) (in /usr/lib/libProducer.so) ==6988== by 0x47DF75F: Producer::Camera::frame(bool) (in /usr/lib/libProducer.so) ==6988== by 0x47E2589: Producer::CameraGroup::_singleThreadedFrame() (in /usr/lib/libProducer.so) So I searched a bit, and I created the following patch, which touches src/mesa/drivers/dri/common/dri_util.c, and more specifically __driUtilUpdateDrawableInfo, calling _mesa_free for pdp->pClipRects and pdp->pBackClipRects only inside if. Does this look sane? Valgrind does not warn anymore. -- Papadakos Panagiotis |
From: Panagiotis P. <pap...@cs...> - 2007-02-02 17:26:49
Attachments:
patch.diff
|
Well this patch is not correct since it leads to memory leaks, probably due to succeeding calls to dri_interface->getDrawableInfo. So I created the attached patch, but again valgrind warns for invalid reads. Anyone understands why? Thanks P.S. Sorry but I am new to the code! On Thursday 01 February 2007 19:19, Panagiotis Papadakos wrote: > Checking with valgrind I got some invalid reads. The message was like the > following: > > ==6988== Invalid read of size 4 > ==6988== at 0x4B3C7FD: intersect_rect (radeon_state.c:61) > ==6988== by 0x4B3C9DA: radeonRecalcScissorRects (radeon_state.c:108) > ==6988== by 0x4B3CAEC: radeonUpdateScissor (radeon_state.c:131) > ==6988== by 0x4B3CD04: radeonEnable (radeon_state.c:205) > ==6988== by 0x4B4B1C1: r300Enable (r300_state.c:542) > ==6988== by 0x4D13827: _mesa_set_enable (enable.c:956) > ==6988== by 0x4D138A6: _mesa_Enable (enable.c:971) > ==6988== by 0x4769879: glEnable (glapitemp.h:1160) > ==6988== by 0x4613A5F: > osgUtil::RenderStage::drawImplementation(osg::RenderInfo&, > osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== by > 0x4607658: osgUtil::RenderBin::draw(osg::RenderInfo&, > osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== by > 0x46133BC: osgUtil::RenderStage::drawInner(osg::RenderInfo&, > osgUtil::RenderLeaf*&, bool&) (in /usr/lib/libosgUtil.so) ==6988== by > 0x4612E6C: osgUtil::RenderStage::draw(osg::RenderInfo&, > osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== Address > 0x4AF585C is 4 bytes inside a block of size 8 free'd ==6988== at > 0x402303F: free (vg_replace_malloc.c:233) > ==6988== by 0x4BAF503: _mesa_free (imports.c:93) > ==6988== by 0x4B2FF84: __driUtilUpdateDrawableInfo (dri_util.c:430) > ==6988== by 0x4B2FD46: DoBindContext (dri_util.c:339) > ==6988== by 0x4B2FF00: driBindContext (dri_util.c:383) > ==6988== by 0x4735921: BindContextWrapper (glxext.c:1620) > ==6988== by 0x4735A53: MakeContextCurrent (glxext.c:1674) > ==6988== by 0x4735D7C: glXMakeCurrent (glxext.c:1796) > ==6988== by 0x47D8BB3: Producer::RenderSurface::makeCurrent(bool) (in > /usr/lib/libProducer.so) ==6988== by 0x47DEEC6: > Producer::Camera::_frame(bool) (in /usr/lib/libProducer.so) ==6988== by > 0x47DF75F: Producer::Camera::frame(bool) (in /usr/lib/libProducer.so) > ==6988== by 0x47E2589: Producer::CameraGroup::_singleThreadedFrame() (in > /usr/lib/libProducer.so) > > So I searched a bit, and I created the following patch, which touches > src/mesa/drivers/dri/common/dri_util.c, and more specifically > __driUtilUpdateDrawableInfo, calling _mesa_free for pdp->pClipRects and > pdp->pBackClipRects only inside if. > > Does this look sane? > > Valgrind does not warn anymore. -- Papadakos Panagiotis |
From: Panagiotis P. <pap...@cs...> - 2007-02-26 04:58:10
Attachments:
radeonMakeCurrent.diff
|
Well I think I have the correct patch for this. We should call radeonSetCliprects inside radeonMakeCurrent to update to the new pClipRects whenever lastStamps change. Probably this should also be applied to r200. Haven't seen for other drivers. Comments? P.S. Should I try to get an account? On Friday 02 February 2007 19:36, Panagiotis Papadakos wrote: > Well this patch is not correct since it leads to memory leaks, probably due > to succeeding calls to dri_interface->getDrawableInfo. So I created the > attached patch, but again valgrind warns for invalid reads. > > Anyone understands why? > > Thanks > > P.S. Sorry but I am new to the code! > > On Thursday 01 February 2007 19:19, Panagiotis Papadakos wrote: > > Checking with valgrind I got some invalid reads. The message was like the > > following: > > > > ==6988== Invalid read of size 4 > > ==6988== at 0x4B3C7FD: intersect_rect (radeon_state.c:61) > > ==6988== by 0x4B3C9DA: radeonRecalcScissorRects (radeon_state.c:108) > > ==6988== by 0x4B3CAEC: radeonUpdateScissor (radeon_state.c:131) > > ==6988== by 0x4B3CD04: radeonEnable (radeon_state.c:205) > > ==6988== by 0x4B4B1C1: r300Enable (r300_state.c:542) > > ==6988== by 0x4D13827: _mesa_set_enable (enable.c:956) > > ==6988== by 0x4D138A6: _mesa_Enable (enable.c:971) > > ==6988== by 0x4769879: glEnable (glapitemp.h:1160) > > ==6988== by 0x4613A5F: > > osgUtil::RenderStage::drawImplementation(osg::RenderInfo&, > > osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== by > > 0x4607658: osgUtil::RenderBin::draw(osg::RenderInfo&, > > osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== by > > 0x46133BC: osgUtil::RenderStage::drawInner(osg::RenderInfo&, > > osgUtil::RenderLeaf*&, bool&) (in /usr/lib/libosgUtil.so) ==6988== by > > 0x4612E6C: osgUtil::RenderStage::draw(osg::RenderInfo&, > > osgUtil::RenderLeaf*&) (in /usr/lib/libosgUtil.so) ==6988== Address > > 0x4AF585C is 4 bytes inside a block of size 8 free'd ==6988== at > > 0x402303F: free (vg_replace_malloc.c:233) > > ==6988== by 0x4BAF503: _mesa_free (imports.c:93) > > ==6988== by 0x4B2FF84: __driUtilUpdateDrawableInfo (dri_util.c:430) > > ==6988== by 0x4B2FD46: DoBindContext (dri_util.c:339) > > ==6988== by 0x4B2FF00: driBindContext (dri_util.c:383) > > ==6988== by 0x4735921: BindContextWrapper (glxext.c:1620) > > ==6988== by 0x4735A53: MakeContextCurrent (glxext.c:1674) > > ==6988== by 0x4735D7C: glXMakeCurrent (glxext.c:1796) > > ==6988== by 0x47D8BB3: Producer::RenderSurface::makeCurrent(bool) (in > > /usr/lib/libProducer.so) ==6988== by 0x47DEEC6: > > Producer::Camera::_frame(bool) (in /usr/lib/libProducer.so) ==6988== > > by 0x47DF75F: Producer::Camera::frame(bool) (in /usr/lib/libProducer.so) > > ==6988== by 0x47E2589: Producer::CameraGroup::_singleThreadedFrame() > > (in /usr/lib/libProducer.so) > > > > So I searched a bit, and I created the following patch, which touches > > src/mesa/drivers/dri/common/dri_util.c, and more specifically > > __driUtilUpdateDrawableInfo, calling _mesa_free for pdp->pClipRects and > > pdp->pBackClipRects only inside if. > > > > Does this look sane? > > > > Valgrind does not warn anymore. -- Papadakos Panagiotis |
From: Michel <mi...@tu...> - 2007-03-02 14:41:38
|
On Mon, 2007-02-26 at 07:09 +0200, Panagiotis Papadakos wrote: > Well I think I have the correct patch for this. I'm afraid not; the test modification is incorrect. The test is an optimization to avoid doing unnecessary work when the context is already up to date wrt the drawables passed in and doesn't have anything to do with the stamps. > We should call radeonSetCliprects inside radeonMakeCurrent to update to > the new pClipRects whenever lastStamps change. I think that makes sense though. Maybe the updates of radeon->lastStamp could also be moved into radeonSetCliprects and/or the common parts of radeonMakeCurrent and r300RegainedLock moved into a separate function called by both. BTW, the patch also has superfluous whitespace-only changes, which should be avoided. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Panagiotis P. <pap...@cs...> - 2007-03-07 16:51:27
|
On Friday 02 March 2007 16:41, Michel D=C3=A4nzer wrote: > On Mon, 2007-02-26 at 07:09 +0200, Panagiotis Papadakos wrote: > > Well I think I have the correct patch for this. > > I'm afraid not; the test modification is incorrect. The test is an > optimization to avoid doing unnecessary work when the context is already > up to date wrt the drawables passed in and doesn't have anything to do > with the stamps. > Well I think we should do this, because __driUtilUpdateDrawableInfo, is called when lastStamp !=3D stamp, and although the drawables have not changed, we get new cliprects. So we have to call radeonSetCliprects, else we point to memory freed in DoBindContext. =2D-=20 Papadakos Panagiotis |
From: Michel <mi...@tu...> - 2007-03-07 17:10:32
|
On Wed, 2007-03-07 at 19:02 +0200, Panagiotis Papadakos wrote: > On Friday 02 March 2007 16:41, Michel Dänzer wrote: > > On Mon, 2007-02-26 at 07:09 +0200, Panagiotis Papadakos wrote: > > > Well I think I have the correct patch for this. > > > > I'm afraid not; the test modification is incorrect. The test is an > > optimization to avoid doing unnecessary work when the context is already > > up to date wrt the drawables passed in and doesn't have anything to do > > with the stamps. > > Well I think we should do this, because __driUtilUpdateDrawableInfo, > is called when lastStamp != stamp, and although the drawables have not > changed, we get new cliprects. So we have to call radeonSetCliprects, else > we point to memory freed in DoBindContext. Right, just your test modification was wrong. This turned out to also fix issues with apps that don't call glViewport by default, so I pushed something based on it in the meantime, see http://bugs.freedesktop.org/show_bug.cgi?id=9876 . Thanks. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Panagiotis P. <pap...@cs...> - 2007-03-07 21:07:34
|
The problem is that I still get invalid reads when going from fullscreen mo= de=20 to window mode for the first time, with my OSG application. On Wednesday 07 March 2007 19:10, Michel D=C3=A4nzer wrote: > On Wed, 2007-03-07 at 19:02 +0200, Panagiotis Papadakos wrote: > > On Friday 02 March 2007 16:41, Michel D=C3=A4nzer wrote: > > > On Mon, 2007-02-26 at 07:09 +0200, Panagiotis Papadakos wrote: > > > > Well I think I have the correct patch for this. > > > > > > I'm afraid not; the test modification is incorrect. The test is an > > > optimization to avoid doing unnecessary work when the context is > > > already up to date wrt the drawables passed in and doesn't have > > > anything to do with the stamps. > > > > Well I think we should do this, because __driUtilUpdateDrawableInfo, > > is called when lastStamp !=3D stamp, and although the drawables have not > > changed, we get new cliprects. So we have to call radeonSetCliprects, > > else we point to memory freed in DoBindContext. > > Right, just your test modification was wrong. This turned out to also > fix issues with apps that don't call glViewport by default, so I pushed > something based on it in the meantime, see > http://bugs.freedesktop.org/show_bug.cgi?id=3D9876 . Thanks. =2D-=20 Papadakos Panagiotis |
From: Michel <mi...@tu...> - 2007-03-08 10:12:22
|
On Wed, 2007-03-07 at 23:19 +0200, Panagiotis Papadakos wrote: > The problem is that I still get invalid reads when going from fullscreen mode > to window mode for the first time, with my OSG application. Hmm, then I guess it re-binds the context in the process of switching between fullscreen and windowed? Does calling radeonSetCliprects unconditionally before _mesa_make_current help? I don't see a way for radeonMakeCurrent to be sure DoBindContext didn't update the cliprects. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Panagiotis P. <pap...@cs...> - 2007-03-08 12:09:35
|
On Thursday 08 March 2007 12:12, Michel D=C3=A4nzer wrote: > Does calling radeonSetCliprects unconditionally before > _mesa_make_current help? I don't see a way for radeonMakeCurrent to be > sure DoBindContext didn't update the cliprects. Yep it helps. Calling radeonSetCliprects when stamp!=3DlastStamp also helps! Regards Papadakos Panagiotis |
From: Panagiotis P. <pap...@cs...> - 2007-03-08 12:13:35
|
> Calling radeonSetCliprects when stamp!=lastStamp also helps! Sorry meant radeon->lastStamp != driDrawPriv->lastStamp and radeon->lastStamp != driReadPriv->lastStamp. |
From: Michel <mi...@tu...> - 2007-03-08 13:32:00
Attachments:
radeon_context.diff
|
On Thu, 2007-03-08 at 14:25 +0200, Panagiotis Papadakos wrote: > > Calling radeonSetCliprects when stamp!=lastStamp also helps! > Sorry meant radeon->lastStamp != driDrawPriv->lastStamp That'll usually be fine, but in theory the stamps could be identical when a different drawable was bound previously. How does this patch look? > and radeon->lastStamp != driReadPriv->lastStamp. That comparison makes no sense as the cliprects and stamps only apply to the drawable bound for drawing. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Michel <mi...@tu...> - 2007-03-08 13:42:19
Attachments:
radeon_context.diff
|
On Thu, 2007-03-08 at 14:31 +0100, Michel Dänzer wrote: > On Thu, 2007-03-08 at 14:25 +0200, Panagiotis Papadakos wrote: > > > Calling radeonSetCliprects when stamp!=lastStamp also helps! > > Sorry meant radeon->lastStamp != driDrawPriv->lastStamp > > That'll usually be fine, but in theory the stamps could be identical > when a different drawable was bound previously. > > How does this patch look? Or how about one that actually compiles and doesn't crash... -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Panagiotis P. <pap...@cs...> - 2007-03-08 14:52:05
|
I still get invalid reads with your latest patch. On Thursday 08 March 2007 15:42, Michel D=E4nzer wrote: > On Thu, 2007-03-08 at 14:31 +0100, Michel D=E4nzer wrote: > > On Thu, 2007-03-08 at 14:25 +0200, Panagiotis Papadakos wrote: > > > > Calling radeonSetCliprects when stamp!=3DlastStamp also helps! > > > > > > Sorry meant radeon->lastStamp !=3D driDrawPriv->lastStamp > > > > That'll usually be fine, but in theory the stamps could be identical > > when a different drawable was bound previously. > > > > How does this patch look? > > Or how about one that actually compiles and doesn't crash... =2D-=20 Papadakos Panagiotis |
From: Michel <mi...@tu...> - 2007-03-08 15:05:40
|
On Thu, 2007-03-08 at 17:03 +0200, Panagiotis Papadakos wrote: > I still get invalid reads with your latest patch. Any idea what's going on? The only situation where radeonSetCliprects doesn't get called from radeonMakeCurrent is if neither the drawable nor its stamp has changed... -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Panagiotis P. <pap...@cs...> - 2007-03-08 16:55:47
|
On Thursday 08 March 2007 17:05, Michel D=C3=A4nzer wrote: > > Any idea what's going on? The only situation where radeonSetCliprects > doesn't get called from radeonMakeCurrent is if neither the drawable nor > its stamp has changed... We have to call radeonSetCliprects(radeon), before=20 r300UpdateViewportOffset(radeon->glCtx). =2D-=20 Papadakos Panagiotis |
From: Michel <mi...@tu...> - 2007-03-09 08:46:33
|
On Thu, 2007-03-08 at 19:07 +0200, Panagiotis Papadakos wrote: > On Thursday 08 March 2007 17:05, Michel Dänzer wrote: > > > > Any idea what's going on? The only situation where radeonSetCliprects > > doesn't get called from radeonMakeCurrent is if neither the drawable nor > > its stamp has changed... > > We have to call radeonSetCliprects(radeon), before > r300UpdateViewportOffset(radeon->glCtx). Ah right, like this? -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |
From: Panagiotis P. <pap...@cs...> - 2007-03-09 11:38:01
|
Yep, I think it is OK! On Friday 09 March 2007 10:46, Michel D=E4nzer wrote: > On Thu, 2007-03-08 at 19:07 +0200, Panagiotis Papadakos wrote: > > On Thursday 08 March 2007 17:05, Michel D=E4nzer wrote: > > > Any idea what's going on? The only situation where radeonSetCliprects > > > doesn't get called from radeonMakeCurrent is if neither the drawable > > > nor its stamp has changed... > > > > We have to call radeonSetCliprects(radeon), before > > r300UpdateViewportOffset(radeon->glCtx). > > Ah right, like this? =2D-=20 Papadakos Panagiotis |
From: Michel <mi...@tu...> - 2007-03-09 13:47:23
|
On Fri, 2007-03-09 at 13:49 +0200, Panagiotis Papadakos wrote: > Yep, I think it is OK! Pushed, thanks. -- Earthling Michel Dänzer | http://tungstengraphics.com Libre software enthusiast | Debian, X and DRI developer |