[PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property
Thomas Zimmermann
tzimmermann at suse.de
Tue Nov 9 14:56:11 UTC 2021
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: <https://lists.freedesktop.org/archives/dri-devel/attachments/20211109/eb796004/attachment-0001.sig>
More information about the dri-devel
mailing list