From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v4 5/5] drm/i915: remove intel_crtc_cursor_set_obj() Date: Fri, 24 Oct 2014 18:16:11 +0300 Message-ID: <20141024151611.GH4284@intel.com> References: <1414158695-31605-1-git-send-email-gustavo@padovan.org> <1414158695-31605-5-git-send-email-gustavo@padovan.org> 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: <1414158695-31605-5-git-send-email-gustavo@padovan.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Gustavo Padovan Cc: intel-gfx@lists.freedesktop.org, Gustavo Padovan , dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Oct 24, 2014 at 02:51:35PM +0100, 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_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 > --- > drivers/gpu/drm/i915/intel_display.c | 215 ++++++++++++++++-------------= ------ > 1 file changed, 100 insertions(+), 115 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 9a913f5..60ec165 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8453,109 +8453,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 =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 *g= reen, > u16 *blue, uint32_t start, uint32_t size) > { > @@ -11906,7 +11803,8 @@ intel_cursor_plane_disable(struct drm_plane *plan= e) > = > 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 > @@ -11970,26 +11868,113 @@ intel_commit_cursor_plane(struct drm_plane *pl= ane, > 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; > + 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->active) { > + if (old_width !=3D intel_crtc->cursor_width) > + intel_update_watermarks(crtc); > + intel_crtc_update_cursor(crtc, state->visible); So we need to make sure state->visible=3D=3Dfalse when there's no fb. I suppose we should just do that in drm_plane_helper_check_update(). > + > + 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); > + drm_gem_object_unreference_unlocked(&obj->base); > + return ret; > } > = > static int > -- = > 1.9.3 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC