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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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@xxxxxxxxxx>
> >
> > Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >
> > > > ---
> > > >   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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature