From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gustavo Padovan Subject: Re: [PATCH v3 05/11] drm/i915: remove intel_crtc_cursor_set_obj() Date: Fri, 24 Oct 2014 14:23:35 +0100 Message-ID: <20141024132335.GD14165@joana> References: <1411579232-8668-1-git-send-email-gustavo@padovan.org> <1411579232-8668-5-git-send-email-gustavo@padovan.org> <20141007145933.GX32511@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <20141007145933.GX32511@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 2014-10-07 Ville Syrj=E4l=E4 : > On Wed, Sep 24, 2014 at 02:20:26PM -0300, Gustavo Padovan wrote: > > From: Gustavo Padovan > > = > > Merge it into the plane update_plane() callback and make other > > users use the update_plane() functions instead. > > = > > The fb !=3D crtc->cursor->fb was already inside intel_crtc_cursor_set_o= bj() > > so we fold intel_crtc_cursor_set_obj() inside intel_commit_cursor_plane= () > > and merge both paths into one. > > = > > Signed-off-by: Gustavo Padovan > > --- > > 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/i91= 5/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 *d= ev, > > 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 =3D crtc->dev; > > - struct drm_i915_private *dev_priv =3D dev->dev_private; > > - struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > - enum pipe pipe =3D 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 =3D 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 =3D 0; > > - if (need_vtd_wa(dev)) > > - alignment =3D 64*1024; > > - > > - ret =3D 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 =3D 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 =3D i915_gem_obj_ggtt_offset(obj); > > - > > - intel_runtime_pm_put(dev_priv); > > - } else { > > - int align =3D IS_I830(dev) ? 16 * 1024 : 256; > > - ret =3D i915_gem_object_attach_phys(obj, align); > > - if (ret) { > > - DRM_DEBUG_KMS("failed to attach phys object\n"); > > - goto fail_locked; > > - } > > - addr =3D 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 =3D intel_crtc->cursor_width; > > - > > - intel_crtc->cursor_addr =3D addr; > > - intel_crtc->cursor_bo =3D obj; > > - intel_crtc->cursor_width =3D width; > > - intel_crtc->cursor_height =3D height; > > - > > - if (intel_crtc->active) { > > - if (old_width !=3D width) > > - intel_update_watermarks(crtc); > > - intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo !=3D 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 *pl= ane) > > = > > 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 =3D state->crtc; > > + struct drm_device *dev =3D crtc->dev; > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > struct drm_framebuffer *fb =3D state->fb; > > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > - struct intel_framebuffer *intel_fb =3D to_intel_framebuffer(fb); > > - struct drm_i915_gem_object *obj =3D intel_fb->obj; > > - int crtc_w, crtc_h; > > + struct drm_i915_gem_object *obj =3D intel_fb_obj(fb); > > + enum pipe pipe =3D intel_crtc->pipe; > > + unsigned old_width; > > + uint32_t addr; > > + bool on; > > + int ret; > > = > > crtc->cursor_x =3D state->orig_dst.x1; > > crtc->cursor_y =3D state->orig_dst.y1; > > - if (fb !=3D crtc->cursor->fb) { > > - crtc_w =3D drm_rect_width(&state->orig_dst); > > - crtc_h =3D drm_rect_height(&state->orig_dst); > > - return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h); > > + > > + if (intel_crtc->cursor_bo =3D=3D obj) > > + goto update; > > + > > + /* if we want to turn off the cursor ignore width and height */ > > + if (!obj) { > > + DRM_DEBUG_KMS("cursor off\n"); > > + addr =3D 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 =3D 0; > > + if (need_vtd_wa(dev)) > > + alignment =3D 64*1024; > > + > > + ret =3D 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 =3D 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 =3D i915_gem_obj_ggtt_offset(obj); > > + > > + intel_runtime_pm_put(dev_priv); > > } else { > > - intel_crtc_update_cursor(crtc, state->visible); > > + int align =3D IS_I830(dev) ? 16 * 1024 : 256; > > + ret =3D i915_gem_object_attach_phys(obj, align); > > + if (ret) { > > + DRM_DEBUG_KMS("failed to attach phys object\n"); > > + goto fail_locked; > > + } > > + addr =3D 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 =3D addr; > > + intel_crtc->cursor_bo =3D obj; > > +update: > > + old_width =3D intel_crtc->cursor_width; > > + > > + intel_crtc->cursor_width =3D drm_rect_width(&state->orig_dst); > > + intel_crtc->cursor_height =3D drm_rect_height(&state->orig_dst); > > + > > + if (intel_crtc->cursor_bo =3D=3D obj) > > + on =3D state->visible; > > + else > > + on =3D !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 =3D !!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 !=3D 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=3D=3Dtrue. 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