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

[Michel Dänzer]

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.

Some comments on your patch, assuming we end up going with that:


> +      /* If we need to rerun the validation, make sure we unreference the
> +       * textures first. The validate function will just clear the pointers.
> +       */
> +
> +      if (stfb->iface_stamp != new_stamp) {
> +         for (i = 0; i < stfb->num_statts; ++i) {
> +            pipe_resource_reference(&textures[i], NULL);
> +         }
> +      }

I'd drop the blank line after the comment and the curly braces around
the inner loop, but either way, with

Fixes: e96d175c7d2e "loader/dri3: Make sure we invalidate a drawable on
                     size change"

added to the commit log,

Reviewed-and-Tested-by: Michel Dänzer <michel.daenzer at amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-st-mesa-Initialize-textures-array-in-st_framebuffer_.patch
Type: text/x-patch
Size: 5246 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171016/b5628c88/attachment-0001.bin>