[Intel-gfx] [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj()
Gustavo Padovan
gustavo.padovan at collabora.co.uk
Fri Oct 24 06:23:35 PDT 2014
2014-10-07 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> >
> > Merge it into the plane update_plane() callback and make other
> > users use the update_plane() functions instead.
> >
> > The fb != crtc->cursor->fb was already inside intel_crtc_cursor_set_obj()
> > so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane()
> > and merge both paths into one.
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 221 +++++++++++++++++------------------
> > 1 file changed, 106 insertions(+), 115 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f91a5b0..a583abd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8457,109 +8457,6 @@ static bool cursor_size_ok(struct drm_device *dev,
> > return true;
> > }
> >
> > -static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> > - struct drm_i915_gem_object *obj,
> > - uint32_t width, uint32_t height)
> > -{
> > - struct drm_device *dev = crtc->dev;
> > - 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;
> > - uint32_t addr;
> > - int ret;
> > -
> > - /* if we want to turn off the cursor ignore width and height */
> > - if (!obj) {
> > - DRM_DEBUG_KMS("cursor off\n");
> > - addr = 0;
> > - mutex_lock(&dev->struct_mutex);
> > - goto finish;
> > - }
> > -
> > - /* 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;
> > -
> > - /*
> > - * Global gtt pte registers are special registers which actually
> > - * forward writes to a chunk of system memory. Which means that
> > - * there is no risk that the register values disappear as soon
> > - * as we call intel_runtime_pm_put(), so it is correct to wrap
> > - * only the pin/unpin/fence and not more.
> > - */
> > - intel_runtime_pm_get(dev_priv);
> > -
> > - /* Note that the w/a also requires 2 PTE of padding following
> > - * the bo. We currently fill all unused PTE with the shadow
> > - * page and so we should always have valid PTE following the
> > - * cursor preventing the VT-d warning.
> > - */
> > - alignment = 0;
> > - if (need_vtd_wa(dev))
> > - alignment = 64*1024;
> > -
> > - ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> > - if (ret) {
> > - DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> > - intel_runtime_pm_put(dev_priv);
> > - goto fail_locked;
> > - }
> > -
> > - ret = i915_gem_object_put_fence(obj);
> > - if (ret) {
> > - DRM_DEBUG_KMS("failed to release fence for cursor");
> > - intel_runtime_pm_put(dev_priv);
> > - goto fail_unpin;
> > - }
> > -
> > - addr = i915_gem_obj_ggtt_offset(obj);
> > -
> > - intel_runtime_pm_put(dev_priv);
> > - } else {
> > - int align = IS_I830(dev) ? 16 * 1024 : 256;
> > - ret = i915_gem_object_attach_phys(obj, align);
> > - if (ret) {
> > - DRM_DEBUG_KMS("failed to attach phys object\n");
> > - goto fail_locked;
> > - }
> > - addr = obj->phys_handle->busaddr;
> > - }
> > -
> > - finish:
> > - if (intel_crtc->cursor_bo) {
> > - if (!INTEL_INFO(dev)->cursor_needs_physical)
> > - i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > - }
> > -
> > - i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > - INTEL_FRONTBUFFER_CURSOR(pipe));
> > - mutex_unlock(&dev->struct_mutex);
> > -
> > - old_width = intel_crtc->cursor_width;
> > -
> > - intel_crtc->cursor_addr = addr;
> > - intel_crtc->cursor_bo = obj;
> > - intel_crtc->cursor_width = width;
> > - intel_crtc->cursor_height = height;
> > -
> > - if (intel_crtc->active) {
> > - if (old_width != width)
> > - intel_update_watermarks(crtc);
> > - intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> > - }
> > -
> > - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
> > -
> > - return 0;
> > -fail_unpin:
> > - i915_gem_object_unpin_from_display_plane(obj);
> > -fail_locked:
> > - mutex_unlock(&dev->struct_mutex);
> > - return ret;
> > -}
> > -
> > static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> > u16 *blue, uint32_t start, uint32_t size)
> > {
> > @@ -11897,7 +11794,8 @@ intel_cursor_plane_disable(struct drm_plane *plane)
> >
> > BUG_ON(!plane->crtc);
> >
> > - return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> > + return plane->funcs->update_plane(plane, plane->crtc, NULL,
> > + 0, 0, 0, 0, 0, 0, 0, 0);
> > }
> >
> > static int
> > @@ -11961,26 +11859,119 @@ intel_commit_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_i915_private *dev_priv = dev->dev_private;
> > struct drm_framebuffer *fb = state->fb;
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > - struct drm_i915_gem_object *obj = intel_fb->obj;
> > - int crtc_w, crtc_h;
> > + struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > + enum pipe pipe = intel_crtc->pipe;
> > + unsigned old_width;
> > + uint32_t addr;
> > + bool on;
> > + int ret;
> >
> > crtc->cursor_x = state->orig_dst.x1;
> > crtc->cursor_y = state->orig_dst.y1;
> > - if (fb != crtc->cursor->fb) {
> > - crtc_w = drm_rect_width(&state->orig_dst);
> > - crtc_h = drm_rect_height(&state->orig_dst);
> > - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> > +
> > + if (intel_crtc->cursor_bo == obj)
> > + goto update;
> > +
> > + /* if we want to turn off the cursor ignore width and height */
> > + if (!obj) {
> > + DRM_DEBUG_KMS("cursor off\n");
> > + addr = 0;
> > + mutex_lock(&dev->struct_mutex);
> > + goto finish;
> > + }
> > +
> > + /* 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;
> > +
> > + /*
> > + * Global gtt pte registers are special registers which actually
> > + * forward writes to a chunk of system memory. Which means that
> > + * there is no risk that the register values disappear as soon
> > + * as we call intel_runtime_pm_put(), so it is correct to wrap
> > + * only the pin/unpin/fence and not more.
> > + */
> > + intel_runtime_pm_get(dev_priv);
> > +
> > + /* Note that the w/a also requires 2 PTE of padding following
> > + * the bo. We currently fill all unused PTE with the shadow
> > + * page and so we should always have valid PTE following the
> > + * cursor preventing the VT-d warning.
> > + */
> > + alignment = 0;
> > + if (need_vtd_wa(dev))
> > + alignment = 64*1024;
> > +
> > + ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> > + if (ret) {
> > + DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
> > + intel_runtime_pm_put(dev_priv);
> > + goto fail_locked;
> > + }
> > +
> > + ret = i915_gem_object_put_fence(obj);
> > + if (ret) {
> > + DRM_DEBUG_KMS("failed to release fence for cursor");
> > + intel_runtime_pm_put(dev_priv);
> > + goto fail_unpin;
> > + }
> > +
> > + addr = i915_gem_obj_ggtt_offset(obj);
> > +
> > + intel_runtime_pm_put(dev_priv);
> > } else {
> > - intel_crtc_update_cursor(crtc, state->visible);
> > + int align = IS_I830(dev) ? 16 * 1024 : 256;
> > + ret = i915_gem_object_attach_phys(obj, align);
> > + if (ret) {
> > + DRM_DEBUG_KMS("failed to attach phys object\n");
> > + goto fail_locked;
> > + }
> > + addr = obj->phys_handle->busaddr;
> > + }
> >
> > - intel_frontbuffer_flip(crtc->dev,
> > - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
> > +finish:
> > + if (intel_crtc->cursor_bo) {
> > + if (!INTEL_INFO(dev)->cursor_needs_physical)
> > + i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> > + }
> >
> > - return 0;
> > + i915_gem_track_fb(intel_crtc->cursor_bo, obj,
> > + INTEL_FRONTBUFFER_CURSOR(pipe));
> > + mutex_unlock(&dev->struct_mutex);
> > +
> > + intel_crtc->cursor_addr = addr;
> > + intel_crtc->cursor_bo = obj;
> > +update:
> > + old_width = intel_crtc->cursor_width;
> > +
> > + intel_crtc->cursor_width = drm_rect_width(&state->orig_dst);
> > + intel_crtc->cursor_height = drm_rect_height(&state->orig_dst);
> > +
> > + if (intel_crtc->cursor_bo == obj)
> > + on = state->visible;
> > + else
> > + on = !obj;
>
> That looks fishy. Why do we need to care if the bo changed here, and why
> would we turn on the cursor when there's no bo?
Yes, this should actually have been on = !!obj. But all I was doing here is
reordering the code, so this is how it was before. The flow is the same.
>
> The cursor is either visible or it isn't, and that's all that
> intel_crtc_update_cursor() should care about.
Right, I'll remove the if-else and call intel_crtc_update_cursor() with
state->visible as parameter.
>
> > +
> > + if (intel_crtc->active) {
> > + if (old_width != intel_crtc->cursor_width)
> > + intel_update_watermarks(crtc);
> > + intel_crtc_update_cursor(crtc, on);
> > }
> > +
> > + intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
>
> This should probably be done only when crtc->active==true. I see we're
> not doing it that way currently, so I guess we could have a separate
> patch to change that.
Okay, I have created a separated patch for this.
Gustavo
More information about the dri-devel
mailing list