[PATCH] drm/vkms: introduce prepare_fb and cleanup_fb functions

[Maíra Canal]

Hi,

Thanks for the review!

On 1/6/23 05:10, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.01.23 um 19:43 schrieb Melissa Wen:
>> On 01/05, Maíra Canal wrote:
>>> With commit 359c6649cd9a ("drm/gem: Implement shadow-plane {begin,
>>> end}_fb_access with vmap"), the behavior of the shadow-plane helpers
>>> changed and the vunmap is now performed at the end of
>>> the current pageflip, instead of the end of the following pageflip.
>>>
>>> By performing the vunmap at the end of the current pageflip, invalid
>>> memory is accessed by the vkms during the plane composition, as the data
>>> is being unmapped before being used.
>>
>> Hi Maíra,
>>
>> Thanks for investigating this issue. Can you add in the commit message
>> the kernel Oops caused by this change?
>>
>> Besides that, I wonder if the right thing would be to restore the
>> previous behavior of vunmap in shadow-plane helpers, instead of
>> reintroduce driver-specific hooks for vmap/vunmap correctly to vkms.
>>
>> Maybe Thomas has some inputs on this shadow-plane changing to help us on
>> figuring out the proper fix (?)
> 
> The fix looks good. I left some minor-important comments below.
> 
> I would suggest to rethink the overall driver design. Instead of keeping these buffer pinned for long. It might be better to have a per-plane intermediate buffer that you update on each call to atomic_update. That's how real drivers interact with their hardware.
> 
>>
>> Best Regards,
>>
>> Melissa
>>
>>>
>>> Therefore, introduce again prepare_fb and cleanup_fb functions to the
>>> vkms, which were previously removed on commit b43e2ec03b0d ("drm/vkms:
>>> Let shadow-plane helpers prepare the plane's FB").
>>>
>>> Fixes: 359c6649cd9a ("drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap")
>>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
> 
> Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
> 
>>> ---
>>>   drivers/gpu/drm/vkms/vkms_plane.c | 36 ++++++++++++++++++++++++++++++-
>>>   1 file changed, 35 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>> index c3a845220e10..b3f8a115cc23 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>> @@ -160,10 +160,44 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
>>>     return 0;
>>>   }
>>>
>>> +static int vkms_prepare_fb(struct drm_plane *plane,
>>> +               struct drm_plane_state *state)
>>> +{
>>> +    struct drm_shadow_plane_state *shadow_plane_state;
>>> +    struct drm_framebuffer *fb = state->fb;
>>> +    int ret;
>>> +
>>> +    if (!fb)
>>> +        return 0;
> 
> IIRC this cannot be NULL. Only active planes will be 'prepared'.> 
>>> +
>>> +    shadow_plane_state = to_drm_shadow_plane_state(state);
>>> +
>>> +    ret = drm_gem_plane_helper_prepare_fb(plane, state);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
>>> +}
>>> +
>>> +static void vkms_cleanup_fb(struct drm_plane *plane,
>>> +                struct drm_plane_state *state)
>>> +{
>>> +    struct drm_shadow_plane_state *shadow_plane_state;
>>> +    struct drm_framebuffer *fb = state->fb;
>>> +
>>> +    if (!fb)
>>> +        return;
> 
> Same here. This function won't be called if there has not been a framebuffer.

After removing those two checks, I started to get some NULL pointer dereference
errors, so I believe they are somehow necessary.

> 
>>> +
>>> +    shadow_plane_state = to_drm_shadow_plane_state(state);
>>> +
>>> +    drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>>> +}
>>> +
>>>   static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
>>>     .atomic_update        = vkms_plane_atomic_update,
>>>     .atomic_check        = vkms_plane_atomic_check,
>>> -    DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> 
> You're still installing {being/end}_fb_access, which should not be necessary now. Open-coding DRM_GEM_SHADOW_PLANE_HELPER_FUNCS without those helpers would fix that.

I'm sorry but I didn't understand this comment. AFAIK I {being/end}_fb_access are
NULL as I removed the DRM_GEM_SHADOW_PLANE_HELPER_FUNCS macro.

Best Regards,
- Maíra Canal

> 
> Best regards
> Thomas
> 
>>> +    .prepare_fb        = vkms_prepare_fb,
>>> +    .cleanup_fb        = vkms_cleanup_fb,
>>>   };
>>>
>>>   struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>>> -- 
>>> 2.39.0
>>>
>