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

["Agrawal, Akshu" <akshu.agrawal@intel.com>]

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.

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