| Message ID | 20171221111008.38764-2-lukasz.spintzyk@displaylink.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote: > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > --- > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > drivers/gpu/drm/drm_plane.c | 1 + > include/drm/drm_mode_config.h | 5 +++++ > include/drm/drm_plane.h | 3 +++ > 5 files changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index b76d49218cf1..cd3b4ed7b04c 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > state->rotation = val; > } else if (property == plane->zpos_property) { > state->zpos = val; > + } else if (property == config->dirty_rects_property) { > + bool replaced = false; > + int ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->dirty_blob, > + val, > + -1, > + &replaced); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > *val = state->rotation; > } else if (property == plane->zpos_property) { > *val = state->zpos; > + } else if (property == config->dirty_rects_property) { > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); > } else { > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index bc5c46306b3d..d5f1021c6ece 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_crtc_id = prop; > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > + "DIRTY_RECTS", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.dirty_rects_property = prop; > + > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC, > "ACTIVE"); > if (!prop) > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 37a93cdffb4a..add110f025e5 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, > drm_object_attach_property(&plane->base, config->prop_src_y, 0); > drm_object_attach_property(&plane->base, config->prop_src_w, 0); > drm_object_attach_property(&plane->base, config->prop_src_h, 0); > + drm_object_attach_property(&plane->base, config->dirty_rects_property, 0); > } > > if (config->allow_fb_modifiers) > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index e5f3b43014e1..65f64eb04c0c 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -599,6 +599,11 @@ struct drm_mode_config { > * &drm_crtc. > */ > struct drm_property *prop_crtc_id; > + /** > + * @dirty_rects_property: Optional plane property to mark damaged > + * regions on the plane framebuffer. What exactly would the blob contain? The comment seems to be implying these are in fb coordiantes as opposed to plane crtc coordinates? Not sure which would be more convenient. At least if they're fb coordinates then you probably want some helpers to translate/rotate/scale those rects to the crtc coordinates. Actual use depends on the driver/hw I suppose. > + */ > + struct drm_property *dirty_rects_property; > /** > * @prop_active: Default atomic CRTC property to control the active > * state, which is the simplified implementation for DPMS in atomic > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 8185e3468a23..7d45b164ccce 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -131,6 +131,9 @@ struct drm_plane_state { > */ > struct drm_crtc_commit *commit; > > + /* Optional blob property with damaged regions. */ > + struct drm_property_blob *dirty_blob; > + > struct drm_atomic_state *state; > }; > > -- > 2.15.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote: > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote: > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ > > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > > drivers/gpu/drm/drm_plane.c | 1 + > > include/drm/drm_mode_config.h | 5 +++++ > > include/drm/drm_plane.h | 3 +++ > > 5 files changed, 25 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index b76d49218cf1..cd3b4ed7b04c 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > state->rotation = val; > > } else if (property == plane->zpos_property) { > > state->zpos = val; > > + } else if (property == config->dirty_rects_property) { > > + bool replaced = false; > > + int ret = drm_atomic_replace_property_blob_from_id(dev, > > + &state->dirty_blob, > > + val, > > + -1, > > + &replaced); > > + return ret; > > } else if (plane->funcs->atomic_set_property) { > > return plane->funcs->atomic_set_property(plane, state, > > property, val); > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > > *val = state->rotation; > > } else if (property == plane->zpos_property) { > > *val = state->zpos; > > + } else if (property == config->dirty_rects_property) { > > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; > > } else if (plane->funcs->atomic_get_property) { > > return plane->funcs->atomic_get_property(plane, state, property, val); > > } else { > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index bc5c46306b3d..d5f1021c6ece 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > > return -ENOMEM; > > dev->mode_config.prop_crtc_id = prop; > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > > + "DIRTY_RECTS", 0); > > + if (!prop) > > + return -ENOMEM; > > + dev->mode_config.dirty_rects_property = prop; > > + > > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC, > > "ACTIVE"); > > if (!prop) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > > index 37a93cdffb4a..add110f025e5 100644 > > --- a/drivers/gpu/drm/drm_plane.c > > +++ b/drivers/gpu/drm/drm_plane.c > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, > > drm_object_attach_property(&plane->base, config->prop_src_y, 0); > > drm_object_attach_property(&plane->base, config->prop_src_w, 0); > > drm_object_attach_property(&plane->base, config->prop_src_h, 0); > > + drm_object_attach_property(&plane->base, config->dirty_rects_property, 0); > > } > > > > if (config->allow_fb_modifiers) > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > index e5f3b43014e1..65f64eb04c0c 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -599,6 +599,11 @@ struct drm_mode_config { > > * &drm_crtc. > > */ > > struct drm_property *prop_crtc_id; > > + /** > > + * @dirty_rects_property: Optional plane property to mark damaged > > + * regions on the plane framebuffer. > > What exactly would the blob contain? > > The comment seems to be implying these are in fb coordiantes as > opposed to plane crtc coordinates? Not sure which would be more > convenient. At least if they're fb coordinates then you probably > want some helpers to translate/rotate/scale those rects to the > crtc coordinates. Actual use depends on the driver/hw I suppose. Yeah I think we also should add a decoded state to the drm_plane_state, which has the full structure and all the details. And when we discussed this iirc we've identified a clear need for at least some drivers to deal in crtc dirty rectangles. I think the initial core support should include a helper which takes an atomic update for a given crtc, and converts all the plane dirty rectangles into crtc rectangles. Including any full-plane or full-crtc upgrades needed due to e.g. reposition, changed gamma, changed blendign/zpos or anything else really that would affect the entire plane respectively crtc. That would also provide a really good model for what damage actually means. Plus ofc we need userspace for this, preferrably as a patch to something generic like weston or xfree86-video-modesetting. And an example kernel implementation. Cheers, Daniel > > > + */ > > + struct drm_property *dirty_rects_property; > > /** > > * @prop_active: Default atomic CRTC property to control the active > > * state, which is the simplified implementation for DPMS in atomic > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > index 8185e3468a23..7d45b164ccce 100644 > > --- a/include/drm/drm_plane.h > > +++ b/include/drm/drm_plane.h > > @@ -131,6 +131,9 @@ struct drm_plane_state { > > */ > > struct drm_crtc_commit *commit; > > > > + /* Optional blob property with damaged regions. */ > > + struct drm_property_blob *dirty_blob; > > + > > struct drm_atomic_state *state; > > }; > > > > -- > > 2.15.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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>
Hi, Lukasz! (Sorry for top-posting). We have Deepak from our team working on the same subject. I think he's in over the holidays so I'll add him to the CC list. Adding damage to the plane state is, IMO the correct way to do it. However, from your proposal it's not clear whether damage is given in the plane-, crtc- or framebuffer coordinates. The last conclusion from our email thread discussion was that they should be given in framebuffer coordinates with helpers to compute plane coordinates or crtc coordinates. The reason being that it's easier for user-space apps to send damage that way, and again, we have the full information that we can clip and scale if necessary. Most drivers probably want the damage in clipped plane- or crtc coordinates. Helpers could for example be in the form of region iterators. Full (multi-rect) damage regions are OK with us, although we should keep in mind that we won't be able to compute region unions in the kernel (yet?). Implying: Should we forbid overlapping rects at the interface level or should we just recommend rects not to be overlapping? The former would be pretty hard to enforce efficiently. Another thing we should think about is how to use this interface for the legacy "dirtyfb" call. Probably we need to clear the damage property on each state-update, or set a flag that "this is a dirtyfb state update". IMO we should also have as an end goal of this work to have gnome-shell on drm sending damage regions on page-flip, which means either porting gnome-shell to atomic or set up a new legacy page-flip-with-atomic ioctl. /Thomas On 12/21/2017 12:10 PM, Lukasz Spintzyk wrote: > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > --- > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > drivers/gpu/drm/drm_plane.c | 1 + > include/drm/drm_mode_config.h | 5 +++++ > include/drm/drm_plane.h | 3 +++ > 5 files changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index b76d49218cf1..cd3b4ed7b04c 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > state->rotation = val; > } else if (property == plane->zpos_property) { > state->zpos = val; > + } else if (property == config->dirty_rects_property) { > + bool replaced = false; > + int ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->dirty_blob, > + val, > + -1, > + &replaced); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > *val = state->rotation; > } else if (property == plane->zpos_property) { > *val = state->zpos; > + } else if (property == config->dirty_rects_property) { > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); > } else { > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index bc5c46306b3d..d5f1021c6ece 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -293,6 +293,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_crtc_id = prop; > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > + "DIRTY_RECTS", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.dirty_rects_property = prop; > + > prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC, > "ACTIVE"); > if (!prop) > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 37a93cdffb4a..add110f025e5 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane, > drm_object_attach_property(&plane->base, config->prop_src_y, 0); > drm_object_attach_property(&plane->base, config->prop_src_w, 0); > drm_object_attach_property(&plane->base, config->prop_src_h, 0); > + drm_object_attach_property(&plane->base, config->dirty_rects_property, 0); > } > > if (config->allow_fb_modifiers) > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index e5f3b43014e1..65f64eb04c0c 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -599,6 +599,11 @@ struct drm_mode_config { > * &drm_crtc. > */ > struct drm_property *prop_crtc_id; > + /** > + * @dirty_rects_property: Optional plane property to mark damaged > + * regions on the plane framebuffer. > + */ > + struct drm_property *dirty_rects_property; > /** > * @prop_active: Default atomic CRTC property to control the active > * state, which is the simplified implementation for DPMS in atomic > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 8185e3468a23..7d45b164ccce 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -131,6 +131,9 @@ struct drm_plane_state { > */ > struct drm_crtc_commit *commit; > > + /* Optional blob property with damaged regions. */ > + struct drm_property_blob *dirty_blob; > + > struct drm_atomic_state *state; > }; >
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
> -----Original Message----- > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf > Of Daniel Vetter > Sent: Thursday, December 21, 2017 5:11 AM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: airlied@linux.ie; daniel.vetter@intel.com; dri- > devel@lists.freedesktop.org; Lukasz Spintzyk > <lukasz.spintzyk@displaylink.com> > Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for > drm_plane > > On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote: > > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote: > > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > --- > > > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ > > > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > > > drivers/gpu/drm/drm_plane.c | 1 + > > > include/drm/drm_mode_config.h | 5 +++++ > > > include/drm/drm_plane.h | 3 +++ > > > 5 files changed, 25 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c > b/drivers/gpu/drm/drm_atomic.c > > > index b76d49218cf1..cd3b4ed7b04c 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct > drm_plane *plane, > > > state->rotation = val; > > > } else if (property == plane->zpos_property) { > > > state->zpos = val; > > > + } else if (property == config->dirty_rects_property) { > > > + bool replaced = false; > > > + int ret = drm_atomic_replace_property_blob_from_id(dev, > > > + &state->dirty_blob, > > > + val, > > > + -1, > > > + &replaced); > > > + return ret; > > > } else if (plane->funcs->atomic_set_property) { > > > return plane->funcs->atomic_set_property(plane, state, > > > property, val); > > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct > drm_plane *plane, > > > *val = state->rotation; > > > } else if (property == plane->zpos_property) { > > > *val = state->zpos; > > > + } else if (property == config->dirty_rects_property) { > > > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; > > > } else if (plane->funcs->atomic_get_property) { > > > return plane->funcs->atomic_get_property(plane, state, > property, val); > > > } else { > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > > > index bc5c46306b3d..d5f1021c6ece 100644 > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > @@ -293,6 +293,12 @@ static int > drm_mode_create_standard_properties(struct drm_device *dev) > > > return -ENOMEM; > > > dev->mode_config.prop_crtc_id = prop; > > > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > > > + "DIRTY_RECTS", 0); > > > + if (!prop) > > > + return -ENOMEM; > > > + dev->mode_config.dirty_rects_property = prop; > > > + > > > prop = drm_property_create_bool(dev, > DRM_MODE_PROP_ATOMIC, > > > "ACTIVE"); > > > if (!prop) > > > diff --git a/drivers/gpu/drm/drm_plane.c > b/drivers/gpu/drm/drm_plane.c > > > index 37a93cdffb4a..add110f025e5 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device > *dev, struct drm_plane *plane, > > > drm_object_attach_property(&plane->base, config- > >prop_src_y, 0); > > > drm_object_attach_property(&plane->base, config- > >prop_src_w, 0); > > > drm_object_attach_property(&plane->base, config- > >prop_src_h, 0); > > > + drm_object_attach_property(&plane->base, config- > >dirty_rects_property, 0); > > > } > > > > > > if (config->allow_fb_modifiers) > > > diff --git a/include/drm/drm_mode_config.h > b/include/drm/drm_mode_config.h > > > index e5f3b43014e1..65f64eb04c0c 100644 > > > --- a/include/drm/drm_mode_config.h > > > +++ b/include/drm/drm_mode_config.h > > > @@ -599,6 +599,11 @@ struct drm_mode_config { > > > * &drm_crtc. > > > */ > > > struct drm_property *prop_crtc_id; > > > + /** > > > + * @dirty_rects_property: Optional plane property to mark damaged > > > + * regions on the plane framebuffer. > > > > What exactly would the blob contain? > > > > The comment seems to be implying these are in fb coordiantes as > > opposed to plane crtc coordinates? Not sure which would be more > > convenient. At least if they're fb coordinates then you probably > > want some helpers to translate/rotate/scale those rects to the > > crtc coordinates. Actual use depends on the driver/hw I suppose. > > Yeah I think we also should add a decoded state to the drm_plane_state, > which has the full structure and all the details. Hi Daniel, I am working on this functionality to implement the helper functions to convert dirty clips from framebuffer coordinates to planes coordinates and finally to crtc coordinates. Is there a reason we should have decoded dirty clips in plane_state ? I was thinking to have the clips remain in blob property and decode them when needed with the helper function which accept plane state and similarly another helper for crtc coordinates. So driver call these helper only if there is a need for dirty clips and otherwise can ignore. Thanks, Deepak > > And when we discussed this iirc we've identified a clear need for at least > some drivers to deal in crtc dirty rectangles. I think the initial core > support should include a helper which takes an atomic update for a given > crtc, and converts all the plane dirty rectangles into crtc rectangles. > Including any full-plane or full-crtc upgrades needed due to e.g. > reposition, changed gamma, changed blendign/zpos or anything else really > that would affect the entire plane respectively crtc. That would also > provide a really good model for what damage actually means. > > Plus ofc we need userspace for this, preferrably as a patch to something > generic like weston or xfree86-video-modesetting. And an example kernel > implementation. > > Cheers, Daniel > > > > > > + */ > > > + struct drm_property *dirty_rects_property; > > > /** > > > * @prop_active: Default atomic CRTC property to control the active > > > * state, which is the simplified implementation for DPMS in atomic > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > index 8185e3468a23..7d45b164ccce 100644 > > > --- a/include/drm/drm_plane.h > > > +++ b/include/drm/drm_plane.h > > > @@ -131,6 +131,9 @@ struct drm_plane_state { > > > */ > > > struct drm_crtc_commit *commit; > > > > > > + /* Optional blob property with damaged regions. */ > > > + struct drm_property_blob *dirty_blob; > > > + > > > struct drm_atomic_state *state; > > > }; > > > > > > -- > > > 2.15.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__lists.freedesktop.org_mailman_listinfo_dri- > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__lists.freedesktop.org_mailman_listinfo_dri- > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > > -- > Daniel Vetter > Software Engineer, Intel Corporation > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK > 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2- > IBYMRBEEBmnUTRTo7czA810&e= > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__lists.freedesktop.org_mailman_listinfo_dri- > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote: > > > > -----Original Message----- > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf > > Of Daniel Vetter > > Sent: Thursday, December 21, 2017 5:11 AM > > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: airlied@linux.ie; daniel.vetter@intel.com; dri- > > devel@lists.freedesktop.org; Lukasz Spintzyk > > <lukasz.spintzyk@displaylink.com> > > Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for > > drm_plane > > > > On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote: > > > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote: > > > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > --- > > > > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ > > > > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > > > > drivers/gpu/drm/drm_plane.c | 1 + > > > > include/drm/drm_mode_config.h | 5 +++++ > > > > include/drm/drm_plane.h | 3 +++ > > > > 5 files changed, 25 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c > > b/drivers/gpu/drm/drm_atomic.c > > > > index b76d49218cf1..cd3b4ed7b04c 100644 > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct > > drm_plane *plane, > > > > state->rotation = val; > > > > } else if (property == plane->zpos_property) { > > > > state->zpos = val; > > > > + } else if (property == config->dirty_rects_property) { > > > > + bool replaced = false; > > > > + int ret = drm_atomic_replace_property_blob_from_id(dev, > > > > + &state->dirty_blob, > > > > + val, > > > > + -1, > > > > + &replaced); > > > > + return ret; > > > > } else if (plane->funcs->atomic_set_property) { > > > > return plane->funcs->atomic_set_property(plane, state, > > > > property, val); > > > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct > > drm_plane *plane, > > > > *val = state->rotation; > > > > } else if (property == plane->zpos_property) { > > > > *val = state->zpos; > > > > + } else if (property == config->dirty_rects_property) { > > > > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; > > > > } else if (plane->funcs->atomic_get_property) { > > > > return plane->funcs->atomic_get_property(plane, state, > > property, val); > > > > } else { > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > b/drivers/gpu/drm/drm_mode_config.c > > > > index bc5c46306b3d..d5f1021c6ece 100644 > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > @@ -293,6 +293,12 @@ static int > > drm_mode_create_standard_properties(struct drm_device *dev) > > > > return -ENOMEM; > > > > dev->mode_config.prop_crtc_id = prop; > > > > > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > > > > + "DIRTY_RECTS", 0); > > > > + if (!prop) > > > > + return -ENOMEM; > > > > + dev->mode_config.dirty_rects_property = prop; > > > > + > > > > prop = drm_property_create_bool(dev, > > DRM_MODE_PROP_ATOMIC, > > > > "ACTIVE"); > > > > if (!prop) > > > > diff --git a/drivers/gpu/drm/drm_plane.c > > b/drivers/gpu/drm/drm_plane.c > > > > index 37a93cdffb4a..add110f025e5 100644 > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device > > *dev, struct drm_plane *plane, > > > > drm_object_attach_property(&plane->base, config- > > >prop_src_y, 0); > > > > drm_object_attach_property(&plane->base, config- > > >prop_src_w, 0); > > > > drm_object_attach_property(&plane->base, config- > > >prop_src_h, 0); > > > > + drm_object_attach_property(&plane->base, config- > > >dirty_rects_property, 0); > > > > } > > > > > > > > if (config->allow_fb_modifiers) > > > > diff --git a/include/drm/drm_mode_config.h > > b/include/drm/drm_mode_config.h > > > > index e5f3b43014e1..65f64eb04c0c 100644 > > > > --- a/include/drm/drm_mode_config.h > > > > +++ b/include/drm/drm_mode_config.h > > > > @@ -599,6 +599,11 @@ struct drm_mode_config { > > > > * &drm_crtc. > > > > */ > > > > struct drm_property *prop_crtc_id; > > > > + /** > > > > + * @dirty_rects_property: Optional plane property to mark damaged > > > > + * regions on the plane framebuffer. > > > > > > What exactly would the blob contain? > > > > > > The comment seems to be implying these are in fb coordiantes as > > > opposed to plane crtc coordinates? Not sure which would be more > > > convenient. At least if they're fb coordinates then you probably > > > want some helpers to translate/rotate/scale those rects to the > > > crtc coordinates. Actual use depends on the driver/hw I suppose. > > > > Yeah I think we also should add a decoded state to the drm_plane_state, > > which has the full structure and all the details. > > Hi Daniel, > > I am working on this functionality to implement the helper functions to > convert dirty clips from framebuffer coordinates to planes coordinates > and finally to crtc coordinates. > > Is there a reason we should have decoded dirty clips in plane_state ? > I was thinking to have the clips remain in blob property and decode > them when needed with the helper function which accept plane state and > similarly another helper for crtc coordinates. So driver call these helper > only if there is a need for dirty clips and otherwise can ignore. Immediately decoding the state blobs is simply best practices, to avoid duplicated code and subtle differences in driver behaviour. If there's a good reason for completely different behaviour (like crtc vs plane coordinate based damage tracking), then decoding using a helper, on-demand, seems sensible. I'd still think we should store the resulting derived state in drm_crtc/plane_state, like we do for the plane clipping helpers (at least after Ville's latest patch series has landed). -Daniel > > Thanks, > Deepak > > > > > > And when we discussed this iirc we've identified a clear need for at least > > some drivers to deal in crtc dirty rectangles. I think the initial core > > support should include a helper which takes an atomic update for a given > > crtc, and converts all the plane dirty rectangles into crtc rectangles. > > Including any full-plane or full-crtc upgrades needed due to e.g. > > reposition, changed gamma, changed blendign/zpos or anything else really > > that would affect the entire plane respectively crtc. That would also > > provide a really good model for what damage actually means. > > > > Plus ofc we need userspace for this, preferrably as a patch to something > > generic like weston or xfree86-video-modesetting. And an example kernel > > implementation. > > > > Cheers, Daniel > > > > > > > > > + */ > > > > + struct drm_property *dirty_rects_property; > > > > /** > > > > * @prop_active: Default atomic CRTC property to control the active > > > > * state, which is the simplified implementation for DPMS in atomic > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > > index 8185e3468a23..7d45b164ccce 100644 > > > > --- a/include/drm/drm_plane.h > > > > +++ b/include/drm/drm_plane.h > > > > @@ -131,6 +131,9 @@ struct drm_plane_state { > > > > */ > > > > struct drm_crtc_commit *commit; > > > > > > > > + /* Optional blob property with damaged regions. */ > > > > + struct drm_property_blob *dirty_blob; > > > > + > > > > struct drm_atomic_state *state; > > > > }; > > > > > > > > -- > > > > 2.15.1 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__lists.freedesktop.org_mailman_listinfo_dri- > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__lists.freedesktop.org_mailman_listinfo_dri- > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > https://urldefense.proofpoint.com/v2/url?u=http- > > 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK > > 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2- > > IBYMRBEEBmnUTRTo7czA810&e= > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__lists.freedesktop.org_mailman_listinfo_dri- > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
On 03/05/2018 09:37 AM, Daniel Vetter wrote: > On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote: >> >>> -----Original Message----- >>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf >>> Of Daniel Vetter >>> Sent: Thursday, December 21, 2017 5:11 AM >>> To: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: airlied@linux.ie; daniel.vetter@intel.com; dri- >>> devel@lists.freedesktop.org; Lukasz Spintzyk >>> <lukasz.spintzyk@displaylink.com> >>> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for >>> drm_plane >>> >>> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote: >>>> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote: >>>>> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 >>>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >>>>> --- >>>>> drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ >>>>> drivers/gpu/drm/drm_mode_config.c | 6 ++++++ >>>>> drivers/gpu/drm/drm_plane.c | 1 + >>>>> include/drm/drm_mode_config.h | 5 +++++ >>>>> include/drm/drm_plane.h | 3 +++ >>>>> 5 files changed, 25 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c >>> b/drivers/gpu/drm/drm_atomic.c >>>>> index b76d49218cf1..cd3b4ed7b04c 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct >>> drm_plane *plane, >>>>> state->rotation = val; >>>>> } else if (property == plane->zpos_property) { >>>>> state->zpos = val; >>>>> + } else if (property == config->dirty_rects_property) { >>>>> + bool replaced = false; >>>>> + int ret = drm_atomic_replace_property_blob_from_id(dev, >>>>> + &state->dirty_blob, >>>>> + val, >>>>> + -1, >>>>> + &replaced); >>>>> + return ret; >>>>> } else if (plane->funcs->atomic_set_property) { >>>>> return plane->funcs->atomic_set_property(plane, state, >>>>> property, val); >>>>> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct >>> drm_plane *plane, >>>>> *val = state->rotation; >>>>> } else if (property == plane->zpos_property) { >>>>> *val = state->zpos; >>>>> + } else if (property == config->dirty_rects_property) { >>>>> + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; >>>>> } else if (plane->funcs->atomic_get_property) { >>>>> return plane->funcs->atomic_get_property(plane, state, >>> property, val); >>>>> } else { >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c >>> b/drivers/gpu/drm/drm_mode_config.c >>>>> index bc5c46306b3d..d5f1021c6ece 100644 >>>>> --- a/drivers/gpu/drm/drm_mode_config.c >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c >>>>> @@ -293,6 +293,12 @@ static int >>> drm_mode_create_standard_properties(struct drm_device *dev) >>>>> return -ENOMEM; >>>>> dev->mode_config.prop_crtc_id = prop; >>>>> >>>>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, >>>>> + "DIRTY_RECTS", 0); >>>>> + if (!prop) >>>>> + return -ENOMEM; >>>>> + dev->mode_config.dirty_rects_property = prop; >>>>> + >>>>> prop = drm_property_create_bool(dev, >>> DRM_MODE_PROP_ATOMIC, >>>>> "ACTIVE"); >>>>> if (!prop) >>>>> diff --git a/drivers/gpu/drm/drm_plane.c >>> b/drivers/gpu/drm/drm_plane.c >>>>> index 37a93cdffb4a..add110f025e5 100644 >>>>> --- a/drivers/gpu/drm/drm_plane.c >>>>> +++ b/drivers/gpu/drm/drm_plane.c >>>>> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device >>> *dev, struct drm_plane *plane, >>>>> drm_object_attach_property(&plane->base, config- >>>> prop_src_y, 0); >>>>> drm_object_attach_property(&plane->base, config- >>>> prop_src_w, 0); >>>>> drm_object_attach_property(&plane->base, config- >>>> prop_src_h, 0); >>>>> + drm_object_attach_property(&plane->base, config- >>>> dirty_rects_property, 0); >>>>> } >>>>> >>>>> if (config->allow_fb_modifiers) >>>>> diff --git a/include/drm/drm_mode_config.h >>> b/include/drm/drm_mode_config.h >>>>> index e5f3b43014e1..65f64eb04c0c 100644 >>>>> --- a/include/drm/drm_mode_config.h >>>>> +++ b/include/drm/drm_mode_config.h >>>>> @@ -599,6 +599,11 @@ struct drm_mode_config { >>>>> * &drm_crtc. >>>>> */ >>>>> struct drm_property *prop_crtc_id; >>>>> + /** >>>>> + * @dirty_rects_property: Optional plane property to mark damaged >>>>> + * regions on the plane framebuffer. >>>> What exactly would the blob contain? >>>> >>>> The comment seems to be implying these are in fb coordiantes as >>>> opposed to plane crtc coordinates? Not sure which would be more >>>> convenient. At least if they're fb coordinates then you probably >>>> want some helpers to translate/rotate/scale those rects to the >>>> crtc coordinates. Actual use depends on the driver/hw I suppose. >>> Yeah I think we also should add a decoded state to the drm_plane_state, >>> which has the full structure and all the details. >> Hi Daniel, >> >> I am working on this functionality to implement the helper functions to >> convert dirty clips from framebuffer coordinates to planes coordinates >> and finally to crtc coordinates. >> >> Is there a reason we should have decoded dirty clips in plane_state ? >> I was thinking to have the clips remain in blob property and decode >> them when needed with the helper function which accept plane state and >> similarly another helper for crtc coordinates. So driver call these helper >> only if there is a need for dirty clips and otherwise can ignore. > Immediately decoding the state blobs is simply best practices, to avoid > duplicated code and subtle differences in driver behaviour. If there's a > good reason for completely different behaviour (like crtc vs plane > coordinate based damage tracking), then decoding using a helper, > on-demand, seems sensible. > > I'd still think we should store the resulting derived state in > drm_crtc/plane_state, like we do for the plane clipping helpers (at least > after Ville's latest patch series has landed). > -Daniel The idea here would be to use a helper iterator to construct the new coordinates needed; the iterator being initialized with the blob. The reason not to store the decoded state is unnecessary duplication. While I guess most drivers would use crtc damage tracking, potentially there might be three coordinate systems used by drivers: Plane fb coordinates, Plane crtc coordinates and crtc coordinates. And typically they'd be used only once... /Thomas >> Thanks, >> Deepak >> >> >>> And when we discussed this iirc we've identified a clear need for at least >>> some drivers to deal in crtc dirty rectangles. I think the initial core >>> support should include a helper which takes an atomic update for a given >>> crtc, and converts all the plane dirty rectangles into crtc rectangles. >>> Including any full-plane or full-crtc upgrades needed due to e.g. >>> reposition, changed gamma, changed blendign/zpos or anything else really >>> that would affect the entire plane respectively crtc. That would also >>> provide a really good model for what damage actually means. >>> >>> Plus ofc we need userspace for this, preferrably as a patch to something >>> generic like weston or xfree86-video-modesetting. And an example kernel >>> implementation. >>> >>> Cheers, Daniel >>> >>>>> + */ >>>>> + struct drm_property *dirty_rects_property; >>>>> /** >>>>> * @prop_active: Default atomic CRTC property to control the active >>>>> * state, which is the simplified implementation for DPMS in atomic >>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>>>> index 8185e3468a23..7d45b164ccce 100644 >>>>> --- a/include/drm/drm_plane.h >>>>> +++ b/include/drm/drm_plane.h >>>>> @@ -131,6 +131,9 @@ struct drm_plane_state { >>>>> */ >>>>> struct drm_crtc_commit *commit; >>>>> >>>>> + /* Optional blob property with damaged regions. */ >>>>> + struct drm_property_blob *dirty_blob; >>>>> + >>>>> struct drm_atomic_state *state; >>>>> }; >>>>> >>>>> -- >>>>> 2.15.1 >>>>> >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://urldefense.proofpoint.com/v2/url?u=https- >>> 3A__lists.freedesktop.org_mailman_listinfo_dri- >>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf >>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- >>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- >>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= >>>> -- >>>> Ville Syrjälä >>>> Intel OTC >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://urldefense.proofpoint.com/v2/url?u=https- >>> 3A__lists.freedesktop.org_mailman_listinfo_dri- >>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf >>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- >>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- >>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= >>> >>> -- >>> Daniel Vetter >>> Software Engineer, Intel Corporation >>> https://urldefense.proofpoint.com/v2/url?u=http- >>> 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK >>> 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- >>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2- >>> IBYMRBEEBmnUTRTo7czA810&e= >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://urldefense.proofpoint.com/v2/url?u=https- >>> 3A__lists.freedesktop.org_mailman_listinfo_dri- >>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf >>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- >>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- >>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
On Mon, Mar 05, 2018 at 10:22:09AM +0100, Thomas Hellstrom wrote: > On 03/05/2018 09:37 AM, Daniel Vetter wrote: > > On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote: > > > > > > > -----Original Message----- > > > > From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf > > > > Of Daniel Vetter > > > > Sent: Thursday, December 21, 2017 5:11 AM > > > > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: airlied@linux.ie; daniel.vetter@intel.com; dri- > > > > devel@lists.freedesktop.org; Lukasz Spintzyk > > > > <lukasz.spintzyk@displaylink.com> > > > > Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for > > > > drm_plane > > > > > > > > On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote: > > > > > On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote: > > > > > > Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5 > > > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > > > --- > > > > > > drivers/gpu/drm/drm_atomic.c | 10 ++++++++++ > > > > > > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > > > > > > drivers/gpu/drm/drm_plane.c | 1 + > > > > > > include/drm/drm_mode_config.h | 5 +++++ > > > > > > include/drm/drm_plane.h | 3 +++ > > > > > > 5 files changed, 25 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c > > > > b/drivers/gpu/drm/drm_atomic.c > > > > > > index b76d49218cf1..cd3b4ed7b04c 100644 > > > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > > > @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct > > > > drm_plane *plane, > > > > > > state->rotation = val; > > > > > > } else if (property == plane->zpos_property) { > > > > > > state->zpos = val; > > > > > > + } else if (property == config->dirty_rects_property) { > > > > > > + bool replaced = false; > > > > > > + int ret = drm_atomic_replace_property_blob_from_id(dev, > > > > > > + &state->dirty_blob, > > > > > > + val, > > > > > > + -1, > > > > > > + &replaced); > > > > > > + return ret; > > > > > > } else if (plane->funcs->atomic_set_property) { > > > > > > return plane->funcs->atomic_set_property(plane, state, > > > > > > property, val); > > > > > > @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct > > > > drm_plane *plane, > > > > > > *val = state->rotation; > > > > > > } else if (property == plane->zpos_property) { > > > > > > *val = state->zpos; > > > > > > + } else if (property == config->dirty_rects_property) { > > > > > > + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0; > > > > > > } else if (plane->funcs->atomic_get_property) { > > > > > > return plane->funcs->atomic_get_property(plane, state, > > > > property, val); > > > > > > } else { > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > > b/drivers/gpu/drm/drm_mode_config.c > > > > > > index bc5c46306b3d..d5f1021c6ece 100644 > > > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > > > @@ -293,6 +293,12 @@ static int > > > > drm_mode_create_standard_properties(struct drm_device *dev) > > > > > > return -ENOMEM; > > > > > > dev->mode_config.prop_crtc_id = prop; > > > > > > > > > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, > > > > > > + "DIRTY_RECTS", 0); > > > > > > + if (!prop) > > > > > > + return -ENOMEM; > > > > > > + dev->mode_config.dirty_rects_property = prop; > > > > > > + > > > > > > prop = drm_property_create_bool(dev, > > > > DRM_MODE_PROP_ATOMIC, > > > > > > "ACTIVE"); > > > > > > if (!prop) > > > > > > diff --git a/drivers/gpu/drm/drm_plane.c > > > > b/drivers/gpu/drm/drm_plane.c > > > > > > index 37a93cdffb4a..add110f025e5 100644 > > > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > > > @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device > > > > *dev, struct drm_plane *plane, > > > > > > drm_object_attach_property(&plane->base, config- > > > > > prop_src_y, 0); > > > > > > drm_object_attach_property(&plane->base, config- > > > > > prop_src_w, 0); > > > > > > drm_object_attach_property(&plane->base, config- > > > > > prop_src_h, 0); > > > > > > + drm_object_attach_property(&plane->base, config- > > > > > dirty_rects_property, 0); > > > > > > } > > > > > > > > > > > > if (config->allow_fb_modifiers) > > > > > > diff --git a/include/drm/drm_mode_config.h > > > > b/include/drm/drm_mode_config.h > > > > > > index e5f3b43014e1..65f64eb04c0c 100644 > > > > > > --- a/include/drm/drm_mode_config.h > > > > > > +++ b/include/drm/drm_mode_config.h > > > > > > @@ -599,6 +599,11 @@ struct drm_mode_config { > > > > > > * &drm_crtc. > > > > > > */ > > > > > > struct drm_property *prop_crtc_id; > > > > > > + /** > > > > > > + * @dirty_rects_property: Optional plane property to mark damaged > > > > > > + * regions on the plane framebuffer. > > > > > What exactly would the blob contain? > > > > > > > > > > The comment seems to be implying these are in fb coordiantes as > > > > > opposed to plane crtc coordinates? Not sure which would be more > > > > > convenient. At least if they're fb coordinates then you probably > > > > > want some helpers to translate/rotate/scale those rects to the > > > > > crtc coordinates. Actual use depends on the driver/hw I suppose. > > > > Yeah I think we also should add a decoded state to the drm_plane_state, > > > > which has the full structure and all the details. > > > Hi Daniel, > > > > > > I am working on this functionality to implement the helper functions to > > > convert dirty clips from framebuffer coordinates to planes coordinates > > > and finally to crtc coordinates. > > > > > > Is there a reason we should have decoded dirty clips in plane_state ? > > > I was thinking to have the clips remain in blob property and decode > > > them when needed with the helper function which accept plane state and > > > similarly another helper for crtc coordinates. So driver call these helper > > > only if there is a need for dirty clips and otherwise can ignore. > > Immediately decoding the state blobs is simply best practices, to avoid > > duplicated code and subtle differences in driver behaviour. If there's a > > good reason for completely different behaviour (like crtc vs plane > > coordinate based damage tracking), then decoding using a helper, > > on-demand, seems sensible. > > > > I'd still think we should store the resulting derived state in > > drm_crtc/plane_state, like we do for the plane clipping helpers (at least > > after Ville's latest patch series has landed). > > -Daniel > > The idea here would be to use a helper iterator to construct the new > coordinates needed; the iterator being initialized with the blob. > > The reason not to store the decoded state is unnecessary duplication. While > I guess most drivers would use crtc damage tracking, potentially there might > be three coordinate systems used by drivers: Plane fb coordinates, Plane > crtc coordinates and crtc coordinates. And typically they'd be used only > once... Well I was thinking of a helper to compute a single, overall clip rect for the entire crtc. Which would then also need to take into account various plane state changes, and stuff like background color. For a simple coordinate system transform iterators sound like a good idea, but honestly I don't expect that to be a common use-case in drivers. Either it's about uploading the right dirty data in each plane, or about uploading the right final/blended pixels to the screen (in which case all other state changes that can affect colors must be taken into account too). -Daniel > > /Thomas > > > > > Thanks, > > > Deepak > > > > > > > > > > And when we discussed this iirc we've identified a clear need for at least > > > > some drivers to deal in crtc dirty rectangles. I think the initial core > > > > support should include a helper which takes an atomic update for a given > > > > crtc, and converts all the plane dirty rectangles into crtc rectangles. > > > > Including any full-plane or full-crtc upgrades needed due to e.g. > > > > reposition, changed gamma, changed blendign/zpos or anything else really > > > > that would affect the entire plane respectively crtc. That would also > > > > provide a really good model for what damage actually means. > > > > > > > > Plus ofc we need userspace for this, preferrably as a patch to something > > > > generic like weston or xfree86-video-modesetting. And an example kernel > > > > implementation. > > > > > > > > Cheers, Daniel > > > > > > > > > > + */ > > > > > > + struct drm_property *dirty_rects_property; > > > > > > /** > > > > > > * @prop_active: Default atomic CRTC property to control the active > > > > > > * state, which is the simplified implementation for DPMS in atomic > > > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > > > > index 8185e3468a23..7d45b164ccce 100644 > > > > > > --- a/include/drm/drm_plane.h > > > > > > +++ b/include/drm/drm_plane.h > > > > > > @@ -131,6 +131,9 @@ struct drm_plane_state { > > > > > > */ > > > > > > struct drm_crtc_commit *commit; > > > > > > > > > > > > + /* Optional blob property with damaged regions. */ > > > > > > + struct drm_property_blob *dirty_blob; > > > > > > + > > > > > > struct drm_atomic_state *state; > > > > > > }; > > > > > > > > > > > > -- > > > > > > 2.15.1 > > > > > > > > > > > > _______________________________________________ > > > > > > dri-devel mailing list > > > > > > dri-devel@lists.freedesktop.org > > > > > > https://urldefense.proofpoint.com/v2/url?u=https- > > > > 3A__lists.freedesktop.org_mailman_listinfo_dri- > > > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > > > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > > > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > > > > > -- > > > > > Ville Syrjälä > > > > > Intel OTC > > > > > _______________________________________________ > > > > > dri-devel mailing list > > > > > dri-devel@lists.freedesktop.org > > > > > https://urldefense.proofpoint.com/v2/url?u=https- > > > > 3A__lists.freedesktop.org_mailman_listinfo_dri- > > > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > > > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > > > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > https://urldefense.proofpoint.com/v2/url?u=http- > > > > 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK > > > > 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2- > > > > IBYMRBEEBmnUTRTo7czA810&e= > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://urldefense.proofpoint.com/v2/url?u=https- > > > > 3A__lists.freedesktop.org_mailman_listinfo_dri- > > > > 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf > > > > -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F- > > > > 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=- > > > > PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e= > >
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 >>>
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; };
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(+)