Re: [Intel-gfx] [PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Move checks inside intel_crtc_cursor_set_obj() to
> intel_check_cursor_plane(), we only use they there so move them out to
> make the merge of intel_crtc_cursor_set_obj() into
> intel_check_cursor_plane() easier.
> 
> This is another step toward the atomic modesetting support and unification
> of plane operations such pin/unpin of fb objects on i915.
> 
> v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the
> code
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++------------
>  1 file changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ef1836..3f37e93 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> -	unsigned old_width, stride;
> +	unsigned old_width;
>  	uint32_t addr;
>  	int ret;
>  
> @@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  		goto finish;
>  	}
>  
> -	/* Check for which cursor types we support */
> -	if (!cursor_size_ok(dev, width, height)) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> -		return -EINVAL;
> -	}
> -
> -	stride = roundup_pow_of_two(width) * 4;
> -	if (obj->base.size < stride * height) {
> -		DRM_DEBUG_KMS("buffer is too small\n");
> -		return -ENOMEM;
> -	}
> -
>  	/* we only need to pin inside GTT if cursor is non-phy */
>  	mutex_lock(&dev->struct_mutex);
>  	if (!INTEL_INFO(dev)->cursor_needs_physical) {
>  		unsigned alignment;
>  
> -		if (obj->tiling_mode) {
> -			DRM_DEBUG_KMS("cursor cannot be tiled\n");
> -			ret = -EINVAL;
> -			goto fail_locked;
> -		}
> -
>  		/*
>  		 * Global gtt pte registers are special registers which actually
>  		 * forward writes to a chunk of system memory. Which means that
> @@ -11826,16 +11808,55 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	int crtc_w, crtc_h;
> +	unsigned stride;
> +	int ret;
>  
> -	return drm_plane_helper_check_update(plane, crtc, fb,
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    true, true, &state->visible);
> +	if (ret)
> +		return ret;
> +
> +
> +	/* if we want to turn off the cursor ignore width and height */
> +	if (!obj)
> +		return 0;
> +
> +	if (fb == crtc->cursor->fb)
> +		return 0;

Hmm. This check needs to be after the cursor/obj size checks. Otherwise
we wouldn't reject invalid sized cursors when the fb didn't change. I
suppose we could also just drop this check, but it can still save us from
doing the tiling check since that can't have changed if the fb hasn't
changed so maybe it's worth keeping. But either solution is fine by me.

> +
> +	/* Check for which cursor types we support */
> +	crtc_w = drm_rect_width(&state->orig_dst);
> +	crtc_h = drm_rect_height(&state->orig_dst);
> +	if (!cursor_size_ok(dev, crtc_w, crtc_h)) {
> +		DRM_DEBUG("Cursor dimension not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	stride = roundup_pow_of_two(crtc_w) * 4;
> +	if (obj->base.size < stride * crtc_h) {
> +		DRM_DEBUG_KMS("buffer is too small\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* we only need to pin inside GTT if cursor is non-phy */
> +	mutex_lock(&dev->struct_mutex);
> +	if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
> +		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;

'return 0' seems better here.

With these issues fixed:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static int
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC