Message-ID: <MWHPR05MB3117649FFA7D68D00E99AA89BABB0@MWHPR05MB3117.namprd05.prod.outlook.com>
Date:   Thu, 5 Apr 2018 23:07:19 +0000
From:   Deepak Singh Rawat <drawat@...are.com>
To:     Daniel Vetter <daniel@...ll.ch>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Sinclair Yeh <syeh@...are.com>,
        linux-graphics-maintainer <linux-graphics-maintainer@...are.com>,
        "ville.syrjala@...ux.intel.com" <ville.syrjala@...ux.intel.com>,
        "lukasz.spintzyk@...playlink.com" <lukasz.spintzyk@...playlink.com>,
        "noralf@...nnes.org" <noralf@...nnes.org>,
        "robdclark@...il.com" <robdclark@...il.com>,
        "gustavo@...ovan.org" <gustavo@...ovan.org>,
        "maarten.lankhorst@...ux.intel.com" 
        <maarten.lankhorst@...ux.intel.com>,
        "seanpaul@...omium.org" <seanpaul@...omium.org>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane

> 
> On Wed, Apr 04, 2018 at 04:49:06PM -0700, Deepak Rawat wrote:
> > From: Lukasz Spintzyk <lukasz.spintzyk@...playlink.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@...playlink.com>
> > Signed-off-by: Deepak Rawat <drawat@...are.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=

Powered by blists - more mailing lists