[Mesa-dev] [PATCH 2/2] loader/dri3: Make sure we invalidate a drawable on size change

[Michel Dänzer]

On 16/10/17 07:16 PM, Thomas Hellstrom wrote:
> On 10/16/2017 04:39 PM, Michel Dänzer wrote:
>> On 16/10/17 02:21 PM, Thomas Hellstrom wrote:
>>> On 10/16/2017 12:53 PM, Thomas Hellstrom wrote:
>>>> Hi, Michel,
>>>>
>>>> On 10/16/2017 12:35 PM, Michel Dänzer wrote:
>>>>> Hi Thomas,
>>>>>
>>>>> On 05/09/17 10:15 AM, Thomas Hellstrom wrote:
>>>>>> If we're seeing a drawable size change, in particular after
>>>>>> processing a
>>>>>> configure notify event, make sure we invalidate so that the state
>>>>>> tracker
>>>>>> picks up the new geometry.
>>>>>>
>>>>>> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
>>>>>> ---
>>>>>>    src/loader/loader_dri3_helper.c | 2 ++
>>>>>>    1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/src/loader/loader_dri3_helper.c
>>>>>> b/src/loader/loader_dri3_helper.c
>>>>>> index 51e4e97..bcd5a66 100644
>>>>>> --- a/src/loader/loader_dri3_helper.c
>>>>>> +++ b/src/loader/loader_dri3_helper.c
>>>>>> @@ -348,6 +348,7 @@ dri3_handle_present_event(struct
>>>>>> loader_dri3_drawable *draw,
>>>>>>          draw->width = ce->width;
>>>>>>          draw->height = ce->height;
>>>>>>          draw->vtable->set_drawable_size(draw, draw->width,
>>>>>> draw->height);
>>>>>> + draw->ext->flush->invalidate(draw->dri_drawable);
>>>>>>          break;
>>>>>>       }
>>>>>>       case XCB_PRESENT_COMPLETE_NOTIFY: {
>>>>>> @@ -1592,6 +1593,7 @@ loader_dri3_update_drawable_geometry(struct
>>>>>> loader_dri3_drawable *draw)
>>>>>>          draw->width = geom_reply->width;
>>>>>>          draw->height = geom_reply->height;
>>>>>>          draw->vtable->set_drawable_size(draw, draw->width,
>>>>>> draw->height);
>>>>>> + draw->ext->flush->invalidate(draw->dri_drawable);
>>>>>>            free(geom_reply);
>>>>>>       }
>>>>>>
>>>>> unfortunately, I just bisected a regression to this commit. With
>>>>> it, the
>>>>> pipe_resource textures backing DRI3 back buffers are leaked when the
>>>>> window is resized:
>>>>>
>>>>> ==3408== 273,760 (228,464 direct, 45,296 indirect) bytes in 131
>>>>> blocks are definitely lost in loss record 1,285 of 1,286
>>>>> ==3408==    at 0x4C2DC05: calloc (vg_replace_malloc.c:711)
>>>>> ==3408==    by 0xA04D5D3: r600_texture_create_object
>>>>> (r600_texture.c:1125)
>>>>> ==3408==    by 0xA04E1C1: si_texture_create (r600_texture.c:1382)
>>>>> ==3408==    by 0x9CE3A17: dri2_allocate_textures (dri2.c:803)
>>>>> ==3408==    by 0x9CDDAE2: dri_st_framebuffer_validate
>>>>> (dri_drawable.c:85)
>>>>> ==3408==    by 0x9B6D07F: st_framebuffer_validate (st_manager.c:195)
>>>>> ==3408==    by 0x9B6EE84: st_manager_validate_framebuffers
>>>>> (st_manager.c:1063)
>>>>> ==3408==    by 0x9B21807: st_validate_state (st_atom.c:202)
>>>>> ==3408==    by 0x9B2AF75: st_Clear (st_cb_clear.c:411)
>>>>> ==3408==    by 0x10A6BD: ??? (in /usr/bin/glxgears)
>>>>> ==3408==    by 0x109E37: ??? (in /usr/bin/glxgears)
>>>>> ==3408==    by 0x5E202E0: (below main) (libc-start.c:291)
>>>>> ==3408==
>>>>> ==3408== 362,768 (230,208 direct, 132,560 indirect) bytes in 132
>>>>> blocks are definitely lost in loss record 1,286 of 1,286
>>>>> ==3408==    at 0x4C2DC05: calloc (vg_replace_malloc.c:711)
>>>>> ==3408==    by 0xA04D5D3: r600_texture_create_object
>>>>> (r600_texture.c:1125)
>>>>> ==3408==    by 0xA04E1C1: si_texture_create (r600_texture.c:1382)
>>>>> ==3408==    by 0x9CE0C0D: dri2_create_image_common (dri2.c:1119)
>>>>> ==3408==    by 0x9CE0C6F: dri2_create_image (dri2.c:1140)
>>>>> ==3408==    by 0x5386BA8: dri3_alloc_render_buffer
>>>>> (loader_dri3_helper.c:1030)
>>>>> ==3408==    by 0x53876F4: dri3_get_buffer.isra.15
>>>>> (loader_dri3_helper.c:1364)
>>>>> ==3408==    by 0x53886DC: loader_dri3_get_buffers
>>>>> (loader_dri3_helper.c:1549)
>>>>> ==3408==    by 0x9CE298C: dri_image_drawable_get_buffers (dri2.c:452)
>>>>> ==3408==    by 0x9CE298C: dri2_allocate_textures (dri2.c:576)
>>>>> ==3408==    by 0x9CDDAE2: dri_st_framebuffer_validate
>>>>> (dri_drawable.c:85)
>>>>> ==3408==    by 0x9B6D07F: st_framebuffer_validate (st_manager.c:195)
>>>>> ==3408==    by 0x9B6EE84: st_manager_validate_framebuffers
>>>>> (st_manager.c:1063)
>>>>>
>>>>>
>>>>> Can you reproduce this with vmwgfx as well? Any ideas why this is
>>>>> happening / how to fix it?
>>>>>
>>>>>
>>>> Looks like other buffers (depth ?) are leaked too, outside dri3, right?
>>>>
>>>> I'll take a look.
>>>>
>>>> /Thomas
>>>>
>>>>
>>> Could you try the attached patch? Fixes the issue on my side.
>> Yes, it fixes the problem for me as well, thanks! I was looking at that
>> code as well, but didn't realize what's going on.
>>
>> I think the attached patch would be a cleaner solution though.
> 
> I agree. I had that in mind but had a bit too much on my plate today.
> 
> Perhaps we should not restrict the application of the patch patch with a
> Fixes: tag, though:
> Although improbable, the bug could be hit also without that dri3 patch.

Fair enough, replaced with a generic stable tag in v2.


> Also do we need to do a os_calloc() of the texture pointer array in
> st_framebuffer_validate(),
> cant we just memset() the fixed size array?  Your decision.

Right, I had the same thought in the meantime. :) Fixed in v2.


> Reviewed-by:
> Thomas Hellstrom <thellstrom at vmware.com>

Thanks!


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer