Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:59:57PM +, Deepak Singh Rawat wrote:
> > plane damage.
> > 
> > On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> > >
> > > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
> > 
> > I'm assuming CRTC plane coordinates here. They are used for uploading
> > contents of hardware planes. Like, in the simplest case, cursor images.
> 
> Yes they are CRTC plane coordinates, so is TYPE_PLANE naming confusing ?
> And should be named to TYPE_CRTC_PLANE but then it is confusing with
> TYPE_CRTC.

Yeah, I think TYPE_PLANE is really confusing, and too much aimied at your
vmwgfx special case (where the virtual hw requires that this all lines up
properly). I think providing FB coordinates, and doing the vmwgfx-specific
remapping in vmwgfx code is better.

And someone else can then figure out how to handle CRTC overall damage for
physical devices. As mentioned by me (and Rob Clark too), most hw only
allows for 1 (or maybe 2) overall damage rects, so that helper would need
to combine all the damge rects into 1. Plus take stuff like
gamma/ctm/alpha into account too. Better we leave that to someone who
needs it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Deepak Singh Rawat]

> plane damage.
> 
> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> >
> > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
> 
> I'm assuming CRTC plane coordinates here. They are used for uploading
> contents of hardware planes. Like, in the simplest case, cursor images.

Yes they are CRTC plane coordinates, so is TYPE_PLANE naming confusing ?
And should be named to TYPE_CRTC_PLANE but then it is confusing with
TYPE_CRTC.

> 
> /Thomas

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Deepak Singh Rawat]

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

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.

> 
> > +   }
> > +
> > +   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(>plane_src, >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(>crtc_src, -iter->translate_crtc_x,
> > +  

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Sinclair Yeh]

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 
> ---
>  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;
> + }
> +
> + 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(>plane_src, >fb_src);
> +
> + 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);
   ^
I believe you mean translate_crtc_y here


> +
> + /* Clip crtc src rect to plane dimensions */
> + drm_rect_translate(>crtc_src, -iter->translate_crtc_x,
> +-iter->translate_crtc_x);
Also here^


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Daniel Vetter]

On Thu, Apr 05, 2018 at 10:51:42AM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:52 AM, Daniel Vetter wrote:
> > 
> > TYPE_PLANE I have no idea who needs that. I suggest we just drop it.
> 
> I'm assuming CRTC plane coordinates here. They are used for uploading
> contents of hardware planes. Like, in the simplest case, cursor images.

Hm, I guess I'd need to see how it's being used, since I'm neither
following the intent of the original code nor your explanation here I
think ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Daniel Vetter]

On Thu, Apr 05, 2018 at 01:51:49PM +0200, Thomas Hellstrom wrote:
> 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 
> > > > 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 >> 

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Thomas Hellstrom]

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 

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(>plane_src, >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(>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 

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Daniel Vetter]

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 
> > 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(>plane_src, >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 

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Thomas Hellstrom]

On 04/05/2018 09:52 AM, Daniel Vetter wrote:


TYPE_PLANE I have no idea who needs that. I suggest we just drop it.


I'm assuming CRTC plane coordinates here. They are used for uploading 
contents of hardware planes. Like, in the simplest case, cursor images.


/Thomas

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Thomas Hellstrom]

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 

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(>plane_src, >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(>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.




+   drm_rect_intersect(>crtc_src, >plane_src);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
+
+/**
+ * drm_atomic_helper_damage_iter_next - advance the damage iterator
+ * @iter: The 

Re: [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Daniel Vetter]

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 

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(>plane_src, >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(>crtc_src, -iter->translate_crtc_x,
> +-iter->translate_crtc_x);

We can also scale.

> + drm_rect_intersect(>crtc_src, >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 

[RFC 2/3] drm: Add helper iterator functions to iterate over plane damage.

[Deepak Rawat]

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 
---
 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;
+   }
+
+   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(>plane_src, >fb_src);
+
+   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(>crtc_src, -iter->translate_crtc_x,
+  -iter->translate_crtc_x);
+   drm_rect_intersect(>crtc_src, >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 = >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, >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, >plane_src))
+   goto next_clip;
+   else if (iter->type &