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

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

[Maíra Canal]

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.

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

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

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4143 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [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@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




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

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

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

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

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

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5267 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Daniel Vetter]

On Fri, Jan 06, 2023 at 08:34:13AM -0300, Maíra Canal wrote:
> 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.

I'm honestly not sure whether any driver does not have this check ...
might be worth to go through them, and if they all have it, pull it into
helpers. It does look a bit silly like that :-)
-Daniel

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

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