diff mbox series

[05/18] drm: Add helper to implement legacy dirtyfb

Message ID 20180928003020.1737-5-drawat@vmware.com (mailing list archive)
State New, archived
Headers show
Series [01/18] drm: Add a new plane property to send damage during plane update | expand

Commit Message

Deepak Singh Rawat Sept. 28, 2018, 12:30 a.m. UTC
From: Rob Clark <robdclark@gmail.com>

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

v2: Modified the helper to use plane fb_damage_clips property and
removed plane_state::dirty flag.

v3:
- Use uapi drm_mode_rect.
- Support annotate flags.

Cc: ville.syrjala@linux.intel.com
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Signed-off-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/drm_damage_helper.c | 121 ++++++++++++++++++++++++++++
 include/drm/drm_damage_helper.h     |   4 +
 2 files changed, 125 insertions(+)

Comments

Daniel Vetter Oct. 1, 2018, 8:08 a.m. UTC | #1
On Thu, Sep 27, 2018 at 05:30:07PM -0700, Deepak Rawat wrote:
> From: Rob Clark <robdclark@gmail.com>
> 
> Add an atomic helper to implement dirtyfb support.  This is needed to
> support DSI command-mode panels with x11 userspace (ie. when we can't
> rely on pageflips to trigger a flush to the panel).
> 
> v2: Modified the helper to use plane fb_damage_clips property and
> removed plane_state::dirty flag.
> 
> v3:
> - Use uapi drm_mode_rect.
> - Support annotate flags.
> 
> Cc: ville.syrjala@linux.intel.com
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>  drivers/gpu/drm/drm_damage_helper.c | 121 ++++++++++++++++++++++++++++
>  include/drm/drm_damage_helper.h     |   4 +
>  2 files changed, 125 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index bc734ddaf180..ecf26275543f 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -26,6 +26,7 @@
>   *
>   * Authors:
>   * Deepak Rawat <drawat@vmware.com>
> + * Rob Clark <robdclark@gmail.com>
>   *
>   **************************************************************************/
>  
> @@ -70,6 +71,21 @@
>   * rectangles clipped to &drm_plane_state.src.
>   */
>  
> +static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
> +				      struct drm_mode_rect *dest,
> +				      uint32_t num_clips, uint32_t src_inc)
> +{
> +	while (num_clips > 0) {
> +		dest->x1 = src->x1;
> +		dest->y1 = src->y1;
> +		dest->x2 = src->x2;
> +		dest->y2 = src->y2;
> +		src += src_inc;
> +		dest++;
> +		num_clips--;
> +	}
> +}
> +
>  /**
>   * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips property.
>   * @plane: Plane on which to enable damage clips property.
> @@ -123,6 +139,111 @@ void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
>  
> +/**
> + * drm_atomic_helper_dirtyfb - Helper for dirtyfb.
> + * @fb: DRM framebuffer.
> + * @file_priv: Drm file for the ioctl call.
> + * @flags: Dirty fb annotate flags.
> + * @color: Color for annotate fill.
> + * @clips: Dirty region.
> + * @num_clips: Count of clip in clips.
> + *
> + * A helper to implement &drm_framebuffer_funcs::dirty using damage interface
> + * during plane update. Similar to ioctl, this helper do a full plane update
> + * when no clips are provided.

The above kerneldoc doesn't make sense to me ... we do have clips provided
here, it's just a different layout.

> + *
> + * Returns: Zero on success, negative errno on failure.
> + */
> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> +			      struct drm_file *file_priv, unsigned flags,
> +			      unsigned color, struct drm_clip_rect *clips,
> +			      unsigned num_clips)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_property_blob *damage = NULL;
> +	struct drm_mode_rect *rects = NULL;
> +	struct drm_atomic_state *state;
> +	struct drm_plane *plane;
> +	int ret = 0;
> +
> +	/*
> +	 * When called from ioctl, we are interruptable, but not when called
> +	 * internally (ie. defio worker)
> +	 */
> +	drm_modeset_acquire_init(&ctx,
> +		file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> +
> +	state = drm_atomic_state_alloc(fb->dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	state->acquire_ctx = &ctx;
> +
> +	if (clips) {
> +		uint32_t inc = 1;
> +
> +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
> +			inc = 2;
> +			num_clips /= 2;
> +		}
> +
> +		rects = kcalloc(num_clips, sizeof(*rects), GFP_KERNEL);
> +		if (!rects) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		convert_clip_rect_to_rect(clips, rects, num_clips, inc);
> +		damage = drm_property_create_blob(fb->dev,
> +						  num_clips * sizeof(*rects),
> +						  rects);
> +		if (IS_ERR(damage)) {
> +			ret = PTR_ERR(damage);
> +			damage = NULL;
> +			goto out;
> +		}
> +	}
> +
> +retry:
> +	drm_for_each_plane(plane, fb->dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		if (plane->state->fb != fb)
> +			continue;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto out;
> +		}
> +
> +		drm_property_replace_blob(&plane_state->fb_damage_clips,
> +					  damage);
> +	}
> +
> +	ret = drm_atomic_commit(state);

This blocks, which matches what udl does (one of the very first
implementations of the fb->dirty callback). I got lost a bit in the vmwgfx
dirty implementation, so no idea whether that one blocked or not.

If the blocking here is a problem, then we need a separate worker
similarly to what the fbdev worker does (since using the atomic
nonblocking stuff has uapi side-effects).

Either way, probably something we need to document in the kernel-doc.
-Daniel

> +
> +out:
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry;
> +	}
> +
> +	drm_property_blob_put(damage);
> +	kfree(rects);
> +	drm_atomic_state_put(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
> +
>  /**
>   * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
>   * @iter: The iterator to initialize.
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> index af188799820f..e4fe7036cab2 100644
> --- a/include/drm/drm_damage_helper.h
> +++ b/include/drm/drm_damage_helper.h
> @@ -67,6 +67,10 @@ struct drm_atomic_helper_damage_iter {
>  void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
>  void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
>  					  struct drm_plane_state *plane_state);
> +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> +			      struct drm_file *file_priv, unsigned flags,
> +			      unsigned color, struct drm_clip_rect *clips,
> +			      unsigned num_clips);
>  void
>  drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>  				   const struct drm_plane_state *old_state,
> -- 
> 2.17.1
>
Deepak Singh Rawat Oct. 2, 2018, 4:30 p.m. UTC | #2
> On Thu, Sep 27, 2018 at 05:30:07PM -0700, Deepak Rawat wrote:
> > From: Rob Clark <robdclark@gmail.com>
> >
> > Add an atomic helper to implement dirtyfb support.  This is needed to
> > support DSI command-mode panels with x11 userspace (ie. when we can't
> > rely on pageflips to trigger a flush to the panel).
> >
> > v2: Modified the helper to use plane fb_damage_clips property and
> > removed plane_state::dirty flag.
> >
> > v3:
> > - Use uapi drm_mode_rect.
> > - Support annotate flags.
> >
> > Cc: ville.syrjala@linux.intel.com
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > Signed-off-by: Deepak Rawat <drawat@vmware.com>
> > ---
> >  drivers/gpu/drm/drm_damage_helper.c | 121
> ++++++++++++++++++++++++++++
> >  include/drm/drm_damage_helper.h     |   4 +
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> b/drivers/gpu/drm/drm_damage_helper.c
> > index bc734ddaf180..ecf26275543f 100644
> > --- a/drivers/gpu/drm/drm_damage_helper.c
> > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > @@ -26,6 +26,7 @@
> >   *
> >   * Authors:
> >   * Deepak Rawat <drawat@vmware.com>
> > + * Rob Clark <robdclark@gmail.com>
> >   *
> >
> **********************************************************
> ****************/
> >
> > @@ -70,6 +71,21 @@
> >   * rectangles clipped to &drm_plane_state.src.
> >   */
> >
> > +static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
> > +				      struct drm_mode_rect *dest,
> > +				      uint32_t num_clips, uint32_t src_inc)
> > +{
> > +	while (num_clips > 0) {
> > +		dest->x1 = src->x1;
> > +		dest->y1 = src->y1;
> > +		dest->x2 = src->x2;
> > +		dest->y2 = src->y2;
> > +		src += src_inc;
> > +		dest++;
> > +		num_clips--;
> > +	}
> > +}
> > +
> >  /**
> >   * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips
> property.
> >   * @plane: Plane on which to enable damage clips property.
> > @@ -123,6 +139,111 @@ void
> drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
> >
> > +/**
> > + * drm_atomic_helper_dirtyfb - Helper for dirtyfb.
> > + * @fb: DRM framebuffer.
> > + * @file_priv: Drm file for the ioctl call.
> > + * @flags: Dirty fb annotate flags.
> > + * @color: Color for annotate fill.
> > + * @clips: Dirty region.
> > + * @num_clips: Count of clip in clips.
> > + *
> > + * A helper to implement &drm_framebuffer_funcs::dirty using damage
> interface
> > + * during plane update. Similar to ioctl, this helper do a full plane update
> > + * when no clips are provided.
> 
> The above kerneldoc doesn't make sense to me ... we do have clips provided
> here, it's just a different layout.

Thanks Daniel for the review. I meant if clips are NULL, which is possible.

> 
> > + *
> > + * Returns: Zero on success, negative errno on failure.
> > + */
> > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> > +			      struct drm_file *file_priv, unsigned flags,
> > +			      unsigned color, struct drm_clip_rect *clips,
> > +			      unsigned num_clips)
> > +{
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_property_blob *damage = NULL;
> > +	struct drm_mode_rect *rects = NULL;
> > +	struct drm_atomic_state *state;
> > +	struct drm_plane *plane;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * When called from ioctl, we are interruptable, but not when called
> > +	 * internally (ie. defio worker)
> > +	 */
> > +	drm_modeset_acquire_init(&ctx,
> > +		file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> > +
> > +	state = drm_atomic_state_alloc(fb->dev);
> > +	if (!state) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +	state->acquire_ctx = &ctx;
> > +
> > +	if (clips) {
> > +		uint32_t inc = 1;
> > +
> > +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
> > +			inc = 2;
> > +			num_clips /= 2;
> > +		}
> > +
> > +		rects = kcalloc(num_clips, sizeof(*rects), GFP_KERNEL);
> > +		if (!rects) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +
> > +		convert_clip_rect_to_rect(clips, rects, num_clips, inc);
> > +		damage = drm_property_create_blob(fb->dev,
> > +						  num_clips * sizeof(*rects),
> > +						  rects);
> > +		if (IS_ERR(damage)) {
> > +			ret = PTR_ERR(damage);
> > +			damage = NULL;
> > +			goto out;
> > +		}
> > +	}
> > +
> > +retry:
> > +	drm_for_each_plane(plane, fb->dev) {
> > +		struct drm_plane_state *plane_state;
> > +
> > +		if (plane->state->fb != fb)
> > +			continue;
> > +
> > +		plane_state = drm_atomic_get_plane_state(state, plane);
> > +		if (IS_ERR(plane_state)) {
> > +			ret = PTR_ERR(plane_state);
> > +			goto out;
> > +		}
> > +
> > +		drm_property_replace_blob(&plane_state-
> >fb_damage_clips,
> > +					  damage);
> > +	}
> > +
> > +	ret = drm_atomic_commit(state);
> 
> This blocks, which matches what udl does (one of the very first
> implementations of the fb->dirty callback). I got lost a bit in the vmwgfx
> dirty implementation, so no idea whether that one blocked or not.
> 
> If the blocking here is a problem, then we need a separate worker
> similarly to what the fbdev worker does (since using the atomic
> nonblocking stuff has uapi side-effects).
> 
> Either way, probably something we need to document in the kernel-doc.
> -Daniel

IIUC vmwgfx dirty implementation also block. vmwgfx dirty implementation
will make sure that commands are flushed to virtual device and all the mutex
lock are held until then.

With my testing on vmwgfx software only VM I see that non-blocking will cause
rendering issues with so many dirty_fb ioctl()'s and kernel returns EBUSY(?).

Yes I agree I can update the kernel-doc to specify blocking and if someone
need non-blocking version in future that can be easily done.

Thanks,
Deepak
> 
> > +
> > +out:
> > +	if (ret == -EDEADLK) {
> > +		drm_atomic_state_clear(state);
> > +		ret = drm_modeset_backoff(&ctx);
> > +		if (!ret)
> > +			goto retry;
> > +	}
> > +
> > +	drm_property_blob_put(damage);
> > +	kfree(rects);
> > +	drm_atomic_state_put(state);
> > +
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> > +	return ret;
> > +
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
> > +
> >  /**
> >   * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
> >   * @iter: The iterator to initialize.
> > diff --git a/include/drm/drm_damage_helper.h
> b/include/drm/drm_damage_helper.h
> > index af188799820f..e4fe7036cab2 100644
> > --- a/include/drm/drm_damage_helper.h
> > +++ b/include/drm/drm_damage_helper.h
> > @@ -67,6 +67,10 @@ struct drm_atomic_helper_damage_iter {
> >  void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
> >  void drm_atomic_helper_check_plane_damage(struct drm_atomic_state
> *state,
> >  					  struct drm_plane_state
> *plane_state);
> > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> > +			      struct drm_file *file_priv, unsigned flags,
> > +			      unsigned color, struct drm_clip_rect *clips,
> > +			      unsigned num_clips);
> >  void
> >  drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> >  				   const struct drm_plane_state *old_state,
> > --
> > 2.17.1
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff
> wll.ch&amp;data=02%7C01%7Cdrawat%40vmware.com%7C0835e0509d8849
> 89c6c208d6277513ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7
> C636739781135270102&amp;sdata=6%2Fr%2FS%2B3k7nJDZKNd9tRnmofCGf
> CG2TKqDy4tG6lojsA%3D&amp;reserved=0
Daniel Vetter Oct. 3, 2018, 8:45 a.m. UTC | #3
On Tue, Oct 02, 2018 at 04:30:30PM +0000, Deepak Singh Rawat wrote:
> > On Thu, Sep 27, 2018 at 05:30:07PM -0700, Deepak Rawat wrote:
> > > From: Rob Clark <robdclark@gmail.com>
> > >
> > > Add an atomic helper to implement dirtyfb support.  This is needed to
> > > support DSI command-mode panels with x11 userspace (ie. when we can't
> > > rely on pageflips to trigger a flush to the panel).
> > >
> > > v2: Modified the helper to use plane fb_damage_clips property and
> > > removed plane_state::dirty flag.
> > >
> > > v3:
> > > - Use uapi drm_mode_rect.
> > > - Support annotate flags.
> > >
> > > Cc: ville.syrjala@linux.intel.com
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > Signed-off-by: Deepak Rawat <drawat@vmware.com>
> > > ---
> > >  drivers/gpu/drm/drm_damage_helper.c | 121
> > ++++++++++++++++++++++++++++
> > >  include/drm/drm_damage_helper.h     |   4 +
> > >  2 files changed, 125 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_damage_helper.c
> > b/drivers/gpu/drm/drm_damage_helper.c
> > > index bc734ddaf180..ecf26275543f 100644
> > > --- a/drivers/gpu/drm/drm_damage_helper.c
> > > +++ b/drivers/gpu/drm/drm_damage_helper.c
> > > @@ -26,6 +26,7 @@
> > >   *
> > >   * Authors:
> > >   * Deepak Rawat <drawat@vmware.com>
> > > + * Rob Clark <robdclark@gmail.com>
> > >   *
> > >
> > **********************************************************
> > ****************/
> > >
> > > @@ -70,6 +71,21 @@
> > >   * rectangles clipped to &drm_plane_state.src.
> > >   */
> > >
> > > +static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
> > > +				      struct drm_mode_rect *dest,
> > > +				      uint32_t num_clips, uint32_t src_inc)
> > > +{
> > > +	while (num_clips > 0) {
> > > +		dest->x1 = src->x1;
> > > +		dest->y1 = src->y1;
> > > +		dest->x2 = src->x2;
> > > +		dest->y2 = src->y2;
> > > +		src += src_inc;
> > > +		dest++;
> > > +		num_clips--;
> > > +	}
> > > +}
> > > +
> > >  /**
> > >   * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips
> > property.
> > >   * @plane: Plane on which to enable damage clips property.
> > > @@ -123,6 +139,111 @@ void
> > drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
> > >  }
> > >  EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
> > >
> > > +/**
> > > + * drm_atomic_helper_dirtyfb - Helper for dirtyfb.
> > > + * @fb: DRM framebuffer.
> > > + * @file_priv: Drm file for the ioctl call.
> > > + * @flags: Dirty fb annotate flags.
> > > + * @color: Color for annotate fill.
> > > + * @clips: Dirty region.
> > > + * @num_clips: Count of clip in clips.
> > > + *
> > > + * A helper to implement &drm_framebuffer_funcs::dirty using damage
> > interface
> > > + * during plane update. Similar to ioctl, this helper do a full plane update
> > > + * when no clips are provided.
> > 
> > The above kerneldoc doesn't make sense to me ... we do have clips provided
> > here, it's just a different layout.
> 
> Thanks Daniel for the review. I meant if clips are NULL, which is possible.

Ah right, I've forgotten that the dirty ioctl allows num_clips == 0 iff
clips == NULL. Might be good to document that, for people like me :-)

> > > + *
> > > + * Returns: Zero on success, negative errno on failure.
> > > + */
> > > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> > > +			      struct drm_file *file_priv, unsigned flags,
> > > +			      unsigned color, struct drm_clip_rect *clips,
> > > +			      unsigned num_clips)
> > > +{
> > > +	struct drm_modeset_acquire_ctx ctx;
> > > +	struct drm_property_blob *damage = NULL;
> > > +	struct drm_mode_rect *rects = NULL;
> > > +	struct drm_atomic_state *state;
> > > +	struct drm_plane *plane;
> > > +	int ret = 0;
> > > +
> > > +	/*
> > > +	 * When called from ioctl, we are interruptable, but not when called
> > > +	 * internally (ie. defio worker)
> > > +	 */
> > > +	drm_modeset_acquire_init(&ctx,
> > > +		file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
> > > +
> > > +	state = drm_atomic_state_alloc(fb->dev);
> > > +	if (!state) {
> > > +		ret = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +	state->acquire_ctx = &ctx;
> > > +
> > > +	if (clips) {
> > > +		uint32_t inc = 1;
> > > +
> > > +		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
> > > +			inc = 2;
> > > +			num_clips /= 2;
> > > +		}
> > > +
> > > +		rects = kcalloc(num_clips, sizeof(*rects), GFP_KERNEL);
> > > +		if (!rects) {
> > > +			ret = -ENOMEM;
> > > +			goto out;
> > > +		}
> > > +
> > > +		convert_clip_rect_to_rect(clips, rects, num_clips, inc);
> > > +		damage = drm_property_create_blob(fb->dev,
> > > +						  num_clips * sizeof(*rects),
> > > +						  rects);
> > > +		if (IS_ERR(damage)) {
> > > +			ret = PTR_ERR(damage);
> > > +			damage = NULL;
> > > +			goto out;
> > > +		}
> > > +	}
> > > +
> > > +retry:
> > > +	drm_for_each_plane(plane, fb->dev) {
> > > +		struct drm_plane_state *plane_state;
> > > +
> > > +		if (plane->state->fb != fb)
> > > +			continue;
> > > +
> > > +		plane_state = drm_atomic_get_plane_state(state, plane);
> > > +		if (IS_ERR(plane_state)) {
> > > +			ret = PTR_ERR(plane_state);
> > > +			goto out;
> > > +		}
> > > +
> > > +		drm_property_replace_blob(&plane_state-
> > >fb_damage_clips,
> > > +					  damage);
> > > +	}
> > > +
> > > +	ret = drm_atomic_commit(state);
> > 
> > This blocks, which matches what udl does (one of the very first
> > implementations of the fb->dirty callback). I got lost a bit in the vmwgfx
> > dirty implementation, so no idea whether that one blocked or not.
> > 
> > If the blocking here is a problem, then we need a separate worker
> > similarly to what the fbdev worker does (since using the atomic
> > nonblocking stuff has uapi side-effects).
> > 
> > Either way, probably something we need to document in the kernel-doc.
> > -Daniel
> 
> IIUC vmwgfx dirty implementation also block. vmwgfx dirty implementation
> will make sure that commands are flushed to virtual device and all the mutex
> lock are held until then.
> 
> With my testing on vmwgfx software only VM I see that non-blocking will cause
> rendering issues with so many dirty_fb ioctl()'s and kernel returns EBUSY(?).

Yup, drm_atomic_commit_nonblocking will not work here, wrong semantics. As
I mentioned, we'd need a separate struct_work and similar logic like in
the fbdev defio support in drm_fb_helper.c.

> Yes I agree I can update the kernel-doc to specify blocking and if someone
> need non-blocking version in future that can be easily done.

Sounds good, and much less work :-)
-Daniel

> 
> Thanks,
> Deepak
> > 
> > > +
> > > +out:
> > > +	if (ret == -EDEADLK) {
> > > +		drm_atomic_state_clear(state);
> > > +		ret = drm_modeset_backoff(&ctx);
> > > +		if (!ret)
> > > +			goto retry;
> > > +	}
> > > +
> > > +	drm_property_blob_put(damage);
> > > +	kfree(rects);
> > > +	drm_atomic_state_put(state);
> > > +
> > > +	drm_modeset_drop_locks(&ctx);
> > > +	drm_modeset_acquire_fini(&ctx);
> > > +
> > > +	return ret;
> > > +
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
> > > +
> > >  /**
> > >   * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
> > >   * @iter: The iterator to initialize.
> > > diff --git a/include/drm/drm_damage_helper.h
> > b/include/drm/drm_damage_helper.h
> > > index af188799820f..e4fe7036cab2 100644
> > > --- a/include/drm/drm_damage_helper.h
> > > +++ b/include/drm/drm_damage_helper.h
> > > @@ -67,6 +67,10 @@ struct drm_atomic_helper_damage_iter {
> > >  void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
> > >  void drm_atomic_helper_check_plane_damage(struct drm_atomic_state
> > *state,
> > >  					  struct drm_plane_state
> > *plane_state);
> > > +int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
> > > +			      struct drm_file *file_priv, unsigned flags,
> > > +			      unsigned color, struct drm_clip_rect *clips,
> > > +			      unsigned num_clips);
> > >  void
> > >  drm_atomic_helper_damage_iter_init(struct
> > drm_atomic_helper_damage_iter *iter,
> > >  				   const struct drm_plane_state *old_state,
> > > --
> > > 2.17.1
> > >
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff
> > wll.ch&amp;data=02%7C01%7Cdrawat%40vmware.com%7C0835e0509d8849
> > 89c6c208d6277513ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7
> > C636739781135270102&amp;sdata=6%2Fr%2FS%2B3k7nJDZKNd9tRnmofCGf
> > CG2TKqDy4tG6lojsA%3D&amp;reserved=0
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index bc734ddaf180..ecf26275543f 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -26,6 +26,7 @@ 
  *
  * Authors:
  * Deepak Rawat <drawat@vmware.com>
+ * Rob Clark <robdclark@gmail.com>
  *
  **************************************************************************/
 
@@ -70,6 +71,21 @@ 
  * rectangles clipped to &drm_plane_state.src.
  */
 
+static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
+				      struct drm_mode_rect *dest,
+				      uint32_t num_clips, uint32_t src_inc)
+{
+	while (num_clips > 0) {
+		dest->x1 = src->x1;
+		dest->y1 = src->y1;
+		dest->x2 = src->x2;
+		dest->y2 = src->y2;
+		src += src_inc;
+		dest++;
+		num_clips--;
+	}
+}
+
 /**
  * drm_plane_enable_fb_damage_clips - Enables plane fb damage clips property.
  * @plane: Plane on which to enable damage clips property.
@@ -123,6 +139,111 @@  void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
 
+/**
+ * drm_atomic_helper_dirtyfb - Helper for dirtyfb.
+ * @fb: DRM framebuffer.
+ * @file_priv: Drm file for the ioctl call.
+ * @flags: Dirty fb annotate flags.
+ * @color: Color for annotate fill.
+ * @clips: Dirty region.
+ * @num_clips: Count of clip in clips.
+ *
+ * A helper to implement &drm_framebuffer_funcs::dirty using damage interface
+ * during plane update. Similar to ioctl, this helper do a full plane update
+ * when no clips are provided.
+ *
+ * Returns: Zero on success, negative errno on failure.
+ */
+int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
+			      struct drm_file *file_priv, unsigned flags,
+			      unsigned color, struct drm_clip_rect *clips,
+			      unsigned num_clips)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_property_blob *damage = NULL;
+	struct drm_mode_rect *rects = NULL;
+	struct drm_atomic_state *state;
+	struct drm_plane *plane;
+	int ret = 0;
+
+	/*
+	 * When called from ioctl, we are interruptable, but not when called
+	 * internally (ie. defio worker)
+	 */
+	drm_modeset_acquire_init(&ctx,
+		file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0);
+
+	state = drm_atomic_state_alloc(fb->dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	state->acquire_ctx = &ctx;
+
+	if (clips) {
+		uint32_t inc = 1;
+
+		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
+			inc = 2;
+			num_clips /= 2;
+		}
+
+		rects = kcalloc(num_clips, sizeof(*rects), GFP_KERNEL);
+		if (!rects) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		convert_clip_rect_to_rect(clips, rects, num_clips, inc);
+		damage = drm_property_create_blob(fb->dev,
+						  num_clips * sizeof(*rects),
+						  rects);
+		if (IS_ERR(damage)) {
+			ret = PTR_ERR(damage);
+			damage = NULL;
+			goto out;
+		}
+	}
+
+retry:
+	drm_for_each_plane(plane, fb->dev) {
+		struct drm_plane_state *plane_state;
+
+		if (plane->state->fb != fb)
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto out;
+		}
+
+		drm_property_replace_blob(&plane_state->fb_damage_clips,
+					  damage);
+	}
+
+	ret = drm_atomic_commit(state);
+
+out:
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry;
+	}
+
+	drm_property_blob_put(damage);
+	kfree(rects);
+	drm_atomic_state_put(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+
+}
+EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
+
 /**
  * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
  * @iter: The iterator to initialize.
diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
index af188799820f..e4fe7036cab2 100644
--- a/include/drm/drm_damage_helper.h
+++ b/include/drm/drm_damage_helper.h
@@ -67,6 +67,10 @@  struct drm_atomic_helper_damage_iter {
 void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
 void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 					  struct drm_plane_state *plane_state);
+int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
+			      struct drm_file *file_priv, unsigned flags,
+			      unsigned color, struct drm_clip_rect *clips,
+			      unsigned num_clips);
 void
 drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 				   const struct drm_plane_state *old_state,