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