| Message ID | 1411424597-31662-3-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
On Tue, Sep 23, 2014 at 12:41:56PM -0300, Gustavo Padovan wrote: > 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. I guess we already had the bug then. Not entirely unsurprising given the recent history of this code. Probably better to have the fix as a separate patch then to avoid mixing restructuring and functional changes in the same patch. > > > > + > > > + /* 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. Oh, somehow I missed that one. You can leave the patch as is then. > > Gustavo
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; + + /* 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; } static int