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

2014-09-23 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.

Only if this was a bug before this patch, if you look at the original intel_commit_cursor_plane() we do two things when the fbs are the same:

intel_crtc_update_cursor(); intel_frontbuffer_flip();

No checks are performed, that is why I put the fbs check before any other check to make the code as similar as possible to what it was before.

...

• /* 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.

ret can be -EINVAL, the code inside the mutex 3 lines above can set ret.

Gustavo