thr3ads.net - Virtualization - [PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE

If this information is useful, please help other people find it:
Share via:

Noralf Trønnes

2021-Nov-09 13:04 UTC

[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

Den 09.11.2021 13.38, skrev Thomas Zimmermann:> 
> 
> Am 08.11.21 um 21:55 schrieb Noralf Tr?nnes:
>>
>>
>> Den 01.11.2021 15.15, skrev Thomas Zimmermann:
>>> Enable the FB_DAMAGE_CLIPS property to reduce display-update
>>> overhead. Also fixes a warning in the kernel log.
>>>
>>> ?? simple-framebuffer simple-framebuffer.0: [drm]
>>> drm_plane_enable_fb_damage_clips() not called
>>>
>>> Fix the computation of the blit rectangle. This wasn't an issue
so
>>> far, as simpledrm always blitted the full framebuffer. The code now
>>> supports damage clipping and virtual screen sizes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> ---
>>> ? drivers/gpu/drm/tiny/simpledrm.c | 30
++++++++++++++++++++++--------
>>> ? 1 file changed, 22 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 571f716ff427..e872121e9fb0 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct
>>> drm_simple_display_pipe *pipe,
>>> ????? void *vmap = shadow_plane_state->data[0].vaddr; /* TODO:
Use
>>> mapping abstraction */
>>> ????? struct drm_device *dev = &sdev->dev;
>>> ????? void __iomem *dst = sdev->screen_base;
>>> -??? struct drm_rect clip;
>>> +??? struct drm_rect src_clip, dst_clip;
>>> ????? int idx;
>>> ? ????? if (!fb)
>>> @@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct
>>> drm_simple_display_pipe *pipe,
>>> ????? if (!drm_dev_enter(dev, &idx))
>>> ????????? return;
>>> ? -??? drm_rect_init(&clip, 0, 0, fb->width, fb->height);
>>> +??? drm_rect_fp_to_int(&src_clip, &plane_state->src);
>>> ? -??? dst += drm_fb_clip_offset(sdev->pitch, sdev->format,
&clip);
>>> -??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>> fb, &clip);
>>> +??? dst_clip = plane_state->dst;
>>
>> I assume that src_clip and dst_clip are of the same size, since scaling
>> is not supported. What prevents dst_clip from being bigger than the
>> buffer that's being blitted into? Where is that bounds checking
done?
> 
> The value of dst_clip comes from plane_state->dst, which gets
> initialized in drm_atomic_helper_check_plane_state(). [1] The fields are
> taken from the crtc_{x,y,w,h} values by drm_plane_get_dest(). [2] For
> primary planes, the crtc_{x,y,w,h} values are initialized in
> __drm_atomic_helper_set_config() to the size of the display. [3] That
> size comes directly from the current video mode. [4] From all I can see
> this cannot overflow.
> 
Ok, that takes care of the upper bound.

There's this in drm_atomic_helper_check_plane_state():

	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);

	if (!plane_state->visible)
		/*
		 * Plane isn't visible; some drivers can handle this
		 * so we just return success here.  Drivers that can't
		 * (including those that use the primary plane helper's
		 * update function) will return an error from their
		 * update_plane handler.
		 */
		return 0;

drm_atomic_helper_damage_merged() checks ->visible and returns false if
it is not set so update() is good on the lower bound.

Maybe it's necessary to check ->visible here in enable() before doing
the blit?

Noralf.
> Best regards
> Thomas
> 
> [1]
>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L807
> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/include/drm/drm_plane.h#L257
> [3]
>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic.c#L1590
> 
> [4]
>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L788
> 
> 
>>
>> Noralf.
>>
>>> +??? if (!drm_rect_intersect(&dst_clip, &src_clip))
>>> +??????? return;
>>> +
>>> +??? dst += drm_fb_clip_offset(sdev->pitch, sdev->format,
&dst_clip);
>>> +??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>> fb, &src_clip);
>>> ? ????? drm_dev_exit(idx);
>>> ? }
>>> @@ -686,20 +690,28 @@ simpledrm_simple_display_pipe_update(struct
>>> drm_simple_display_pipe *pipe,
>>> ????? struct drm_framebuffer *fb = plane_state->fb;
>>> ????? struct drm_device *dev = &sdev->dev;
>>> ????? void __iomem *dst = sdev->screen_base;
>>> -??? struct drm_rect clip;
>>> +??? struct drm_rect damage_clip, src_clip, dst_clip;
>>> ????? int idx;
>>> ? ????? if (!fb)
>>> ????????? return;
>>> ? -??? if (!drm_atomic_helper_damage_merged(old_plane_state,
>>> plane_state, &clip))
>>> +??? if (!drm_atomic_helper_damage_merged(old_plane_state,
>>> plane_state, &damage_clip))
>>> +??????? return;
>>> +
>>> +??? drm_rect_fp_to_int(&src_clip, &plane_state->src);
>>> +??? if (!drm_rect_intersect(&src_clip, &damage_clip))
>>> +??????? return;
>>> +
>>> +??? dst_clip = plane_state->dst;
>>> +??? if (!drm_rect_intersect(&dst_clip, &src_clip))
>>> ????????? return;
>>> ? ????? if (!drm_dev_enter(dev, &idx))
>>> ????????? return;
>>> ? -??? dst += drm_fb_clip_offset(sdev->pitch, sdev->format,
&clip);
>>> -??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>> fb, &clip);
>>> +??? dst += drm_fb_clip_offset(sdev->pitch, sdev->format,
&dst_clip);
>>> +??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>> fb, &src_clip);
>>> ? ????? drm_dev_exit(idx);
>>> ? }
>>> @@ -794,6 +806,8 @@ static int simpledrm_device_init_modeset(struct
>>> simpledrm_device *sdev)
>>> ????? if (ret)
>>> ????????? return ret;
>>> ? +??? drm_plane_enable_fb_damage_clips(&pipe->plane);
>>> +
>>> ????? drm_mode_config_reset(dev);
>>> ? ????? return 0;
>>>
>

Thomas Zimmermann

2021-Nov-09 14:56 UTC

head link

[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

Hi,

thanks for looking through all this code.

Am 09.11.21 um 14:04 schrieb Noralf Tr?nnes:> 
> 
> Den 09.11.2021 13.38, skrev Thomas Zimmermann:
>>
>>
>> Am 08.11.21 um 21:55 schrieb Noralf Tr?nnes:
>>>
>>>
>>> Den 01.11.2021 15.15, skrev Thomas Zimmermann:
>>>> Enable the FB_DAMAGE_CLIPS property to reduce display-update
>>>> overhead. Also fixes a warning in the kernel log.
>>>>
>>>>  ?? simple-framebuffer simple-framebuffer.0: [drm]
>>>> drm_plane_enable_fb_damage_clips() not called
>>>>
>>>> Fix the computation of the blit rectangle. This wasn't an
issue so
>>>> far, as simpledrm always blitted the full framebuffer. The code
now
>>>> supports damage clipping and virtual screen sizes.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>> ---
>>>>  ? drivers/gpu/drm/tiny/simpledrm.c | 30
++++++++++++++++++++++--------
>>>>  ? 1 file changed, 22 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>>> index 571f716ff427..e872121e9fb0 100644
>>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>>> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct
>>>> drm_simple_display_pipe *pipe,
>>>>  ????? void *vmap = shadow_plane_state->data[0].vaddr; /*
TODO: Use
>>>> mapping abstraction */
>>>>  ????? struct drm_device *dev = &sdev->dev;
>>>>  ????? void __iomem *dst = sdev->screen_base;
>>>> -??? struct drm_rect clip;
>>>> +??? struct drm_rect src_clip, dst_clip;
>>>>  ????? int idx;
>>>>  ? ????? if (!fb)
>>>> @@ -651,10 +651,14 @@
simpledrm_simple_display_pipe_enable(struct
>>>> drm_simple_display_pipe *pipe,
>>>>  ????? if (!drm_dev_enter(dev, &idx))
>>>>  ????????? return;
>>>>  ? -??? drm_rect_init(&clip, 0, 0, fb->width,
fb->height);
>>>> +??? drm_rect_fp_to_int(&src_clip,
&plane_state->src);
>>>>  ? -??? dst += drm_fb_clip_offset(sdev->pitch,
sdev->format, &clip);
>>>> -??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>>> fb, &clip);
>>>> +??? dst_clip = plane_state->dst;
>>>
>>> I assume that src_clip and dst_clip are of the same size, since
scaling
>>> is not supported. What prevents dst_clip from being bigger than the
>>> buffer that's being blitted into? Where is that bounds checking
done?
>>
>> The value of dst_clip comes from plane_state->dst, which gets
>> initialized in drm_atomic_helper_check_plane_state(). [1] The fields
are
>> taken from the crtc_{x,y,w,h} values by drm_plane_get_dest(). [2] For
>> primary planes, the crtc_{x,y,w,h} values are initialized in
>> __drm_atomic_helper_set_config() to the size of the display. [3] That
>> size comes directly from the current video mode. [4] From all I can see
>> this cannot overflow.
>>
> 
> Ok, that takes care of the upper bound.
> 
> There's this in drm_atomic_helper_check_plane_state():
> 
> 	plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
> 
> 	if (!plane_state->visible)
> 		/*
> 		 * Plane isn't visible; some drivers can handle this
> 		 * so we just return success here.  Drivers that can't
> 		 * (including those that use the primary plane helper's
> 		 * update function) will return an error from their
> 		 * update_plane handler.
> 		 */
> 		return 0;
> 
> drm_atomic_helper_damage_merged() checks ->visible and returns false if
> it is not set so update() is good on the lower bound.
> 
> Maybe it's necessary to check ->visible here in enable() before
doing
> the blit?
Is it even possible to create an invisible primary plane here? We cannot 
scale [1] and if the primary plane is smaller than the framebuffer, we 
hit the case at [2]. The only way I can see this is that the visibility 
test at [3] succeeds. That would require a framebuffer of size 0. That's 
impossible from the code in frambuffer_check(). [4] Unless there's some 
corner case, we're good here.

However, I think that drm_atomic_check_plane_state() could later be 
improved to handle the 'visible' field in a more sophisticated way.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_simple_kms_helper.c#L227
[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L854
[3] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L844
[4] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_framebuffer.c#L185
> 
> Noralf.
> 
>> Best regards
>> Thomas
>>
>> [1]
>>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L807
>>
>> [2]
>>
https://elixir.bootlin.com/linux/latest/source/include/drm/drm_plane.h#L257
>> [3]
>>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic.c#L1590
>>
>> [4]
>>
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L788
>>
>>
>>>
>>> Noralf.
>>>
>>>> +??? if (!drm_rect_intersect(&dst_clip, &src_clip))
>>>> +??????? return;
>>>> +
>>>> +??? dst += drm_fb_clip_offset(sdev->pitch, sdev->format,
&dst_clip);
>>>> +??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>>> fb, &src_clip);
>>>>  ? ????? drm_dev_exit(idx);
>>>>  ? }
>>>> @@ -686,20 +690,28 @@
simpledrm_simple_display_pipe_update(struct
>>>> drm_simple_display_pipe *pipe,
>>>>  ????? struct drm_framebuffer *fb = plane_state->fb;
>>>>  ????? struct drm_device *dev = &sdev->dev;
>>>>  ????? void __iomem *dst = sdev->screen_base;
>>>> -??? struct drm_rect clip;
>>>> +??? struct drm_rect damage_clip, src_clip, dst_clip;
>>>>  ????? int idx;
>>>>  ? ????? if (!fb)
>>>>  ????????? return;
>>>>  ? -??? if (!drm_atomic_helper_damage_merged(old_plane_state,
>>>> plane_state, &clip))
>>>> +??? if (!drm_atomic_helper_damage_merged(old_plane_state,
>>>> plane_state, &damage_clip))
>>>> +??????? return;
>>>> +
>>>> +??? drm_rect_fp_to_int(&src_clip,
&plane_state->src);
>>>> +??? if (!drm_rect_intersect(&src_clip, &damage_clip))
>>>> +??????? return;
>>>> +
>>>> +??? dst_clip = plane_state->dst;
>>>> +??? if (!drm_rect_intersect(&dst_clip, &src_clip))
>>>>  ????????? return;
>>>>  ? ????? if (!drm_dev_enter(dev, &idx))
>>>>  ????????? return;
>>>>  ? -??? dst += drm_fb_clip_offset(sdev->pitch,
sdev->format, &clip);
>>>> -??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>>> fb, &clip);
>>>> +??? dst += drm_fb_clip_offset(sdev->pitch, sdev->format,
&dst_clip);
>>>> +??? drm_fb_blit_toio(dst, sdev->pitch,
sdev->format->format, vmap,
>>>> fb, &src_clip);
>>>>  ? ????? drm_dev_exit(idx);
>>>>  ? }
>>>> @@ -794,6 +806,8 @@ static int
simpledrm_device_init_modeset(struct
>>>> simpledrm_device *sdev)
>>>>  ????? if (ret)
>>>>  ????????? return ret;
>>>>  ? +??? drm_plane_enable_fb_damage_clips(&pipe->plane);
>>>> +
>>>>  ????? drm_mode_config_reset(dev);
>>>>  ? ????? return 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20211109/eb796004/attachment.sig>

Virtualization - Nov 2021 - [PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property