Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Deepak Singh Rawat <drawat@vmware.com>
To:     Daniel Vetter <daniel@ffwll.ch>
CC:     "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Thomas Hellstrom <thellstrom@vmware.com>,
        Sinclair Yeh <syeh@vmware.com>,
        linux-graphics-maintainer <linux-graphics-maintainer@vmware.com>,
        "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
        "lukasz.spintzyk@displaylink.com" <lukasz.spintzyk@displaylink.com>,
        "noralf@tronnes.org" <noralf@tronnes.org>,
        "robdclark@gmail.com" <robdclark@gmail.com>,
        "gustavo@padovan.org" <gustavo@padovan.org>,
        "maarten.lankhorst@linux.intel.com" 
        <maarten.lankhorst@linux.intel.com>,
        "seanpaul@chromium.org" <seanpaul@chromium.org>,
        "airlied@linux.ie" <airlied@linux.ie>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC 2/3] drm: Add helper iterator functions to iterate over
 plane damage.
Thread-Topic: [RFC 2/3] drm: Add helper iterator functions to iterate over
 plane damage.
Thread-Index: AQHTzG/QxRDvLNOwKUiQ/1JAGDt4gKPxzRuAgAEAjMA=
Date:   Thu, 5 Apr 2018 23:19:29 +0000
Message-ID: <MWHPR05MB311740B4ED9BD6E7A51739C3BABB0@MWHPR05MB3117.namprd05.prod.outlook.com>
References: <1522885748-67122-1-git-send-email-drawat@vmware.com>
 <1522885748-67122-3-git-send-email-drawat@vmware.com>
 <20180405075205.GQ3881@phenom.ffwll.local>
In-Reply-To: <20180405075205.GQ3881@phenom.ffwll.local>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c716d0cc-d554-49a6-8b8b-08d59b4bae80
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Apr 2018 23:19:29.2743
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR05MB3597
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>=20
> On Wed, Apr 04, 2018 at 04:49:07PM -0700, Deepak Rawat wrote:
> > With damage property in drm_plane_state, this patch adds helper iterato=
r
> > to traverse the damage clips. Iterator will return the damage rectangle=
s
> > in framebuffer, plane or crtc coordinates as need by driver
> > implementation.
> >
> > Signed-off-by: Deepak Rawat <drawat@vmware.com>
>=20
> I'd really like selftest/unittests for this stuff. There's an awful lot o=
f
> 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.
>=20
> 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.
>=20
> > ---
> >  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 =3D (struct drm_rect *)state->damage_clips->data;
> > +	iter->num_clips =3D state->num_clips;
> > +	iter->type =3D type;
> > +
> > +	/*
> > +	 * Full update in case of scaling or rotation. In future support for
> > +	 * scaling/rotating damage clips can be added
> > +	 */
> > +	if (state->crtc_w !=3D (state->src_w >> 16) ||
> > +	    state->crtc_h !=3D state->src_h >> 16 || state->rotation !=3D 0) =
{
> > +		iter->curr_clip =3D iter->num_clips;
> > +		return 0;
>=20
> 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.
>=20
> Also, I think we should provide a full clip for the case of num_clips =3D=
=3D
> 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=
.

The notion was if iterator does not provide any clip rect then driver need =
a
full update but yes I will work on providing a full clip here.

>=20
> > +	}
> > +
> > +	iter->fb_src.x1 =3D 0;
> > +	iter->fb_src.y1 =3D 0;
> > +	iter->fb_src.x2 =3D state->fb->width;
> > +	iter->fb_src.y2 =3D state->fb->height;
> > +
> > +	iter->plane_src.x1 =3D state->src_x >> 16;
> > +	iter->plane_src.y1 =3D state->src_y >> 16;
> > +	iter->plane_src.x2 =3D iter->plane_src.x1 + (state->src_w >> 16);
> > +	iter->plane_src.y2 =3D iter->plane_src.y1 + (state->src_h >> 16);
> > +	iter->translate_plane_x =3D -iter->plane_src.x1;
> > +	iter->translate_plane_y =3D -iter->plane_src.y1;
> > +
> > +	/* Clip plane src rect to fb dimensions */
> > +	drm_rect_intersect(&iter->plane_src, &iter->fb_src);
>=20
> 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.
>=20
> > +
> > +	iter->crtc_src.x1 =3D 0;
> > +	iter->crtc_src.y1 =3D 0;
> > +	iter->crtc_src.x2 =3D hdisplay;
> > +	iter->crtc_src.y2 =3D vdisplay;
> > +	iter->translate_crtc_x =3D -(iter->plane_src.x1 - state->crtc_x);
> > +	iter->translate_crtc_x =3D -(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);
>=20
> We can also scale.
>=20
> > +	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 i=
nit.
> > + *
> > + * Returns:  true if the output is valid, false if we've reached the e=
nd of
> the
> > + * rectangle list. If the first call return false, means need full upd=
ate.
> > + */
> > +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 >=3D iter->num_clips)
> > +		return false;
> > +
> > +	curr_clip =3D &iter->clips[iter->curr_clip];
> > +	iter->curr_clip++;
> > +
> > +	rect->x1 =3D curr_clip->x1;
> > +	rect->x2 =3D curr_clip->x2;
> > +	rect->y1 =3D curr_clip->y1;
> > +	rect->y2 =3D 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       =3D 0x0,
> > +	DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_PLANE    =3D 0x1,
> > +	DRM_ATOMIC_HELPER_DAMAGE_CLIP_TYPE_CRTC     =3D 0x2,
>=20
> 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.
>=20
> TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
>=20
> 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.
>=20
> 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.
>=20
> Summarizing all this, I'd simplify the iterator to:
>=20
> drm_atomic_helper_damage_iter_init(iter, plane_state);
>=20
> And leave the other 2 use-cases to the next patch series. For crtc damage
> we probably want:
>=20
> drm_atomic_helper_crtc_damage(drm_atomic_state, rect)
>=20
> Which internally loops over all planes and also takes all the other state
> changes into account. That way you also don't have to fix the scaling
> issue, since your current code only handles translation.
>=20
> Another bit: drm_atomic_helper.c is huge, I'd vote to put all this stuff
> into a new drm_damage_helper.[hc], including new section in drm-kms.rst
> and all that. Splitting up drm_atomic_helper.[hc] is somewhere on my todo

Agreed with the conclusion with inputs from other email.

> ...
>=20
> Cheers, Daniel
>=20
> > +};
> > +
> > +/**
> > + * struct drm_atomic_helper_damage_iter - damage clip iterator
> > + *
> > + * This iterator tracks state needed to walk the list of damage clips.
> > + */
> > +struct drm_atomic_helper_damage_iter {
> > +	enum drm_atomic_helper_damage_clip_type type;
> > +	const struct drm_rect *clips;
> > +	uint32_t num_clips;
> > +	uint32_t curr_clip;
> > +	struct drm_rect fb_src;
> > +	int translate_plane_x;
> > +	int translate_plane_y;
> > +	struct drm_rect plane_src;
> > +	int translate_crtc_x;
> > +	int translate_crtc_y;
> > +	struct drm_rect crtc_src;
> > +};
> > +
> >  int drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  				struct drm_atomic_state *state);
> >  int drm_atomic_helper_check_plane_state(struct drm_plane_state
> *plane_state,
> > @@ -185,6 +216,14 @@ int drm_atomic_helper_legacy_gamma_set(struct
> drm_crtc *crtc,
> >  				       struct drm_modeset_acquire_ctx *ctx);
> >  void __drm_atomic_helper_private_obj_duplicate_state(struct
> drm_private_obj *obj,
> >  						     struct drm_private_state
> *state);
> > +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);
> > +bool
> > +drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> > +				   struct drm_rect *rect);
> >
> >  /**
> >   * drm_atomic_crtc_for_each_plane - iterate over planes currently
> attached to CRTC
> > --
> > 2.7.4
> >
>=20
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=3Dhttp-
> 3A__blog.ffwll.ch&d=3DDwIBAg&c=3DuilaK90D4TOVoH58JNXRgQ&r=3DzOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=3DOWr46Afx4MYOgDehJbODL7IzBsDEeoGiJn
> okZtIh2Qc&s=3DBH7dN6UEpjEaMKYHooi2AKk-zLYHXl5F7YT7j55qWO8&e=3D