| Message ID | 1522885748-67122-2-git-send-email-drawat@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > Optional plane property to mark damaged regions on the plane in > framebuffer coordinates of the framebuffer attached to the plane. > > The layout of blob data is simply an array of drm_mode_rect with maximum > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src > coordinates, damage clips are not in 16.16 fixed point. > > Damage clips are a hint to kernel as which area of framebuffer has > changed since last page-flip. This should be helpful for some drivers > especially for virtual devices where each framebuffer change needs to > be transmitted over network, usb, etc. > > Driver which are interested in enabling DAMAGE_CLIPS property for a > plane should enable this property using drm_plane_enable_damage_clips. > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > Signed-off-by: Deepak Rawat <drawat@vmware.com> The property uapi section is missing, see: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties Plane composition feels like the best place to put this. Please use that section to tie all the various bits together, including the helpers you're adding in the following patches for drivers to use. Bunch of nitpicks below, but overall I'm agreeing now with just going with fb coordinate damage rects. Like you say, the thing needed here now is userspace + driver actually implementing this. I'd also say the compat helper to map the legacy fb->dirty to this new atomic way of doing things should be included here (gives us a lot more testing for these new paths). Icing on the cake would be an igt to make sure kernel rejects invalid clip rects correctly. > --- > drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > drivers/gpu/drm/drm_mode_config.c | 5 +++++ > drivers/gpu/drm/drm_plane.c | 12 +++++++++++ > include/drm/drm_mode_config.h | 15 +++++++++++++ > include/drm/drm_plane.h | 16 ++++++++++++++ > include/uapi/drm/drm_mode.h | 15 +++++++++++++ > 7 files changed, 109 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42..9226d24 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, > } > > /** > + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane > + * @state: plane state > + * @blob: damage clips in framebuffer coordinates > + * > + * Returns: > + * > + * Zero on success, error code on failure. > + */ > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, > + struct drm_property_blob *blob) > +{ > + if (blob == state->damage_clips) > + return 0; > + > + drm_property_blob_put(state->damage_clips); > + state->damage_clips = NULL; > + > + if (blob) { > + uint32_t count = blob->length/sizeof(struct drm_rect); > + > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) > + return -EINVAL; > + > + state->damage_clips = drm_property_blob_get(blob); > + state->num_clips = count; > + } else { > + state->damage_clips = NULL; > + state->num_clips = 0; > + } > + > + return 0; > +} > + > +/** > * drm_atomic_get_plane_state - get plane state > * @state: global atomic state object > * @plane: plane to get state object for > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > state->color_encoding = val; > } else if (property == plane->color_range_property) { > state->color_range = val; > + } else if (property == config->prop_damage_clips) { > + struct drm_property_blob *blob = > + drm_property_lookup_blob(dev, val); > + int ret = drm_atomic_set_damage_for_plane(state, blob); There's already a helper with size-checking built-in, see drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd just provide a little inline helper that does the blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). > + drm_property_blob_put(blob); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > *val = state->color_encoding; > } else if (property == plane->color_range_property) { > *val = state->color_range; > + } else if (property == config->prop_damage_clips) { > + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index c356545..55b44e3 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > state->fence = NULL; > state->commit = NULL; > + state->damage_clips = NULL; > + state->num_clips = 0; > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > if (state->commit) > drm_crtc_commit_put(state->commit); > + > + drm_property_blob_put(state->damage_clips); > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index e5c6533..e93b127 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); Bit a bikeshed, but since the coordinates are in fb pixels, not plane pixels, I'd call this "FB_DAMAGE_CLIPS". > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > > return ret; > } > + > +/** > + * drm_plane_enable_damage_clips - enable damage clips property > + * @plane: plane on which this property to enable. > + */ > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > +{ > + struct drm_device *dev = plane->dev; > + struct drm_mode_config *config = &dev->mode_config; > + > + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); > +} > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 7569f22..d8767da 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -628,6 +628,21 @@ struct drm_mode_config { > */ > struct drm_property *prop_crtc_id; > /** > + * @prop_damage_clips: Optional plane property to mark damaged regions > + * on the plane in framebuffer coordinates of the framebuffer attached > + * to the plane. Why should we make this optional? Looks like just another thing drivers might screw up, since we have multiple callbacks and things to set up for proper dirty tracking. One option I'm seeing is that if this is set, and it's an atomic driver, then we just directly call into the default atomic fb->dirty implementation. That way there's only 1 thing drivers need to do to set up dirty rect tracking, and they'll get all of it. > + * > + * The layout of blob data is simply an array of drm_mode_rect with > + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike > + * plane src coordinates, damage clips are not in 16.16 fixed point. I honestly have no idea where this limit is from. Worth keeping? I can easily imagine that userspace could trip over this - it's fairly high, but not unlimited. > + * > + * Damage clips are a hint to kernel as which area of framebuffer has > + * changed since last page-flip. This should be helpful > + * for some drivers especially for virtual devices where each > + * framebuffer change needs to be transmitted over network, usb, etc. I'd also clarify that userspace still must render the entire screen, i.e. make it more clear that it's really just a hint and not mandatory to only scan out the damaged parts. > + */ > + struct drm_property *prop_damage_clips; > + /** > * @prop_active: Default atomic CRTC property to control the active > * state, which is the simplified implementation for DPMS in atomic > * drivers. > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index f7bf4a4..9f24548 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -146,6 +146,21 @@ struct drm_plane_state { > */ > struct drm_crtc_commit *commit; > > + /* > + * @damage_clips > + * > + * blob property with damage as array of drm_rect in framebuffer &drm_rect gives you a nice hyperlink in the generated docs. > + * coodinates. > + */ > + struct drm_property_blob *damage_clips; > + > + /* > + * @num_clips > + * > + * Number of drm_rect in @damage_clips. > + */ > + uint32_t num_clips; > + > struct drm_atomic_state *state; > }; > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, > const uint32_t *formats, unsigned int format_count, > bool is_primary); > void drm_plane_cleanup(struct drm_plane *plane); > +void drm_plane_enable_damage_clips(struct drm_plane *plane); > > /** > * drm_plane_index - find the index of a registered plane > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 50bcf42..0ad0d5b 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { > __u32 lessee_id; > }; > > +/** > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to > + * user-space. > + * @x1: horizontal starting coordinate (inclusive) > + * @y1: vertical starting coordinate (inclusive) > + * @x2: horizontal ending coordinate (exclusive) > + * @y2: vertical ending coordinate (exclusive) > + */ > +struct drm_mode_rect { > + __s32 x1; > + __s32 y1; > + __s32 x2; > + __s32 y2; Why signed? Negative damage rects on an fb don't make sense to me. Also, please specify what this is exactly (to avoid confusion with the 16.16 fixed point src rects), and maybe mention in the commit message why we're not using drm_clip_rect (going to proper uapi types and 32bit makes sense to me). Cheers, Daniel > +}; > + > #if defined(__cplusplus) > } > #endif > -- > 2.7.4 >
On 04/05/2018 09:35 AM, Daniel Vetter wrote: > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: >> From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >> >> Optional plane property to mark damaged regions on the plane in >> framebuffer coordinates of the framebuffer attached to the plane. >> >> The layout of blob data is simply an array of drm_mode_rect with maximum >> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src >> coordinates, damage clips are not in 16.16 fixed point. >> >> Damage clips are a hint to kernel as which area of framebuffer has >> changed since last page-flip. This should be helpful for some drivers >> especially for virtual devices where each framebuffer change needs to >> be transmitted over network, usb, etc. >> >> Driver which are interested in enabling DAMAGE_CLIPS property for a >> plane should enable this property using drm_plane_enable_damage_clips. >> >> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >> Signed-off-by: Deepak Rawat <drawat@vmware.com> > The property uapi section is missing, see: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e= > > Plane composition feels like the best place to put this. Please use that > section to tie all the various bits together, including the helpers you're > adding in the following patches for drivers to use. > > Bunch of nitpicks below, but overall I'm agreeing now with just going with > fb coordinate damage rects. > > Like you say, the thing needed here now is userspace + driver actually > implementing this. I'd also say the compat helper to map the legacy > fb->dirty to this new atomic way of doing things should be included here > (gives us a lot more testing for these new paths). > > Icing on the cake would be an igt to make sure kernel rejects invalid clip > rects correctly. > >> --- >> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >> drivers/gpu/drm/drm_mode_config.c | 5 +++++ >> drivers/gpu/drm/drm_plane.c | 12 +++++++++++ >> include/drm/drm_mode_config.h | 15 +++++++++++++ >> include/drm/drm_plane.h | 16 ++++++++++++++ >> include/uapi/drm/drm_mode.h | 15 +++++++++++++ >> 7 files changed, 109 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 7d25c42..9226d24 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, >> } >> >> /** >> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane >> + * @state: plane state >> + * @blob: damage clips in framebuffer coordinates >> + * >> + * Returns: >> + * >> + * Zero on success, error code on failure. >> + */ >> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, >> + struct drm_property_blob *blob) >> +{ >> + if (blob == state->damage_clips) >> + return 0; >> + >> + drm_property_blob_put(state->damage_clips); >> + state->damage_clips = NULL; >> + >> + if (blob) { >> + uint32_t count = blob->length/sizeof(struct drm_rect); >> + >> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) >> + return -EINVAL; >> + >> + state->damage_clips = drm_property_blob_get(blob); >> + state->num_clips = count; >> + } else { >> + state->damage_clips = NULL; >> + state->num_clips = 0; >> + } >> + >> + return 0; >> +} >> + >> +/** >> * drm_atomic_get_plane_state - get plane state >> * @state: global atomic state object >> * @plane: plane to get state object for >> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >> state->color_encoding = val; >> } else if (property == plane->color_range_property) { >> state->color_range = val; >> + } else if (property == config->prop_damage_clips) { >> + struct drm_property_blob *blob = >> + drm_property_lookup_blob(dev, val); >> + int ret = drm_atomic_set_damage_for_plane(state, blob); > There's already a helper with size-checking built-in, see > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd > just provide a little inline helper that does the > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). > >> + drm_property_blob_put(blob); >> + return ret; >> } else if (plane->funcs->atomic_set_property) { >> return plane->funcs->atomic_set_property(plane, state, >> property, val); >> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, >> *val = state->color_encoding; >> } else if (property == plane->color_range_property) { >> *val = state->color_range; >> + } else if (property == config->prop_damage_clips) { >> + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index c356545..55b44e3 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >> >> state->fence = NULL; >> state->commit = NULL; >> + state->damage_clips = NULL; >> + state->num_clips = 0; >> } >> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); >> >> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >> >> if (state->commit) >> drm_crtc_commit_put(state->commit); >> + >> + drm_property_blob_put(state->damage_clips); >> } >> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); >> >> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >> index e5c6533..e93b127 100644 >> --- a/drivers/gpu/drm/drm_mode_config.c >> +++ b/drivers/gpu/drm/drm_mode_config.c >> @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); > Bit a bikeshed, but since the coordinates are in fb pixels, not plane > pixels, I'd call this "FB_DAMAGE_CLIPS". > >> + if (!prop) >> + return -ENOMEM; >> + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 >> --- a/drivers/gpu/drm/drm_plane.c >> +++ b/drivers/gpu/drm/drm_plane.c >> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, >> >> return ret; >> } >> + >> +/** >> + * drm_plane_enable_damage_clips - enable damage clips property >> + * @plane: plane on which this property to enable. >> + */ >> +void drm_plane_enable_damage_clips(struct drm_plane *plane) >> +{ >> + struct drm_device *dev = plane->dev; >> + struct drm_mode_config *config = &dev->mode_config; >> + >> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); >> +} >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >> index 7569f22..d8767da 100644 >> --- a/include/drm/drm_mode_config.h >> +++ b/include/drm/drm_mode_config.h >> @@ -628,6 +628,21 @@ struct drm_mode_config { >> */ >> struct drm_property *prop_crtc_id; >> /** >> + * @prop_damage_clips: Optional plane property to mark damaged regions >> + * on the plane in framebuffer coordinates of the framebuffer attached >> + * to the plane. > Why should we make this optional? Looks like just another thing drivers > might screw up, since we have multiple callbacks and things to set up for > proper dirty tracking. > > One option I'm seeing is that if this is set, and it's an atomic driver, > then we just directly call into the default atomic fb->dirty > implementation. That way there's only 1 thing drivers need to do to set up > dirty rect tracking, and they'll get all of it. > >> + * >> + * The layout of blob data is simply an array of drm_mode_rect with >> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike >> + * plane src coordinates, damage clips are not in 16.16 fixed point. > I honestly have no idea where this limit is from. Worth keeping? I can > easily imagine that userspace could trip over this - it's fairly high, but > not unlimited. > >> + * >> + * Damage clips are a hint to kernel as which area of framebuffer has >> + * changed since last page-flip. This should be helpful >> + * for some drivers especially for virtual devices where each >> + * framebuffer change needs to be transmitted over network, usb, etc. > I'd also clarify that userspace still must render the entire screen, i.e. > make it more clear that it's really just a hint and not mandatory to only > scan out the damaged parts. > >> + */ >> + struct drm_property *prop_damage_clips; >> + /** >> * @prop_active: Default atomic CRTC property to control the active >> * state, which is the simplified implementation for DPMS in atomic >> * drivers. >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index f7bf4a4..9f24548 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -146,6 +146,21 @@ struct drm_plane_state { >> */ >> struct drm_crtc_commit *commit; >> >> + /* >> + * @damage_clips >> + * >> + * blob property with damage as array of drm_rect in framebuffer > &drm_rect gives you a nice hyperlink in the generated docs. > >> + * coodinates. >> + */ >> + struct drm_property_blob *damage_clips; >> + >> + /* >> + * @num_clips >> + * >> + * Number of drm_rect in @damage_clips. >> + */ >> + uint32_t num_clips; >> + >> struct drm_atomic_state *state; >> }; >> >> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, >> const uint32_t *formats, unsigned int format_count, >> bool is_primary); >> void drm_plane_cleanup(struct drm_plane *plane); >> +void drm_plane_enable_damage_clips(struct drm_plane *plane); >> >> /** >> * drm_plane_index - find the index of a registered plane >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >> index 50bcf42..0ad0d5b 100644 >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { >> __u32 lessee_id; >> }; >> >> +/** >> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to >> + * user-space. >> + * @x1: horizontal starting coordinate (inclusive) >> + * @y1: vertical starting coordinate (inclusive) >> + * @x2: horizontal ending coordinate (exclusive) >> + * @y2: vertical ending coordinate (exclusive) >> + */ >> +struct drm_mode_rect { >> + __s32 x1; >> + __s32 y1; >> + __s32 x2; >> + __s32 y2; > Why signed? Negative damage rects on an fb don't make sense to me. Also, > please specify what this is exactly (to avoid confusion with the 16.16 > fixed point src rects), and maybe mention in the commit message why we're > not using drm_clip_rect (going to proper uapi types and 32bit makes sense > to me). IMO, while we don't expect negative damage coordinates, to avoid yet another drm uapi rect in the future when we actually need negative numbers signed is a good choice... /Thomas
On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote: > On 04/05/2018 09:35 AM, Daniel Vetter wrote: > > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: > > > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > > > Optional plane property to mark damaged regions on the plane in > > > framebuffer coordinates of the framebuffer attached to the plane. > > > > > > The layout of blob data is simply an array of drm_mode_rect with maximum > > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src > > > coordinates, damage clips are not in 16.16 fixed point. > > > > > > Damage clips are a hint to kernel as which area of framebuffer has > > > changed since last page-flip. This should be helpful for some drivers > > > especially for virtual devices where each framebuffer change needs to > > > be transmitted over network, usb, etc. > > > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a > > > plane should enable this property using drm_plane_enable_damage_clips. > > > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > The property uapi section is missing, see: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e= > > > > Plane composition feels like the best place to put this. Please use that > > section to tie all the various bits together, including the helpers you're > > adding in the following patches for drivers to use. > > > > Bunch of nitpicks below, but overall I'm agreeing now with just going with > > fb coordinate damage rects. > > > > Like you say, the thing needed here now is userspace + driver actually > > implementing this. I'd also say the compat helper to map the legacy > > fb->dirty to this new atomic way of doing things should be included here > > (gives us a lot more testing for these new paths). > > > > Icing on the cake would be an igt to make sure kernel rejects invalid clip > > rects correctly. > > > > > --- > > > drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > > drivers/gpu/drm/drm_mode_config.c | 5 +++++ > > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++ > > > include/drm/drm_mode_config.h | 15 +++++++++++++ > > > include/drm/drm_plane.h | 16 ++++++++++++++ > > > include/uapi/drm/drm_mode.h | 15 +++++++++++++ > > > 7 files changed, 109 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 7d25c42..9226d24 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, > > > } > > > /** > > > + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane > > > + * @state: plane state > > > + * @blob: damage clips in framebuffer coordinates > > > + * > > > + * Returns: > > > + * > > > + * Zero on success, error code on failure. > > > + */ > > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, > > > + struct drm_property_blob *blob) > > > +{ > > > + if (blob == state->damage_clips) > > > + return 0; > > > + > > > + drm_property_blob_put(state->damage_clips); > > > + state->damage_clips = NULL; > > > + > > > + if (blob) { > > > + uint32_t count = blob->length/sizeof(struct drm_rect); > > > + > > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) > > > + return -EINVAL; > > > + > > > + state->damage_clips = drm_property_blob_get(blob); > > > + state->num_clips = count; > > > + } else { > > > + state->damage_clips = NULL; > > > + state->num_clips = 0; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +/** > > > * drm_atomic_get_plane_state - get plane state > > > * @state: global atomic state object > > > * @plane: plane to get state object for > > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > > state->color_encoding = val; > > > } else if (property == plane->color_range_property) { > > > state->color_range = val; > > > + } else if (property == config->prop_damage_clips) { > > > + struct drm_property_blob *blob = > > > + drm_property_lookup_blob(dev, val); > > > + int ret = drm_atomic_set_damage_for_plane(state, blob); > > There's already a helper with size-checking built-in, see > > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd > > just provide a little inline helper that does the > > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). > > > > > + drm_property_blob_put(blob); > > > + return ret; > > > } else if (plane->funcs->atomic_set_property) { > > > return plane->funcs->atomic_set_property(plane, state, > > > property, val); > > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > > > *val = state->color_encoding; > > > } else if (property == plane->color_range_property) { > > > *val = state->color_range; > > > + } else if (property == config->prop_damage_clips) { > > > + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index c356545..55b44e3 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > > state->fence = NULL; > > > state->commit = NULL; > > > + state->damage_clips = NULL; > > > + state->num_clips = 0; > > > } > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > > @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > > if (state->commit) > > > drm_crtc_commit_put(state->commit); > > > + > > > + drm_property_blob_put(state->damage_clips); > > > } > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > > index e5c6533..e93b127 100644 > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); > > Bit a bikeshed, but since the coordinates are in fb pixels, not plane > > pixels, I'd call this "FB_DAMAGE_CLIPS". > > > > > + if (!prop) > > > + return -ENOMEM; > > > + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > > > return ret; > > > } > > > + > > > +/** > > > + * drm_plane_enable_damage_clips - enable damage clips property > > > + * @plane: plane on which this property to enable. > > > + */ > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > > > +{ > > > + struct drm_device *dev = plane->dev; > > > + struct drm_mode_config *config = &dev->mode_config; > > > + > > > + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); > > > +} > > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > > index 7569f22..d8767da 100644 > > > --- a/include/drm/drm_mode_config.h > > > +++ b/include/drm/drm_mode_config.h > > > @@ -628,6 +628,21 @@ struct drm_mode_config { > > > */ > > > struct drm_property *prop_crtc_id; > > > /** > > > + * @prop_damage_clips: Optional plane property to mark damaged regions > > > + * on the plane in framebuffer coordinates of the framebuffer attached > > > + * to the plane. > > Why should we make this optional? Looks like just another thing drivers > > might screw up, since we have multiple callbacks and things to set up for > > proper dirty tracking. > > > > One option I'm seeing is that if this is set, and it's an atomic driver, > > then we just directly call into the default atomic fb->dirty > > implementation. That way there's only 1 thing drivers need to do to set up > > dirty rect tracking, and they'll get all of it. > > > > > + * > > > + * The layout of blob data is simply an array of drm_mode_rect with > > > + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike > > > + * plane src coordinates, damage clips are not in 16.16 fixed point. > > I honestly have no idea where this limit is from. Worth keeping? I can > > easily imagine that userspace could trip over this - it's fairly high, but > > not unlimited. > > > > > + * > > > + * Damage clips are a hint to kernel as which area of framebuffer has > > > + * changed since last page-flip. This should be helpful > > > + * for some drivers especially for virtual devices where each > > > + * framebuffer change needs to be transmitted over network, usb, etc. > > I'd also clarify that userspace still must render the entire screen, i.e. > > make it more clear that it's really just a hint and not mandatory to only > > scan out the damaged parts. > > > > > + */ > > > + struct drm_property *prop_damage_clips; > > > + /** > > > * @prop_active: Default atomic CRTC property to control the active > > > * state, which is the simplified implementation for DPMS in atomic > > > * drivers. > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > index f7bf4a4..9f24548 100644 > > > --- a/include/drm/drm_plane.h > > > +++ b/include/drm/drm_plane.h > > > @@ -146,6 +146,21 @@ struct drm_plane_state { > > > */ > > > struct drm_crtc_commit *commit; > > > + /* > > > + * @damage_clips > > > + * > > > + * blob property with damage as array of drm_rect in framebuffer > > &drm_rect gives you a nice hyperlink in the generated docs. > > > > > + * coodinates. > > > + */ > > > + struct drm_property_blob *damage_clips; > > > + > > > + /* > > > + * @num_clips > > > + * > > > + * Number of drm_rect in @damage_clips. > > > + */ > > > + uint32_t num_clips; > > > + > > > struct drm_atomic_state *state; > > > }; > > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, > > > const uint32_t *formats, unsigned int format_count, > > > bool is_primary); > > > void drm_plane_cleanup(struct drm_plane *plane); > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane); > > > /** > > > * drm_plane_index - find the index of a registered plane > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > > index 50bcf42..0ad0d5b 100644 > > > --- a/include/uapi/drm/drm_mode.h > > > +++ b/include/uapi/drm/drm_mode.h > > > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { > > > __u32 lessee_id; > > > }; > > > +/** > > > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to > > > + * user-space. > > > + * @x1: horizontal starting coordinate (inclusive) > > > + * @y1: vertical starting coordinate (inclusive) > > > + * @x2: horizontal ending coordinate (exclusive) > > > + * @y2: vertical ending coordinate (exclusive) > > > + */ > > > +struct drm_mode_rect { > > > + __s32 x1; > > > + __s32 y1; > > > + __s32 x2; > > > + __s32 y2; > > Why signed? Negative damage rects on an fb don't make sense to me. Also, > > please specify what this is exactly (to avoid confusion with the 16.16 > > fixed point src rects), and maybe mention in the commit message why we're > > not using drm_clip_rect (going to proper uapi types and 32bit makes sense > > to me). > > IMO, while we don't expect negative damage coordinates, > to avoid yet another drm uapi rect in the future when we actually need > negative numbers signed is a good choice... Makes sense. Another thing I realized: Since src rect are 16.16 fixed point, do we really need s32? drm_clip_rect is a bit meh, but it gives us s16 already. That would avoid having to sprinkle the code with tons of overflow checks for input validation. On the topic of input validation: Should the kernel check in shared code that all the clip rects are sensible, i.e. within the dimensions of the fb? Relying on drivers for that seems like a bad idea. That could be done in core code in drm_atomic_plane_check(). -Daniel > > /Thomas > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/05/2018 12:03 PM, Daniel Vetter wrote: > On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote: >> On 04/05/2018 09:35 AM, Daniel Vetter wrote: >>> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: >>>> From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >>>> >>>> Optional plane property to mark damaged regions on the plane in >>>> framebuffer coordinates of the framebuffer attached to the plane. >>>> >>>> The layout of blob data is simply an array of drm_mode_rect with maximum >>>> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src >>>> coordinates, damage clips are not in 16.16 fixed point. >>>> >>>> Damage clips are a hint to kernel as which area of framebuffer has >>>> changed since last page-flip. This should be helpful for some drivers >>>> especially for virtual devices where each framebuffer change needs to >>>> be transmitted over network, usb, etc. >>>> >>>> Driver which are interested in enabling DAMAGE_CLIPS property for a >>>> plane should enable this property using drm_plane_enable_damage_clips. >>>> >>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >>>> Signed-off-by: Deepak Rawat <drawat@vmware.com> >>> The property uapi section is missing, see: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e= >>> >>> Plane composition feels like the best place to put this. Please use that >>> section to tie all the various bits together, including the helpers you're >>> adding in the following patches for drivers to use. >>> >>> Bunch of nitpicks below, but overall I'm agreeing now with just going with >>> fb coordinate damage rects. >>> >>> Like you say, the thing needed here now is userspace + driver actually >>> implementing this. I'd also say the compat helper to map the legacy >>> fb->dirty to this new atomic way of doing things should be included here >>> (gives us a lot more testing for these new paths). >>> >>> Icing on the cake would be an igt to make sure kernel rejects invalid clip >>> rects correctly. >>> >>>> --- >>>> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >>>> drivers/gpu/drm/drm_mode_config.c | 5 +++++ >>>> drivers/gpu/drm/drm_plane.c | 12 +++++++++++ >>>> include/drm/drm_mode_config.h | 15 +++++++++++++ >>>> include/drm/drm_plane.h | 16 ++++++++++++++ >>>> include/uapi/drm/drm_mode.h | 15 +++++++++++++ >>>> 7 files changed, 109 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>> index 7d25c42..9226d24 100644 >>>> --- a/drivers/gpu/drm/drm_atomic.c >>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, >>>> } >>>> /** >>>> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane >>>> + * @state: plane state >>>> + * @blob: damage clips in framebuffer coordinates >>>> + * >>>> + * Returns: >>>> + * >>>> + * Zero on success, error code on failure. >>>> + */ >>>> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, >>>> + struct drm_property_blob *blob) >>>> +{ >>>> + if (blob == state->damage_clips) >>>> + return 0; >>>> + >>>> + drm_property_blob_put(state->damage_clips); >>>> + state->damage_clips = NULL; >>>> + >>>> + if (blob) { >>>> + uint32_t count = blob->length/sizeof(struct drm_rect); >>>> + >>>> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) >>>> + return -EINVAL; >>>> + >>>> + state->damage_clips = drm_property_blob_get(blob); >>>> + state->num_clips = count; >>>> + } else { >>>> + state->damage_clips = NULL; >>>> + state->num_clips = 0; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/** >>>> * drm_atomic_get_plane_state - get plane state >>>> * @state: global atomic state object >>>> * @plane: plane to get state object for >>>> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>>> state->color_encoding = val; >>>> } else if (property == plane->color_range_property) { >>>> state->color_range = val; >>>> + } else if (property == config->prop_damage_clips) { >>>> + struct drm_property_blob *blob = >>>> + drm_property_lookup_blob(dev, val); >>>> + int ret = drm_atomic_set_damage_for_plane(state, blob); >>> There's already a helper with size-checking built-in, see >>> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd >>> just provide a little inline helper that does the >>> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). >>> >>>> + drm_property_blob_put(blob); >>>> + return ret; >>>> } else if (plane->funcs->atomic_set_property) { >>>> return plane->funcs->atomic_set_property(plane, state, >>>> property, val); >>>> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, >>>> *val = state->color_encoding; >>>> } else if (property == plane->color_range_property) { >>>> *val = state->color_range; >>>> + } else if (property == config->prop_damage_clips) { >>>> + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>> index c356545..55b44e3 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >>>> state->fence = NULL; >>>> state->commit = NULL; >>>> + state->damage_clips = NULL; >>>> + state->num_clips = 0; >>>> } >>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); >>>> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >>>> if (state->commit) >>>> drm_crtc_commit_put(state->commit); >>>> + >>>> + drm_property_blob_put(state->damage_clips); >>>> } >>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); >>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >>>> index e5c6533..e93b127 100644 >>>> --- a/drivers/gpu/drm/drm_mode_config.c >>>> +++ b/drivers/gpu/drm/drm_mode_config.c >>>> @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); >>> Bit a bikeshed, but since the coordinates are in fb pixels, not plane >>> pixels, I'd call this "FB_DAMAGE_CLIPS". >>> >>>> + if (!prop) >>>> + return -ENOMEM; >>>> + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 >>>> --- a/drivers/gpu/drm/drm_plane.c >>>> +++ b/drivers/gpu/drm/drm_plane.c >>>> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, >>>> return ret; >>>> } >>>> + >>>> +/** >>>> + * drm_plane_enable_damage_clips - enable damage clips property >>>> + * @plane: plane on which this property to enable. >>>> + */ >>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane) >>>> +{ >>>> + struct drm_device *dev = plane->dev; >>>> + struct drm_mode_config *config = &dev->mode_config; >>>> + >>>> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); >>>> +} >>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >>>> index 7569f22..d8767da 100644 >>>> --- a/include/drm/drm_mode_config.h >>>> +++ b/include/drm/drm_mode_config.h >>>> @@ -628,6 +628,21 @@ struct drm_mode_config { >>>> */ >>>> struct drm_property *prop_crtc_id; >>>> /** >>>> + * @prop_damage_clips: Optional plane property to mark damaged regions >>>> + * on the plane in framebuffer coordinates of the framebuffer attached >>>> + * to the plane. >>> Why should we make this optional? Looks like just another thing drivers >>> might screw up, since we have multiple callbacks and things to set up for >>> proper dirty tracking. >>> >>> One option I'm seeing is that if this is set, and it's an atomic driver, >>> then we just directly call into the default atomic fb->dirty >>> implementation. That way there's only 1 thing drivers need to do to set up >>> dirty rect tracking, and they'll get all of it. >>> >>>> + * >>>> + * The layout of blob data is simply an array of drm_mode_rect with >>>> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike >>>> + * plane src coordinates, damage clips are not in 16.16 fixed point. >>> I honestly have no idea where this limit is from. Worth keeping? I can >>> easily imagine that userspace could trip over this - it's fairly high, but >>> not unlimited. >>> >>>> + * >>>> + * Damage clips are a hint to kernel as which area of framebuffer has >>>> + * changed since last page-flip. This should be helpful >>>> + * for some drivers especially for virtual devices where each >>>> + * framebuffer change needs to be transmitted over network, usb, etc. >>> I'd also clarify that userspace still must render the entire screen, i.e. >>> make it more clear that it's really just a hint and not mandatory to only >>> scan out the damaged parts. >>> >>>> + */ >>>> + struct drm_property *prop_damage_clips; >>>> + /** >>>> * @prop_active: Default atomic CRTC property to control the active >>>> * state, which is the simplified implementation for DPMS in atomic >>>> * drivers. >>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>>> index f7bf4a4..9f24548 100644 >>>> --- a/include/drm/drm_plane.h >>>> +++ b/include/drm/drm_plane.h >>>> @@ -146,6 +146,21 @@ struct drm_plane_state { >>>> */ >>>> struct drm_crtc_commit *commit; >>>> + /* >>>> + * @damage_clips >>>> + * >>>> + * blob property with damage as array of drm_rect in framebuffer >>> &drm_rect gives you a nice hyperlink in the generated docs. >>> >>>> + * coodinates. >>>> + */ >>>> + struct drm_property_blob *damage_clips; >>>> + >>>> + /* >>>> + * @num_clips >>>> + * >>>> + * Number of drm_rect in @damage_clips. >>>> + */ >>>> + uint32_t num_clips; >>>> + >>>> struct drm_atomic_state *state; >>>> }; >>>> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, >>>> const uint32_t *formats, unsigned int format_count, >>>> bool is_primary); >>>> void drm_plane_cleanup(struct drm_plane *plane); >>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane); >>>> /** >>>> * drm_plane_index - find the index of a registered plane >>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >>>> index 50bcf42..0ad0d5b 100644 >>>> --- a/include/uapi/drm/drm_mode.h >>>> +++ b/include/uapi/drm/drm_mode.h >>>> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { >>>> __u32 lessee_id; >>>> }; >>>> +/** >>>> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to >>>> + * user-space. >>>> + * @x1: horizontal starting coordinate (inclusive) >>>> + * @y1: vertical starting coordinate (inclusive) >>>> + * @x2: horizontal ending coordinate (exclusive) >>>> + * @y2: vertical ending coordinate (exclusive) >>>> + */ >>>> +struct drm_mode_rect { >>>> + __s32 x1; >>>> + __s32 y1; >>>> + __s32 x2; >>>> + __s32 y2; >>> Why signed? Negative damage rects on an fb don't make sense to me. Also, >>> please specify what this is exactly (to avoid confusion with the 16.16 >>> fixed point src rects), and maybe mention in the commit message why we're >>> not using drm_clip_rect (going to proper uapi types and 32bit makes sense >>> to me). >> IMO, while we don't expect negative damage coordinates, >> to avoid yet another drm uapi rect in the future when we actually need >> negative numbers signed is a good choice... > Makes sense. Another thing I realized: Since src rect are 16.16 fixed > point, do we really need s32? drm_clip_rect is a bit meh, but it gives us > s16 already. That would avoid having to sprinkle the code with tons of > overflow checks for input validation. IMHO, sooner or later we're going to run into the s16 limitation, and I think we should start making user-space APIs > 15 bit coordinate safe. I agree this could lead to some validation grief in the short run, but less to fix up later. How difficult is it to make the kernel-internal 16.16 fixed point 48.16? /Thomas > > On the topic of input validation: Should the kernel check in shared code > that all the clip rects are sensible, i.e. within the dimensions of the > fb? Relying on drivers for that seems like a bad idea. > > That could be done in core code in drm_atomic_plane_check(). > -Daniel >> /Thomas >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/05/2018 12:03 PM, Daniel Vetter wrote: > > On the topic of input validation: Should the kernel check in shared code > that all the clip rects are sensible, i.e. within the dimensions of the > fb? Relying on drivers for that seems like a bad idea. I guess we could do that. There seems to be different needs for different kinds of iterators, but otherwise I was thinking that an iterator could just skip invalid rects if found. Then we don't need a separate validation step, but OTOH user-space won't get notified if that was intended. I'm not 100% sure user-space would do anything sensible with any error information, though. /Thomas > > That could be done in core code in drm_atomic_plane_check(). > -Daniel >> /Thomas >> >> >> _______________________________________________ >> 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=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8&s=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I&e=
On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote: > On 04/05/2018 12:03 PM, Daniel Vetter wrote: > > On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote: > > > On 04/05/2018 09:35 AM, Daniel Vetter wrote: > > > > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: > > > > > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > > > > > > > Optional plane property to mark damaged regions on the plane in > > > > > framebuffer coordinates of the framebuffer attached to the plane. > > > > > > > > > > The layout of blob data is simply an array of drm_mode_rect with maximum > > > > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src > > > > > coordinates, damage clips are not in 16.16 fixed point. > > > > > > > > > > Damage clips are a hint to kernel as which area of framebuffer has > > > > > changed since last page-flip. This should be helpful for some drivers > > > > > especially for virtual devices where each framebuffer change needs to > > > > > be transmitted over network, usb, etc. > > > > > > > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a > > > > > plane should enable this property using drm_plane_enable_damage_clips. > > > > > > > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > > > The property uapi section is missing, see: > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e= > > > > > > > > Plane composition feels like the best place to put this. Please use that > > > > section to tie all the various bits together, including the helpers you're > > > > adding in the following patches for drivers to use. > > > > > > > > Bunch of nitpicks below, but overall I'm agreeing now with just going with > > > > fb coordinate damage rects. > > > > > > > > Like you say, the thing needed here now is userspace + driver actually > > > > implementing this. I'd also say the compat helper to map the legacy > > > > fb->dirty to this new atomic way of doing things should be included here > > > > (gives us a lot more testing for these new paths). > > > > > > > > Icing on the cake would be an igt to make sure kernel rejects invalid clip > > > > rects correctly. > > > > > > > > > --- > > > > > drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ > > > > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > > > > drivers/gpu/drm/drm_mode_config.c | 5 +++++ > > > > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++ > > > > > include/drm/drm_mode_config.h | 15 +++++++++++++ > > > > > include/drm/drm_plane.h | 16 ++++++++++++++ > > > > > include/uapi/drm/drm_mode.h | 15 +++++++++++++ > > > > > 7 files changed, 109 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > > index 7d25c42..9226d24 100644 > > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, > > > > > } > > > > > /** > > > > > + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane > > > > > + * @state: plane state > > > > > + * @blob: damage clips in framebuffer coordinates > > > > > + * > > > > > + * Returns: > > > > > + * > > > > > + * Zero on success, error code on failure. > > > > > + */ > > > > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, > > > > > + struct drm_property_blob *blob) > > > > > +{ > > > > > + if (blob == state->damage_clips) > > > > > + return 0; > > > > > + > > > > > + drm_property_blob_put(state->damage_clips); > > > > > + state->damage_clips = NULL; > > > > > + > > > > > + if (blob) { > > > > > + uint32_t count = blob->length/sizeof(struct drm_rect); > > > > > + > > > > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) > > > > > + return -EINVAL; > > > > > + > > > > > + state->damage_clips = drm_property_blob_get(blob); > > > > > + state->num_clips = count; > > > > > + } else { > > > > > + state->damage_clips = NULL; > > > > > + state->num_clips = 0; > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +/** > > > > > * drm_atomic_get_plane_state - get plane state > > > > > * @state: global atomic state object > > > > > * @plane: plane to get state object for > > > > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > > > > > state->color_encoding = val; > > > > > } else if (property == plane->color_range_property) { > > > > > state->color_range = val; > > > > > + } else if (property == config->prop_damage_clips) { > > > > > + struct drm_property_blob *blob = > > > > > + drm_property_lookup_blob(dev, val); > > > > > + int ret = drm_atomic_set_damage_for_plane(state, blob); > > > > There's already a helper with size-checking built-in, see > > > > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd > > > > just provide a little inline helper that does the > > > > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). > > > > > > > > > + drm_property_blob_put(blob); > > > > > + return ret; > > > > > } else if (plane->funcs->atomic_set_property) { > > > > > return plane->funcs->atomic_set_property(plane, state, > > > > > property, val); > > > > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > > > > > *val = state->color_encoding; > > > > > } else if (property == plane->color_range_property) { > > > > > *val = state->color_range; > > > > > + } else if (property == config->prop_damage_clips) { > > > > > + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > > > index c356545..55b44e3 100644 > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > > @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > > > > state->fence = NULL; > > > > > state->commit = NULL; > > > > > + state->damage_clips = NULL; > > > > > + state->num_clips = 0; > > > > > } > > > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > > > > @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > > > > if (state->commit) > > > > > drm_crtc_commit_put(state->commit); > > > > > + > > > > > + drm_property_blob_put(state->damage_clips); > > > > > } > > > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > > > > index e5c6533..e93b127 100644 > > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > > @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); > > > > Bit a bikeshed, but since the coordinates are in fb pixels, not plane > > > > pixels, I'd call this "FB_DAMAGE_CLIPS". > > > > > > > > > + if (!prop) > > > > > + return -ENOMEM; > > > > > + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 > > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > > > > > return ret; > > > > > } > > > > > + > > > > > +/** > > > > > + * drm_plane_enable_damage_clips - enable damage clips property > > > > > + * @plane: plane on which this property to enable. > > > > > + */ > > > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > > > > > +{ > > > > > + struct drm_device *dev = plane->dev; > > > > > + struct drm_mode_config *config = &dev->mode_config; > > > > > + > > > > > + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); > > > > > +} > > > > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > > > > index 7569f22..d8767da 100644 > > > > > --- a/include/drm/drm_mode_config.h > > > > > +++ b/include/drm/drm_mode_config.h > > > > > @@ -628,6 +628,21 @@ struct drm_mode_config { > > > > > */ > > > > > struct drm_property *prop_crtc_id; > > > > > /** > > > > > + * @prop_damage_clips: Optional plane property to mark damaged regions > > > > > + * on the plane in framebuffer coordinates of the framebuffer attached > > > > > + * to the plane. > > > > Why should we make this optional? Looks like just another thing drivers > > > > might screw up, since we have multiple callbacks and things to set up for > > > > proper dirty tracking. > > > > > > > > One option I'm seeing is that if this is set, and it's an atomic driver, > > > > then we just directly call into the default atomic fb->dirty > > > > implementation. That way there's only 1 thing drivers need to do to set up > > > > dirty rect tracking, and they'll get all of it. > > > > > > > > > + * > > > > > + * The layout of blob data is simply an array of drm_mode_rect with > > > > > + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike > > > > > + * plane src coordinates, damage clips are not in 16.16 fixed point. > > > > I honestly have no idea where this limit is from. Worth keeping? I can > > > > easily imagine that userspace could trip over this - it's fairly high, but > > > > not unlimited. > > > > > > > > > + * > > > > > + * Damage clips are a hint to kernel as which area of framebuffer has > > > > > + * changed since last page-flip. This should be helpful > > > > > + * for some drivers especially for virtual devices where each > > > > > + * framebuffer change needs to be transmitted over network, usb, etc. > > > > I'd also clarify that userspace still must render the entire screen, i.e. > > > > make it more clear that it's really just a hint and not mandatory to only > > > > scan out the damaged parts. > > > > > > > > > + */ > > > > > + struct drm_property *prop_damage_clips; > > > > > + /** > > > > > * @prop_active: Default atomic CRTC property to control the active > > > > > * state, which is the simplified implementation for DPMS in atomic > > > > > * drivers. > > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > > > index f7bf4a4..9f24548 100644 > > > > > --- a/include/drm/drm_plane.h > > > > > +++ b/include/drm/drm_plane.h > > > > > @@ -146,6 +146,21 @@ struct drm_plane_state { > > > > > */ > > > > > struct drm_crtc_commit *commit; > > > > > + /* > > > > > + * @damage_clips > > > > > + * > > > > > + * blob property with damage as array of drm_rect in framebuffer > > > > &drm_rect gives you a nice hyperlink in the generated docs. > > > > > > > > > + * coodinates. > > > > > + */ > > > > > + struct drm_property_blob *damage_clips; > > > > > + > > > > > + /* > > > > > + * @num_clips > > > > > + * > > > > > + * Number of drm_rect in @damage_clips. > > > > > + */ > > > > > + uint32_t num_clips; > > > > > + > > > > > struct drm_atomic_state *state; > > > > > }; > > > > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, > > > > > const uint32_t *formats, unsigned int format_count, > > > > > bool is_primary); > > > > > void drm_plane_cleanup(struct drm_plane *plane); > > > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane); > > > > > /** > > > > > * drm_plane_index - find the index of a registered plane > > > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > > > > index 50bcf42..0ad0d5b 100644 > > > > > --- a/include/uapi/drm/drm_mode.h > > > > > +++ b/include/uapi/drm/drm_mode.h > > > > > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { > > > > > __u32 lessee_id; > > > > > }; > > > > > +/** > > > > > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to > > > > > + * user-space. > > > > > + * @x1: horizontal starting coordinate (inclusive) > > > > > + * @y1: vertical starting coordinate (inclusive) > > > > > + * @x2: horizontal ending coordinate (exclusive) > > > > > + * @y2: vertical ending coordinate (exclusive) > > > > > + */ > > > > > +struct drm_mode_rect { > > > > > + __s32 x1; > > > > > + __s32 y1; > > > > > + __s32 x2; > > > > > + __s32 y2; > > > > Why signed? Negative damage rects on an fb don't make sense to me. Also, > > > > please specify what this is exactly (to avoid confusion with the 16.16 > > > > fixed point src rects), and maybe mention in the commit message why we're > > > > not using drm_clip_rect (going to proper uapi types and 32bit makes sense > > > > to me). > > > IMO, while we don't expect negative damage coordinates, > > > to avoid yet another drm uapi rect in the future when we actually need > > > negative numbers signed is a good choice... > > Makes sense. Another thing I realized: Since src rect are 16.16 fixed > > point, do we really need s32? drm_clip_rect is a bit meh, but it gives us > > s16 already. That would avoid having to sprinkle the code with tons of > > overflow checks for input validation. > > IMHO, sooner or later we're going to run into the s16 limitation, and I > think we should start > making user-space APIs > 15 bit coordinate safe. I agree this could lead to > some validation > grief in the short run, but less to fix up later. If all the corner-case validation comes with selftests I'm happy to review them for completeness. I'm not good enough to figure out whether there's missing stuff by just looking at the validation code generally. > How difficult is it to make the kernel-internal 16.16 fixed point 48.16? Since we range-limit properties it should work out. But that means easy bitshifting will much more likely overflow, and I kinda don't want to audit all those places for something we don't yet need. Even 8k is still a bit off from the limit. Once we have hw that can scan out 32k buffers we'll probably have to fix up everything :-/ -Daniel > > /Thomas > > > > > > On the topic of input validation: Should the kernel check in shared code > > that all the clip rects are sensible, i.e. within the dimensions of the > > fb? Relying on drivers for that seems like a bad idea. > > > > That could be done in core code in drm_atomic_plane_check(). > > -Daniel > > > /Thomas > > > > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Apr 05, 2018 at 01:42:11PM +0200, Thomas Hellstrom wrote: > On 04/05/2018 12:03 PM, Daniel Vetter wrote: > > > > On the topic of input validation: Should the kernel check in shared code > > that all the clip rects are sensible, i.e. within the dimensions of the > > fb? Relying on drivers for that seems like a bad idea. > > I guess we could do that. > > There seems to be different needs for different kinds of iterators, but > otherwise > I was thinking that an iterator could just skip invalid rects if found. Then > we > don't need a separate validation step, but OTOH user-space won't get > notified if > that was intended. > > I'm not 100% sure user-space would do anything sensible with any error > information, though. Fix userspace, and ime letting userspace get away with improper uapi usage is a path full of regrets. That's why I'm advising that we check any unused field for 0, and any unused bits also. Plus anything else that userspace could get wrong. Because if it's not checked, someone will get it wrong, and then we have to keep it working forever. -Daniel > > /Thomas > > > > > > That could be done in core code in drm_atomic_plane_check(). > > -Daniel > > > /Thomas > > > > > > > > > _______________________________________________ > > > 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=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=JV7fhQ4zTiyiKsY8M0Zlf81l7szYePUke8kwSvQv1T8&s=HEcbr-8aoWqRvWGY6RcL1QeAtEyxMRLHbsOtdJFk78I&e= > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 04/05/2018 03:47 PM, Daniel Vetter wrote: > On Thu, Apr 05, 2018 at 01:35:25PM +0200, Thomas Hellstrom wrote: >> On 04/05/2018 12:03 PM, Daniel Vetter wrote: >>> On Thu, Apr 05, 2018 at 11:00:15AM +0200, Thomas Hellstrom wrote: >>>> On 04/05/2018 09:35 AM, Daniel Vetter wrote: >>>>> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: >>>>>> From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >>>>>> >>>>>> Optional plane property to mark damaged regions on the plane in >>>>>> framebuffer coordinates of the framebuffer attached to the plane. >>>>>> >>>>>> The layout of blob data is simply an array of drm_mode_rect with maximum >>>>>> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src >>>>>> coordinates, damage clips are not in 16.16 fixed point. >>>>>> >>>>>> Damage clips are a hint to kernel as which area of framebuffer has >>>>>> changed since last page-flip. This should be helpful for some drivers >>>>>> especially for virtual devices where each framebuffer change needs to >>>>>> be transmitted over network, usb, etc. >>>>>> >>>>>> Driver which are interested in enabling DAMAGE_CLIPS property for a >>>>>> plane should enable this property using drm_plane_enable_damage_clips. >>>>>> >>>>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> >>>>>> Signed-off-by: Deepak Rawat <drawat@vmware.com> >>>>> The property uapi section is missing, see: >>>>> >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane-2Dcomposition-2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=wnSlgOCqfpNS4d02vP68_E9q2BNMCwfD2OZ_6dCFVQQ&m=J31YNV1uz3IVRaorM-eo48msi9__sQurqRZrig2UE1s&s=vzsuquF1agbOw54HSC_18Kk2k9j83m8RusmDCtPAFWQ&e= >>>>> >>>>> Plane composition feels like the best place to put this. Please use that >>>>> section to tie all the various bits together, including the helpers you're >>>>> adding in the following patches for drivers to use. >>>>> >>>>> Bunch of nitpicks below, but overall I'm agreeing now with just going with >>>>> fb coordinate damage rects. >>>>> >>>>> Like you say, the thing needed here now is userspace + driver actually >>>>> implementing this. I'd also say the compat helper to map the legacy >>>>> fb->dirty to this new atomic way of doing things should be included here >>>>> (gives us a lot more testing for these new paths). >>>>> >>>>> Icing on the cake would be an igt to make sure kernel rejects invalid clip >>>>> rects correctly. >>>>> >>>>>> --- >>>>>> drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ >>>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >>>>>> drivers/gpu/drm/drm_mode_config.c | 5 +++++ >>>>>> drivers/gpu/drm/drm_plane.c | 12 +++++++++++ >>>>>> include/drm/drm_mode_config.h | 15 +++++++++++++ >>>>>> include/drm/drm_plane.h | 16 ++++++++++++++ >>>>>> include/uapi/drm/drm_mode.h | 15 +++++++++++++ >>>>>> 7 files changed, 109 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>>>> index 7d25c42..9226d24 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>>> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, >>>>>> } >>>>>> /** >>>>>> + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane >>>>>> + * @state: plane state >>>>>> + * @blob: damage clips in framebuffer coordinates >>>>>> + * >>>>>> + * Returns: >>>>>> + * >>>>>> + * Zero on success, error code on failure. >>>>>> + */ >>>>>> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, >>>>>> + struct drm_property_blob *blob) >>>>>> +{ >>>>>> + if (blob == state->damage_clips) >>>>>> + return 0; >>>>>> + >>>>>> + drm_property_blob_put(state->damage_clips); >>>>>> + state->damage_clips = NULL; >>>>>> + >>>>>> + if (blob) { >>>>>> + uint32_t count = blob->length/sizeof(struct drm_rect); >>>>>> + >>>>>> + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + state->damage_clips = drm_property_blob_get(blob); >>>>>> + state->num_clips = count; >>>>>> + } else { >>>>>> + state->damage_clips = NULL; >>>>>> + state->num_clips = 0; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> * drm_atomic_get_plane_state - get plane state >>>>>> * @state: global atomic state object >>>>>> * @plane: plane to get state object for >>>>>> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, >>>>>> state->color_encoding = val; >>>>>> } else if (property == plane->color_range_property) { >>>>>> state->color_range = val; >>>>>> + } else if (property == config->prop_damage_clips) { >>>>>> + struct drm_property_blob *blob = >>>>>> + drm_property_lookup_blob(dev, val); >>>>>> + int ret = drm_atomic_set_damage_for_plane(state, blob); >>>>> There's already a helper with size-checking built-in, see >>>>> drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips I'd >>>>> just provide a little inline helper that does the >>>>> blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). >>>>> >>>>>> + drm_property_blob_put(blob); >>>>>> + return ret; >>>>>> } else if (plane->funcs->atomic_set_property) { >>>>>> return plane->funcs->atomic_set_property(plane, state, >>>>>> property, val); >>>>>> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, >>>>>> *val = state->color_encoding; >>>>>> } else if (property == plane->color_range_property) { >>>>>> *val = state->color_range; >>>>>> + } else if (property == config->prop_damage_clips) { >>>>>> + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>>>> index c356545..55b44e3 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>>> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >>>>>> state->fence = NULL; >>>>>> state->commit = NULL; >>>>>> + state->damage_clips = NULL; >>>>>> + state->num_clips = 0; >>>>>> } >>>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); >>>>>> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >>>>>> if (state->commit) >>>>>> drm_crtc_commit_put(state->commit); >>>>>> + >>>>>> + drm_property_blob_put(state->damage_clips); >>>>>> } >>>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); >>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >>>>>> index e5c6533..e93b127 100644 >>>>>> --- a/drivers/gpu/drm/drm_mode_config.c >>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c >>>>>> @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); >>>>> Bit a bikeshed, but since the coordinates are in fb pixels, not plane >>>>> pixels, I'd call this "FB_DAMAGE_CLIPS". >>>>> >>>>>> + if (!prop) >>>>>> + return -ENOMEM; >>>>>> + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 >>>>>> --- a/drivers/gpu/drm/drm_plane.c >>>>>> +++ b/drivers/gpu/drm/drm_plane.c >>>>>> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, >>>>>> return ret; >>>>>> } >>>>>> + >>>>>> +/** >>>>>> + * drm_plane_enable_damage_clips - enable damage clips property >>>>>> + * @plane: plane on which this property to enable. >>>>>> + */ >>>>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane) >>>>>> +{ >>>>>> + struct drm_device *dev = plane->dev; >>>>>> + struct drm_mode_config *config = &dev->mode_config; >>>>>> + >>>>>> + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); >>>>>> +} >>>>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >>>>>> index 7569f22..d8767da 100644 >>>>>> --- a/include/drm/drm_mode_config.h >>>>>> +++ b/include/drm/drm_mode_config.h >>>>>> @@ -628,6 +628,21 @@ struct drm_mode_config { >>>>>> */ >>>>>> struct drm_property *prop_crtc_id; >>>>>> /** >>>>>> + * @prop_damage_clips: Optional plane property to mark damaged regions >>>>>> + * on the plane in framebuffer coordinates of the framebuffer attached >>>>>> + * to the plane. >>>>> Why should we make this optional? Looks like just another thing drivers >>>>> might screw up, since we have multiple callbacks and things to set up for >>>>> proper dirty tracking. >>>>> >>>>> One option I'm seeing is that if this is set, and it's an atomic driver, >>>>> then we just directly call into the default atomic fb->dirty >>>>> implementation. That way there's only 1 thing drivers need to do to set up >>>>> dirty rect tracking, and they'll get all of it. >>>>> >>>>>> + * >>>>>> + * The layout of blob data is simply an array of drm_mode_rect with >>>>>> + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike >>>>>> + * plane src coordinates, damage clips are not in 16.16 fixed point. >>>>> I honestly have no idea where this limit is from. Worth keeping? I can >>>>> easily imagine that userspace could trip over this - it's fairly high, but >>>>> not unlimited. >>>>> >>>>>> + * >>>>>> + * Damage clips are a hint to kernel as which area of framebuffer has >>>>>> + * changed since last page-flip. This should be helpful >>>>>> + * for some drivers especially for virtual devices where each >>>>>> + * framebuffer change needs to be transmitted over network, usb, etc. >>>>> I'd also clarify that userspace still must render the entire screen, i.e. >>>>> make it more clear that it's really just a hint and not mandatory to only >>>>> scan out the damaged parts. >>>>> >>>>>> + */ >>>>>> + struct drm_property *prop_damage_clips; >>>>>> + /** >>>>>> * @prop_active: Default atomic CRTC property to control the active >>>>>> * state, which is the simplified implementation for DPMS in atomic >>>>>> * drivers. >>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>>>>> index f7bf4a4..9f24548 100644 >>>>>> --- a/include/drm/drm_plane.h >>>>>> +++ b/include/drm/drm_plane.h >>>>>> @@ -146,6 +146,21 @@ struct drm_plane_state { >>>>>> */ >>>>>> struct drm_crtc_commit *commit; >>>>>> + /* >>>>>> + * @damage_clips >>>>>> + * >>>>>> + * blob property with damage as array of drm_rect in framebuffer >>>>> &drm_rect gives you a nice hyperlink in the generated docs. >>>>> >>>>>> + * coodinates. >>>>>> + */ >>>>>> + struct drm_property_blob *damage_clips; >>>>>> + >>>>>> + /* >>>>>> + * @num_clips >>>>>> + * >>>>>> + * Number of drm_rect in @damage_clips. >>>>>> + */ >>>>>> + uint32_t num_clips; >>>>>> + >>>>>> struct drm_atomic_state *state; >>>>>> }; >>>>>> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, >>>>>> const uint32_t *formats, unsigned int format_count, >>>>>> bool is_primary); >>>>>> void drm_plane_cleanup(struct drm_plane *plane); >>>>>> +void drm_plane_enable_damage_clips(struct drm_plane *plane); >>>>>> /** >>>>>> * drm_plane_index - find the index of a registered plane >>>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >>>>>> index 50bcf42..0ad0d5b 100644 >>>>>> --- a/include/uapi/drm/drm_mode.h >>>>>> +++ b/include/uapi/drm/drm_mode.h >>>>>> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { >>>>>> __u32 lessee_id; >>>>>> }; >>>>>> +/** >>>>>> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to >>>>>> + * user-space. >>>>>> + * @x1: horizontal starting coordinate (inclusive) >>>>>> + * @y1: vertical starting coordinate (inclusive) >>>>>> + * @x2: horizontal ending coordinate (exclusive) >>>>>> + * @y2: vertical ending coordinate (exclusive) >>>>>> + */ >>>>>> +struct drm_mode_rect { >>>>>> + __s32 x1; >>>>>> + __s32 y1; >>>>>> + __s32 x2; >>>>>> + __s32 y2; >>>>> Why signed? Negative damage rects on an fb don't make sense to me. Also, >>>>> please specify what this is exactly (to avoid confusion with the 16.16 >>>>> fixed point src rects), and maybe mention in the commit message why we're >>>>> not using drm_clip_rect (going to proper uapi types and 32bit makes sense >>>>> to me). >>>> IMO, while we don't expect negative damage coordinates, >>>> to avoid yet another drm uapi rect in the future when we actually need >>>> negative numbers signed is a good choice... >>> Makes sense. Another thing I realized: Since src rect are 16.16 fixed >>> point, do we really need s32? drm_clip_rect is a bit meh, but it gives us >>> s16 already. That would avoid having to sprinkle the code with tons of >>> overflow checks for input validation. >> IMHO, sooner or later we're going to run into the s16 limitation, and I >> think we should start >> making user-space APIs > 15 bit coordinate safe. I agree this could lead to >> some validation >> grief in the short run, but less to fix up later. > If all the corner-case validation comes with selftests I'm happy to review > them for completeness. I'm not good enough to figure out whether there's > missing stuff by just looking at the validation code generally. > >> How difficult is it to make the kernel-internal 16.16 fixed point 48.16? > Since we range-limit properties it should work out. But that means easy > bitshifting will much more likely overflow, and I kinda don't want to > audit all those places for something we don't yet need. Even 8k is still a > bit off from the limit. > > Once we have hw that can scan out 32k buffers we'll probably have to fix > up everything :-/ We have frequent requests for 4x4K functionality. In the 3D case we're currently typically restricted by using a single GPU texture as a framebuffer, but when we're 2D only, like mainly on ESX, there's no such restriction. Our kernel driver copies out of the giant framebuffer if needed, but 16 bit is probably soon starting to be a real restriction (8Kx4) , as least as long as compositors insist on a single giant framebuffer. /Thomas
> > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: > > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > Optional plane property to mark damaged regions on the plane in > > framebuffer coordinates of the framebuffer attached to the plane. > > > > The layout of blob data is simply an array of drm_mode_rect with > maximum > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src > > coordinates, damage clips are not in 16.16 fixed point. > > > > Damage clips are a hint to kernel as which area of framebuffer has > > changed since last page-flip. This should be helpful for some drivers > > especially for virtual devices where each framebuffer change needs to > > be transmitted over network, usb, etc. > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a > > plane should enable this property using drm_plane_enable_damage_clips. > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > The property uapi section is missing, see: > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane- > 2Dcomposition- > 2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762 > SxAf- > cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q > z_3vlEC9Q&s=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY&e= > > Plane composition feels like the best place to put this. Please use that > section to tie all the various bits together, including the helpers you're > adding in the following patches for drivers to use. > > Bunch of nitpicks below, but overall I'm agreeing now with just going with > fb coordinate damage rects. > > Like you say, the thing needed here now is userspace + driver actually > implementing this. I'd also say the compat helper to map the legacy > fb->dirty to this new atomic way of doing things should be included here > (gives us a lot more testing for these new paths). > > Icing on the cake would be an igt to make sure kernel rejects invalid clip > rects correctly. > > > --- > > drivers/gpu/drm/drm_atomic.c | 42 > +++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > drivers/gpu/drm/drm_mode_config.c | 5 +++++ > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++ > > include/drm/drm_mode_config.h | 15 +++++++++++++ > > include/drm/drm_plane.h | 16 ++++++++++++++ > > include/uapi/drm/drm_mode.h | 15 +++++++++++++ > > 7 files changed, 109 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c > b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42..9226d24 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct > drm_printer *p, > > } > > > > /** > > + * drm_atomic_set_damage_for_plane - sets the damage clips property > to plane > > + * @state: plane state > > + * @blob: damage clips in framebuffer coordinates > > + * > > + * Returns: > > + * > > + * Zero on success, error code on failure. > > + */ > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state > *state, > > + struct drm_property_blob *blob) > > +{ > > + if (blob == state->damage_clips) > > + return 0; > > + > > + drm_property_blob_put(state->damage_clips); > > + state->damage_clips = NULL; > > + > > + if (blob) { > > + uint32_t count = blob->length/sizeof(struct drm_rect); > > + > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) > > + return -EINVAL; > > + > > + state->damage_clips = drm_property_blob_get(blob); > > + state->num_clips = count; > > + } else { > > + state->damage_clips = NULL; > > + state->num_clips = 0; > > + } > > + > > + return 0; > > +} > > + > > +/** > > * drm_atomic_get_plane_state - get plane state > > * @state: global atomic state object > > * @plane: plane to get state object for > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct > drm_plane *plane, > > state->color_encoding = val; > > } else if (property == plane->color_range_property) { > > state->color_range = val; > > + } else if (property == config->prop_damage_clips) { > > + struct drm_property_blob *blob = > > + drm_property_lookup_blob(dev, val); > > + int ret = drm_atomic_set_damage_for_plane(state, blob); > > There's already a helper with size-checking built-in, see > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips > I'd > just provide a little inline helper that does the > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). > > > + drm_property_blob_put(blob); > > + return ret; > > } else if (plane->funcs->atomic_set_property) { > > return plane->funcs->atomic_set_property(plane, state, > > property, val); > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane > *plane, > > *val = state->color_encoding; > > } else if (property == plane->color_range_property) { > > *val = state->color_range; > > + } else if (property == config->prop_damage_clips) { > > + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > > index c356545..55b44e3 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3506,6 +3506,8 @@ void > __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > > > state->fence = NULL; > > state->commit = NULL; > > + state->damage_clips = NULL; > > + state->num_clips = 0; > > } > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > > > @@ -3550,6 +3552,8 @@ void > __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > > > if (state->commit) > > drm_crtc_commit_put(state->commit); > > + > > + drm_property_blob_put(state->damage_clips); > > } > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > > index e5c6533..e93b127 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -293,6 +293,11 @@ 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, > "DAMAGE_CLIPS", 0); > > Bit a bikeshed, but since the coordinates are in fb pixels, not plane > pixels, I'd call this "FB_DAMAGE_CLIPS". > > > + if (!prop) > > + return -ENOMEM; > > + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 > > --- a/drivers/gpu/drm/drm_plane.c > > +++ b/drivers/gpu/drm/drm_plane.c > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct > drm_device *dev, > > > > return ret; > > } > > + > > +/** > > + * drm_plane_enable_damage_clips - enable damage clips property > > + * @plane: plane on which this property to enable. > > + */ > > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > > +{ > > + struct drm_device *dev = plane->dev; > > + struct drm_mode_config *config = &dev->mode_config; > > + > > + drm_object_attach_property(&plane->base, config- > >prop_damage_clips, 0); > > +} > > diff --git a/include/drm/drm_mode_config.h > b/include/drm/drm_mode_config.h > > index 7569f22..d8767da 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -628,6 +628,21 @@ struct drm_mode_config { > > */ > > struct drm_property *prop_crtc_id; > > /** > > + * @prop_damage_clips: Optional plane property to mark damaged > regions > > + * on the plane in framebuffer coordinates of the framebuffer > attached > > + * to the plane. > > Why should we make this optional? Looks like just another thing drivers > might screw up, since we have multiple callbacks and things to set up for > proper dirty tracking. Thanks Daniel for the review. I think not all compositor will be interested in sending damage, that was the reason to make this optional. Also when damage is not set that means user-space need full update just like eglSwapBuffersWithDamageKHR. I will add better documentation. > > One option I'm seeing is that if this is set, and it's an atomic driver, > then we just directly call into the default atomic fb->dirty > implementation. That way there's only 1 thing drivers need to do to set up > dirty rect tracking, and they'll get all of it. > > > + * > > + * The layout of blob data is simply an array of drm_mode_rect with > > + * maximum array size limited by > DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike > > + * plane src coordinates, damage clips are not in 16.16 fixed point. > > I honestly have no idea where this limit is from. Worth keeping? I can > easily imagine that userspace could trip over this - it's fairly high, but > not unlimited. DRM_MODE_FB_DIRTY_MAX_CLIPS was used to just have an upper limit and its already exposed to user-space. IMHO there has to be an upper limit to avoid unnecessary overflow ? > > > + * > > + * Damage clips are a hint to kernel as which area of framebuffer has > > + * changed since last page-flip. This should be helpful > > + * for some drivers especially for virtual devices where each > > + * framebuffer change needs to be transmitted over network, usb, > etc. > > I'd also clarify that userspace still must render the entire screen, i.e. > make it more clear that it's really just a hint and not mandatory to only > scan out the damaged parts. Yes will work on better documentation. > > > + */ > > + struct drm_property *prop_damage_clips; > > + /** > > * @prop_active: Default atomic CRTC property to control the active > > * state, which is the simplified implementation for DPMS in atomic > > * drivers. > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > index f7bf4a4..9f24548 100644 > > --- a/include/drm/drm_plane.h > > +++ b/include/drm/drm_plane.h > > @@ -146,6 +146,21 @@ struct drm_plane_state { > > */ > > struct drm_crtc_commit *commit; > > > > + /* > > + * @damage_clips > > + * > > + * blob property with damage as array of drm_rect in framebuffer > > &drm_rect gives you a nice hyperlink in the generated docs. > > > + * coodinates. > > + */ > > + struct drm_property_blob *damage_clips; > > + > > + /* > > + * @num_clips > > + * > > + * Number of drm_rect in @damage_clips. > > + */ > > + uint32_t num_clips; > > + > > struct drm_atomic_state *state; > > }; > > > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, > > const uint32_t *formats, unsigned int format_count, > > bool is_primary); > > void drm_plane_cleanup(struct drm_plane *plane); > > +void drm_plane_enable_damage_clips(struct drm_plane *plane); > > > > /** > > * drm_plane_index - find the index of a registered plane > > diff --git a/include/uapi/drm/drm_mode.h > b/include/uapi/drm/drm_mode.h > > index 50bcf42..0ad0d5b 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { > > __u32 lessee_id; > > }; > > > > +/** > > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported > to > > + * user-space. > > + * @x1: horizontal starting coordinate (inclusive) > > + * @y1: vertical starting coordinate (inclusive) > > + * @x2: horizontal ending coordinate (exclusive) > > + * @y2: vertical ending coordinate (exclusive) > > + */ > > +struct drm_mode_rect { > > + __s32 x1; > > + __s32 y1; > > + __s32 x2; > > + __s32 y2; > > Why signed? Negative damage rects on an fb don't make sense to me. Also, > please specify what this is exactly (to avoid confusion with the 16.16 > fixed point src rects), and maybe mention in the commit message why we're > not using drm_clip_rect (going to proper uapi types and 32bit makes sense > to me). > > Cheers, Daniel > > +}; > > + > > #if defined(__cplusplus) > > } > > #endif > > -- > > 2.7.4 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__blog.ffwll.ch&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK > 0762SxAf- > cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q > z_3vlEC9Q&s=kLx5XCTMRYRoecNhwwwN2XItT4Rt0ib12-UP5VB4XLI&e=
On Thu, Apr 05, 2018 at 11:07:19PM +0000, Deepak Singh Rawat wrote: > > > > On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote: > > > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > > > > Optional plane property to mark damaged regions on the plane in > > > framebuffer coordinates of the framebuffer attached to the plane. > > > > > > The layout of blob data is simply an array of drm_mode_rect with > > maximum > > > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src > > > coordinates, damage clips are not in 16.16 fixed point. > > > > > > Damage clips are a hint to kernel as which area of framebuffer has > > > changed since last page-flip. This should be helpful for some drivers > > > especially for virtual devices where each framebuffer change needs to > > > be transmitted over network, usb, etc. > > > > > > Driver which are interested in enabling DAMAGE_CLIPS property for a > > > plane should enable this property using drm_plane_enable_damage_clips. > > > > > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > > Signed-off-by: Deepak Rawat <drawat@vmware.com> > > > > The property uapi section is missing, see: > > > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__dri.freedesktop.org_docs_drm_gpu_drm-2Dkms.html-23plane- > > 2Dcomposition- > > 2Dproperties&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762 > > SxAf- > > cyZdStnd2NQpRu98lJP2iYGw&m=ELAUsNTjD0ICwUEDFjPW4jsmy2A5AkhS5Q > > z_3vlEC9Q&s=nH-HNXPczoJQMj1iwHiGfrhXz4-00b0M8-3kirjm-EY&e= > > > > Plane composition feels like the best place to put this. Please use that > > section to tie all the various bits together, including the helpers you're > > adding in the following patches for drivers to use. > > > > Bunch of nitpicks below, but overall I'm agreeing now with just going with > > fb coordinate damage rects. > > > > Like you say, the thing needed here now is userspace + driver actually > > implementing this. I'd also say the compat helper to map the legacy > > fb->dirty to this new atomic way of doing things should be included here > > (gives us a lot more testing for these new paths). > > > > Icing on the cake would be an igt to make sure kernel rejects invalid clip > > rects correctly. > > > > > --- > > > drivers/gpu/drm/drm_atomic.c | 42 > > +++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > > > drivers/gpu/drm/drm_mode_config.c | 5 +++++ > > > drivers/gpu/drm/drm_plane.c | 12 +++++++++++ > > > include/drm/drm_mode_config.h | 15 +++++++++++++ > > > include/drm/drm_plane.h | 16 ++++++++++++++ > > > include/uapi/drm/drm_mode.h | 15 +++++++++++++ > > > 7 files changed, 109 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c > > b/drivers/gpu/drm/drm_atomic.c > > > index 7d25c42..9226d24 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct > > drm_printer *p, > > > } > > > > > > /** > > > + * drm_atomic_set_damage_for_plane - sets the damage clips property > > to plane > > > + * @state: plane state > > > + * @blob: damage clips in framebuffer coordinates > > > + * > > > + * Returns: > > > + * > > > + * Zero on success, error code on failure. > > > + */ > > > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state > > *state, > > > + struct drm_property_blob *blob) > > > +{ > > > + if (blob == state->damage_clips) > > > + return 0; > > > + > > > + drm_property_blob_put(state->damage_clips); > > > + state->damage_clips = NULL; > > > + > > > + if (blob) { > > > + uint32_t count = blob->length/sizeof(struct drm_rect); > > > + > > > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) > > > + return -EINVAL; > > > + > > > + state->damage_clips = drm_property_blob_get(blob); > > > + state->num_clips = count; > > > + } else { > > > + state->damage_clips = NULL; > > > + state->num_clips = 0; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +/** > > > * drm_atomic_get_plane_state - get plane state > > > * @state: global atomic state object > > > * @plane: plane to get state object for > > > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct > > drm_plane *plane, > > > state->color_encoding = val; > > > } else if (property == plane->color_range_property) { > > > state->color_range = val; > > > + } else if (property == config->prop_damage_clips) { > > > + struct drm_property_blob *blob = > > > + drm_property_lookup_blob(dev, val); > > > + int ret = drm_atomic_set_damage_for_plane(state, blob); > > > > There's already a helper with size-checking built-in, see > > drm_atomic_replace_property_blob_from_id(). Wrt computing num_clips > > I'd > > just provide a little inline helper that does the > > blob->length/sizeof(drm_rect) conversion (it's just a shift, so fast). > > > > > + drm_property_blob_put(blob); > > > + return ret; > > > } else if (plane->funcs->atomic_set_property) { > > > return plane->funcs->atomic_set_property(plane, state, > > > property, val); > > > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane > > *plane, > > > *val = state->color_encoding; > > > } else if (property == plane->color_range_property) { > > > *val = state->color_range; > > > + } else if (property == config->prop_damage_clips) { > > > + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > > index c356545..55b44e3 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -3506,6 +3506,8 @@ void > > __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > > > > > state->fence = NULL; > > > state->commit = NULL; > > > + state->damage_clips = NULL; > > > + state->num_clips = 0; > > > } > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > > > > > @@ -3550,6 +3552,8 @@ void > > __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > > > > > if (state->commit) > > > drm_crtc_commit_put(state->commit); > > > + > > > + drm_property_blob_put(state->damage_clips); > > > } > > > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > b/drivers/gpu/drm/drm_mode_config.c > > > index e5c6533..e93b127 100644 > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > @@ -293,6 +293,11 @@ 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, > > "DAMAGE_CLIPS", 0); > > > > Bit a bikeshed, but since the coordinates are in fb pixels, not plane > > pixels, I'd call this "FB_DAMAGE_CLIPS". > > > > > + if (!prop) > > > + return -ENOMEM; > > > + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct > > drm_device *dev, > > > > > > return ret; > > > } > > > + > > > +/** > > > + * drm_plane_enable_damage_clips - enable damage clips property > > > + * @plane: plane on which this property to enable. > > > + */ > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > > > +{ > > > + struct drm_device *dev = plane->dev; > > > + struct drm_mode_config *config = &dev->mode_config; > > > + > > > + drm_object_attach_property(&plane->base, config- > > >prop_damage_clips, 0); > > > +} > > > diff --git a/include/drm/drm_mode_config.h > > b/include/drm/drm_mode_config.h > > > index 7569f22..d8767da 100644 > > > --- a/include/drm/drm_mode_config.h > > > +++ b/include/drm/drm_mode_config.h > > > @@ -628,6 +628,21 @@ struct drm_mode_config { > > > */ > > > struct drm_property *prop_crtc_id; > > > /** > > > + * @prop_damage_clips: Optional plane property to mark damaged > > regions > > > + * on the plane in framebuffer coordinates of the framebuffer > > attached > > > + * to the plane. > > > > Why should we make this optional? Looks like just another thing drivers > > might screw up, since we have multiple callbacks and things to set up for > > proper dirty tracking. > > Thanks Daniel for the review. > > I think not all compositor will be interested in sending damage, that was the > reason to make this optional. Also when damage is not set that means > user-space need full update just like eglSwapBuffersWithDamageKHR. > > I will add better documentation. I think if we also handle this case in the helper that'd be even better: In the case of no damage, the helper/core code could automatically supply a damage rect for the entire buffer. That way drivers don't have to handle this case specially. -Daniel
> > > > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > > > > +{ > > > > + struct drm_device *dev = plane->dev; > > > > + struct drm_mode_config *config = &dev->mode_config; > > > > + > > > > + drm_object_attach_property(&plane->base, config- > > > >prop_damage_clips, 0); > > > > +} > > > > diff --git a/include/drm/drm_mode_config.h > > > b/include/drm/drm_mode_config.h > > > > index 7569f22..d8767da 100644 > > > > --- a/include/drm/drm_mode_config.h > > > > +++ b/include/drm/drm_mode_config.h > > > > @@ -628,6 +628,21 @@ struct drm_mode_config { > > > > */ > > > > struct drm_property *prop_crtc_id; > > > > /** > > > > + * @prop_damage_clips: Optional plane property to mark damaged > > > regions > > > > + * on the plane in framebuffer coordinates of the framebuffer > > > attached > > > > + * to the plane. > > > > > > Why should we make this optional? Looks like just another thing drivers > > > might screw up, since we have multiple callbacks and things to set up for > > > proper dirty tracking. > > > > Thanks Daniel for the review. > > > > I think not all compositor will be interested in sending damage, that was the > > reason to make this optional. Also when damage is not set that means > > user-space need full update just like eglSwapBuffersWithDamageKHR. > > > > I will add better documentation. > > I think if we also handle this case in the helper that'd be even better: > In the case of no damage, the helper/core code could automatically supply > a damage rect for the entire buffer. That way drivers don't have to handle > this case specially. > -Daniel Agreed. > --
On 05/04/2018 01:49, Deepak Rawat wrote: > From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > > Optional plane property to mark damaged regions on the plane in > framebuffer coordinates of the framebuffer attached to the plane. > > The layout of blob data is simply an array of drm_mode_rect with maximum > array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src > coordinates, damage clips are not in 16.16 fixed point. > > Damage clips are a hint to kernel as which area of framebuffer has > changed since last page-flip. This should be helpful for some drivers > especially for virtual devices where each framebuffer change needs to > be transmitted over network, usb, etc. > > Driver which are interested in enabling DAMAGE_CLIPS property for a > plane should enable this property using drm_plane_enable_damage_clips. > > Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com> > Signed-off-by: Deepak Rawat <drawat@vmware.com> > --- > drivers/gpu/drm/drm_atomic.c | 42 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ > drivers/gpu/drm/drm_mode_config.c | 5 +++++ > drivers/gpu/drm/drm_plane.c | 12 +++++++++++ > include/drm/drm_mode_config.h | 15 +++++++++++++ > include/drm/drm_plane.h | 16 ++++++++++++++ > include/uapi/drm/drm_mode.h | 15 +++++++++++++ > 7 files changed, 109 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42..9226d24 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, > } > > /** > + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane > + * @state: plane state > + * @blob: damage clips in framebuffer coordinates > + * > + * Returns: > + * > + * Zero on success, error code on failure. > + */ > +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, > + struct drm_property_blob *blob) > +{ > + if (blob == state->damage_clips) > + return 0; > + > + drm_property_blob_put(state->damage_clips); > + state->damage_clips = NULL; > + > + if (blob) { > + uint32_t count = blob->length/sizeof(struct drm_rect); > + > + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) > + return -EINVAL; > + > + state->damage_clips = drm_property_blob_get(blob); > + state->num_clips = count; > + } else { > + state->damage_clips = NULL; > + state->num_clips = 0; > + } > + > + return 0; > +} > + > +/** > * drm_atomic_get_plane_state - get plane state > * @state: global atomic state object > * @plane: plane to get state object for > @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > state->color_encoding = val; > } else if (property == plane->color_range_property) { > state->color_range = val; > + } else if (property == config->prop_damage_clips) { > + struct drm_property_blob *blob = > + drm_property_lookup_blob(dev, val); > + int ret = drm_atomic_set_damage_for_plane(state, blob); > + drm_property_blob_put(blob); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, > *val = state->color_encoding; > } else if (property == plane->color_range_property) { > *val = state->color_range; > + } else if (property == config->prop_damage_clips) { > + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index c356545..55b44e3 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > state->fence = NULL; > state->commit = NULL; > + state->damage_clips = NULL; > + state->num_clips = 0; > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > if (state->commit) > drm_crtc_commit_put(state->commit); > + > + drm_property_blob_put(state->damage_clips); > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index e5c6533..e93b127 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > > return ret; > } > + > +/** > + * drm_plane_enable_damage_clips - enable damage clips property > + * @plane: plane on which this property to enable. > + */ > +void drm_plane_enable_damage_clips(struct drm_plane *plane) > +{ > + struct drm_device *dev = plane->dev; > + struct drm_mode_config *config = &dev->mode_config; > + > + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); > +} > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 7569f22..d8767da 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -628,6 +628,21 @@ struct drm_mode_config { > */ > struct drm_property *prop_crtc_id; > /** > + * @prop_damage_clips: Optional plane property to mark damaged regions > + * on the plane in framebuffer coordinates of the framebuffer attached > + * to the plane. > + * > + * The layout of blob data is simply an array of drm_mode_rect with > + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike > + * plane src coordinates, damage clips are not in 16.16 fixed point. > + * > + * Damage clips are a hint to kernel as which area of framebuffer has > + * changed since last page-flip. This should be helpful > + * for some drivers especially for virtual devices where each > + * framebuffer change needs to be transmitted over network, usb, etc. > + */ > + struct drm_property *prop_damage_clips; > + /** > * @prop_active: Default atomic CRTC property to control the active > * state, which is the simplified implementation for DPMS in atomic > * drivers. > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index f7bf4a4..9f24548 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -146,6 +146,21 @@ struct drm_plane_state { > */ > struct drm_crtc_commit *commit; > > + /* > + * @damage_clips > + * > + * blob property with damage as array of drm_rect in framebuffer > + * coodinates. > + */ > + struct drm_property_blob *damage_clips; > + > + /* > + * @num_clips > + * > + * Number of drm_rect in @damage_clips. > + */ > + uint32_t num_clips; > + > struct drm_atomic_state *state; > }; > > @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, > const uint32_t *formats, unsigned int format_count, > bool is_primary); > void drm_plane_cleanup(struct drm_plane *plane); > +void drm_plane_enable_damage_clips(struct drm_plane *plane); > > /** > * drm_plane_index - find the index of a registered plane > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 50bcf42..0ad0d5b 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { > __u32 lessee_id; > }; > > +/** > + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to > + * user-space. > + * @x1: horizontal starting coordinate (inclusive) > + * @y1: vertical starting coordinate (inclusive) > + * @x2: horizontal ending coordinate (exclusive) > + * @y2: vertical ending coordinate (exclusive) > + */ > +struct drm_mode_rect { > + __s32 x1; > + __s32 y1; > + __s32 x2; > + __s32 y2; > +}; I wonder why we can't use move 'struct drm_rect' definition from 'include/drm/drm_rect.h' and include 'uapi/drm/drm_mode.h' in private header 'include/drm/drm_rect.h'. Is there any general rule that disallows it? > + > #if defined(__cplusplus) > } > #endif
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d25c42..9226d24 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p, } /** + * drm_atomic_set_damage_for_plane - sets the damage clips property to plane + * @state: plane state + * @blob: damage clips in framebuffer coordinates + * + * Returns: + * + * Zero on success, error code on failure. + */ +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state, + struct drm_property_blob *blob) +{ + if (blob == state->damage_clips) + return 0; + + drm_property_blob_put(state->damage_clips); + state->damage_clips = NULL; + + if (blob) { + uint32_t count = blob->length/sizeof(struct drm_rect); + + if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS) + return -EINVAL; + + state->damage_clips = drm_property_blob_get(blob); + state->num_clips = count; + } else { + state->damage_clips = NULL; + state->num_clips = 0; + } + + return 0; +} + +/** * drm_atomic_get_plane_state - get plane state * @state: global atomic state object * @plane: plane to get state object for @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->color_encoding = val; } else if (property == plane->color_range_property) { state->color_range = val; + } else if (property == config->prop_damage_clips) { + struct drm_property_blob *blob = + drm_property_lookup_blob(dev, val); + int ret = drm_atomic_set_damage_for_plane(state, blob); + drm_property_blob_put(blob); + return ret; } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->color_encoding; } else if (property == plane->color_range_property) { *val = state->color_range; + } else if (property == config->prop_damage_clips) { + *val = (state->damage_clips) ? state->damage_clips->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_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c356545..55b44e3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, state->fence = NULL; state->commit = NULL; + state->damage_clips = NULL; + state->num_clips = 0; } EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) if (state->commit) drm_crtc_commit_put(state->commit); + + drm_property_blob_put(state->damage_clips); } EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index e5c6533..e93b127 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -293,6 +293,11 @@ 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, "DAMAGE_CLIPS", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_damage_clips = 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 6d2a6e4..071221b 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, return ret; } + +/** + * drm_plane_enable_damage_clips - enable damage clips property + * @plane: plane on which this property to enable. + */ +void drm_plane_enable_damage_clips(struct drm_plane *plane) +{ + struct drm_device *dev = plane->dev; + struct drm_mode_config *config = &dev->mode_config; + + drm_object_attach_property(&plane->base, config->prop_damage_clips, 0); +} diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 7569f22..d8767da 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -628,6 +628,21 @@ struct drm_mode_config { */ struct drm_property *prop_crtc_id; /** + * @prop_damage_clips: Optional plane property to mark damaged regions + * on the plane in framebuffer coordinates of the framebuffer attached + * to the plane. + * + * The layout of blob data is simply an array of drm_mode_rect with + * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike + * plane src coordinates, damage clips are not in 16.16 fixed point. + * + * Damage clips are a hint to kernel as which area of framebuffer has + * changed since last page-flip. This should be helpful + * for some drivers especially for virtual devices where each + * framebuffer change needs to be transmitted over network, usb, etc. + */ + struct drm_property *prop_damage_clips; + /** * @prop_active: Default atomic CRTC property to control the active * state, which is the simplified implementation for DPMS in atomic * drivers. diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index f7bf4a4..9f24548 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -146,6 +146,21 @@ struct drm_plane_state { */ struct drm_crtc_commit *commit; + /* + * @damage_clips + * + * blob property with damage as array of drm_rect in framebuffer + * coodinates. + */ + struct drm_property_blob *damage_clips; + + /* + * @num_clips + * + * Number of drm_rect in @damage_clips. + */ + uint32_t num_clips; + struct drm_atomic_state *state; }; @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev, const uint32_t *formats, unsigned int format_count, bool is_primary); void drm_plane_cleanup(struct drm_plane *plane); +void drm_plane_enable_damage_clips(struct drm_plane *plane); /** * drm_plane_index - find the index of a registered plane diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 50bcf42..0ad0d5b 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease { __u32 lessee_id; }; +/** + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to + * user-space. + * @x1: horizontal starting coordinate (inclusive) + * @y1: vertical starting coordinate (inclusive) + * @x2: horizontal ending coordinate (exclusive) + * @y2: vertical ending coordinate (exclusive) + */ +struct drm_mode_rect { + __s32 x1; + __s32 y1; + __s32 x2; + __s32 y2; +}; + #if defined(__cplusplus) } #endif