Hi

Am 06.01.23 um 12:34 schrieb 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@igalia.com>
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@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.

Ok.

> 
>>
>>>> +
>>>> +    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.

Sorry, I misread that line. You're right.

Best regards
Thomas

> 
> 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
>>>>
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev