* [PATCH 0/1] Damage rectangles interface for DRM
@ 2017-12-21 11:10 Lukasz Spintzyk
2017-12-21 11:10 ` [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane Lukasz Spintzyk
0 siblings, 1 reply; 16+ messages in thread
From: Lukasz Spintzyk @ 2017-12-21 11:10 UTC (permalink / raw)
To: dri-devel, daniel.vetter, gustavo, seanpaul, airlied; +Cc: 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-21 11:10 [PATCH 0/1] Damage rectangles interface for DRM Lukasz Spintzyk
@ 2017-12-21 11:10 ` Lukasz Spintzyk
2017-12-21 12:46 ` Ville Syrjälä
2017-12-28 9:03 ` Thomas Hellstrom
0 siblings, 2 replies; 16+ messages in thread
From: Lukasz Spintzyk @ 2017-12-21 11:10 UTC (permalink / raw)
To: dri-devel, daniel.vetter, gustavo, seanpaul, airlied; +Cc: 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-21 11:10 ` [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane Lukasz Spintzyk
@ 2017-12-21 12:46 ` Ville Syrjälä
2017-12-21 13:10 ` Daniel Vetter
2017-12-28 9:03 ` Thomas Hellstrom
1 sibling, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2017-12-21 12:46 UTC (permalink / raw)
To: Lukasz Spintzyk; +Cc: airlied, daniel.vetter, dri-devel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-21 12:46 ` Ville Syrjälä
@ 2017-12-21 13:10 ` Daniel Vetter
2017-12-22 11:44 ` Lukasz Spintzyk
2018-02-28 21:10 ` Deepak Singh Rawat
0 siblings, 2 replies; 16+ messages in thread
From: Daniel Vetter @ 2017-12-21 13:10 UTC (permalink / raw)
To: Ville Syrjälä
Cc: airlied, daniel.vetter, dri-devel, Lukasz Spintzyk
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-21 13:10 ` Daniel Vetter
@ 2017-12-22 11:44 ` Lukasz Spintzyk
2018-02-28 21:10 ` Deepak Singh Rawat
1 sibling, 0 replies; 16+ messages in thread
From: Lukasz Spintzyk @ 2017-12-22 11:44 UTC (permalink / raw)
To: Daniel Vetter, Ville Syrjälä; +Cc: airlied, daniel.vetter, dri-devel
[-- 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-21 11:10 ` [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane Lukasz Spintzyk
2017-12-21 12:46 ` Ville Syrjälä
@ 2017-12-28 9:03 ` Thomas Hellstrom
2018-01-04 13:52 ` Lukasz Spintzyk
1 sibling, 1 reply; 16+ messages in thread
From: Thomas Hellstrom @ 2017-12-28 9:03 UTC (permalink / raw)
To: Lukasz Spintzyk, dri-devel, daniel.vetter, gustavo, seanpaul,
airlied, Deepak Singh Rawat
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-28 9:03 ` Thomas Hellstrom
@ 2018-01-04 13:52 ` Lukasz Spintzyk
2018-01-04 15:30 ` Thomas Hellstrom
2018-02-07 22:44 ` Deepak Singh Rawat
0 siblings, 2 replies; 16+ messages in thread
From: Lukasz Spintzyk @ 2018-01-04 13:52 UTC (permalink / raw)
To: Thomas Hellstrom, dri-devel, daniel.vetter, gustavo, seanpaul,
airlied, Deepak Singh Rawat
[-- 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-01-04 13:52 ` Lukasz Spintzyk
@ 2018-01-04 15:30 ` Thomas Hellstrom
2018-02-07 22:44 ` Deepak Singh Rawat
1 sibling, 0 replies; 16+ messages in thread
From: Thomas Hellstrom @ 2018-01-04 15:30 UTC (permalink / raw)
To: Lukasz Spintzyk, dri-devel, daniel.vetter, gustavo, seanpaul,
airlied, Deepak Singh Rawat
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-01-04 13:52 ` Lukasz Spintzyk
2018-01-04 15:30 ` Thomas Hellstrom
@ 2018-02-07 22:44 ` Deepak Singh Rawat
1 sibling, 0 replies; 16+ messages in thread
From: Deepak Singh Rawat @ 2018-02-07 22:44 UTC (permalink / raw)
To: Lukasz Spintzyk; +Cc: airlied, dri-devel, daniel.vetter
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2017-12-21 13:10 ` Daniel Vetter
2017-12-22 11:44 ` Lukasz Spintzyk
@ 2018-02-28 21:10 ` Deepak Singh Rawat
2018-03-05 8:37 ` Daniel Vetter
1 sibling, 1 reply; 16+ messages in thread
From: Deepak Singh Rawat @ 2018-02-28 21:10 UTC (permalink / raw)
To: Daniel Vetter
Cc: Thomas Hellstrom, airlied, dri-devel, Lukasz Spintzyk, daniel.vetter
> -----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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-02-28 21:10 ` Deepak Singh Rawat
@ 2018-03-05 8:37 ` Daniel Vetter
2018-03-05 9:22 ` Thomas Hellstrom
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2018-03-05 8:37 UTC (permalink / raw)
To: Deepak Singh Rawat
Cc: Thomas Hellstrom, airlied, dri-devel, Lukasz Spintzyk, 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-03-05 8:37 ` Daniel Vetter
@ 2018-03-05 9:22 ` Thomas Hellstrom
2018-03-05 9:39 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Hellstrom @ 2018-03-05 9:22 UTC (permalink / raw)
To: Daniel Vetter, Deepak Singh Rawat
Cc: airlied, daniel.vetter, Lukasz Spintzyk, dri-devel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-03-05 9:22 ` Thomas Hellstrom
@ 2018-03-05 9:39 ` Daniel Vetter
2018-03-05 10:01 ` Thomas Hellstrom
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2018-03-05 9:39 UTC (permalink / raw)
To: Thomas Hellstrom
Cc: airlied, dri-devel, daniel.vetter, Deepak Singh Rawat, Lukasz Spintzyk
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-03-05 9:39 ` Daniel Vetter
@ 2018-03-05 10:01 ` Thomas Hellstrom
2018-03-06 7:08 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Thomas Hellstrom @ 2018-03-05 10:01 UTC (permalink / raw)
To: Daniel Vetter
Cc: airlied, dri-devel, daniel.vetter, Deepak Singh Rawat, Lukasz Spintzyk
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-03-05 10:01 ` Thomas Hellstrom
@ 2018-03-06 7:08 ` Daniel Vetter
2018-03-06 7:25 ` Thomas Hellstrom
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2018-03-06 7:08 UTC (permalink / raw)
To: Thomas Hellstrom
Cc: airlied, dri-devel, daniel.vetter, Deepak Singh Rawat, Lukasz Spintzyk
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
2018-03-06 7:08 ` Daniel Vetter
@ 2018-03-06 7:25 ` Thomas Hellstrom
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Hellstrom @ 2018-03-06 7:25 UTC (permalink / raw)
To: Daniel Vetter
Cc: airlied, dri-devel, daniel.vetter, Deepak Singh Rawat, Lukasz Spintzyk
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-03-06 7:25 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-21 11:10 [PATCH 0/1] Damage rectangles interface for DRM Lukasz Spintzyk
2017-12-21 11:10 ` [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane Lukasz Spintzyk
2017-12-21 12:46 ` Ville Syrjälä
2017-12-21 13:10 ` Daniel Vetter
2017-12-22 11:44 ` Lukasz Spintzyk
2018-02-28 21:10 ` Deepak Singh Rawat
2018-03-05 8:37 ` Daniel Vetter
2018-03-05 9:22 ` Thomas Hellstrom
2018-03-05 9:39 ` Daniel Vetter
2018-03-05 10:01 ` Thomas Hellstrom
2018-03-06 7:08 ` Daniel Vetter
2018-03-06 7:25 ` Thomas Hellstrom
2017-12-28 9:03 ` Thomas Hellstrom
2018-01-04 13:52 ` Lukasz Spintzyk
2018-01-04 15:30 ` Thomas Hellstrom
2018-02-07 22:44 ` Deepak Singh Rawat
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.