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

[Thomas Hellstrom]

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.

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.

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

Thanks,
Thomas