Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Lukasz Spintzyk]

On 05/04/2018 01:49, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 
---
  drivers/gpu/drm/drm_atomic.c| 42 +
  drivers/gpu/drm/drm_atomic_helper.c |  4 
  drivers/gpu/drm/drm_mode_config.c   |  5 +
  drivers/gpu/drm/drm_plane.c | 12 +++
  include/drm/drm_mode_config.h   | 15 +
  include/drm/drm_plane.h | 16 ++
  include/uapi/drm/drm_mode.h | 15 +
  7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
  }
  
  /**

+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
   * drm_atomic_get_plane_state - get plane state
   * @state: global atomic state object
   * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);
+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
*val = state->color_range;
+   } else if (property == config->prop_damage_clips) {
+   *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c356545..55b44e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
  
  	state->fence = NULL;

state->commit = NULL;
+   state->damage_clips = NULL;
+   state->num_clips = 0;
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
  
@@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
  
  	if (state->commit)

drm_crtc_commit_put(state->commit);
+
+   drm_property_blob_put(state->damage_clips);
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
  
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c

index e5c6533..e93b127 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Lukasz Spintzyk]

On 05/04/2018 01:49, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 
---
  drivers/gpu/drm/drm_atomic.c| 42 +
  drivers/gpu/drm/drm_atomic_helper.c |  4 
  drivers/gpu/drm/drm_mode_config.c   |  5 +
  drivers/gpu/drm/drm_plane.c | 12 +++
  include/drm/drm_mode_config.h   | 15 +
  include/drm/drm_plane.h | 16 ++
  include/uapi/drm/drm_mode.h | 15 +
  7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
  }
  
  /**

+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
   * drm_atomic_get_plane_state - get plane state
   * @state: global atomic state object
   * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);
+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
*val = state->color_range;
+   } else if (property == config->prop_damage_clips) {
+   *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c356545..55b44e3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
  
  	state->fence = NULL;

state->commit = NULL;
+   state->damage_clips = NULL;
+   state->num_clips = 0;
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
  
@@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
  
  	if (state->commit)

drm_crtc_commit_put(state->commit);
+
+   drm_property_blob_put(state->damage_clips);
  }
  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
  
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c

index e5c6533..e93b127 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struct 
drm_device *dev)
  

RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Deepak Singh Rawat]

> > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > > +{
> > > > +   struct drm_device *dev = plane->dev;
> > > > +   struct drm_mode_config *config = >mode_config;
> > > > +
> > > > +   drm_object_attach_property(>base, config-
> > > >prop_damage_clips, 0);
> > > > +}
> > > > diff --git a/include/drm/drm_mode_config.h
> > > b/include/drm/drm_mode_config.h
> > > > index 7569f22..d8767da 100644
> > > > --- a/include/drm/drm_mode_config.h
> > > > +++ b/include/drm/drm_mode_config.h
> > > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > >  */
> > > > struct drm_property *prop_crtc_id;
> > > > /**
> > > > +* @prop_damage_clips: Optional plane property to mark damaged
> > > regions
> > > > +* on the plane in framebuffer coordinates of the framebuffer
> > > attached
> > > > +* to the plane.
> > >
> > > Why should we make this optional? Looks like just another thing drivers
> > > might screw up, since we have multiple callbacks and things to set up for
> > > proper dirty tracking.
> >
> > Thanks Daniel for the review.
> >
> > I think not all compositor will be interested in sending damage, that was 
> > the
> > reason to make this optional. Also when damage is not set that means
> > user-space need full update just like eglSwapBuffersWithDamageKHR.
> >
> > I will add better documentation.
> 
> I think if we also handle this case in the helper that'd be even better:
> In the case of no damage, the helper/core code could automatically supply
> a damage rect for the entire buffer. That way drivers don't have to handle
> this case specially.
> -Daniel

Agreed.

> --

RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Deepak Singh Rawat]

> > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> > > > +{
> > > > +   struct drm_device *dev = plane->dev;
> > > > +   struct drm_mode_config *config = >mode_config;
> > > > +
> > > > +   drm_object_attach_property(>base, config-
> > > >prop_damage_clips, 0);
> > > > +}
> > > > diff --git a/include/drm/drm_mode_config.h
> > > b/include/drm/drm_mode_config.h
> > > > index 7569f22..d8767da 100644
> > > > --- a/include/drm/drm_mode_config.h
> > > > +++ b/include/drm/drm_mode_config.h
> > > > @@ -628,6 +628,21 @@ struct drm_mode_config {
> > > >  */
> > > > struct drm_property *prop_crtc_id;
> > > > /**
> > > > +* @prop_damage_clips: Optional plane property to mark damaged
> > > regions
> > > > +* on the plane in framebuffer coordinates of the framebuffer
> > > attached
> > > > +* to the plane.
> > >
> > > Why should we make this optional? Looks like just another thing drivers
> > > might screw up, since we have multiple callbacks and things to set up for
> > > proper dirty tracking.
> >
> > Thanks Daniel for the review.
> >
> > I think not all compositor will be interested in sending damage, that was 
> > the
> > reason to make this optional. Also when damage is not set that means
> > user-space need full update just like eglSwapBuffersWithDamageKHR.
> >
> > I will add better documentation.
> 
> I think if we also handle this case in the helper that'd be even better:
> In the case of no damage, the helper/core code could automatically supply
> a damage rect for the entire buffer. That way drivers don't have to handle
> this case specially.
> -Daniel

Agreed.

> --

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:07:19PM +, Deepak Singh Rawat wrote:
> > 
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > From: Lukasz Spintzyk 
> > >
> > > Optional plane property to mark damaged regions on the plane in
> > > framebuffer coordinates of the framebuffer attached to the plane.
> > >
> > > The layout of blob data is simply an array of drm_mode_rect with
> > maximum
> > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > coordinates, damage clips are not in 16.16 fixed point.
> > >
> > > Damage clips are a hint to kernel as which area of framebuffer has
> > > changed since last page-flip. This should be helpful for some drivers
> > > especially for virtual devices where each framebuffer change needs to
> > > be transmitted over network, usb, etc.
> > >
> > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > plane should enable this property using drm_plane_enable_damage_clips.
> > >
> > > Signed-off-by: Lukasz Spintzyk 
> > > Signed-off-by: Deepak Rawat 
> > 
> > The property uapi section is missing, see:
> > 
> > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> > 2Dcomposition-
> > 2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=zOOG28inJK0762
> > SxAf-
> > cyZdStnd2NQpRu98lJP2iYGw=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> > z_3vlEC9Q=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY=
> > 
> > Plane composition feels like the best place to put this. Please use that
> > section to tie all the various bits together, including the helpers you're
> > adding in the following patches for drivers to use.
> > 
> > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > fb coordinate damage rects.
> > 
> > Like you say, the thing needed here now is userspace + driver actually
> > implementing this. I'd also say the compat helper to map the legacy
> > fb->dirty to this new atomic way of doing things should be included here
> > (gives us a lot more testing for these new paths).
> > 
> > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > rects correctly.
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c| 42
> > +
> > >  drivers/gpu/drm/drm_atomic_helper.c |  4 
> > >  drivers/gpu/drm/drm_mode_config.c   |  5 +
> > >  drivers/gpu/drm/drm_plane.c | 12 +++
> > >  include/drm/drm_mode_config.h   | 15 +
> > >  include/drm/drm_plane.h | 16 ++
> > >  include/uapi/drm/drm_mode.h | 15 +
> > >  7 files changed, 109 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..9226d24 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> > drm_printer *p,
> > >  }
> > >
> > >  /**
> > > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> > to plane
> > > + * @state: plane state
> > > + * @blob: damage clips in framebuffer coordinates
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, error code on failure.
> > > + */
> > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> > *state,
> > > +struct drm_property_blob *blob)
> > > +{
> > > + if (blob == state->damage_clips)
> > > + return 0;
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > + state->damage_clips = NULL;
> > > +
> > > + if (blob) {
> > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > +
> > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > + return -EINVAL;
> > > +
> > > + state->damage_clips = drm_property_blob_get(blob);
> > > + state->num_clips = count;
> > > + } else {
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > >   * drm_atomic_get_plane_state - get plane state
> > >   * @state: global atomic state object
> > >   * @plane: plane to get state object for
> > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> > drm_plane *plane,
> > >   state->color_encoding = val;
> > >   } else if (property == plane->color_range_property) {
> > >   state->color_range = val;
> > > + } else if (property == config->prop_damage_clips) {
> > > + struct drm_property_blob *blob =
> > > + drm_property_lookup_blob(dev, val);
> > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> > 
> > There's already a helper with size-checking built-in, see
> > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> > I'd
> > just provide a little inline helper that 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:07:19PM +, Deepak Singh Rawat wrote:
> > 
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > From: Lukasz Spintzyk 
> > >
> > > Optional plane property to mark damaged regions on the plane in
> > > framebuffer coordinates of the framebuffer attached to the plane.
> > >
> > > The layout of blob data is simply an array of drm_mode_rect with
> > maximum
> > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > coordinates, damage clips are not in 16.16 fixed point.
> > >
> > > Damage clips are a hint to kernel as which area of framebuffer has
> > > changed since last page-flip. This should be helpful for some drivers
> > > especially for virtual devices where each framebuffer change needs to
> > > be transmitted over network, usb, etc.
> > >
> > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > plane should enable this property using drm_plane_enable_damage_clips.
> > >
> > > Signed-off-by: Lukasz Spintzyk 
> > > Signed-off-by: Deepak Rawat 
> > 
> > The property uapi section is missing, see:
> > 
> > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> > 2Dcomposition-
> > 2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=zOOG28inJK0762
> > SxAf-
> > cyZdStnd2NQpRu98lJP2iYGw=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> > z_3vlEC9Q=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY=
> > 
> > Plane composition feels like the best place to put this. Please use that
> > section to tie all the various bits together, including the helpers you're
> > adding in the following patches for drivers to use.
> > 
> > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > fb coordinate damage rects.
> > 
> > Like you say, the thing needed here now is userspace + driver actually
> > implementing this. I'd also say the compat helper to map the legacy
> > fb->dirty to this new atomic way of doing things should be included here
> > (gives us a lot more testing for these new paths).
> > 
> > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > rects correctly.
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c| 42
> > +
> > >  drivers/gpu/drm/drm_atomic_helper.c |  4 
> > >  drivers/gpu/drm/drm_mode_config.c   |  5 +
> > >  drivers/gpu/drm/drm_plane.c | 12 +++
> > >  include/drm/drm_mode_config.h   | 15 +
> > >  include/drm/drm_plane.h | 16 ++
> > >  include/uapi/drm/drm_mode.h | 15 +
> > >  7 files changed, 109 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..9226d24 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> > drm_printer *p,
> > >  }
> > >
> > >  /**
> > > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> > to plane
> > > + * @state: plane state
> > > + * @blob: damage clips in framebuffer coordinates
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, error code on failure.
> > > + */
> > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> > *state,
> > > +struct drm_property_blob *blob)
> > > +{
> > > + if (blob == state->damage_clips)
> > > + return 0;
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > + state->damage_clips = NULL;
> > > +
> > > + if (blob) {
> > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > +
> > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > + return -EINVAL;
> > > +
> > > + state->damage_clips = drm_property_blob_get(blob);
> > > + state->num_clips = count;
> > > + } else {
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > >   * drm_atomic_get_plane_state - get plane state
> > >   * @state: global atomic state object
> > >   * @plane: plane to get state object for
> > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> > drm_plane *plane,
> > >   state->color_encoding = val;
> > >   } else if (property == plane->color_range_property) {
> > >   state->color_range = val;
> > > + } else if (property == config->prop_damage_clips) {
> > > + struct drm_property_blob *blob =
> > > + drm_property_lookup_blob(dev, val);
> > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> > 
> > There's already a helper with size-checking built-in, see
> > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> > I'd
> > just provide a little inline helper that does the
> > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> > 

RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Deepak Singh Rawat]

> 
> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > From: Lukasz Spintzyk 
> >
> > Optional plane property to mark damaged regions on the plane in
> > framebuffer coordinates of the framebuffer attached to the plane.
> >
> > The layout of blob data is simply an array of drm_mode_rect with
> maximum
> > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > coordinates, damage clips are not in 16.16 fixed point.
> >
> > Damage clips are a hint to kernel as which area of framebuffer has
> > changed since last page-flip. This should be helpful for some drivers
> > especially for virtual devices where each framebuffer change needs to
> > be transmitted over network, usb, etc.
> >
> > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > plane should enable this property using drm_plane_enable_damage_clips.
> >
> > Signed-off-by: Lukasz Spintzyk 
> > Signed-off-by: Deepak Rawat 
> 
> The property uapi section is missing, see:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> 2Dcomposition-
> 2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=zOOG28inJK0762
> SxAf-
> cyZdStnd2NQpRu98lJP2iYGw=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> z_3vlEC9Q=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY=
> 
> Plane composition feels like the best place to put this. Please use that
> section to tie all the various bits together, including the helpers you're
> adding in the following patches for drivers to use.
> 
> Bunch of nitpicks below, but overall I'm agreeing now with just going with
> fb coordinate damage rects.
> 
> Like you say, the thing needed here now is userspace + driver actually
> implementing this. I'd also say the compat helper to map the legacy
> fb->dirty to this new atomic way of doing things should be included here
> (gives us a lot more testing for these new paths).
> 
> Icing on the cake would be an igt to make sure kernel rejects invalid clip
> rects correctly.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 42
> +
> >  drivers/gpu/drm/drm_atomic_helper.c |  4 
> >  drivers/gpu/drm/drm_mode_config.c   |  5 +
> >  drivers/gpu/drm/drm_plane.c | 12 +++
> >  include/drm/drm_mode_config.h   | 15 +
> >  include/drm/drm_plane.h | 16 ++
> >  include/uapi/drm/drm_mode.h | 15 +
> >  7 files changed, 109 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42..9226d24 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> >  }
> >
> >  /**
> > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> to plane
> > + * @state: plane state
> > + * @blob: damage clips in framebuffer coordinates
> > + *
> > + * Returns:
> > + *
> > + * Zero on success, error code on failure.
> > + */
> > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> *state,
> > +  struct drm_property_blob *blob)
> > +{
> > +   if (blob == state->damage_clips)
> > +   return 0;
> > +
> > +   drm_property_blob_put(state->damage_clips);
> > +   state->damage_clips = NULL;
> > +
> > +   if (blob) {
> > +   uint32_t count = blob->length/sizeof(struct drm_rect);
> > +
> > +   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > +   return -EINVAL;
> > +
> > +   state->damage_clips = drm_property_blob_get(blob);
> > +   state->num_clips = count;
> > +   } else {
> > +   state->damage_clips = NULL;
> > +   state->num_clips = 0;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +/**
> >   * drm_atomic_get_plane_state - get plane state
> >   * @state: global atomic state object
> >   * @plane: plane to get state object for
> > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > state->color_encoding = val;
> > } else if (property == plane->color_range_property) {
> > state->color_range = val;
> > +   } else if (property == config->prop_damage_clips) {
> > +   struct drm_property_blob *blob =
> > +   drm_property_lookup_blob(dev, val);
> > +   int ret = drm_atomic_set_damage_for_plane(state, blob);
> 
> There's already a helper with size-checking built-in, see
> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> I'd
> just provide a little inline helper that does the
> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> 
> > +   drm_property_blob_put(blob);
> > +   return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return 

RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Deepak Singh Rawat]

> 
> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > From: Lukasz Spintzyk 
> >
> > Optional plane property to mark damaged regions on the plane in
> > framebuffer coordinates of the framebuffer attached to the plane.
> >
> > The layout of blob data is simply an array of drm_mode_rect with
> maximum
> > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > coordinates, damage clips are not in 16.16 fixed point.
> >
> > Damage clips are a hint to kernel as which area of framebuffer has
> > changed since last page-flip. This should be helpful for some drivers
> > especially for virtual devices where each framebuffer change needs to
> > be transmitted over network, usb, etc.
> >
> > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > plane should enable this property using drm_plane_enable_damage_clips.
> >
> > Signed-off-by: Lukasz Spintzyk 
> > Signed-off-by: Deepak Rawat 
> 
> The property uapi section is missing, see:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-
> 2Dcomposition-
> 2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=zOOG28inJK0762
> SxAf-
> cyZdStnd2NQpRu98lJP2iYGw=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q
> z_3vlEC9Q=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY=
> 
> Plane composition feels like the best place to put this. Please use that
> section to tie all the various bits together, including the helpers you're
> adding in the following patches for drivers to use.
> 
> Bunch of nitpicks below, but overall I'm agreeing now with just going with
> fb coordinate damage rects.
> 
> Like you say, the thing needed here now is userspace + driver actually
> implementing this. I'd also say the compat helper to map the legacy
> fb->dirty to this new atomic way of doing things should be included here
> (gives us a lot more testing for these new paths).
> 
> Icing on the cake would be an igt to make sure kernel rejects invalid clip
> rects correctly.
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 42
> +
> >  drivers/gpu/drm/drm_atomic_helper.c |  4 
> >  drivers/gpu/drm/drm_mode_config.c   |  5 +
> >  drivers/gpu/drm/drm_plane.c | 12 +++
> >  include/drm/drm_mode_config.h   | 15 +
> >  include/drm/drm_plane.h | 16 ++
> >  include/uapi/drm/drm_mode.h | 15 +
> >  7 files changed, 109 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42..9226d24 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct
> drm_printer *p,
> >  }
> >
> >  /**
> > + * drm_atomic_set_damage_for_plane - sets the damage clips property
> to plane
> > + * @state: plane state
> > + * @blob: damage clips in framebuffer coordinates
> > + *
> > + * Returns:
> > + *
> > + * Zero on success, error code on failure.
> > + */
> > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state
> *state,
> > +  struct drm_property_blob *blob)
> > +{
> > +   if (blob == state->damage_clips)
> > +   return 0;
> > +
> > +   drm_property_blob_put(state->damage_clips);
> > +   state->damage_clips = NULL;
> > +
> > +   if (blob) {
> > +   uint32_t count = blob->length/sizeof(struct drm_rect);
> > +
> > +   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > +   return -EINVAL;
> > +
> > +   state->damage_clips = drm_property_blob_get(blob);
> > +   state->num_clips = count;
> > +   } else {
> > +   state->damage_clips = NULL;
> > +   state->num_clips = 0;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +/**
> >   * drm_atomic_get_plane_state - get plane state
> >   * @state: global atomic state object
> >   * @plane: plane to get state object for
> > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > state->color_encoding = val;
> > } else if (property == plane->color_range_property) {
> > state->color_range = val;
> > +   } else if (property == config->prop_damage_clips) {
> > +   struct drm_property_blob *blob =
> > +   drm_property_lookup_blob(dev, val);
> > +   int ret = drm_atomic_set_damage_for_plane(state, blob);
> 
> There's already a helper with size-checking built-in, see
> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips
> I'd
> just provide a little inline helper that does the
> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).
> 
> > +   drm_property_blob_put(blob);
> > +   return ret;
> > } else if (plane->funcs->atomic_set_property) {
> > return plane->funcs->atomic_set_property(plane, state,
> > property, val);
> > @@ -856,6 +896,8 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

On 04/05/2018 03:47 PM, Daniel Vetter wrote:

On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote:

On 04/05/2018 12:03 PM, Daniel Vetter wrote:

On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:

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

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.


---
drivers/gpu/drm/drm_atomic.c| 42 
+
drivers/gpu/drm/drm_atomic_helper.c |  4 
drivers/gpu/drm/drm_mode_config.c   |  5 +
drivers/gpu/drm/drm_plane.c | 12 +++
include/drm/drm_mode_config.h   | 15 +
include/drm/drm_plane.h | 16 ++
include/uapi/drm/drm_mode.h | 15 +
7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
}
/**
+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
 * drm_atomic_get_plane_state - get plane state
 * @state: global atomic state object
 * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).


+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

On 04/05/2018 03:47 PM, Daniel Vetter wrote:

On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote:

On 04/05/2018 12:03 PM, Daniel Vetter wrote:

On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:

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

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.


---
drivers/gpu/drm/drm_atomic.c| 42 
+
drivers/gpu/drm/drm_atomic_helper.c |  4 
drivers/gpu/drm/drm_mode_config.c   |  5 +
drivers/gpu/drm/drm_plane.c | 12 +++
include/drm/drm_mode_config.h   | 15 +
include/drm/drm_plane.h | 16 ++
include/uapi/drm/drm_mode.h | 15 +
7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
}
/**
+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
 * drm_atomic_get_plane_state - get plane state
 * @state: global atomic state object
 * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).


+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 01:42:11PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> > 
> > On the topic of input validation: Should the kernel check in shared code
> > that all the clip rects are sensible, i.e. within the dimensions of the
> > fb? Relying on drivers for that seems like a bad idea.
> 
> I guess we could do that.
> 
> There seems to be different needs for different kinds of iterators, but
> otherwise
> I was thinking that an iterator could just skip invalid rects if found. Then
> we
> don't need a separate validation step, but OTOH user-space won't get
> notified if
> that was intended.
> 
> I'm not 100% sure user-space would do anything sensible with any error
> information, though.

Fix userspace, and ime letting userspace get away with improper uapi usage
is a path full of regrets. That's why I'm advising that we check any
unused field for 0, and any unused bits also. Plus anything else that
userspace could get wrong. Because if it's not checked, someone will get
it wrong, and then we have to keep it working forever.
-Daniel

> 
> /Thomas
> 
> 
> > 
> > That could be done in core code in drm_atomic_plane_check().
> > -Daniel
> > > /Thomas
> > > 
> > > 
> > > ___
> > > dri-devel mailing list
> > > dri-de...@lists.freedesktop.org
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I=
> 
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 01:42:11PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> > 
> > On the topic of input validation: Should the kernel check in shared code
> > that all the clip rects are sensible, i.e. within the dimensions of the
> > fb? Relying on drivers for that seems like a bad idea.
> 
> I guess we could do that.
> 
> There seems to be different needs for different kinds of iterators, but
> otherwise
> I was thinking that an iterator could just skip invalid rects if found. Then
> we
> don't need a separate validation step, but OTOH user-space won't get
> notified if
> that was intended.
> 
> I'm not 100% sure user-space would do anything sensible with any error
> information, though.

Fix userspace, and ime letting userspace get away with improper uapi usage
is a path full of regrets. That's why I'm advising that we check any
unused field for 0, and any unused bits also. Plus anything else that
userspace could get wrong. Because if it's not checked, someone will get
it wrong, and then we have to keep it working forever.
-Daniel

> 
> /Thomas
> 
> 
> > 
> > That could be done in core code in drm_atomic_plane_check().
> > -Daniel
> > > /Thomas
> > > 
> > > 
> > > ___
> > > dri-devel mailing list
> > > dri-de...@lists.freedesktop.org
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I=
> 
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> > On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
> > > On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> > > > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > > > From: Lukasz Spintzyk 
> > > > > 
> > > > > Optional plane property to mark damaged regions on the plane in
> > > > > framebuffer coordinates of the framebuffer attached to the plane.
> > > > > 
> > > > > The layout of blob data is simply an array of drm_mode_rect with 
> > > > > maximum
> > > > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > > > coordinates, damage clips are not in 16.16 fixed point.
> > > > > 
> > > > > Damage clips are a hint to kernel as which area of framebuffer has
> > > > > changed since last page-flip. This should be helpful for some drivers
> > > > > especially for virtual devices where each framebuffer change needs to
> > > > > be transmitted over network, usb, etc.
> > > > > 
> > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > > > plane should enable this property using drm_plane_enable_damage_clips.
> > > > > 
> > > > > Signed-off-by: Lukasz Spintzyk 
> > > > > Signed-off-by: Deepak Rawat 
> > > > The property uapi section is missing, see:
> > > > 
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=
> > > > 
> > > > Plane composition feels like the best place to put this. Please use that
> > > > section to tie all the various bits together, including the helpers 
> > > > you're
> > > > adding in the following patches for drivers to use.
> > > > 
> > > > Bunch of nitpicks below, but overall I'm agreeing now with just going 
> > > > with
> > > > fb coordinate damage rects.
> > > > 
> > > > Like you say, the thing needed here now is userspace + driver actually
> > > > implementing this. I'd also say the compat helper to map the legacy
> > > > fb->dirty to this new atomic way of doing things should be included here
> > > > (gives us a lot more testing for these new paths).
> > > > 
> > > > Icing on the cake would be an igt to make sure kernel rejects invalid 
> > > > clip
> > > > rects correctly.
> > > > 
> > > > > ---
> > > > >drivers/gpu/drm/drm_atomic.c| 42 
> > > > > +
> > > > >drivers/gpu/drm/drm_atomic_helper.c |  4 
> > > > >drivers/gpu/drm/drm_mode_config.c   |  5 +
> > > > >drivers/gpu/drm/drm_plane.c | 12 +++
> > > > >include/drm/drm_mode_config.h   | 15 +
> > > > >include/drm/drm_plane.h | 16 ++
> > > > >include/uapi/drm/drm_mode.h | 15 +
> > > > >7 files changed, 109 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_atomic.c 
> > > > > b/drivers/gpu/drm/drm_atomic.c
> > > > > index 7d25c42..9226d24 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct 
> > > > > drm_printer *p,
> > > > >}
> > > > >/**
> > > > > + * drm_atomic_set_damage_for_plane - sets the damage clips property 
> > > > > to plane
> > > > > + * @state: plane state
> > > > > + * @blob: damage clips in framebuffer coordinates
> > > > > + *
> > > > > + * Returns:
> > > > > + *
> > > > > + * Zero on success, error code on failure.
> > > > > + */
> > > > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state 
> > > > > *state,
> > > > > +struct drm_property_blob 
> > > > > *blob)
> > > > > +{
> > > > > + if (blob == state->damage_clips)
> > > > > + return 0;
> > > > > +
> > > > > + drm_property_blob_put(state->damage_clips);
> > > > > + state->damage_clips = NULL;
> > > > > +
> > > > > + if (blob) {
> > > > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > > > +
> > > > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + state->damage_clips = drm_property_blob_get(blob);
> > > > > + state->num_clips = count;
> > > > > + } else {
> > > > > + state->damage_clips = NULL;
> > > > > + state->num_clips = 0;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > * drm_atomic_get_plane_state - get plane state
> > > > > * @state: global atomic state object
> > > > > * @plane: plane to get state object for
> > > > > @@ 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 12:03 PM, Daniel Vetter wrote:
> > On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
> > > On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> > > > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > > > From: Lukasz Spintzyk 
> > > > > 
> > > > > Optional plane property to mark damaged regions on the plane in
> > > > > framebuffer coordinates of the framebuffer attached to the plane.
> > > > > 
> > > > > The layout of blob data is simply an array of drm_mode_rect with 
> > > > > maximum
> > > > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > > > coordinates, damage clips are not in 16.16 fixed point.
> > > > > 
> > > > > Damage clips are a hint to kernel as which area of framebuffer has
> > > > > changed since last page-flip. This should be helpful for some drivers
> > > > > especially for virtual devices where each framebuffer change needs to
> > > > > be transmitted over network, usb, etc.
> > > > > 
> > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > > > plane should enable this property using drm_plane_enable_damage_clips.
> > > > > 
> > > > > Signed-off-by: Lukasz Spintzyk 
> > > > > Signed-off-by: Deepak Rawat 
> > > > The property uapi section is missing, see:
> > > > 
> > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=
> > > > 
> > > > Plane composition feels like the best place to put this. Please use that
> > > > section to tie all the various bits together, including the helpers 
> > > > you're
> > > > adding in the following patches for drivers to use.
> > > > 
> > > > Bunch of nitpicks below, but overall I'm agreeing now with just going 
> > > > with
> > > > fb coordinate damage rects.
> > > > 
> > > > Like you say, the thing needed here now is userspace + driver actually
> > > > implementing this. I'd also say the compat helper to map the legacy
> > > > fb->dirty to this new atomic way of doing things should be included here
> > > > (gives us a lot more testing for these new paths).
> > > > 
> > > > Icing on the cake would be an igt to make sure kernel rejects invalid 
> > > > clip
> > > > rects correctly.
> > > > 
> > > > > ---
> > > > >drivers/gpu/drm/drm_atomic.c| 42 
> > > > > +
> > > > >drivers/gpu/drm/drm_atomic_helper.c |  4 
> > > > >drivers/gpu/drm/drm_mode_config.c   |  5 +
> > > > >drivers/gpu/drm/drm_plane.c | 12 +++
> > > > >include/drm/drm_mode_config.h   | 15 +
> > > > >include/drm/drm_plane.h | 16 ++
> > > > >include/uapi/drm/drm_mode.h | 15 +
> > > > >7 files changed, 109 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_atomic.c 
> > > > > b/drivers/gpu/drm/drm_atomic.c
> > > > > index 7d25c42..9226d24 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct 
> > > > > drm_printer *p,
> > > > >}
> > > > >/**
> > > > > + * drm_atomic_set_damage_for_plane - sets the damage clips property 
> > > > > to plane
> > > > > + * @state: plane state
> > > > > + * @blob: damage clips in framebuffer coordinates
> > > > > + *
> > > > > + * Returns:
> > > > > + *
> > > > > + * Zero on success, error code on failure.
> > > > > + */
> > > > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state 
> > > > > *state,
> > > > > +struct drm_property_blob 
> > > > > *blob)
> > > > > +{
> > > > > + if (blob == state->damage_clips)
> > > > > + return 0;
> > > > > +
> > > > > + drm_property_blob_put(state->damage_clips);
> > > > > + state->damage_clips = NULL;
> > > > > +
> > > > > + if (blob) {
> > > > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > > > +
> > > > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + state->damage_clips = drm_property_blob_get(blob);
> > > > > + state->num_clips = count;
> > > > > + } else {
> > > > > + state->damage_clips = NULL;
> > > > > + state->num_clips = 0;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > * drm_atomic_get_plane_state - get plane state
> > > > > * @state: global atomic state object
> > > > > * @plane: plane to get state object for
> > > > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct 
> > > > > 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

On 04/05/2018 12:03 PM, Daniel Vetter wrote:


On the topic of input validation: Should the kernel check in shared code
that all the clip rects are sensible, i.e. within the dimensions of the
fb? Relying on drivers for that seems like a bad idea.


I guess we could do that.

There seems to be different needs for different kinds of iterators, but 
otherwise
I was thinking that an iterator could just skip invalid rects if found. 
Then we
don't need a separate validation step, but OTOH user-space won't get 
notified if

that was intended.

I'm not 100% sure user-space would do anything sensible with any error 
information, though.


/Thomas




That could be done in core code in drm_atomic_plane_check().
-Daniel

/Thomas


___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I=

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

On 04/05/2018 12:03 PM, Daniel Vetter wrote:


On the topic of input validation: Should the kernel check in shared code
that all the clip rects are sensible, i.e. within the dimensions of the
fb? Relying on drivers for that seems like a bad idea.


I guess we could do that.

There seems to be different needs for different kinds of iterators, but 
otherwise
I was thinking that an iterator could just skip invalid rects if found. 
Then we
don't need a separate validation step, but OTOH user-space won't get 
notified if

that was intended.

I'm not 100% sure user-space would do anything sensible with any error 
information, though.


/Thomas




That could be done in core code in drm_atomic_plane_check().
-Daniel

/Thomas


___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I=

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

On 04/05/2018 12:03 PM, Daniel Vetter wrote:

On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:

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

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.


---
   drivers/gpu/drm/drm_atomic.c| 42 
+
   drivers/gpu/drm/drm_atomic_helper.c |  4 
   drivers/gpu/drm/drm_mode_config.c   |  5 +
   drivers/gpu/drm/drm_plane.c | 12 +++
   include/drm/drm_mode_config.h   | 15 +
   include/drm/drm_plane.h | 16 ++
   include/uapi/drm/drm_mode.h | 15 +
   7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
   }
   /**
+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
* drm_atomic_get_plane_state - get plane state
* @state: global atomic state object
* @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).


+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

On 04/05/2018 12:03 PM, Daniel Vetter wrote:

On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:

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

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.


---
   drivers/gpu/drm/drm_atomic.c| 42 
+
   drivers/gpu/drm/drm_atomic_helper.c |  4 
   drivers/gpu/drm/drm_mode_config.c   |  5 +
   drivers/gpu/drm/drm_plane.c | 12 +++
   include/drm/drm_mode_config.h   | 15 +
   include/drm/drm_plane.h | 16 ++
   include/uapi/drm/drm_mode.h | 15 +
   7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
   }
   /**
+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
* drm_atomic_get_plane_state - get plane state
* @state: global atomic state object
* @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).


+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > From: Lukasz Spintzyk 
> > > 
> > > Optional plane property to mark damaged regions on the plane in
> > > framebuffer coordinates of the framebuffer attached to the plane.
> > > 
> > > The layout of blob data is simply an array of drm_mode_rect with maximum
> > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > coordinates, damage clips are not in 16.16 fixed point.
> > > 
> > > Damage clips are a hint to kernel as which area of framebuffer has
> > > changed since last page-flip. This should be helpful for some drivers
> > > especially for virtual devices where each framebuffer change needs to
> > > be transmitted over network, usb, etc.
> > > 
> > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > plane should enable this property using drm_plane_enable_damage_clips.
> > > 
> > > Signed-off-by: Lukasz Spintzyk 
> > > Signed-off-by: Deepak Rawat 
> > The property uapi section is missing, see:
> > 
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=
> > 
> > Plane composition feels like the best place to put this. Please use that
> > section to tie all the various bits together, including the helpers you're
> > adding in the following patches for drivers to use.
> > 
> > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > fb coordinate damage rects.
> > 
> > Like you say, the thing needed here now is userspace + driver actually
> > implementing this. I'd also say the compat helper to map the legacy
> > fb->dirty to this new atomic way of doing things should be included here
> > (gives us a lot more testing for these new paths).
> > 
> > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > rects correctly.
> > 
> > > ---
> > >   drivers/gpu/drm/drm_atomic.c| 42 
> > > +
> > >   drivers/gpu/drm/drm_atomic_helper.c |  4 
> > >   drivers/gpu/drm/drm_mode_config.c   |  5 +
> > >   drivers/gpu/drm/drm_plane.c | 12 +++
> > >   include/drm/drm_mode_config.h   | 15 +
> > >   include/drm/drm_plane.h | 16 ++
> > >   include/uapi/drm/drm_mode.h | 15 +
> > >   7 files changed, 109 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..9226d24 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct 
> > > drm_printer *p,
> > >   }
> > >   /**
> > > + * drm_atomic_set_damage_for_plane - sets the damage clips property to 
> > > plane
> > > + * @state: plane state
> > > + * @blob: damage clips in framebuffer coordinates
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, error code on failure.
> > > + */
> > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> > > +struct drm_property_blob *blob)
> > > +{
> > > + if (blob == state->damage_clips)
> > > + return 0;
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > + state->damage_clips = NULL;
> > > +
> > > + if (blob) {
> > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > +
> > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > + return -EINVAL;
> > > +
> > > + state->damage_clips = drm_property_blob_get(blob);
> > > + state->num_clips = count;
> > > + } else {
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > >* drm_atomic_get_plane_state - get plane state
> > >* @state: global atomic state object
> > >* @plane: plane to get state object for
> > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct 
> > > drm_plane *plane,
> > >   state->color_encoding = val;
> > >   } else if (property == plane->color_range_property) {
> > >   state->color_range = val;
> > > + } else if (property == config->prop_damage_clips) {
> > > + struct drm_property_blob *blob =
> > > + drm_property_lookup_blob(dev, val);
> > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> > There's already a helper with size-checking built-in, see
> > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
> > just 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote:
> On 04/05/2018 09:35 AM, Daniel Vetter wrote:
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > > From: Lukasz Spintzyk 
> > > 
> > > Optional plane property to mark damaged regions on the plane in
> > > framebuffer coordinates of the framebuffer attached to the plane.
> > > 
> > > The layout of blob data is simply an array of drm_mode_rect with maximum
> > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> > > coordinates, damage clips are not in 16.16 fixed point.
> > > 
> > > Damage clips are a hint to kernel as which area of framebuffer has
> > > changed since last page-flip. This should be helpful for some drivers
> > > especially for virtual devices where each framebuffer change needs to
> > > be transmitted over network, usb, etc.
> > > 
> > > Driver which are interested in enabling DAMAGE_CLIPS property for a
> > > plane should enable this property using drm_plane_enable_damage_clips.
> > > 
> > > Signed-off-by: Lukasz Spintzyk 
> > > Signed-off-by: Deepak Rawat 
> > The property uapi section is missing, see:
> > 
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=
> > 
> > Plane composition feels like the best place to put this. Please use that
> > section to tie all the various bits together, including the helpers you're
> > adding in the following patches for drivers to use.
> > 
> > Bunch of nitpicks below, but overall I'm agreeing now with just going with
> > fb coordinate damage rects.
> > 
> > Like you say, the thing needed here now is userspace + driver actually
> > implementing this. I'd also say the compat helper to map the legacy
> > fb->dirty to this new atomic way of doing things should be included here
> > (gives us a lot more testing for these new paths).
> > 
> > Icing on the cake would be an igt to make sure kernel rejects invalid clip
> > rects correctly.
> > 
> > > ---
> > >   drivers/gpu/drm/drm_atomic.c| 42 
> > > +
> > >   drivers/gpu/drm/drm_atomic_helper.c |  4 
> > >   drivers/gpu/drm/drm_mode_config.c   |  5 +
> > >   drivers/gpu/drm/drm_plane.c | 12 +++
> > >   include/drm/drm_mode_config.h   | 15 +
> > >   include/drm/drm_plane.h | 16 ++
> > >   include/uapi/drm/drm_mode.h | 15 +
> > >   7 files changed, 109 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42..9226d24 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct 
> > > drm_printer *p,
> > >   }
> > >   /**
> > > + * drm_atomic_set_damage_for_plane - sets the damage clips property to 
> > > plane
> > > + * @state: plane state
> > > + * @blob: damage clips in framebuffer coordinates
> > > + *
> > > + * Returns:
> > > + *
> > > + * Zero on success, error code on failure.
> > > + */
> > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> > > +struct drm_property_blob *blob)
> > > +{
> > > + if (blob == state->damage_clips)
> > > + return 0;
> > > +
> > > + drm_property_blob_put(state->damage_clips);
> > > + state->damage_clips = NULL;
> > > +
> > > + if (blob) {
> > > + uint32_t count = blob->length/sizeof(struct drm_rect);
> > > +
> > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> > > + return -EINVAL;
> > > +
> > > + state->damage_clips = drm_property_blob_get(blob);
> > > + state->num_clips = count;
> > > + } else {
> > > + state->damage_clips = NULL;
> > > + state->num_clips = 0;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > >* drm_atomic_get_plane_state - get plane state
> > >* @state: global atomic state object
> > >* @plane: plane to get state object for
> > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct 
> > > drm_plane *plane,
> > >   state->color_encoding = val;
> > >   } else if (property == plane->color_range_property) {
> > >   state->color_range = val;
> > > + } else if (property == config->prop_damage_clips) {
> > > + struct drm_property_blob *blob =
> > > + drm_property_lookup_blob(dev, val);
> > > + int ret = drm_atomic_set_damage_for_plane(state, blob);
> > There's already a helper with size-checking built-in, see
> > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
> > just provide a little inline helper that does the
> > blob->length/sizeof(drm_rect) conversion 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

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

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.


---
  drivers/gpu/drm/drm_atomic.c| 42 +
  drivers/gpu/drm/drm_atomic_helper.c |  4 
  drivers/gpu/drm/drm_mode_config.c   |  5 +
  drivers/gpu/drm/drm_plane.c | 12 +++
  include/drm/drm_mode_config.h   | 15 +
  include/drm/drm_plane.h | 16 ++
  include/uapi/drm/drm_mode.h | 15 +
  7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
  }
  
  /**

+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
   * drm_atomic_get_plane_state - get plane state
   * @state: global atomic state object
   * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).


+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Thomas Hellstrom]

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

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:

From: Lukasz Spintzyk 

Optional plane property to mark damaged regions on the plane in
framebuffer coordinates of the framebuffer attached to the plane.

The layout of blob data is simply an array of drm_mode_rect with maximum
array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
coordinates, damage clips are not in 16.16 fixed point.

Damage clips are a hint to kernel as which area of framebuffer has
changed since last page-flip. This should be helpful for some drivers
especially for virtual devices where each framebuffer change needs to
be transmitted over network, usb, etc.

Driver which are interested in enabling DAMAGE_CLIPS property for a
plane should enable this property using drm_plane_enable_damage_clips.

Signed-off-by: Lukasz Spintzyk 
Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties=DwIBAg=uilaK90D4TOVoH58JNXRgQ=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ=

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.


---
  drivers/gpu/drm/drm_atomic.c| 42 +
  drivers/gpu/drm/drm_atomic_helper.c |  4 
  drivers/gpu/drm/drm_mode_config.c   |  5 +
  drivers/gpu/drm/drm_plane.c | 12 +++
  include/drm/drm_mode_config.h   | 15 +
  include/drm/drm_plane.h | 16 ++
  include/uapi/drm/drm_mode.h | 15 +
  7 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..9226d24 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer 
*p,
  }
  
  /**

+ * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
+ * @state: plane state
+ * @blob: damage clips in framebuffer coordinates
+ *
+ * Returns:
+ *
+ * Zero on success, error code on failure.
+ */
+static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
+  struct drm_property_blob *blob)
+{
+   if (blob == state->damage_clips)
+   return 0;
+
+   drm_property_blob_put(state->damage_clips);
+   state->damage_clips = NULL;
+
+   if (blob) {
+   uint32_t count = blob->length/sizeof(struct drm_rect);
+
+   if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
+   return -EINVAL;
+
+   state->damage_clips = drm_property_blob_get(blob);
+   state->num_clips = count;
+   } else {
+   state->damage_clips = NULL;
+   state->num_clips = 0;
+   }
+
+   return 0;
+}
+
+/**
   * drm_atomic_get_plane_state - get plane state
   * @state: global atomic state object
   * @plane: plane to get state object for
@@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
state->color_encoding = val;
} else if (property == plane->color_range_property) {
state->color_range = val;
+   } else if (property == config->prop_damage_clips) {
+   struct drm_property_blob *blob =
+   drm_property_lookup_blob(dev, val);
+   int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).


+   drm_property_blob_put(blob);
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
*val = state->color_range;
+   } else if (property == 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> From: Lukasz Spintzyk 
> 
> Optional plane property to mark damaged regions on the plane in
> framebuffer coordinates of the framebuffer attached to the plane.
> 
> The layout of blob data is simply an array of drm_mode_rect with maximum
> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> coordinates, damage clips are not in 16.16 fixed point.
> 
> Damage clips are a hint to kernel as which area of framebuffer has
> changed since last page-flip. This should be helpful for some drivers
> especially for virtual devices where each framebuffer change needs to
> be transmitted over network, usb, etc.
> 
> Driver which are interested in enabling DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
> 
> Signed-off-by: Lukasz Spintzyk 
> Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.

> ---
>  drivers/gpu/drm/drm_atomic.c| 42 
> +
>  drivers/gpu/drm/drm_atomic_helper.c |  4 
>  drivers/gpu/drm/drm_mode_config.c   |  5 +
>  drivers/gpu/drm/drm_plane.c | 12 +++
>  include/drm/drm_mode_config.h   | 15 +
>  include/drm/drm_plane.h | 16 ++
>  include/uapi/drm/drm_mode.h | 15 +
>  7 files changed, 109 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..9226d24 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct 
> drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> + * @state: plane state
> + * @blob: damage clips in framebuffer coordinates
> + *
> + * Returns:
> + *
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> +struct drm_property_blob *blob)
> +{
> + if (blob == state->damage_clips)
> + return 0;
> +
> + drm_property_blob_put(state->damage_clips);
> + state->damage_clips = NULL;
> +
> + if (blob) {
> + uint32_t count = blob->length/sizeof(struct drm_rect);
> +
> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> + return -EINVAL;
> +
> + state->damage_clips = drm_property_blob_get(blob);
> + state->num_clips = count;
> + } else {
> + state->damage_clips = NULL;
> + state->num_clips = 0;
> + }
> +
> + return 0;
> +}
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct 
> drm_plane *plane,
>   state->color_encoding = val;
>   } else if (property == plane->color_range_property) {
>   state->color_range = val;
> + } else if (property == config->prop_damage_clips) {
> + struct drm_property_blob *blob =
> + drm_property_lookup_blob(dev, val);
> + int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).

> + drm_property_blob_put(blob);
> + return ret;
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->color_encoding;
>   } else if (property == plane->color_range_property) {
>   *val = state->color_range;
> + } else if (property == config->prop_damage_clips) {
> + *val = (state->damage_clips) ? 

Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

[Daniel Vetter]

On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> From: Lukasz Spintzyk 
> 
> Optional plane property to mark damaged regions on the plane in
> framebuffer coordinates of the framebuffer attached to the plane.
> 
> The layout of blob data is simply an array of drm_mode_rect with maximum
> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> coordinates, damage clips are not in 16.16 fixed point.
> 
> Damage clips are a hint to kernel as which area of framebuffer has
> changed since last page-flip. This should be helpful for some drivers
> especially for virtual devices where each framebuffer change needs to
> be transmitted over network, usb, etc.
> 
> Driver which are interested in enabling DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
> 
> Signed-off-by: Lukasz Spintzyk 
> Signed-off-by: Deepak Rawat 

The property uapi section is missing, see:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Plane composition feels like the best place to put this. Please use that
section to tie all the various bits together, including the helpers you're
adding in the following patches for drivers to use.

Bunch of nitpicks below, but overall I'm agreeing now with just going with
fb coordinate damage rects.

Like you say, the thing needed here now is userspace + driver actually
implementing this. I'd also say the compat helper to map the legacy
fb->dirty to this new atomic way of doing things should be included here
(gives us a lot more testing for these new paths).

Icing on the cake would be an igt to make sure kernel rejects invalid clip
rects correctly.

> ---
>  drivers/gpu/drm/drm_atomic.c| 42 
> +
>  drivers/gpu/drm/drm_atomic_helper.c |  4 
>  drivers/gpu/drm/drm_mode_config.c   |  5 +
>  drivers/gpu/drm/drm_plane.c | 12 +++
>  include/drm/drm_mode_config.h   | 15 +
>  include/drm/drm_plane.h | 16 ++
>  include/uapi/drm/drm_mode.h | 15 +
>  7 files changed, 109 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..9226d24 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct 
> drm_printer *p,
>  }
>  
>  /**
> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane
> + * @state: plane state
> + * @blob: damage clips in framebuffer coordinates
> + *
> + * Returns:
> + *
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state,
> +struct drm_property_blob *blob)
> +{
> + if (blob == state->damage_clips)
> + return 0;
> +
> + drm_property_blob_put(state->damage_clips);
> + state->damage_clips = NULL;
> +
> + if (blob) {
> + uint32_t count = blob->length/sizeof(struct drm_rect);
> +
> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> + return -EINVAL;
> +
> + state->damage_clips = drm_property_blob_get(blob);
> + state->num_clips = count;
> + } else {
> + state->damage_clips = NULL;
> + state->num_clips = 0;
> + }
> +
> + return 0;
> +}
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct 
> drm_plane *plane,
>   state->color_encoding = val;
>   } else if (property == plane->color_range_property) {
>   state->color_range = val;
> + } else if (property == config->prop_damage_clips) {
> + struct drm_property_blob *blob =
> + drm_property_lookup_blob(dev, val);
> + int ret = drm_atomic_set_damage_for_plane(state, blob);

There's already a helper with size-checking built-in, see
drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd
just provide a little inline helper that does the
blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast).

> + drm_property_blob_put(blob);
> + return ret;
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->color_encoding;
>   } else if (property == plane->color_range_property) {
>   *val = state->color_range;
> + } else if (property == config->prop_damage_clips) {
> + *val = (state->damage_clips) ? state->damage_clips->base.id : 0;
>   } else if (plane->funcs->atomic_get_property) {
>