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

[Daniel Vetter]

On Fri, Jan 06, 2023 at 09:10:17AM +0100, 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.

That would mean more copying, and vkms already copies a lot by recomputing
the crc every frame. Fundamentally with vkms the cpu is the display
engine, and it needs a mapping for as long as the buffer is in use.

Also I guess we really need gitlab ci going, it's a bit silly we're
breaking pure sw drivers :-/
-Daniel

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




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch