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 (?)

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>
> ---
>  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;
> +
> +	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;
> +
> +	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,
> +	.prepare_fb		= vkms_prepare_fb,
> +	.cleanup_fb		= vkms_cleanup_fb,
>  };
> 
>  struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> --
> 2.39.0
>