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.

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