Re: [PATCH 2/2] drm/i915: Crop cursor image for CHV pipe C cursor issue

From: Daniel Vetter <daniel@ffwll.ch>
To: "Agrawal, Akshu" <akshu.agrawal@intel.com>
Cc: jani.nikula@intel.com, daniel.vetter@intel.com,
	intel-gfx@lists.freedesktop.org, shobhit.kumar@intel.com
Subject: Re: [PATCH 2/2] drm/i915: Crop cursor image for CHV pipe C cursor issue
Date: Mon, 13 Jun 2016 16:22:27 +0200	[thread overview]
Message-ID: <20160613142227.GB1338@phenom.ffwll.local> (raw)
In-Reply-To: <c541eae8-e6dd-9b7f-b347-a73014dd089f@intel.com>

On Fri, Jun 10, 2016 at 03:14:36PM +0530, Agrawal, Akshu wrote:
> On 6/8/2016 2:10 PM, Daniel Vetter wrote:
> > On Wed, Jun 08, 2016 at 01:57:44PM +0530, Akshu Agrawal wrote:
> > > CHV pipe C hits underrun when we get -ve X values of cursor. To avoid
> > > this we crop the cursor image for by -ve X value and thus use '0' as
> > > least X value.
> > 
> > You're talking about "-ve" here and there's absolutely no "-ve" anywhere
> > in your patch. That makes your commit message non-understandable.
> 
> Will change -ve to "negative" for better readability.
> 
> > 
> > But I think I get what you're doing here, and nope, you can't override the
> > cursor image like that. If we go with this w/a then you need to allocate a
> > new cursor gem bo instead. This is userspace-visible.
> 
> Can't we use the same gem bo? As we are calling
> "i915_gem_object_set_to_gtt_domain" to get write access for CPU after
> unbinding from GTT.

Userspace can write to the underlying bo without telling us. That'll cause
tearing. And it could also read from it, which would result in even more
serious bugs.

We can't just change userspace-visible state for a w/a. Same reasoning
applies to the crtc_state stuff. Again we need copies, to avoid
accidentally confusion userspace.

With all those copies it's probably best to write a special
atomic_update_plane function for cursor pipe C, to avoid spreading all of
them all over the driver.
-Daniel

> 
> Regards,
> Akshu
> > 
> > For similar reasons you're also not allowed to change crtc->state.
> > -Daniel
> > 
> > > Signed-off-by: Akshu Agrawal <akshu.agrawal@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 113 +++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +
> > >  2 files changed, 116 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index bca9245..e6e6568 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14279,6 +14279,81 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *
> > >  				plane->base.state->rotation);
> > >  }
> > > 
> > > +static void vlv_unpin_buffer_obj(struct drm_i915_gem_object *obj,
> > > +			  char __iomem *buffer_start)
> > > +{
> > > +	iounmap(buffer_start);
> > > +	i915_gem_object_ggtt_unpin(obj);
> > > +}
> > > +
> > > +static char __iomem *vlv_pin_and_map_buffer_obj
> > > +				(struct drm_i915_gem_object *obj)
> > > +{
> > > +	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > > +	char __iomem *buffer_start;
> > > +	int ret;
> > > +
> > > +	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
> > > +	if (ret)
> > > +		return NULL;
> > > +
> > > +	ret = i915_gem_object_set_to_gtt_domain(obj, true);
> > > +	if (ret) {
> > > +		i915_gem_object_ggtt_unpin(obj);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	buffer_start = ioremap_wc(dev_priv->gtt.mappable_base +
> > > +			i915_gem_obj_ggtt_offset(obj), obj->base.size);
> > > +	if (buffer_start == NULL) {
> > > +		i915_gem_object_ggtt_unpin(obj);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	return buffer_start;
> > > +}
> > > +
> > > +static int vlv_cursor_crop(struct intel_plane_state *state,
> > > +				      int prev_x)
> > > +{
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> > > +	struct drm_framebuffer *fb = state->base.fb;
> > > +	char __iomem *buffer = state->vlv_cursor_image;
> > > +	char __iomem *base, *cursor_base;
> > > +	int size = obj->base.size;
> > > +	int i, x = state->base.crtc_x;
> > > +	int bytes_per_pixel = fb->bits_per_pixel / 8;
> > > +
> > > +	base = vlv_pin_and_map_buffer_obj(obj);
> > > +	if (base == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	if (prev_x >= 0) {
> > > +		if (buffer != NULL)
> > > +			kfree(buffer);
> > > +		buffer = kzalloc(size, GFP_KERNEL);
> > > +		state->vlv_cursor_image = buffer;
> > > +		if (buffer == NULL)
> > > +			return -ENOMEM;
> > > +		memcpy(buffer, base, size);
> > > +	}
> > > +	cursor_base = buffer;
> > > +	x = -x;
> > > +	for (i = 0; i < state->base.crtc_h; i++) {
> > > +		cursor_base += x * bytes_per_pixel;
> > > +		memcpy(base, cursor_base,
> > > +			(state->base.crtc_w - x) * bytes_per_pixel);
> > > +		base += (state->base.crtc_w - x) * bytes_per_pixel;
> > > +		memset(base, 0, x * bytes_per_pixel);
> > > +		base += x * bytes_per_pixel;
> > > +		cursor_base += (state->base.crtc_w - x) * bytes_per_pixel;
> > > +	}
> > > +
> > > +	vlv_unpin_buffer_obj(obj, base);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int
> > >  intel_check_cursor_plane(struct drm_plane *plane,
> > >  			 struct intel_crtc_state *crtc_state,
> > > @@ -14287,8 +14362,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >  	struct drm_crtc *crtc = crtc_state->base.crtc;
> > >  	struct drm_framebuffer *fb = state->base.fb;
> > >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +	enum pipe pipe = to_intel_plane(plane)->pipe;
> > >  	unsigned stride;
> > >  	int ret;
> > > +	int crtc_prev_x = state->vlv_cursor_prev_x;
> > > 
> > >  	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src,
> > >  					    &state->dst, &state->clip,
> > > @@ -14309,6 +14386,32 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >  		return -EINVAL;
> > >  	}
> > > 
> > > +	/*
> > > +	 * There is an issue in CHV PIPE C where we hit underrun on
> > > +	 * -ve value of cursor. To avoid this we are cropping the
> > > +	 *  image for all PIPE C -ve values.
> > > +	 */
> > > +	if (IS_CHERRYVIEW(plane->dev)) {
> > > +		if (pipe == PIPE_C && state->visible &&
> > > +		    state->base.crtc_x < 0) {
> > > +			ret = vlv_cursor_crop(state, crtc_prev_x);
> > > +			if (ret)
> > > +				return -ENOMEM;
> > > +		} else if (crtc_prev_x < 0) { /* Restore the image back */
> > > +			char __iomem *base;
> > > +			char __iomem *org_image = state->vlv_cursor_image;
> > > +			int size = obj->base.size;
> > > +
> > > +			if (org_image == NULL)
> > > +				return -ENOMEM;
> > > +			base = vlv_pin_and_map_buffer_obj(obj);
> > > +			if (base == NULL)
> > > +				return -ENOMEM;
> > > +			memcpy(base, org_image, size);
> > > +			vlv_unpin_buffer_obj(obj, base);
> > > +		}
> > > +	}
> > > +
> > >  	stride = roundup_pow_of_two(state->base.crtc_w) * 4;
> > >  	if (obj->base.size < stride * state->base.crtc_h) {
> > >  		DRM_DEBUG_KMS("buffer is too small\n");
> > > @@ -14320,6 +14423,16 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >  		return -EINVAL;
> > >  	}
> > > 
> > > +	if (IS_CHERRYVIEW(plane->dev)) {
> > > +		if (pipe == PIPE_C &&
> > > +		    state->visible && state->base.crtc_x < 0) {
> > > +			state->vlv_cursor_prev_x = state->base.crtc_x;
> > > +			state->base.crtc_x = 0;
> > > +		} else {
> > > +			state->vlv_cursor_prev_x = state->base.crtc_x;
> > > +		}
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index ebe7b34..0c406d1 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -350,6 +350,9 @@ struct intel_plane_state {
> > > 
> > >  	/* async flip related structures */
> > >  	struct drm_i915_gem_request *wait_req;
> > > +
> > > +	char __iomem *vlv_cursor_image;
> > > +	int vlv_cursor_prev_x;
> > >  };
> > > 
> > >  struct intel_initial_plane_config {
> > > --
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx