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

From: Deepak Singh Rawat <drawat@vmware.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	Sinclair Yeh <syeh@vmware.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	linux-graphics-maintainer <linux-graphics-maintainer@vmware.com>
Subject: RE: [PATCH 05/18] drm: Add helper to implement legacy dirtyfb
Date: Tue, 2 Oct 2018 16:30:30 +0000	[thread overview]
Message-ID: <BYAPR05MB418186CCF211FB94A48EE853BAE80@BYAPR05MB4181.namprd05.prod.outlook.com> (raw)
In-Reply-To: <20181001080826.GT11082@phenom.ffwll.local>

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel