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
