[PATCH 0/1] Damage rectangles interface for DRM

[PATCH 0/1] Damage rectangles interface for DRM

[Lukasz Spintzyk]

	This is a draft of damage interface for drm. Alluding to the topic "RFC: page-flip with damage?" on dri-devel
https://lists.freedesktop.org/archives/dri-devel/2017-September/153235.html
The following patch is a proof of concept, how we can deliver dirty rectangles to the drm drivers.
Patch was tested on Chromium OS to deliver damage regions to atomic version of evdi driver. Hence this supports atomic drivers for now.
We should agree on the approach how this can be implemented.
In the patch, I intend to use drm blob properties to pass c-style array of drm_clip_rects.
Is DRM properties framework suitable for this solution? The only doubt I have is that it requires to create and destroy property blob every atomic_commit/page flip as this is the only way to update blob with damage regions. This is associated with kmalloc and free for each flip. Do you think if this approach relates to some performance risk?

	The proposed solution is attaching DIRTY_RECTS property to the drm_plane. The property means to hold list of rectangles in plane coordinates.
This is opposite to initial plan in thread from September 2017. I wanted to present our rationale for this.
Single rect vs rect list:
	Some compositors like Chromium can have quite precise damage information that would be lost if we would collapse them. 
Damage rectangles on crtc vs per plane
 	The property is attached to drm plane initially. We had a talk with guys from Chromium compositor. The plan is to use overlay planes more extensively. Also, Chromium compositor operates on dirty rects per plane so it would be easier to use them as is.
In my opinion this is more natural to deliver information about damage per plane. Damage information is used primarily for bandwidth savings. For driver that deliver plane data it is more useful to know what parts of the plane have changed so it can send only these memory areas.
I don't know if this reasoning is valid. Probably it depends on hardware and driver.
I can speak about use cases I have faced. One of them is additional cursor plane. With damage regions on crtc I don't know if it is related to primary plane, or it is connected to cursor movement. Also, user space applications must generate additional damage regions when such cursor plane is moving over primary plane. For such cases passing dirty rectangles for planes seems to be more straightforward.

This is draft with minimal functionality. I would like to hear what is your opinion on it.  
I am happy to hear what are the requirements of other driver vendors.


Thanks
Lukasz Spintzyk

Lukasz Spintzyk (1):
  drm: Add dirty_rects atomic blob property for drm_plane

 drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
 drivers/gpu/drm/drm_mode_config.c |  6 ++++++
 drivers/gpu/drm/drm_plane.c       |  1 +
 include/drm/drm_mode_config.h     |  5 +++++
 include/drm/drm_plane.h           |  3 +++
 5 files changed, 25 insertions(+)

-- 
2.15.1

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

[PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Lukasz Spintzyk]

Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
---
 drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
 drivers/gpu/drm/drm_mode_config.c |  6 ++++++
 drivers/gpu/drm/drm_plane.c       |  1 +
 include/drm/drm_mode_config.h     |  5 +++++
 include/drm/drm_plane.h           |  3 +++
 5 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b76d49218cf1..cd3b4ed7b04c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 		state->rotation = val;
 	} else if (property == plane->zpos_property) {
 		state->zpos = val;
+	} else if (property == config->dirty_rects_property) {
+		bool replaced = false;
+		int ret = drm_atomic_replace_property_blob_from_id(dev,
+					&state->dirty_blob,
+					val,
+					-1,
+					&replaced);
+		return ret;
 	} else if (plane->funcs->atomic_set_property) {
 		return plane->funcs->atomic_set_property(plane, state,
 				property, val);
@@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 		*val = state->rotation;
 	} else if (property == plane->zpos_property) {
 		*val = state->zpos;
+	} else if (property == config->dirty_rects_property) {
+		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index bc5c46306b3d..d5f1021c6ece 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_crtc_id = prop;
 
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+			"DIRTY_RECTS", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.dirty_rects_property = prop;
+
 	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
 			"ACTIVE");
 	if (!prop)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 37a93cdffb4a..add110f025e5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
 		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
 		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
+		drm_object_attach_property(&plane->base, config->dirty_rects_property, 0);
 	}
 
 	if (config->allow_fb_modifiers)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5f3b43014e1..65f64eb04c0c 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -599,6 +599,11 @@ struct drm_mode_config {
 	 * &drm_crtc.
 	 */
 	struct drm_property *prop_crtc_id;
+	/**
+	 * @dirty_rects_property: Optional plane property to mark damaged
+	 * regions on the plane framebuffer.
+	 */
+	struct drm_property *dirty_rects_property;
 	/**
 	 * @prop_active: Default atomic CRTC property to control the active
 	 * state, which is the simplified implementation for DPMS in atomic
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 8185e3468a23..7d45b164ccce 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -131,6 +131,9 @@ struct drm_plane_state {
 	 */
 	struct drm_crtc_commit *commit;
 
+	/* Optional blob property with damaged regions. */
+	struct drm_property_blob *dirty_blob;
+
 	struct drm_atomic_state *state;
 };
 
-- 
2.15.1

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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Ville Syrjälä]

On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> ---
>  drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
>  drivers/gpu/drm/drm_mode_config.c |  6 ++++++
>  drivers/gpu/drm/drm_plane.c       |  1 +
>  include/drm/drm_mode_config.h     |  5 +++++
>  include/drm/drm_plane.h           |  3 +++
>  5 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b76d49218cf1..cd3b4ed7b04c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		state->rotation = val;
>  	} else if (property == plane->zpos_property) {
>  		state->zpos = val;
> +	} else if (property == config->dirty_rects_property) {
> +		bool replaced = false;
> +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> +					&state->dirty_blob,
> +					val,
> +					-1,
> +					&replaced);
> +		return ret;
>  	} else if (plane->funcs->atomic_set_property) {
>  		return plane->funcs->atomic_set_property(plane, state,
>  				property, val);
> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>  		*val = state->rotation;
>  	} else if (property == plane->zpos_property) {
>  		*val = state->zpos;
> +	} else if (property == config->dirty_rects_property) {
> +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index bc5c46306b3d..d5f1021c6ece 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_crtc_id = prop;
>  
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +			"DIRTY_RECTS", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.dirty_rects_property = prop;
> +
>  	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>  			"ACTIVE");
>  	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 37a93cdffb4a..add110f025e5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
>  		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
>  		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> +		drm_object_attach_property(&plane->base, config->dirty_rects_property, 0);
>  	}
>  
>  	if (config->allow_fb_modifiers)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5f3b43014e1..65f64eb04c0c 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -599,6 +599,11 @@ struct drm_mode_config {
>  	 * &drm_crtc.
>  	 */
>  	struct drm_property *prop_crtc_id;
> +	/**
> +	 * @dirty_rects_property: Optional plane property to mark damaged
> +	 * regions on the plane framebuffer.

What exactly would the blob contain?

The comment seems to be implying these are in fb coordiantes as
opposed to plane crtc coordinates? Not sure which would be more
convenient. At least if they're fb coordinates then you probably
want some helpers to translate/rotate/scale those rects to the
crtc coordinates. Actual use depends on the driver/hw I suppose.

> +	 */
> +	struct drm_property *dirty_rects_property;
>  	/**
>  	 * @prop_active: Default atomic CRTC property to control the active
>  	 * state, which is the simplified implementation for DPMS in atomic
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..7d45b164ccce 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -131,6 +131,9 @@ struct drm_plane_state {
>  	 */
>  	struct drm_crtc_commit *commit;
>  
> +	/* Optional blob property with damaged regions. */
> +	struct drm_property_blob *dirty_blob;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 2.15.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Daniel Vetter]

On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
> >  drivers/gpu/drm/drm_mode_config.c |  6 ++++++
> >  drivers/gpu/drm/drm_plane.c       |  1 +
> >  include/drm/drm_mode_config.h     |  5 +++++
> >  include/drm/drm_plane.h           |  3 +++
> >  5 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index b76d49218cf1..cd3b4ed7b04c 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> >  		state->rotation = val;
> >  	} else if (property == plane->zpos_property) {
> >  		state->zpos = val;
> > +	} else if (property == config->dirty_rects_property) {
> > +		bool replaced = false;
> > +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> > +					&state->dirty_blob,
> > +					val,
> > +					-1,
> > +					&replaced);
> > +		return ret;
> >  	} else if (plane->funcs->atomic_set_property) {
> >  		return plane->funcs->atomic_set_property(plane, state,
> >  				property, val);
> > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >  		*val = state->rotation;
> >  	} else if (property == plane->zpos_property) {
> >  		*val = state->zpos;
> > +	} else if (property == config->dirty_rects_property) {
> > +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index bc5c46306b3d..d5f1021c6ece 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> >  		return -ENOMEM;
> >  	dev->mode_config.prop_crtc_id = prop;
> >  
> > +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > +			"DIRTY_RECTS", 0);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	dev->mode_config.dirty_rects_property = prop;
> > +
> >  	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> >  			"ACTIVE");
> >  	if (!prop)
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 37a93cdffb4a..add110f025e5 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >  		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
> >  		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
> >  		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> > +		drm_object_attach_property(&plane->base, config->dirty_rects_property, 0);
> >  	}
> >  
> >  	if (config->allow_fb_modifiers)
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index e5f3b43014e1..65f64eb04c0c 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -599,6 +599,11 @@ struct drm_mode_config {
> >  	 * &drm_crtc.
> >  	 */
> >  	struct drm_property *prop_crtc_id;
> > +	/**
> > +	 * @dirty_rects_property: Optional plane property to mark damaged
> > +	 * regions on the plane framebuffer.
> 
> What exactly would the blob contain?
> 
> The comment seems to be implying these are in fb coordiantes as
> opposed to plane crtc coordinates? Not sure which would be more
> convenient. At least if they're fb coordinates then you probably
> want some helpers to translate/rotate/scale those rects to the
> crtc coordinates. Actual use depends on the driver/hw I suppose.

Yeah I think we also should add a decoded state to the drm_plane_state,
which has the full structure and all the details.

And when we discussed this iirc we've identified a clear need for at least
some drivers to deal in crtc dirty rectangles. I think the initial core
support should include a helper which takes an atomic update for a given
crtc, and converts all the plane dirty rectangles into crtc rectangles.
Including any full-plane or full-crtc upgrades needed due to e.g.
reposition, changed gamma, changed blendign/zpos or anything else really
that would affect the entire plane respectively crtc. That would also
provide a really good model for what damage actually means.

Plus ofc we need userspace for this, preferrably as a patch to something
generic like weston or xfree86-video-modesetting. And an example kernel
implementation.

Cheers, Daniel

> 
> > +	 */
> > +	struct drm_property *dirty_rects_property;
> >  	/**
> >  	 * @prop_active: Default atomic CRTC property to control the active
> >  	 * state, which is the simplified implementation for DPMS in atomic
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8185e3468a23..7d45b164ccce 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -131,6 +131,9 @@ struct drm_plane_state {
> >  	 */
> >  	struct drm_crtc_commit *commit;
> >  
> > +	/* Optional blob property with damaged regions. */
> > +	struct drm_property_blob *dirty_blob;
> > +
> >  	struct drm_atomic_state *state;
> >  };
> >  
> > -- 
> > 2.15.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Lukasz Spintzyk]

[-- Attachment #1.1: Type: text/plain, Size: 7012 bytes --]

Thanks Ville and Daniel for for your response.

I will try to come back with something later.

thanks
Lukasz
On 21/12/2017 14:10, Daniel Vetter wrote:
> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++
> > > drivers/gpu/drm/drm_mode_config.c | 6 ++++++
> > > drivers/gpu/drm/drm_plane.c | 1 +
> > > include/drm/drm_mode_config.h | 5 +++++
> > > include/drm/drm_plane.h | 3 +++
> > > 5 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c 
> b/drivers/gpu/drm/drm_atomic.c
> > > index b76d49218cf1..cd3b4ed7b04c 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -759,6 +759,14 @@ static int 
> drm_atomic_plane_set_property(struct drm_plane *plane,
> > > state->rotation = val;
> > > } else if (property == plane->zpos_property) {
> > > state->zpos = val;
> > > + } else if (property == config->dirty_rects_property) {
> > > + bool replaced = false;
> > > + int ret = drm_atomic_replace_property_blob_from_id(dev,
> > > + &state->dirty_blob,
> > > + val,
> > > + -1,
> > > + &replaced);
> > > + return ret;
> > > } else if (plane->funcs->atomic_set_property) {
> > > return plane->funcs->atomic_set_property(plane, state,
> > > property, val);
> > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane 
> *plane,
> > > *val = state->rotation;
> > > } else if (property == plane->zpos_property) {
> > > *val = state->zpos;
> > > + } else if (property == config->dirty_rects_property) {
> > > + *val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c 
> b/drivers/gpu/drm/drm_mode_config.c
> > > index bc5c46306b3d..d5f1021c6ece 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -293,6 +293,12 @@ static int 
> drm_mode_create_standard_properties(struct drm_device *dev)
> > > return -ENOMEM;
> > > dev->mode_config.prop_crtc_id = prop;
> > >
> > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > + "DIRTY_RECTS", 0);
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + dev->mode_config.dirty_rects_property = prop;
> > > +
> > > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> > > "ACTIVE");
> > > if (!prop)
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 37a93cdffb4a..add110f025e5 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device 
> *dev, struct drm_plane *plane,
> > > drm_object_attach_property(&plane->base, config->prop_src_y, 0);
> > > drm_object_attach_property(&plane->base, config->prop_src_w, 0);
> > > drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> > > + drm_object_attach_property(&plane->base, 
> config->dirty_rects_property, 0);
> > > }
> > >
> > > if (config->allow_fb_modifiers)
> > > diff --git a/include/drm/drm_mode_config.h 
> b/include/drm/drm_mode_config.h
> > > index e5f3b43014e1..65f64eb04c0c 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > > * &drm_crtc.
> > > */
> > > struct drm_property *prop_crtc_id;
> > > + /**
> > > + * @dirty_rects_property: Optional plane property to mark damaged
> > > + * regions on the plane framebuffer.
> >
> > What exactly would the blob contain?
> >
> > The comment seems to be implying these are in fb coordiantes as
> > opposed to plane crtc coordinates? Not sure which would be more
> > convenient. At least if they're fb coordinates then you probably
> > want some helpers to translate/rotate/scale those rects to the
> > crtc coordinates. Actual use depends on the driver/hw I suppose.
>
> Yeah I think we also should add a decoded state to the drm_plane_state,
> which has the full structure and all the details.
Ok.
Initially for model I was thinking to take struct drm_drawable_info with 
simple c style array of struct drm_clip_rect *rects in it.
Do you think that something more complex will be needed?

>
> And when we discussed this iirc we've identified a clear need for at least
> some drivers to deal in crtc dirty rectangles. I think the initial core
> support should include a helper which takes an atomic update for a given
> crtc, and converts all the plane dirty rectangles into crtc rectangles.
> Including any full-plane or full-crtc upgrades needed due to e.g.
> reposition, changed gamma, changed blendign/zpos or anything else really
> that would affect the entire plane respectively crtc. That would also
> provide a really good model for what damage actually means.
Ok.
> Plus ofc we need userspace for this, preferrably as a patch to something
> generic like weston or xfree86-video-modesetting. And an example kernel
> implementation.
What kernel example would you think is the quickest/simplest for the 
proof of concept?

I wanted to have something working on ChromeOS compositor first.
Do you think it would satisfy need of userspace application?

>
> Cheers, Daniel
>
> >
> > > + */
> > > + struct drm_property *dirty_rects_property;
> > > /**
> > > * @prop_active: Default atomic CRTC property to control the active
> > > * state, which is the simplified implementation for DPMS in atomic
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > index 8185e3468a23..7d45b164ccce 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -131,6 +131,9 @@ struct drm_plane_state {
> > > */
> > > struct drm_crtc_commit *commit;
> > >
> > > + /* Optional blob property with damaged regions. */
> > > + struct drm_property_blob *dirty_blob;
> > > +
> > > struct drm_atomic_state *state;
> > > };
> > >
> > > --
> > > 2.15.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel 
> <https://lists.freedesktop.org/mailman/listinfo/dri-devel>
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel 
> <https://lists.freedesktop.org/mailman/listinfo/dri-devel>
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch 
> <http://blog.ffwll.ch>


[-- Attachment #1.2: Type: text/html, Size: 10473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

RE: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Deepak Singh Rawat]

> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> Of Daniel Vetter
> Sent: Thursday, December 21, 2017 5:11 AM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
> devel@lists.freedesktop.org; Lukasz Spintzyk
> <lukasz.spintzyk@displaylink.com>
> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
> drm_plane
> 
> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
> > >  drivers/gpu/drm/drm_mode_config.c |  6 ++++++
> > >  drivers/gpu/drm/drm_plane.c       |  1 +
> > >  include/drm/drm_mode_config.h     |  5 +++++
> > >  include/drm/drm_plane.h           |  3 +++
> > >  5 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> > > index b76d49218cf1..cd3b4ed7b04c 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> > >  		state->rotation = val;
> > >  	} else if (property == plane->zpos_property) {
> > >  		state->zpos = val;
> > > +	} else if (property == config->dirty_rects_property) {
> > > +		bool replaced = false;
> > > +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> > > +					&state->dirty_blob,
> > > +					val,
> > > +					-1,
> > > +					&replaced);
> > > +		return ret;
> > >  	} else if (plane->funcs->atomic_set_property) {
> > >  		return plane->funcs->atomic_set_property(plane, state,
> > >  				property, val);
> > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
> drm_plane *plane,
> > >  		*val = state->rotation;
> > >  	} else if (property == plane->zpos_property) {
> > >  		*val = state->zpos;
> > > +	} else if (property == config->dirty_rects_property) {
> > > +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> > > index bc5c46306b3d..d5f1021c6ece 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -293,6 +293,12 @@ static int
> drm_mode_create_standard_properties(struct drm_device *dev)
> > >  		return -ENOMEM;
> > >  	dev->mode_config.prop_crtc_id = prop;
> > >
> > > +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > +			"DIRTY_RECTS", 0);
> > > +	if (!prop)
> > > +		return -ENOMEM;
> > > +	dev->mode_config.dirty_rects_property = prop;
> > > +
> > >  	prop = drm_property_create_bool(dev,
> DRM_MODE_PROP_ATOMIC,
> > >  			"ACTIVE");
> > >  	if (!prop)
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> b/drivers/gpu/drm/drm_plane.c
> > > index 37a93cdffb4a..add110f025e5 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
> *dev, struct drm_plane *plane,
> > >  		drm_object_attach_property(&plane->base, config-
> >prop_src_y, 0);
> > >  		drm_object_attach_property(&plane->base, config-
> >prop_src_w, 0);
> > >  		drm_object_attach_property(&plane->base, config-
> >prop_src_h, 0);
> > > +		drm_object_attach_property(&plane->base, config-
> >dirty_rects_property, 0);
> > >  	}
> > >
> > >  	if (config->allow_fb_modifiers)
> > > diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
> > > index e5f3b43014e1..65f64eb04c0c 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > >  	 * &drm_crtc.
> > >  	 */
> > >  	struct drm_property *prop_crtc_id;
> > > +	/**
> > > +	 * @dirty_rects_property: Optional plane property to mark damaged
> > > +	 * regions on the plane framebuffer.
> >
> > What exactly would the blob contain?
> >
> > The comment seems to be implying these are in fb coordiantes as
> > opposed to plane crtc coordinates? Not sure which would be more
> > convenient. At least if they're fb coordinates then you probably
> > want some helpers to translate/rotate/scale those rects to the
> > crtc coordinates. Actual use depends on the driver/hw I suppose.
> 
> Yeah I think we also should add a decoded state to the drm_plane_state,
> which has the full structure and all the details.

Hi Daniel,

I am working on this functionality to implement the helper functions to
convert dirty clips from framebuffer coordinates to planes coordinates
and finally to crtc coordinates.

Is there a reason we should have decoded dirty clips in plane_state ?
I was thinking to have the clips remain in blob property and decode
them when needed with the helper function which accept plane state and
similarly another helper for crtc coordinates. So driver call these helper
only if there is a need for dirty clips and otherwise can ignore.

Thanks,
Deepak


> 
> And when we discussed this iirc we've identified a clear need for at least
> some drivers to deal in crtc dirty rectangles. I think the initial core
> support should include a helper which takes an atomic update for a given
> crtc, and converts all the plane dirty rectangles into crtc rectangles.
> Including any full-plane or full-crtc upgrades needed due to e.g.
> reposition, changed gamma, changed blendign/zpos or anything else really
> that would affect the entire plane respectively crtc. That would also
> provide a really good model for what damage actually means.
> 
> Plus ofc we need userspace for this, preferrably as a patch to something
> generic like weston or xfree86-video-modesetting. And an example kernel
> implementation.
> 
> Cheers, Daniel
> 
> >
> > > +	 */
> > > +	struct drm_property *dirty_rects_property;
> > >  	/**
> > >  	 * @prop_active: Default atomic CRTC property to control the active
> > >  	 * state, which is the simplified implementation for DPMS in atomic
> > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > index 8185e3468a23..7d45b164ccce 100644
> > > --- a/include/drm/drm_plane.h
> > > +++ b/include/drm/drm_plane.h
> > > @@ -131,6 +131,9 @@ struct drm_plane_state {
> > >  	 */
> > >  	struct drm_crtc_commit *commit;
> > >
> > > +	/* Optional blob property with damaged regions. */
> > > +	struct drm_property_blob *dirty_blob;
> > > +
> > >  	struct drm_atomic_state *state;
> > >  };
> > >
> > > --
> > > 2.15.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_dri-
> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_dri-
> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2-
> IBYMRBEEBmnUTRTo7czA810&e=
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_dri-
> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Daniel Vetter]

On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> > Of Daniel Vetter
> > Sent: Thursday, December 21, 2017 5:11 AM
> > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
> > devel@lists.freedesktop.org; Lukasz Spintzyk
> > <lukasz.spintzyk@displaylink.com>
> > Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
> > drm_plane
> > 
> > On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> > > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
> > > >  drivers/gpu/drm/drm_mode_config.c |  6 ++++++
> > > >  drivers/gpu/drm/drm_plane.c       |  1 +
> > > >  include/drm/drm_mode_config.h     |  5 +++++
> > > >  include/drm/drm_plane.h           |  3 +++
> > > >  5 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > b/drivers/gpu/drm/drm_atomic.c
> > > > index b76d49218cf1..cd3b4ed7b04c 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
> > drm_plane *plane,
> > > >  		state->rotation = val;
> > > >  	} else if (property == plane->zpos_property) {
> > > >  		state->zpos = val;
> > > > +	} else if (property == config->dirty_rects_property) {
> > > > +		bool replaced = false;
> > > > +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > +					&state->dirty_blob,
> > > > +					val,
> > > > +					-1,
> > > > +					&replaced);
> > > > +		return ret;
> > > >  	} else if (plane->funcs->atomic_set_property) {
> > > >  		return plane->funcs->atomic_set_property(plane, state,
> > > >  				property, val);
> > > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
> > drm_plane *plane,
> > > >  		*val = state->rotation;
> > > >  	} else if (property == plane->zpos_property) {
> > > >  		*val = state->zpos;
> > > > +	} else if (property == config->dirty_rects_property) {
> > > > +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
> > b/drivers/gpu/drm/drm_mode_config.c
> > > > index bc5c46306b3d..d5f1021c6ece 100644
> > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > @@ -293,6 +293,12 @@ static int
> > drm_mode_create_standard_properties(struct drm_device *dev)
> > > >  		return -ENOMEM;
> > > >  	dev->mode_config.prop_crtc_id = prop;
> > > >
> > > > +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > > +			"DIRTY_RECTS", 0);
> > > > +	if (!prop)
> > > > +		return -ENOMEM;
> > > > +	dev->mode_config.dirty_rects_property = prop;
> > > > +
> > > >  	prop = drm_property_create_bool(dev,
> > DRM_MODE_PROP_ATOMIC,
> > > >  			"ACTIVE");
> > > >  	if (!prop)
> > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > b/drivers/gpu/drm/drm_plane.c
> > > > index 37a93cdffb4a..add110f025e5 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
> > *dev, struct drm_plane *plane,
> > > >  		drm_object_attach_property(&plane->base, config-
> > >prop_src_y, 0);
> > > >  		drm_object_attach_property(&plane->base, config-
> > >prop_src_w, 0);
> > > >  		drm_object_attach_property(&plane->base, config-
> > >prop_src_h, 0);
> > > > +		drm_object_attach_property(&plane->base, config-
> > >dirty_rects_property, 0);
> > > >  	}
> > > >
> > > >  	if (config->allow_fb_modifiers)
> > > > diff --git a/include/drm/drm_mode_config.h
> > b/include/drm/drm_mode_config.h
> > > > index e5f3b43014e1..65f64eb04c0c 100644
> > > > --- a/include/drm/drm_mode_config.h
> > > > +++ b/include/drm/drm_mode_config.h
> > > > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > > >  	 * &drm_crtc.
> > > >  	 */
> > > >  	struct drm_property *prop_crtc_id;
> > > > +	/**
> > > > +	 * @dirty_rects_property: Optional plane property to mark damaged
> > > > +	 * regions on the plane framebuffer.
> > >
> > > What exactly would the blob contain?
> > >
> > > The comment seems to be implying these are in fb coordiantes as
> > > opposed to plane crtc coordinates? Not sure which would be more
> > > convenient. At least if they're fb coordinates then you probably
> > > want some helpers to translate/rotate/scale those rects to the
> > > crtc coordinates. Actual use depends on the driver/hw I suppose.
> > 
> > Yeah I think we also should add a decoded state to the drm_plane_state,
> > which has the full structure and all the details.
> 
> Hi Daniel,
> 
> I am working on this functionality to implement the helper functions to
> convert dirty clips from framebuffer coordinates to planes coordinates
> and finally to crtc coordinates.
> 
> Is there a reason we should have decoded dirty clips in plane_state ?
> I was thinking to have the clips remain in blob property and decode
> them when needed with the helper function which accept plane state and
> similarly another helper for crtc coordinates. So driver call these helper
> only if there is a need for dirty clips and otherwise can ignore.

Immediately decoding the state blobs is simply best practices, to avoid
duplicated code and subtle differences in driver behaviour. If there's a
good reason for completely different behaviour (like crtc vs plane
coordinate based damage tracking), then decoding using a helper,
on-demand, seems sensible.

I'd still think we should store the resulting derived state in
drm_crtc/plane_state, like we do for the plane clipping helpers (at least
after Ville's latest patch series has landed).
-Daniel
> 
> Thanks,
> Deepak
> 
> 
> > 
> > And when we discussed this iirc we've identified a clear need for at least
> > some drivers to deal in crtc dirty rectangles. I think the initial core
> > support should include a helper which takes an atomic update for a given
> > crtc, and converts all the plane dirty rectangles into crtc rectangles.
> > Including any full-plane or full-crtc upgrades needed due to e.g.
> > reposition, changed gamma, changed blendign/zpos or anything else really
> > that would affect the entire plane respectively crtc. That would also
> > provide a really good model for what damage actually means.
> > 
> > Plus ofc we need userspace for this, preferrably as a patch to something
> > generic like weston or xfree86-video-modesetting. And an example kernel
> > implementation.
> > 
> > Cheers, Daniel
> > 
> > >
> > > > +	 */
> > > > +	struct drm_property *dirty_rects_property;
> > > >  	/**
> > > >  	 * @prop_active: Default atomic CRTC property to control the active
> > > >  	 * state, which is the simplified implementation for DPMS in atomic
> > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > index 8185e3468a23..7d45b164ccce 100644
> > > > --- a/include/drm/drm_plane.h
> > > > +++ b/include/drm/drm_plane.h
> > > > @@ -131,6 +131,9 @@ struct drm_plane_state {
> > > >  	 */
> > > >  	struct drm_crtc_commit *commit;
> > > >
> > > > +	/* Optional blob property with damaged regions. */
> > > > +	struct drm_property_blob *dirty_blob;
> > > > +
> > > >  	struct drm_atomic_state *state;
> > > >  };
> > > >
> > > > --
> > > > 2.15.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__lists.freedesktop.org_mailman_listinfo_dri-
> > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__lists.freedesktop.org_mailman_listinfo_dri-
> > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> > 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2-
> > IBYMRBEEBmnUTRTo7czA810&e=
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__lists.freedesktop.org_mailman_listinfo_dri-
> > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=

-- 
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: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Thomas Hellstrom]

On 03/05/2018 09:37 AM, Daniel Vetter wrote:
> On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
>>
>>> -----Original Message-----
>>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
>>> Of Daniel Vetter
>>> Sent: Thursday, December 21, 2017 5:11 AM
>>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
>>> devel@lists.freedesktop.org; Lukasz Spintzyk
>>> <lukasz.spintzyk@displaylink.com>
>>> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
>>> drm_plane
>>>
>>> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
>>>> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
>>>>> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
>>>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
>>>>>   drivers/gpu/drm/drm_mode_config.c |  6 ++++++
>>>>>   drivers/gpu/drm/drm_plane.c       |  1 +
>>>>>   include/drm/drm_mode_config.h     |  5 +++++
>>>>>   include/drm/drm_plane.h           |  3 +++
>>>>>   5 files changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
>>> b/drivers/gpu/drm/drm_atomic.c
>>>>> index b76d49218cf1..cd3b4ed7b04c 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
>>> drm_plane *plane,
>>>>>   		state->rotation = val;
>>>>>   	} else if (property == plane->zpos_property) {
>>>>>   		state->zpos = val;
>>>>> +	} else if (property == config->dirty_rects_property) {
>>>>> +		bool replaced = false;
>>>>> +		int ret = drm_atomic_replace_property_blob_from_id(dev,
>>>>> +					&state->dirty_blob,
>>>>> +					val,
>>>>> +					-1,
>>>>> +					&replaced);
>>>>> +		return ret;
>>>>>   	} else if (plane->funcs->atomic_set_property) {
>>>>>   		return plane->funcs->atomic_set_property(plane, state,
>>>>>   				property, val);
>>>>> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
>>> drm_plane *plane,
>>>>>   		*val = state->rotation;
>>>>>   	} else if (property == plane->zpos_property) {
>>>>>   		*val = state->zpos;
>>>>> +	} else if (property == config->dirty_rects_property) {
>>>>> +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>> index bc5c46306b3d..d5f1021c6ece 100644
>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>> @@ -293,6 +293,12 @@ static int
>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>>>   		return -ENOMEM;
>>>>>   	dev->mode_config.prop_crtc_id = prop;
>>>>>
>>>>> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>>>>> +			"DIRTY_RECTS", 0);
>>>>> +	if (!prop)
>>>>> +		return -ENOMEM;
>>>>> +	dev->mode_config.dirty_rects_property = prop;
>>>>> +
>>>>>   	prop = drm_property_create_bool(dev,
>>> DRM_MODE_PROP_ATOMIC,
>>>>>   			"ACTIVE");
>>>>>   	if (!prop)
>>>>> diff --git a/drivers/gpu/drm/drm_plane.c
>>> b/drivers/gpu/drm/drm_plane.c
>>>>> index 37a93cdffb4a..add110f025e5 100644
>>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>>> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
>>> *dev, struct drm_plane *plane,
>>>>>   		drm_object_attach_property(&plane->base, config-
>>>> prop_src_y, 0);
>>>>>   		drm_object_attach_property(&plane->base, config-
>>>> prop_src_w, 0);
>>>>>   		drm_object_attach_property(&plane->base, config-
>>>> prop_src_h, 0);
>>>>> +		drm_object_attach_property(&plane->base, config-
>>>> dirty_rects_property, 0);
>>>>>   	}
>>>>>
>>>>>   	if (config->allow_fb_modifiers)
>>>>> diff --git a/include/drm/drm_mode_config.h
>>> b/include/drm/drm_mode_config.h
>>>>> index e5f3b43014e1..65f64eb04c0c 100644
>>>>> --- a/include/drm/drm_mode_config.h
>>>>> +++ b/include/drm/drm_mode_config.h
>>>>> @@ -599,6 +599,11 @@ struct drm_mode_config {
>>>>>   	 * &drm_crtc.
>>>>>   	 */
>>>>>   	struct drm_property *prop_crtc_id;
>>>>> +	/**
>>>>> +	 * @dirty_rects_property: Optional plane property to mark damaged
>>>>> +	 * regions on the plane framebuffer.
>>>> What exactly would the blob contain?
>>>>
>>>> The comment seems to be implying these are in fb coordiantes as
>>>> opposed to plane crtc coordinates? Not sure which would be more
>>>> convenient. At least if they're fb coordinates then you probably
>>>> want some helpers to translate/rotate/scale those rects to the
>>>> crtc coordinates. Actual use depends on the driver/hw I suppose.
>>> Yeah I think we also should add a decoded state to the drm_plane_state,
>>> which has the full structure and all the details.
>> Hi Daniel,
>>
>> I am working on this functionality to implement the helper functions to
>> convert dirty clips from framebuffer coordinates to planes coordinates
>> and finally to crtc coordinates.
>>
>> Is there a reason we should have decoded dirty clips in plane_state ?
>> I was thinking to have the clips remain in blob property and decode
>> them when needed with the helper function which accept plane state and
>> similarly another helper for crtc coordinates. So driver call these helper
>> only if there is a need for dirty clips and otherwise can ignore.
> Immediately decoding the state blobs is simply best practices, to avoid
> duplicated code and subtle differences in driver behaviour. If there's a
> good reason for completely different behaviour (like crtc vs plane
> coordinate based damage tracking), then decoding using a helper,
> on-demand, seems sensible.
>
> I'd still think we should store the resulting derived state in
> drm_crtc/plane_state, like we do for the plane clipping helpers (at least
> after Ville's latest patch series has landed).
> -Daniel

The idea here would be to use a helper iterator to construct the new 
coordinates needed; the iterator being initialized with the blob.

The reason not to store the decoded state is unnecessary duplication. 
While I guess most drivers would use crtc damage tracking, potentially 
there might be three coordinate systems used by drivers: Plane fb 
coordinates, Plane crtc coordinates and crtc coordinates. And typically 
they'd be used only once...

/Thomas


>> Thanks,
>> Deepak
>>
>>
>>> And when we discussed this iirc we've identified a clear need for at least
>>> some drivers to deal in crtc dirty rectangles. I think the initial core
>>> support should include a helper which takes an atomic update for a given
>>> crtc, and converts all the plane dirty rectangles into crtc rectangles.
>>> Including any full-plane or full-crtc upgrades needed due to e.g.
>>> reposition, changed gamma, changed blendign/zpos or anything else really
>>> that would affect the entire plane respectively crtc. That would also
>>> provide a really good model for what damage actually means.
>>>
>>> Plus ofc we need userspace for this, preferrably as a patch to something
>>> generic like weston or xfree86-video-modesetting. And an example kernel
>>> implementation.
>>>
>>> Cheers, Daniel
>>>
>>>>> +	 */
>>>>> +	struct drm_property *dirty_rects_property;
>>>>>   	/**
>>>>>   	 * @prop_active: Default atomic CRTC property to control the active
>>>>>   	 * state, which is the simplified implementation for DPMS in atomic
>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>> index 8185e3468a23..7d45b164ccce 100644
>>>>> --- a/include/drm/drm_plane.h
>>>>> +++ b/include/drm/drm_plane.h
>>>>> @@ -131,6 +131,9 @@ struct drm_plane_state {
>>>>>   	 */
>>>>>   	struct drm_crtc_commit *commit;
>>>>>
>>>>> +	/* Optional blob property with damaged regions. */
>>>>> +	struct drm_property_blob *dirty_blob;
>>>>> +
>>>>>   	struct drm_atomic_state *state;
>>>>>   };
>>>>>
>>>>> --
>>>>> 2.15.1
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-
>>> 3A__lists.freedesktop.org_mailman_listinfo_dri-
>>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
>>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
>>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
>>>> --
>>>> Ville Syrjälä
>>>> Intel OTC
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://urldefense.proofpoint.com/v2/url?u=https-
>>> 3A__lists.freedesktop.org_mailman_listinfo_dri-
>>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
>>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
>>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> https://urldefense.proofpoint.com/v2/url?u=http-
>>> 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
>>> 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2-
>>> IBYMRBEEBmnUTRTo7czA810&e=
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://urldefense.proofpoint.com/v2/url?u=https-
>>> 3A__lists.freedesktop.org_mailman_listinfo_dri-
>>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
>>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
>>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=


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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Daniel Vetter]

On Mon, Mar 05, 2018 at 10:22:09AM +0100, Thomas Hellstrom wrote:
> On 03/05/2018 09:37 AM, Daniel Vetter wrote:
> > On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
> > > 
> > > > -----Original Message-----
> > > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> > > > Of Daniel Vetter
> > > > Sent: Thursday, December 21, 2017 5:11 AM
> > > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
> > > > devel@lists.freedesktop.org; Lukasz Spintzyk
> > > > <lukasz.spintzyk@displaylink.com>
> > > > Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
> > > > drm_plane
> > > > 
> > > > On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> > > > > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > > > > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> > > > > > ---
> > > > > >   drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
> > > > > >   drivers/gpu/drm/drm_mode_config.c |  6 ++++++
> > > > > >   drivers/gpu/drm/drm_plane.c       |  1 +
> > > > > >   include/drm/drm_mode_config.h     |  5 +++++
> > > > > >   include/drm/drm_plane.h           |  3 +++
> > > > > >   5 files changed, 25 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > > > b/drivers/gpu/drm/drm_atomic.c
> > > > > > index b76d49218cf1..cd3b4ed7b04c 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
> > > > drm_plane *plane,
> > > > > >   		state->rotation = val;
> > > > > >   	} else if (property == plane->zpos_property) {
> > > > > >   		state->zpos = val;
> > > > > > +	} else if (property == config->dirty_rects_property) {
> > > > > > +		bool replaced = false;
> > > > > > +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > > > +					&state->dirty_blob,
> > > > > > +					val,
> > > > > > +					-1,
> > > > > > +					&replaced);
> > > > > > +		return ret;
> > > > > >   	} else if (plane->funcs->atomic_set_property) {
> > > > > >   		return plane->funcs->atomic_set_property(plane, state,
> > > > > >   				property, val);
> > > > > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
> > > > drm_plane *plane,
> > > > > >   		*val = state->rotation;
> > > > > >   	} else if (property == plane->zpos_property) {
> > > > > >   		*val = state->zpos;
> > > > > > +	} else if (property == config->dirty_rects_property) {
> > > > > > +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
> > > > b/drivers/gpu/drm/drm_mode_config.c
> > > > > > index bc5c46306b3d..d5f1021c6ece 100644
> > > > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > > > @@ -293,6 +293,12 @@ static int
> > > > drm_mode_create_standard_properties(struct drm_device *dev)
> > > > > >   		return -ENOMEM;
> > > > > >   	dev->mode_config.prop_crtc_id = prop;
> > > > > > 
> > > > > > +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > > > > +			"DIRTY_RECTS", 0);
> > > > > > +	if (!prop)
> > > > > > +		return -ENOMEM;
> > > > > > +	dev->mode_config.dirty_rects_property = prop;
> > > > > > +
> > > > > >   	prop = drm_property_create_bool(dev,
> > > > DRM_MODE_PROP_ATOMIC,
> > > > > >   			"ACTIVE");
> > > > > >   	if (!prop)
> > > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > b/drivers/gpu/drm/drm_plane.c
> > > > > > index 37a93cdffb4a..add110f025e5 100644
> > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
> > > > *dev, struct drm_plane *plane,
> > > > > >   		drm_object_attach_property(&plane->base, config-
> > > > > prop_src_y, 0);
> > > > > >   		drm_object_attach_property(&plane->base, config-
> > > > > prop_src_w, 0);
> > > > > >   		drm_object_attach_property(&plane->base, config-
> > > > > prop_src_h, 0);
> > > > > > +		drm_object_attach_property(&plane->base, config-
> > > > > dirty_rects_property, 0);
> > > > > >   	}
> > > > > > 
> > > > > >   	if (config->allow_fb_modifiers)
> > > > > > diff --git a/include/drm/drm_mode_config.h
> > > > b/include/drm/drm_mode_config.h
> > > > > > index e5f3b43014e1..65f64eb04c0c 100644
> > > > > > --- a/include/drm/drm_mode_config.h
> > > > > > +++ b/include/drm/drm_mode_config.h
> > > > > > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > > > > >   	 * &drm_crtc.
> > > > > >   	 */
> > > > > >   	struct drm_property *prop_crtc_id;
> > > > > > +	/**
> > > > > > +	 * @dirty_rects_property: Optional plane property to mark damaged
> > > > > > +	 * regions on the plane framebuffer.
> > > > > What exactly would the blob contain?
> > > > > 
> > > > > The comment seems to be implying these are in fb coordiantes as
> > > > > opposed to plane crtc coordinates? Not sure which would be more
> > > > > convenient. At least if they're fb coordinates then you probably
> > > > > want some helpers to translate/rotate/scale those rects to the
> > > > > crtc coordinates. Actual use depends on the driver/hw I suppose.
> > > > Yeah I think we also should add a decoded state to the drm_plane_state,
> > > > which has the full structure and all the details.
> > > Hi Daniel,
> > > 
> > > I am working on this functionality to implement the helper functions to
> > > convert dirty clips from framebuffer coordinates to planes coordinates
> > > and finally to crtc coordinates.
> > > 
> > > Is there a reason we should have decoded dirty clips in plane_state ?
> > > I was thinking to have the clips remain in blob property and decode
> > > them when needed with the helper function which accept plane state and
> > > similarly another helper for crtc coordinates. So driver call these helper
> > > only if there is a need for dirty clips and otherwise can ignore.
> > Immediately decoding the state blobs is simply best practices, to avoid
> > duplicated code and subtle differences in driver behaviour. If there's a
> > good reason for completely different behaviour (like crtc vs plane
> > coordinate based damage tracking), then decoding using a helper,
> > on-demand, seems sensible.
> > 
> > I'd still think we should store the resulting derived state in
> > drm_crtc/plane_state, like we do for the plane clipping helpers (at least
> > after Ville's latest patch series has landed).
> > -Daniel
> 
> The idea here would be to use a helper iterator to construct the new
> coordinates needed; the iterator being initialized with the blob.
> 
> The reason not to store the decoded state is unnecessary duplication. While
> I guess most drivers would use crtc damage tracking, potentially there might
> be three coordinate systems used by drivers: Plane fb coordinates, Plane
> crtc coordinates and crtc coordinates. And typically they'd be used only
> once...

Well I was thinking of a helper to compute a single, overall clip rect for
the entire crtc. Which would then also need to take into account various
plane state changes, and stuff like background color. For a simple
coordinate system transform iterators sound like a good idea, but honestly
I don't expect that to be a common use-case in drivers. Either it's about
uploading the right dirty data in each plane, or about uploading the right
final/blended pixels to the screen (in which case all other state changes
that can affect colors must be taken into account too).
-Daniel

> 
> /Thomas
> 
> 
> > > Thanks,
> > > Deepak
> > > 
> > > 
> > > > And when we discussed this iirc we've identified a clear need for at least
> > > > some drivers to deal in crtc dirty rectangles. I think the initial core
> > > > support should include a helper which takes an atomic update for a given
> > > > crtc, and converts all the plane dirty rectangles into crtc rectangles.
> > > > Including any full-plane or full-crtc upgrades needed due to e.g.
> > > > reposition, changed gamma, changed blendign/zpos or anything else really
> > > > that would affect the entire plane respectively crtc. That would also
> > > > provide a really good model for what damage actually means.
> > > > 
> > > > Plus ofc we need userspace for this, preferrably as a patch to something
> > > > generic like weston or xfree86-video-modesetting. And an example kernel
> > > > implementation.
> > > > 
> > > > Cheers, Daniel
> > > > 
> > > > > > +	 */
> > > > > > +	struct drm_property *dirty_rects_property;
> > > > > >   	/**
> > > > > >   	 * @prop_active: Default atomic CRTC property to control the active
> > > > > >   	 * state, which is the simplified implementation for DPMS in atomic
> > > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > > > > > index 8185e3468a23..7d45b164ccce 100644
> > > > > > --- a/include/drm/drm_plane.h
> > > > > > +++ b/include/drm/drm_plane.h
> > > > > > @@ -131,6 +131,9 @@ struct drm_plane_state {
> > > > > >   	 */
> > > > > >   	struct drm_crtc_commit *commit;
> > > > > > 
> > > > > > +	/* Optional blob property with damaged regions. */
> > > > > > +	struct drm_property_blob *dirty_blob;
> > > > > > +
> > > > > >   	struct drm_atomic_state *state;
> > > > > >   };
> > > > > > 
> > > > > > --
> > > > > > 2.15.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > > > 3A__lists.freedesktop.org_mailman_listinfo_dri-
> > > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> > > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> > > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > > > 3A__lists.freedesktop.org_mailman_listinfo_dri-
> > > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> > > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> > > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> > > > 
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > https://urldefense.proofpoint.com/v2/url?u=http-
> > > > 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
> > > > 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2-
> > > > IBYMRBEEBmnUTRTo7czA810&e=
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > > > 3A__lists.freedesktop.org_mailman_listinfo_dri-
> > > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
> > > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
> > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
> > > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
> 
> 

-- 
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: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Thomas Hellstrom]

On 03/05/2018 10:39 AM, Daniel Vetter wrote:
> On Mon, Mar 05, 2018 at 10:22:09AM +0100, Thomas Hellstrom wrote:
>> On 03/05/2018 09:37 AM, Daniel Vetter wrote:
>>> On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
>>>>> -----Original Message-----
>>>>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
>>>>> Of Daniel Vetter
>>>>> Sent: Thursday, December 21, 2017 5:11 AM
>>>>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
>>>>> devel@lists.freedesktop.org; Lukasz Spintzyk
>>>>> <lukasz.spintzyk@displaylink.com>
>>>>> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
>>>>> drm_plane
>>>>>
>>>>> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
>>>>>> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
>>>>>>> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
>>>>>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
>>>>>>>    drivers/gpu/drm/drm_mode_config.c |  6 ++++++
>>>>>>>    drivers/gpu/drm/drm_plane.c       |  1 +
>>>>>>>    include/drm/drm_mode_config.h     |  5 +++++
>>>>>>>    include/drm/drm_plane.h           |  3 +++
>>>>>>>    5 files changed, 25 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
>>>>> b/drivers/gpu/drm/drm_atomic.c
>>>>>>> index b76d49218cf1..cd3b4ed7b04c 100644
>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
>>>>> drm_plane *plane,
>>>>>>>    		state->rotation = val;
>>>>>>>    	} else if (property == plane->zpos_property) {
>>>>>>>    		state->zpos = val;
>>>>>>> +	} else if (property == config->dirty_rects_property) {
>>>>>>> +		bool replaced = false;
>>>>>>> +		int ret = drm_atomic_replace_property_blob_from_id(dev,
>>>>>>> +					&state->dirty_blob,
>>>>>>> +					val,
>>>>>>> +					-1,
>>>>>>> +					&replaced);
>>>>>>> +		return ret;
>>>>>>>    	} else if (plane->funcs->atomic_set_property) {
>>>>>>>    		return plane->funcs->atomic_set_property(plane, state,
>>>>>>>    				property, val);
>>>>>>> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
>>>>> drm_plane *plane,
>>>>>>>    		*val = state->rotation;
>>>>>>>    	} else if (property == plane->zpos_property) {
>>>>>>>    		*val = state->zpos;
>>>>>>> +	} else if (property == config->dirty_rects_property) {
>>>>>>> +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>>>> index bc5c46306b3d..d5f1021c6ece 100644
>>>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>>>> @@ -293,6 +293,12 @@ static int
>>>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>>>>>    		return -ENOMEM;
>>>>>>>    	dev->mode_config.prop_crtc_id = prop;
>>>>>>>
>>>>>>> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>>>>>>> +			"DIRTY_RECTS", 0);
>>>>>>> +	if (!prop)
>>>>>>> +		return -ENOMEM;
>>>>>>> +	dev->mode_config.dirty_rects_property = prop;
>>>>>>> +
>>>>>>>    	prop = drm_property_create_bool(dev,
>>>>> DRM_MODE_PROP_ATOMIC,
>>>>>>>    			"ACTIVE");
>>>>>>>    	if (!prop)
>>>>>>> diff --git a/drivers/gpu/drm/drm_plane.c
>>>>> b/drivers/gpu/drm/drm_plane.c
>>>>>>> index 37a93cdffb4a..add110f025e5 100644
>>>>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>>>>> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
>>>>> *dev, struct drm_plane *plane,
>>>>>>>    		drm_object_attach_property(&plane->base, config-
>>>>>> prop_src_y, 0);
>>>>>>>    		drm_object_attach_property(&plane->base, config-
>>>>>> prop_src_w, 0);
>>>>>>>    		drm_object_attach_property(&plane->base, config-
>>>>>> prop_src_h, 0);
>>>>>>> +		drm_object_attach_property(&plane->base, config-
>>>>>> dirty_rects_property, 0);
>>>>>>>    	}
>>>>>>>
>>>>>>>    	if (config->allow_fb_modifiers)
>>>>>>> diff --git a/include/drm/drm_mode_config.h
>>>>> b/include/drm/drm_mode_config.h
>>>>>>> index e5f3b43014e1..65f64eb04c0c 100644
>>>>>>> --- a/include/drm/drm_mode_config.h
>>>>>>> +++ b/include/drm/drm_mode_config.h
>>>>>>> @@ -599,6 +599,11 @@ struct drm_mode_config {
>>>>>>>    	 * &drm_crtc.
>>>>>>>    	 */
>>>>>>>    	struct drm_property *prop_crtc_id;
>>>>>>> +	/**
>>>>>>> +	 * @dirty_rects_property: Optional plane property to mark damaged
>>>>>>> +	 * regions on the plane framebuffer.
>>>>>> What exactly would the blob contain?
>>>>>>
>>>>>> The comment seems to be implying these are in fb coordiantes as
>>>>>> opposed to plane crtc coordinates? Not sure which would be more
>>>>>> convenient. At least if they're fb coordinates then you probably
>>>>>> want some helpers to translate/rotate/scale those rects to the
>>>>>> crtc coordinates. Actual use depends on the driver/hw I suppose.
>>>>> Yeah I think we also should add a decoded state to the drm_plane_state,
>>>>> which has the full structure and all the details.
>>>> Hi Daniel,
>>>>
>>>> I am working on this functionality to implement the helper functions to
>>>> convert dirty clips from framebuffer coordinates to planes coordinates
>>>> and finally to crtc coordinates.
>>>>
>>>> Is there a reason we should have decoded dirty clips in plane_state ?
>>>> I was thinking to have the clips remain in blob property and decode
>>>> them when needed with the helper function which accept plane state and
>>>> similarly another helper for crtc coordinates. So driver call these helper
>>>> only if there is a need for dirty clips and otherwise can ignore.
>>> Immediately decoding the state blobs is simply best practices, to avoid
>>> duplicated code and subtle differences in driver behaviour. If there's a
>>> good reason for completely different behaviour (like crtc vs plane
>>> coordinate based damage tracking), then decoding using a helper,
>>> on-demand, seems sensible.
>>>
>>> I'd still think we should store the resulting derived state in
>>> drm_crtc/plane_state, like we do for the plane clipping helpers (at least
>>> after Ville's latest patch series has landed).
>>> -Daniel
>> The idea here would be to use a helper iterator to construct the new
>> coordinates needed; the iterator being initialized with the blob.
>>
>> The reason not to store the decoded state is unnecessary duplication. While
>> I guess most drivers would use crtc damage tracking, potentially there might
>> be three coordinate systems used by drivers: Plane fb coordinates, Plane
>> crtc coordinates and crtc coordinates. And typically they'd be used only
>> once...
> Well I was thinking of a helper to compute a single, overall clip rect for
> the entire crtc. Which would then also need to take into account various
> plane state changes, and stuff like background color. For a simple
> coordinate system transform iterators sound like a good idea, but honestly
> I don't expect that to be a common use-case in drivers.

So far, the drivers that have shown interest in this (VMware and 
DisplayLink) appear to be mostly interested in getting their hands on
a set of cliprects (rather than a bounding box) in crtc coordinates. 
While we recognize the user-space apps will want to hand over the set of 
cliprects in fb coordinates. We're also working on a solution where we, 
as generically as possible want to be able to attach a remoting server 
(like a VNC server) to KMS and that solution would probably want the 
same thing. For these things I think an iterator would be a good solution.

However that doesn't really stop us from, based on other driver's use 
cases, add additional plane / crtc state and use the iterators to 
populate that. Given that, do you think that iterators computing 
transformed coordinates would be a reasonable first step? That would 
help us enable this path in vmwgfx and start flushing and test page-flip 
with damage on real applications and also to figure out a reasonable 
atomic counterpart to the dirty ioctl.

Thanks,
Thomas

> Either it's about
> uploading the right dirty data in each plane, or about uploading the right
> final/blended pixels to the screen (in which case all other state changes
> that can affect colors must be taken into account too).
> -Daniel
>

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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Daniel Vetter]

On Mon, Mar 05, 2018 at 11:01:42AM +0100, Thomas Hellstrom wrote:
> On 03/05/2018 10:39 AM, Daniel Vetter wrote:
> > On Mon, Mar 05, 2018 at 10:22:09AM +0100, Thomas Hellstrom wrote:
> > > On 03/05/2018 09:37 AM, Daniel Vetter wrote:
> > > > On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
> > > > > > -----Original Message-----
> > > > > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
> > > > > > Of Daniel Vetter
> > > > > > Sent: Thursday, December 21, 2017 5:11 AM
> > > > > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
> > > > > > devel@lists.freedesktop.org; Lukasz Spintzyk
> > > > > > <lukasz.spintzyk@displaylink.com>
> > > > > > Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
> > > > > > drm_plane
> > > > > > 
> > > > > > On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
> > > > > > > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
> > > > > > > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> > > > > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> > > > > > > > ---
> > > > > > > >    drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
> > > > > > > >    drivers/gpu/drm/drm_mode_config.c |  6 ++++++
> > > > > > > >    drivers/gpu/drm/drm_plane.c       |  1 +
> > > > > > > >    include/drm/drm_mode_config.h     |  5 +++++
> > > > > > > >    include/drm/drm_plane.h           |  3 +++
> > > > > > > >    5 files changed, 25 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > > > > > b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > index b76d49218cf1..cd3b4ed7b04c 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > > > > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
> > > > > > drm_plane *plane,
> > > > > > > >    		state->rotation = val;
> > > > > > > >    	} else if (property == plane->zpos_property) {
> > > > > > > >    		state->zpos = val;
> > > > > > > > +	} else if (property == config->dirty_rects_property) {
> > > > > > > > +		bool replaced = false;
> > > > > > > > +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> > > > > > > > +					&state->dirty_blob,
> > > > > > > > +					val,
> > > > > > > > +					-1,
> > > > > > > > +					&replaced);
> > > > > > > > +		return ret;
> > > > > > > >    	} else if (plane->funcs->atomic_set_property) {
> > > > > > > >    		return plane->funcs->atomic_set_property(plane, state,
> > > > > > > >    				property, val);
> > > > > > > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
> > > > > > drm_plane *plane,
> > > > > > > >    		*val = state->rotation;
> > > > > > > >    	} else if (property == plane->zpos_property) {
> > > > > > > >    		*val = state->zpos;
> > > > > > > > +	} else if (property == config->dirty_rects_property) {
> > > > > > > > +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
> > > > > > b/drivers/gpu/drm/drm_mode_config.c
> > > > > > > > index bc5c46306b3d..d5f1021c6ece 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > > > > > > @@ -293,6 +293,12 @@ static int
> > > > > > drm_mode_create_standard_properties(struct drm_device *dev)
> > > > > > > >    		return -ENOMEM;
> > > > > > > >    	dev->mode_config.prop_crtc_id = prop;
> > > > > > > > 
> > > > > > > > +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > > > > > > > +			"DIRTY_RECTS", 0);
> > > > > > > > +	if (!prop)
> > > > > > > > +		return -ENOMEM;
> > > > > > > > +	dev->mode_config.dirty_rects_property = prop;
> > > > > > > > +
> > > > > > > >    	prop = drm_property_create_bool(dev,
> > > > > > DRM_MODE_PROP_ATOMIC,
> > > > > > > >    			"ACTIVE");
> > > > > > > >    	if (!prop)
> > > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > > > b/drivers/gpu/drm/drm_plane.c
> > > > > > > > index 37a93cdffb4a..add110f025e5 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
> > > > > > *dev, struct drm_plane *plane,
> > > > > > > >    		drm_object_attach_property(&plane->base, config-
> > > > > > > prop_src_y, 0);
> > > > > > > >    		drm_object_attach_property(&plane->base, config-
> > > > > > > prop_src_w, 0);
> > > > > > > >    		drm_object_attach_property(&plane->base, config-
> > > > > > > prop_src_h, 0);
> > > > > > > > +		drm_object_attach_property(&plane->base, config-
> > > > > > > dirty_rects_property, 0);
> > > > > > > >    	}
> > > > > > > > 
> > > > > > > >    	if (config->allow_fb_modifiers)
> > > > > > > > diff --git a/include/drm/drm_mode_config.h
> > > > > > b/include/drm/drm_mode_config.h
> > > > > > > > index e5f3b43014e1..65f64eb04c0c 100644
> > > > > > > > --- a/include/drm/drm_mode_config.h
> > > > > > > > +++ b/include/drm/drm_mode_config.h
> > > > > > > > @@ -599,6 +599,11 @@ struct drm_mode_config {
> > > > > > > >    	 * &drm_crtc.
> > > > > > > >    	 */
> > > > > > > >    	struct drm_property *prop_crtc_id;
> > > > > > > > +	/**
> > > > > > > > +	 * @dirty_rects_property: Optional plane property to mark damaged
> > > > > > > > +	 * regions on the plane framebuffer.
> > > > > > > What exactly would the blob contain?
> > > > > > > 
> > > > > > > The comment seems to be implying these are in fb coordiantes as
> > > > > > > opposed to plane crtc coordinates? Not sure which would be more
> > > > > > > convenient. At least if they're fb coordinates then you probably
> > > > > > > want some helpers to translate/rotate/scale those rects to the
> > > > > > > crtc coordinates. Actual use depends on the driver/hw I suppose.
> > > > > > Yeah I think we also should add a decoded state to the drm_plane_state,
> > > > > > which has the full structure and all the details.
> > > > > Hi Daniel,
> > > > > 
> > > > > I am working on this functionality to implement the helper functions to
> > > > > convert dirty clips from framebuffer coordinates to planes coordinates
> > > > > and finally to crtc coordinates.
> > > > > 
> > > > > Is there a reason we should have decoded dirty clips in plane_state ?
> > > > > I was thinking to have the clips remain in blob property and decode
> > > > > them when needed with the helper function which accept plane state and
> > > > > similarly another helper for crtc coordinates. So driver call these helper
> > > > > only if there is a need for dirty clips and otherwise can ignore.
> > > > Immediately decoding the state blobs is simply best practices, to avoid
> > > > duplicated code and subtle differences in driver behaviour. If there's a
> > > > good reason for completely different behaviour (like crtc vs plane
> > > > coordinate based damage tracking), then decoding using a helper,
> > > > on-demand, seems sensible.
> > > > 
> > > > I'd still think we should store the resulting derived state in
> > > > drm_crtc/plane_state, like we do for the plane clipping helpers (at least
> > > > after Ville's latest patch series has landed).
> > > > -Daniel
> > > The idea here would be to use a helper iterator to construct the new
> > > coordinates needed; the iterator being initialized with the blob.
> > > 
> > > The reason not to store the decoded state is unnecessary duplication. While
> > > I guess most drivers would use crtc damage tracking, potentially there might
> > > be three coordinate systems used by drivers: Plane fb coordinates, Plane
> > > crtc coordinates and crtc coordinates. And typically they'd be used only
> > > once...
> > Well I was thinking of a helper to compute a single, overall clip rect for
> > the entire crtc. Which would then also need to take into account various
> > plane state changes, and stuff like background color. For a simple
> > coordinate system transform iterators sound like a good idea, but honestly
> > I don't expect that to be a common use-case in drivers.
> 
> So far, the drivers that have shown interest in this (VMware and
> DisplayLink) appear to be mostly interested in getting their hands on
> a set of cliprects (rather than a bounding box) in crtc coordinates. While
> we recognize the user-space apps will want to hand over the set of cliprects
> in fb coordinates. We're also working on a solution where we, as generically
> as possible want to be able to attach a remoting server (like a VNC server)
> to KMS and that solution would probably want the same thing. For these
> things I think an iterator would be a good solution.
> 
> However that doesn't really stop us from, based on other driver's use cases,
> add additional plane / crtc state and use the iterators to populate that.
> Given that, do you think that iterators computing transformed coordinates
> would be a reasonable first step? That would help us enable this path in
> vmwgfx and start flushing and test page-flip with damage on real
> applications and also to figure out a reasonable atomic counterpart to the
> dirty ioctl.

Oh, I'd have expected that you folks want fb-coordinate cliprects, since
you upload the buffers. Otoh the difference is fairly irrelevant if you
only have one framebuffer that has to be full-screen and unscaled, so not
exactly sure why you want to transform to crtc coordinates (since for you
they're the same I'd assume).

Anyway, if you see I real use for the iterator style I'll of course not
block it :-) I simply didn't see the use-case, which is why I ignored it.
And once another driver that wants a single crtc bounding box implements
support, that team can type the helper infrastructure (that's not on you
for sure).

I guess we've reached the point where talking in the abstract only leads
to misunderstanding, and a bit of (driver) code will be the next step.
-Daniel
> 
> Thanks,
> Thomas
> 
> > Either it's about
> > uploading the right dirty data in each plane, or about uploading the right
> > final/blended pixels to the screen (in which case all other state changes
> > that can affect colors must be taken into account too).
> > -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: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Thomas Hellstrom]

On 03/06/2018 08:08 AM, Daniel Vetter wrote:
> On Mon, Mar 05, 2018 at 11:01:42AM +0100, Thomas Hellstrom wrote:
>> On 03/05/2018 10:39 AM, Daniel Vetter wrote:
>>> On Mon, Mar 05, 2018 at 10:22:09AM +0100, Thomas Hellstrom wrote:
>>>> On 03/05/2018 09:37 AM, Daniel Vetter wrote:
>>>>> On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf
>>>>>>> Of Daniel Vetter
>>>>>>> Sent: Thursday, December 21, 2017 5:11 AM
>>>>>>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>> Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
>>>>>>> devel@lists.freedesktop.org; Lukasz Spintzyk
>>>>>>> <lukasz.spintzyk@displaylink.com>
>>>>>>> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
>>>>>>> drm_plane
>>>>>>>
>>>>>>> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
>>>>>>>> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
>>>>>>>>> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
>>>>>>>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
>>>>>>>>>     drivers/gpu/drm/drm_mode_config.c |  6 ++++++
>>>>>>>>>     drivers/gpu/drm/drm_plane.c       |  1 +
>>>>>>>>>     include/drm/drm_mode_config.h     |  5 +++++
>>>>>>>>>     include/drm/drm_plane.h           |  3 +++
>>>>>>>>>     5 files changed, 25 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
>>>>>>> b/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> index b76d49218cf1..cd3b4ed7b04c 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
>>>>>>> drm_plane *plane,
>>>>>>>>>     		state->rotation = val;
>>>>>>>>>     	} else if (property == plane->zpos_property) {
>>>>>>>>>     		state->zpos = val;
>>>>>>>>> +	} else if (property == config->dirty_rects_property) {
>>>>>>>>> +		bool replaced = false;
>>>>>>>>> +		int ret = drm_atomic_replace_property_blob_from_id(dev,
>>>>>>>>> +					&state->dirty_blob,
>>>>>>>>> +					val,
>>>>>>>>> +					-1,
>>>>>>>>> +					&replaced);
>>>>>>>>> +		return ret;
>>>>>>>>>     	} else if (plane->funcs->atomic_set_property) {
>>>>>>>>>     		return plane->funcs->atomic_set_property(plane, state,
>>>>>>>>>     				property, val);
>>>>>>>>> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
>>>>>>> drm_plane *plane,
>>>>>>>>>     		*val = state->rotation;
>>>>>>>>>     	} else if (property == plane->zpos_property) {
>>>>>>>>>     		*val = state->zpos;
>>>>>>>>> +	} else if (property == config->dirty_rects_property) {
>>>>>>>>> +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c
>>>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>>>>>> index bc5c46306b3d..d5f1021c6ece 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>>>>>> @@ -293,6 +293,12 @@ static int
>>>>>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>>>>>>>     		return -ENOMEM;
>>>>>>>>>     	dev->mode_config.prop_crtc_id = prop;
>>>>>>>>>
>>>>>>>>> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>>>>>>>>> +			"DIRTY_RECTS", 0);
>>>>>>>>> +	if (!prop)
>>>>>>>>> +		return -ENOMEM;
>>>>>>>>> +	dev->mode_config.dirty_rects_property = prop;
>>>>>>>>> +
>>>>>>>>>     	prop = drm_property_create_bool(dev,
>>>>>>> DRM_MODE_PROP_ATOMIC,
>>>>>>>>>     			"ACTIVE");
>>>>>>>>>     	if (!prop)
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_plane.c
>>>>>>> b/drivers/gpu/drm/drm_plane.c
>>>>>>>>> index 37a93cdffb4a..add110f025e5 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>>>>>>> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
>>>>>>> *dev, struct drm_plane *plane,
>>>>>>>>>     		drm_object_attach_property(&plane->base, config-
>>>>>>>> prop_src_y, 0);
>>>>>>>>>     		drm_object_attach_property(&plane->base, config-
>>>>>>>> prop_src_w, 0);
>>>>>>>>>     		drm_object_attach_property(&plane->base, config-
>>>>>>>> prop_src_h, 0);
>>>>>>>>> +		drm_object_attach_property(&plane->base, config-
>>>>>>>> dirty_rects_property, 0);
>>>>>>>>>     	}
>>>>>>>>>
>>>>>>>>>     	if (config->allow_fb_modifiers)
>>>>>>>>> diff --git a/include/drm/drm_mode_config.h
>>>>>>> b/include/drm/drm_mode_config.h
>>>>>>>>> index e5f3b43014e1..65f64eb04c0c 100644
>>>>>>>>> --- a/include/drm/drm_mode_config.h
>>>>>>>>> +++ b/include/drm/drm_mode_config.h
>>>>>>>>> @@ -599,6 +599,11 @@ struct drm_mode_config {
>>>>>>>>>     	 * &drm_crtc.
>>>>>>>>>     	 */
>>>>>>>>>     	struct drm_property *prop_crtc_id;
>>>>>>>>> +	/**
>>>>>>>>> +	 * @dirty_rects_property: Optional plane property to mark damaged
>>>>>>>>> +	 * regions on the plane framebuffer.
>>>>>>>> What exactly would the blob contain?
>>>>>>>>
>>>>>>>> The comment seems to be implying these are in fb coordiantes as
>>>>>>>> opposed to plane crtc coordinates? Not sure which would be more
>>>>>>>> convenient. At least if they're fb coordinates then you probably
>>>>>>>> want some helpers to translate/rotate/scale those rects to the
>>>>>>>> crtc coordinates. Actual use depends on the driver/hw I suppose.
>>>>>>> Yeah I think we also should add a decoded state to the drm_plane_state,
>>>>>>> which has the full structure and all the details.
>>>>>> Hi Daniel,
>>>>>>
>>>>>> I am working on this functionality to implement the helper functions to
>>>>>> convert dirty clips from framebuffer coordinates to planes coordinates
>>>>>> and finally to crtc coordinates.
>>>>>>
>>>>>> Is there a reason we should have decoded dirty clips in plane_state ?
>>>>>> I was thinking to have the clips remain in blob property and decode
>>>>>> them when needed with the helper function which accept plane state and
>>>>>> similarly another helper for crtc coordinates. So driver call these helper
>>>>>> only if there is a need for dirty clips and otherwise can ignore.
>>>>> Immediately decoding the state blobs is simply best practices, to avoid
>>>>> duplicated code and subtle differences in driver behaviour. If there's a
>>>>> good reason for completely different behaviour (like crtc vs plane
>>>>> coordinate based damage tracking), then decoding using a helper,
>>>>> on-demand, seems sensible.
>>>>>
>>>>> I'd still think we should store the resulting derived state in
>>>>> drm_crtc/plane_state, like we do for the plane clipping helpers (at least
>>>>> after Ville's latest patch series has landed).
>>>>> -Daniel
>>>> The idea here would be to use a helper iterator to construct the new
>>>> coordinates needed; the iterator being initialized with the blob.
>>>>
>>>> The reason not to store the decoded state is unnecessary duplication. While
>>>> I guess most drivers would use crtc damage tracking, potentially there might
>>>> be three coordinate systems used by drivers: Plane fb coordinates, Plane
>>>> crtc coordinates and crtc coordinates. And typically they'd be used only
>>>> once...
>>> Well I was thinking of a helper to compute a single, overall clip rect for
>>> the entire crtc. Which would then also need to take into account various
>>> plane state changes, and stuff like background color. For a simple
>>> coordinate system transform iterators sound like a good idea, but honestly
>>> I don't expect that to be a common use-case in drivers.
>> So far, the drivers that have shown interest in this (VMware and
>> DisplayLink) appear to be mostly interested in getting their hands on
>> a set of cliprects (rather than a bounding box) in crtc coordinates. While
>> we recognize the user-space apps will want to hand over the set of cliprects
>> in fb coordinates. We're also working on a solution where we, as generically
>> as possible want to be able to attach a remoting server (like a VNC server)
>> to KMS and that solution would probably want the same thing. For these
>> things I think an iterator would be a good solution.
>>
>> However that doesn't really stop us from, based on other driver's use cases,
>> add additional plane / crtc state and use the iterators to populate that.
>> Given that, do you think that iterators computing transformed coordinates
>> would be a reasonable first step? That would help us enable this path in
>> vmwgfx and start flushing and test page-flip with damage on real
>> applications and also to figure out a reasonable atomic counterpart to the
>> dirty ioctl.
> Oh, I'd have expected that you folks want fb-coordinate cliprects, since
> you upload the buffers. Otoh the difference is fairly irrelevant if you
> only have one framebuffer that has to be full-screen and unscaled, so not
> exactly sure why you want to transform to crtc coordinates (since for you
> they're the same I'd assume).

> Anyway, if you see I real use for the iterator style I'll of course not
> block it :-) I simply didn't see the use-case, which is why I ignored it.
> And once another driver that wants a single crtc bounding box implements
> support, that team can type the helper infrastructure (that's not on you
> for sure).
>
> I guess we've reached the point where talking in the abstract only leads
> to misunderstanding, and a bit of (driver) code will be the next step.

Agreed!

/Thomas

> -Daniel
>> Thanks,
>> Thomas
>>
>>> Either it's about
>>> uploading the right dirty data in each plane, or about uploading the right
>>> final/blended pixels to the screen (in which case all other state changes
>>> that can affect colors must be taken into account too).
>>> -Daniel
>>>

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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Thomas Hellstrom]

Hi, Lukasz!

(Sorry for top-posting).

We have Deepak from our team working on the same subject. I think he's 
in over the holidays so I'll add him to the CC list.

Adding damage to the plane state is, IMO the correct way to do it. 
However, from your proposal it's not clear whether damage is given in 
the plane-, crtc- or framebuffer coordinates. The last conclusion from 
our email thread discussion was that they should be given in framebuffer 
coordinates with helpers to compute plane coordinates or crtc 
coordinates. The reason being that it's easier for user-space apps to 
send damage that way, and again, we have the full information that we 
can clip and scale if necessary. Most drivers probably want the damage 
in clipped plane- or crtc coordinates. Helpers could for example be in 
the form of region iterators.

Full (multi-rect) damage regions are OK with us, although we should keep 
in mind that we won't be able to compute region unions in the kernel 
(yet?). Implying: Should we forbid overlapping rects at the interface 
level or should we just recommend rects not to be overlapping? The 
former would be pretty hard to enforce efficiently.

Another thing we should think about is how to use this interface for the 
legacy "dirtyfb" call. Probably we need to clear the damage property on 
each state-update, or set a flag that "this is a dirtyfb state update".

IMO we should also have as an end goal of this work to have gnome-shell 
on drm sending damage regions on page-flip, which means either porting 
gnome-shell to atomic or set up a new legacy page-flip-with-atomic ioctl.

/Thomas


On 12/21/2017 12:10 PM, Lukasz Spintzyk wrote:
> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> ---
>   drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
>   drivers/gpu/drm/drm_mode_config.c |  6 ++++++
>   drivers/gpu/drm/drm_plane.c       |  1 +
>   include/drm/drm_mode_config.h     |  5 +++++
>   include/drm/drm_plane.h           |  3 +++
>   5 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b76d49218cf1..cd3b4ed7b04c 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		state->rotation = val;
>   	} else if (property == plane->zpos_property) {
>   		state->zpos = val;
> +	} else if (property == config->dirty_rects_property) {
> +		bool replaced = false;
> +		int ret = drm_atomic_replace_property_blob_from_id(dev,
> +					&state->dirty_blob,
> +					val,
> +					-1,
> +					&replaced);
> +		return ret;
>   	} else if (plane->funcs->atomic_set_property) {
>   		return plane->funcs->atomic_set_property(plane, state,
>   				property, val);
> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   		*val = state->rotation;
>   	} else if (property == plane->zpos_property) {
>   		*val = state->zpos;
> +	} else if (property == config->dirty_rects_property) {
> +		*val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index bc5c46306b3d..d5f1021c6ece 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->mode_config.prop_crtc_id = prop;
>   
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +			"DIRTY_RECTS", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.dirty_rects_property = prop;
> +
>   	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>   			"ACTIVE");
>   	if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 37a93cdffb4a..add110f025e5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>   		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
>   		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
>   		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> +		drm_object_attach_property(&plane->base, config->dirty_rects_property, 0);
>   	}
>   
>   	if (config->allow_fb_modifiers)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5f3b43014e1..65f64eb04c0c 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -599,6 +599,11 @@ struct drm_mode_config {
>   	 * &drm_crtc.
>   	 */
>   	struct drm_property *prop_crtc_id;
> +	/**
> +	 * @dirty_rects_property: Optional plane property to mark damaged
> +	 * regions on the plane framebuffer.
> +	 */
> +	struct drm_property *dirty_rects_property;
>   	/**
>   	 * @prop_active: Default atomic CRTC property to control the active
>   	 * state, which is the simplified implementation for DPMS in atomic
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..7d45b164ccce 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -131,6 +131,9 @@ struct drm_plane_state {
>   	 */
>   	struct drm_crtc_commit *commit;
>   
> +	/* Optional blob property with damaged regions. */
> +	struct drm_property_blob *dirty_blob;
> +
>   	struct drm_atomic_state *state;
>   };
>   


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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Lukasz Spintzyk]

[-- Attachment #1.1: Type: text/plain, Size: 7201 bytes --]



On 28/12/2017 10:03, Thomas Hellstrom wrote:
> Hi, Lukasz!
>
> (Sorry for top-posting).
>
> We have Deepak from our team working on the same subject. I think he's 
> in over the holidays so I'll add him to the CC list.
Great!
>
> Adding damage to the plane state is, IMO the correct way to do it. 
> However, from your proposal it's not clear whether damage is given in 
> the plane-, crtc- or framebuffer coordinates. The last conclusion from 
> our email thread discussion was that they should be given in 
> framebuffer coordinates with helpers to compute plane coordinates or 
> crtc coordinates. The reason being that it's easier for user-space 
> apps to send damage that way, and again, we have the full information 
> that we can clip and scale if necessary. Most drivers probably want 
> the damage in clipped plane- or crtc coordinates. Helpers could for 
> example be in the form of region iterators.
Personally i don't know the difference between plane rects and 
framebuffer rects. I don't know what would be better. I was thinking 
about coordinates of framebuffer that is attached to drm_plane_state.
>
> Full (multi-rect) damage regions are OK with us, although we should 
> keep in mind that we won't be able to compute region unions in the 
> kernel (yet?). Implying: Should we forbid overlapping rects at the 
> interface level or should we just recommend rects not to be 
> overlapping? The former would be pretty hard to enforce efficiently.
I would be for recommendation. We can add some helper functions to 
combine rects and set some limits on number of rects to prevent abuse of 
that interface.
>
> Another thing we should think about is how to use this interface for 
> the legacy "dirtyfb" call. Probably we need to clear the damage 
> property on each state-update, or set a flag that "this is a dirtyfb 
> state update".
>
> IMO we should also have as an end goal of this work to have 
> gnome-shell on drm sending damage regions on page-flip, which means 
> either porting gnome-shell to atomic or set up a new legacy 
> page-flip-with-atomic ioctl.
Can't we reuse dirtyfb ioctl for this purpose? It would be called before 
page_flip ioctl?
>
> /Thomas
>
>
> On 12/21/2017 12:10 PM, Lukasz Spintzyk wrote:
>> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
>> ---
>>   drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++
>>   drivers/gpu/drm/drm_mode_config.c |  6 ++++++
>>   drivers/gpu/drm/drm_plane.c       |  1 +
>>   include/drm/drm_mode_config.h     |  5 +++++
>>   include/drm/drm_plane.h           |  3 +++
>>   5 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index b76d49218cf1..cd3b4ed7b04c 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct 
>> drm_plane *plane,
>>           state->rotation = val;
>>       } else if (property == plane->zpos_property) {
>>           state->zpos = val;
>> +    } else if (property == config->dirty_rects_property) {
>> +        bool replaced = false;
>> +        int ret = drm_atomic_replace_property_blob_from_id(dev,
>> +                    &state->dirty_blob,
>> +                    val,
>> +                    -1,
>> +                    &replaced);
>> +        return ret;
>>       } else if (plane->funcs->atomic_set_property) {
>>           return plane->funcs->atomic_set_property(plane, state,
>>                   property, val);
>> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane 
>> *plane,
>>           *val = state->rotation;
>>       } else if (property == plane->zpos_property) {
>>           *val = state->zpos;
>> +    } else if (property == config->dirty_rects_property) {
>> +        *val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c 
>> b/drivers/gpu/drm/drm_mode_config.c
>> index bc5c46306b3d..d5f1021c6ece 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -293,6 +293,12 @@ static int 
>> drm_mode_create_standard_properties(struct drm_device *dev)
>>           return -ENOMEM;
>>       dev->mode_config.prop_crtc_id = prop;
>>   +    prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> +            "DIRTY_RECTS", 0);
>> +    if (!prop)
>> +        return -ENOMEM;
>> +    dev->mode_config.dirty_rects_property = prop;
>> +
>>       prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>>               "ACTIVE");
>>       if (!prop)
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 37a93cdffb4a..add110f025e5 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device 
>> *dev, struct drm_plane *plane,
>>           drm_object_attach_property(&plane->base, 
>> config->prop_src_y, 0);
>>           drm_object_attach_property(&plane->base, 
>> config->prop_src_w, 0);
>>           drm_object_attach_property(&plane->base, 
>> config->prop_src_h, 0);
>> +        drm_object_attach_property(&plane->base, 
>> config->dirty_rects_property, 0);
>>       }
>>         if (config->allow_fb_modifiers)
>> diff --git a/include/drm/drm_mode_config.h 
>> b/include/drm/drm_mode_config.h
>> index e5f3b43014e1..65f64eb04c0c 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -599,6 +599,11 @@ struct drm_mode_config {
>>        * &drm_crtc.
>>        */
>>       struct drm_property *prop_crtc_id;
>> +    /**
>> +     * @dirty_rects_property: Optional plane property to mark damaged
>> +     * regions on the plane framebuffer.
>> +     */
>> +    struct drm_property *dirty_rects_property;
>>       /**
>>        * @prop_active: Default atomic CRTC property to control the 
>> active
>>        * state, which is the simplified implementation for DPMS in 
>> atomic
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 8185e3468a23..7d45b164ccce 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -131,6 +131,9 @@ struct drm_plane_state {
>>        */
>>       struct drm_crtc_commit *commit;
>>   +    /* Optional blob property with damaged regions. */
>> +    struct drm_property_blob *dirty_blob;
>> +
>>       struct drm_atomic_state *state;
>>   };
>
>


[-- Attachment #1.2: Type: text/html, Size: 9653 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Thomas Hellstrom]

Hi,

On 01/04/2018 02:52 PM, Lukasz Spintzyk wrote:
>
>
>
> On 28/12/2017 10:03, Thomas Hellstrom wrote:
>> Hi, Lukasz!
>>
>> (Sorry for top-posting).
>>
>> We have Deepak from our team working on the same subject. I think 
>> he's in over the holidays so I'll add him to the CC list.
> Great!
>>
>> Adding damage to the plane state is, IMO the correct way to do it. 
>> However, from your proposal it's not clear whether damage is given in 
>> the plane-, crtc- or framebuffer coordinates. The last conclusion 
>> from our email thread discussion was that they should be given in 
>> framebuffer coordinates with helpers to compute plane coordinates or 
>> crtc coordinates. The reason being that it's easier for user-space 
>> apps to send damage that way, and again, we have the full information 
>> that we can clip and scale if necessary. Most drivers probably want 
>> the damage in clipped plane- or crtc coordinates. Helpers could for 
>> example be in the form of region iterators.
> Personally i don't know the difference between plane rects and 
> framebuffer rects. I don't know what would be better. I was thinking 
> about coordinates of framebuffer that is attached to drm_plane_state.

A given point with coordinates (0, 0) in the plane coordinate system 
would have (state->crtc_x, state->crtc_y) in the crtc coordinate system 
and (state->src_x, state->src_y) in the framebuffer coordinate system.

So the question is, which is the suitable coordinate system, and where 
is the origin for the damage rects?

>>
>> Full (multi-rect) damage regions are OK with us, although we should 
>> keep in mind that we won't be able to compute region unions in the 
>> kernel (yet?). Implying: Should we forbid overlapping rects at the 
>> interface level or should we just recommend rects not to be 
>> overlapping? The former would be pretty hard to enforce efficiently.
> I would be for recommendation. We can add some helper functions to 
> combine rects and set some limits on number of rects to prevent abuse 
> of that interface.

I agree.

>>
>> Another thing we should think about is how to use this interface for 
>> the legacy "dirtyfb" call. Probably we need to clear the damage 
>> property on each state-update, or set a flag that "this is a dirtyfb 
>> state update".
>>
>> IMO we should also have as an end goal of this work to have 
>> gnome-shell on drm sending damage regions on page-flip, which means 
>> either porting gnome-shell to atomic or set up a new legacy 
>> page-flip-with-atomic ioctl.
> Can't we reuse dirtyfb ioctl for this purpose? It would be called 
> before page_flip ioctl?

No we can't. The dirtyfb ioctl causes an immediate repaint of the 
damaged region.

/Thomas

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

RE: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Deepak Singh Rawat]

Hi Lukasz,

I hope you are doing great. I was busy with some other stuff but now will be working on page-flip damage. At this point I have high level understanding of what changes to make and before I start just wanted to confirm from you, whether you have made any progress to avoid duplicate work.

Thanks,
Deepak

From: Lukasz Spintzyk [mailto:lukasz.spintzyk@displaylink.com] 
Sent: Thursday, January 4, 2018 5:53 AM
To: Thomas Hellstrom <thomas@shipmail.org>; dri-devel@lists.freedesktop.org; daniel.vetter@intel.com; gustavo@padovan.org; seanpaul@chromium.org; airlied@linux.ie; Deepak Singh Rawat <drawat@vmware.com>
Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane



On 28/12/2017 10:03, Thomas Hellstrom wrote:
Hi, Lukasz! 

(Sorry for top-posting). 

We have Deepak from our team working on the same subject. I think he's in over the holidays so I'll add him to the CC list. 
Great!


Adding damage to the plane state is, IMO the correct way to do it. However, from your proposal it's not clear whether damage is given in the plane-, crtc- or framebuffer coordinates. The last conclusion from our email thread discussion was that they should be given in framebuffer coordinates with helpers to compute plane coordinates or crtc coordinates. The reason being that it's easier for user-space apps to send damage that way, and again, we have the full information that we can clip and scale if necessary. Most drivers probably want the damage in clipped plane- or crtc coordinates. Helpers could for example be in the form of region iterators. 
Personally i don't know the difference between plane rects and framebuffer rects. I don't know what would be better. I was thinking about coordinates of framebuffer that is attached to drm_plane_state.


Full (multi-rect) damage regions are OK with us, although we should keep in mind that we won't be able to compute region unions in the kernel (yet?). Implying: Should we forbid overlapping rects at the interface level or should we just recommend rects not to be overlapping? The former would be pretty hard to enforce efficiently. 
I would be for recommendation. We can add some helper functions to combine rects and set some limits on number of rects to prevent abuse of that interface.


Another thing we should think about is how to use this interface for the legacy "dirtyfb" call. Probably we need to clear the damage property on each state-update, or set a flag that "this is a dirtyfb state update". 

IMO we should also have as an end goal of this work to have gnome-shell on drm sending damage regions on page-flip, which means either porting gnome-shell to atomic or set up a new legacy page-flip-with-atomic ioctl. 
Can't we reuse dirtyfb ioctl for this purpose? It would be called before page_flip ioctl?


/Thomas 


On 12/21/2017 12:10 PM, Lukasz Spintzyk wrote: 

Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 
Signed-off-by: Lukasz Spintzyk mailto:lukasz.spintzyk@displaylink.com 
--- 
  drivers/gpu/drm/drm_atomic.c      | 10 ++++++++++ 
  drivers/gpu/drm/drm_mode_config.c |  6 ++++++ 
  drivers/gpu/drm/drm_plane.c       |  1 + 
  include/drm/drm_mode_config.h     |  5 +++++ 
  include/drm/drm_plane.h           |  3 +++ 
  5 files changed, 25 insertions(+) 

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c 
index b76d49218cf1..cd3b4ed7b04c 100644 
--- a/drivers/gpu/drm/drm_atomic.c 
+++ b/drivers/gpu/drm/drm_atomic.c 
@@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, 
          state->rotation = val; 
      } else if (property == plane->zpos_property) { 
          state->zpos = val; 
+    } else if (property == config->dirty_rects_property) { 
+        bool replaced = false; 
+        int ret = drm_atomic_replace_property_blob_from_id(dev, 
+                    &state->dirty_blob, 
+                    val, 
+                    -1, 
+                    &replaced); 
+        return ret; 
      } else if (plane->funcs->atomic_set_property) { 
          return plane->funcs->atomic_set_property(plane, state, 
                  property, val); 
@@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, 
          *val = state->rotation; 
      } else if (property == plane->zpos_property) { 
          *val = state->zpos; 
+    } else if (property == config->dirty_rects_property) { 
+        *val = (state->dirty_blob) ? state->dirty_blob->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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c 
index bc5c46306b3d..d5f1021c6ece 100644 
--- a/drivers/gpu/drm/drm_mode_config.c 
+++ b/drivers/gpu/drm/drm_mode_config.c 
@@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) 
          return -ENOMEM; 
      dev->mode_config.prop_crtc_id = prop; 
  +    prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, 
+            "DIRTY_RECTS", 0); 
+    if (!prop) 
+        return -ENOMEM; 
+    dev->mode_config.dirty_rects_property = prop; 
+ 
      prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC, 
              "ACTIVE"); 
      if (!prop) 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c 
index 37a93cdffb4a..add110f025e5 100644 
--- a/drivers/gpu/drm/drm_plane.c 
+++ b/drivers/gpu/drm/drm_plane.c 
@@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, 
          drm_object_attach_property(&plane->base, config->prop_src_y, 0); 
          drm_object_attach_property(&plane->base, config->prop_src_w, 0); 
          drm_object_attach_property(&plane->base, config->prop_src_h, 0); 
+        drm_object_attach_property(&plane->base, config->dirty_rects_property, 0); 
      } 
        if (config->allow_fb_modifiers) 
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h 
index e5f3b43014e1..65f64eb04c0c 100644 
--- a/include/drm/drm_mode_config.h 
+++ b/include/drm/drm_mode_config.h 
@@ -599,6 +599,11 @@ struct drm_mode_config { 
       * &drm_crtc. 
       */ 
      struct drm_property *prop_crtc_id; 
+    /** 
+     * @dirty_rects_property: Optional plane property to mark damaged 
+     * regions on the plane framebuffer. 
+     */ 
+    struct drm_property *dirty_rects_property; 
      /** 
       * @prop_active: Default atomic CRTC property to control the active 
       * state, which is the simplified implementation for DPMS in atomic 
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h 
index 8185e3468a23..7d45b164ccce 100644 
--- a/include/drm/drm_plane.h 
+++ b/include/drm/drm_plane.h 
@@ -131,6 +131,9 @@ struct drm_plane_state { 
       */ 
      struct drm_crtc_commit *commit; 
  +    /* Optional blob property with damaged regions. */ 
+    struct drm_property_blob *dirty_blob; 
+ 
      struct drm_atomic_state *state; 
  }; 
  


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