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(+)
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; };
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;@@ -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;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
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;@@ -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;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
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
-----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;@@ -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;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=
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;@@ -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;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=
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;@@ -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;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=
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;@@ -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;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=
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
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
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
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;@@ -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;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; };
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; };
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
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@lists.freedesktop.org
-
Daniel Vetter -
Deepak Singh Rawat -
Lukasz Spintzyk -
Thomas Hellstrom -
Thomas Hellstrom -
Ville Syrjälä