On Fri, Oct 24, 2014 at 02:23:35PM +0100, Gustavo Padovan wrote:
> 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.

I just realized that we don't seem to actually compute state->visible
correctly when there's no fb. So that definitely needs to be done if we
want to share the .update_plane() as .disable_plane().

> 
> > 
> > > +
> > > + 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

-- 
Ville Syrj?l?
Intel OTC

Reply via email to