Message-ID: <b819de9c-f4e0-82cc-1ea4-21723070cc7e@vmware.com>
Date:   Thu, 5 Apr 2018 13:51:49 +0200
From:   Thomas Hellstrom <thellstrom@...are.com>
To:     Deepak Rawat <drawat@...are.com>, dri-devel@...ts.freedesktop.org,
        syeh@...are.com, linux-graphics-maintainer@...are.com,
        ville.syrjala@...ux.intel.com, lukasz.spintzyk@...playlink.com,
        noralf@...nnes.org, robdclark@...il.com, gustavo@...ovan.org,
        maarten.lankhorst@...ux.intel.com, seanpaul@...omium.org,
        airlied@...ux.ie, linux-kernel@...r.kernel.org
Subject: Re: [RFC 2/3] drm: Add helper iterator functions to iterate over
 plane damage.

On 04/05/2018 12:10 PM, Daniel Vetter wrote:
> On Thu, Apr 05, 2018 at 10:49:09AM +0200, Thomas Hellstrom wrote:
>> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
>>> On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
>>>> With damage property in drm_plane_state, this patch adds helper iterator
>>>> to traverse the damage clips. Iterator will return the damage rectangles
>>>> in framebuffer, plane or crtc coordinates as need by driver
>>>> implementation.
>>>>
>>>> Signed-off-by: Deepak Rawat <drawat@...are.com>
>>> I'd really like selftest/unittests for this stuff. There's an awful lot of
>>> cornercases in this here (for any of the transformed iterators at least),
>>> and unit tests is the best way to make sure we handle them all correctly.
>>>
>>> Bonus points if you integrate the new selftests into igt so intel CI can
>>> run them, seel igt/tests/drm_mm.c for an example. drm_mm selftest is also
>>> the framework I'd copy for this stuff.
>>>
>>>> ---
>>>>    drivers/gpu/drm/drm_atomic_helper.c | 122 ++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_atomic_helper.h     |  39 ++++++++++++
>>>>    2 files changed, 161 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 55b44e3..355b514 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -3865,3 +3865,125 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj
>>>>    	memcpy(state, obj->state, sizeof(*state));
>>>>    }
>>>>    EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state);
>>>> +
>>>> +/**
>>>> + * drm_atomic_helper_damage_iter_init - initialize the damage iterator
>>>> + * @iter: The iterator to initialize.
>>>> + * @type: Coordinate type caller is interested in.
>>>> + * @state: plane_state from which to iterate the damage clips.
>>>> + * @hdisplay: Width of crtc on which plane is scanned out.
>>>> + * @vdisplay: Height of crtc on which plane is scanned out.
>>>> + *
>>>> + * Initialize an iterator that is used to translate and clip a set of damage
>>>> + * rectangles in framebuffer coordinates to plane and crtc coordinates. The type
>>>> + * argument specify which type of coordinate to iterate in.
>>>> + *
>>>> + * Returns: 0 on success and negative error code on error. If an error code is
>>>> + * returned then it means the plane state should not update.
>>>> + */
>>>> +int
>>>> +drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>>>> +				   enum drm_atomic_helper_damage_clip_type type,
>>>> +				   const struct drm_plane_state *state,
>>>> +				   uint32_t hdisplay, uint32_t vdisplay)
>>>> +{
>>>> +	if (!state || !state->crtc || !state->fb)
>>>> +		return -EINVAL;
>>>> +
>>>> +	memset(iter, 0, sizeof(*iter));
>>>> +	iter->clips = (struct drm_rect *)state->damage_clips->data;
>>>> +	iter->num_clips = state->num_clips;
>>>> +	iter->type = type;
>>>> +
>>>> +	/*
>>>> +	 * Full update in case of scaling or rotation. In future support for
>>>> +	 * scaling/rotating damage clips can be added
>>>> +	 */
>>>> +	if (state->crtc_w != (state->src_w >> 16) ||
>>>> +	    state->crtc_h != state->src_h >> 16 || state->rotation != 0) {
>>>> +		iter->curr_clip = iter->num_clips;
>>>> +		return 0;
>>> Given there's no user of this I have no idea how this manages to provoke a
>>> full clip rect. selftest code would be perfect for stuff like this.
>>>
>>> Also, I think we should provide a full clip for the case of num_clips ==
>>> 0, so that driver code can simply iterate over all clips and doesn't ever
>>> have to handle the "no clip rects provided" case as a special case itself.
>>>
>>>> +	}
>>>> +
>>>> +	iter->fb_src.x1 = 0;
>>>> +	iter->fb_src.y1 = 0;
>>>> +	iter->fb_src.x2 = state->fb->width;
>>>> +	iter->fb_src.y2 = state->fb->height;
>>>> +
>>>> +	iter->plane_src.x1 = state->src_x >> 16;
>>>> +	iter->plane_src.y1 = state->src_y >> 16;
>>>> +	iter->plane_src.x2 = iter->plane_src.x1 + (state->src_w >> 16);
>>>> +	iter->plane_src.y2 = iter->plane_src.y1 + (state->src_h >> 16);
>>>> +	iter->translate_plane_x = -iter->plane_src.x1;
>>>> +	iter->translate_plane_y = -iter->plane_src.y1;
>>>> +
>>>> +	/* Clip plane src rect to fb dimensions */
>>>> +	drm_rect_intersect(&iter->plane_src, &iter->fb_src);
>>> This smells like driver bug. Also, see Ville's recent efforts to improve
>>> the atomic plane clipping, I think drm_plane_state already has all the
>>> clip rects you want.
>>>
>>>> +
>>>> +	iter->crtc_src.x1 = 0;
>>>> +	iter->crtc_src.y1 = 0;
>>>> +	iter->crtc_src.x2 = hdisplay;
>>>> +	iter->crtc_src.y2 = vdisplay;
>>>> +	iter->translate_crtc_x = -(iter->plane_src.x1 - state->crtc_x);
>>>> +	iter->translate_crtc_x = -(iter->plane_src.y1 - state->crtc_y);
>>>> +
>>>> +	/* Clip crtc src rect to plane dimensions */
>>>> +	drm_rect_translate(&iter->crtc_src, -iter->translate_crtc_x,
>>>> +			   -iter->translate_crtc_x);
>>> We can also scale.
>> I suggest we leave out scaling for now until someone actually needs it.
> In that case we need to WARN_ON and bail out if there's scaling going on.
> I'm totally fine with not solving the world here, but please no trapdoors
> for following driver's use-cases.
>

Agreed.

>>>> +	drm_rect_intersect(&iter->crtc_src, &iter->plane_src);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
>>>> +
>>>> +/**
>>>> + * drm_atomic_helper_damage_iter_next - advance the damage iterator
>>>> + * @iter: The iterator to advance.
>>>> + * @rect: Return a rectangle in coordinate specified during iterator init.
>>>> + *
>>>> + * Returns:  true if the output is valid, false if we've reached the end of the
>>>> + * rectangle list. If the first call return false, means need full update.
>>>> + */
>>>> +bool
>>>> +drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
>>>> +				   struct drm_rect *rect)
>>>> +{
>>>> +	const struct drm_rect *curr_clip;
>>>> +
>>>> +next_clip:
>>>> +	if (iter->curr_clip >= iter->num_clips)
>>>> +		return false;
>>>> +
>>>> +	curr_clip = &iter->clips[iter->curr_clip];
>>>> +	iter->curr_clip++;
>>>> +
>>>> +	rect->x1 = curr_clip->x1;
>>>> +	rect->x2 = curr_clip->x2;
>>>> +	rect->y1 = curr_clip->y1;
>>>> +	rect->y2 = curr_clip->y2;
>>>> +
>>>> +	/* Clip damage rect within fb limit */
>>>> +	if (!drm_rect_intersect(rect, &iter->fb_src))
>>>> +		goto next_clip;
>>>> +	else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB)
>>>> +		return true;
>>>> +
>>>> +	/* Clip damage rect within plane limit */
>>>> +	if (!drm_rect_intersect(rect, &iter->plane_src))
>>>> +		goto next_clip;
>>>> +	else if (iter->type & DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE) {
>>>> +		drm_rect_translate(rect, iter->translate_plane_x,
>>>> +				   iter->translate_plane_y);
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	/* Clip damage rect within crtc limit */
>>>> +	if (!drm_rect_intersect(rect, &iter->crtc_src))
>>>> +		goto next_clip;
>>>> +
>>>> +	drm_rect_translate(rect, iter->translate_crtc_x,
>>>> +			   iter->translate_crtc_y);
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
>>>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>>>> index 26aaba5..ebd4b66 100644
>>>> --- a/include/drm/drm_atomic_helper.h
>>>> +++ b/include/drm/drm_atomic_helper.h
>>>> @@ -36,6 +36,37 @@ struct drm_atomic_state;
>>>>    struct drm_private_obj;
>>>>    struct drm_private_state;
>>>> +/**
>>>> + * enum drm_atomic_helper_damage_clip_type - type of clips to iterator over
>>>> + *
>>>> + * While using drm_atomic_helper_damage_iter the type of clip coordinates caller
>>>> + * is interested in.
>>>> + */
>>>> +enum drm_atomic_helper_damage_clip_type {
>>>> +	DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_FB       = 0x0,
>>>> +	DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE    = 0x1,
>>>> +	DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC     = 0x2,
>>> I'm confused with what exactly these different types of iterators are
>>> supposed to achieve. TYPE_FB makes sense, that's what vmwgfx and other
>>> virtual drivers need to figure out which parts of a buffer to upload to
>>> the host.
>>>
>>> TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>>>
>>> TYPE_CRTC is what I'd want to use for manual upload hw, were instead of
>>> compositing the entire screen we can limit the uploaded area to 1 or 2
>>> rectangles (depending upon how the hw works). But those drivers want all
>>> the crtc clip rects for _all_ the planes combined, not for each plane
>>> individually.
>>>
>>> My suggestion is to drop TYPE_CRTC until someone needs it for a driver.
>>> And most likely the only iterator we want for TYPE_CRTC is one that gives
>>> you the overall damage area, including alpha/ctm/gamma/everything else,
>>> coalesced into just 1 clip rect. So probably an entirely different
>>> function.
>> Actually for vmwgfx, the display part of the hardware comes in different
>> versions and we'd typically expect both the FB coordinates and the CRTC
>> coordinates. In the latest version the surface backing the screen needs to
>> exactly fit the CRTC scanout area, and if the FB doesn't do that, we need to
>> create a surface that does and copy in the kernel driver. After that, as a
>> second step, we need to notify the display system of the damaged area in
>> CRTC coordinates.
> Hm, that's an unexpected use-case. I'm leaning towards just having a fb
> coordination iterator and adjusting the static offset in vmwgfx code. Your
> use-case is rather different from manual upload screens I think, so having
> a CRTC damage iterator which doesn't actually do what most other drivers
> want will be confusing.
>
> And since it's just a static drm_rect_translate for you it'll be only 1
> additional line in vmwgfx. Imo that's worth the safer/cleaner semantics
> for the core helpers.

Sounds reasonable. We could have an fb coord iterator only, and do the
translation to crtc coordinates outside. But I still think we should do 
the plane clipping in
the iterator, if needed with a big WARN_ON for transformed planes.

/Thomas

Powered by blists - more mailing lists