Messages in this thread |  | | Subject | Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane | From | Thomas Hellstrom <> | Date | Thu, 5 Apr 2018 11:00:15 +0200 |
| |
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
|  |