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