| Message ID | 1416858786-25376-5-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/24/2014 09:53 PM, Matt Roper wrote: > Primary and sprite planes have already been refactored to include a > 'prepare' step which handles all the commit-time operations that could > fail (i.e., pinning buffers and such). Refactor the cursor commit in a > similar manner. > > For simplicity and consistency with other plane types, we also switch to > using intel_pin_and_fence_fb_obj() to perform our pinning for > non-physical cursors. This will allow us to more easily migrate the > code into the atomic 'begin' handler in a plane-agnostic manner in a > future patchset. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++--------------------- > 1 file changed, 44 insertions(+), 70 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 17788f8..ff94e43 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane, > } > > static int > +intel_prepare_cursor_plane(struct drm_plane *plane, > + struct intel_plane_state *state) > +{ > + struct drm_device *dev = plane->dev; > + struct drm_framebuffer *fb = state->fb; > + struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc); > + struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > + enum pipe pipe = intel_crtc->pipe; > + int ret = 0; > + > + if (old_obj != obj) { > + /* we only need to pin inside GTT if cursor is non-phy */ > + mutex_lock(&dev->struct_mutex); > + if (!INTEL_INFO(dev)->cursor_needs_physical) { > + if (obj) > + ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > + if (ret == 0) > + i915_gem_track_fb(intel_crtc->cursor_bo, obj, > + INTEL_FRONTBUFFER_CURSOR(pipe)); Shouldn't the frontbuffer bits be updated in the else case too? At least they were, prior to this patch. > + } else { > + int align = IS_I830(dev) ? 16 * 1024 : 256; > + if (obj) > + ret = i915_gem_object_attach_phys(obj, align); > + if (ret) > + DRM_DEBUG_KMS("failed to attach phys object\n"); > + } > + mutex_unlock(&dev->struct_mutex); > + } > + > + return ret; > +} > + > +static void > 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 intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_i915_gem_object *obj = intel_fb_obj(state->fb); > enum pipe pipe = intel_crtc->pipe; > unsigned old_width; > uint32_t addr; > - int ret; > > crtc->cursor_x = state->orig_dst.x1; > crtc->cursor_y = state->orig_dst.y1; > @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane, > 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"); > + if (!obj) > 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; > - } > - > + else if (!INTEL_INFO(dev)->cursor_needs_physical) > 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; > - } > + else > 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); This is now being called without holding dev->struct_mutex. I don't know if that is necessary or not. Ander > } > > - 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: > @@ -12013,13 +11988,6 @@ 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); > - return ret; > } > > static int > @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, > if (ret) > return ret; > > - return intel_commit_cursor_plane(plane, &state); > + ret = intel_prepare_cursor_plane(plane, &state); > + if (ret) > + return ret; > + > + intel_commit_cursor_plane(plane, &state); > + > + return 0; > } > > static const struct drm_plane_funcs intel_cursor_plane_funcs = { >
On 11/28/2014 02:15 PM, Ander Conselvan de Oliveira wrote: > On 11/24/2014 09:53 PM, Matt Roper wrote: >> Primary and sprite planes have already been refactored to include a >> 'prepare' step which handles all the commit-time operations that could >> fail (i.e., pinning buffers and such). Refactor the cursor commit in a >> similar manner. >> >> For simplicity and consistency with other plane types, we also switch to >> using intel_pin_and_fence_fb_obj() to perform our pinning for >> non-physical cursors. This will allow us to more easily migrate the >> code into the atomic 'begin' handler in a plane-agnostic manner in a >> future patchset. >> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 114 >> ++++++++++++++--------------------- >> 1 file changed, 44 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 17788f8..ff94e43 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane >> *plane, >> } >> >> static int >> +intel_prepare_cursor_plane(struct drm_plane *plane, >> + struct intel_plane_state *state) >> +{ >> + struct drm_device *dev = plane->dev; >> + struct drm_framebuffer *fb = state->fb; >> + struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc); >> + struct drm_i915_gem_object *obj = intel_fb_obj(fb); >> + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); >> + enum pipe pipe = intel_crtc->pipe; >> + int ret = 0; >> + >> + if (old_obj != obj) { >> + /* we only need to pin inside GTT if cursor is non-phy */ >> + mutex_lock(&dev->struct_mutex); >> + if (!INTEL_INFO(dev)->cursor_needs_physical) { >> + if (obj) >> + ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); >> + if (ret == 0) >> + i915_gem_track_fb(intel_crtc->cursor_bo, obj, >> + INTEL_FRONTBUFFER_CURSOR(pipe)); > > Shouldn't the frontbuffer bits be updated in the else case too? At least > they were, prior to this patch. > >> + } else { >> + int align = IS_I830(dev) ? 16 * 1024 : 256; >> + if (obj) >> + ret = i915_gem_object_attach_phys(obj, align); >> + if (ret) >> + DRM_DEBUG_KMS("failed to attach phys object\n"); >> + } >> + mutex_unlock(&dev->struct_mutex); >> + } >> + >> + return ret; >> +} >> + >> +static void >> 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 intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct intel_plane *intel_plane = to_intel_plane(plane); >> struct drm_i915_gem_object *obj = intel_fb_obj(state->fb); >> enum pipe pipe = intel_crtc->pipe; >> unsigned old_width; >> uint32_t addr; >> - int ret; >> >> crtc->cursor_x = state->orig_dst.x1; >> crtc->cursor_y = state->orig_dst.y1; >> @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane >> *plane, >> 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"); >> + if (!obj) >> 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; >> - } >> - >> + else if (!INTEL_INFO(dev)->cursor_needs_physical) >> 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; >> - } >> + else >> 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); > > This is now being called without holding dev->struct_mutex. I don't know > if that is necessary or not. While looking at patch 7 in the series, I noticed that this call is replaced by a call to intel_unpin_fb_obj() eventually, and that one actually needs dev->struct_mutex to be held. The difference is that the latter call also deals with fence registers. Since intel_pin_and_fence_fb_obj() might setup fence registers I think we should use intel_unpin_fb_obj() here for symmetry, even though a fence register won't actually be set up because the cursor bo is never tiled. Ander >> } >> >> - 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: >> @@ -12013,13 +11988,6 @@ 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); >> - return ret; >> } >> >> static int >> @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane >> *plane, struct drm_crtc *crtc, >> if (ret) >> return ret; >> >> - return intel_commit_cursor_plane(plane, &state); >> + ret = intel_prepare_cursor_plane(plane, &state); >> + if (ret) >> + return ret; >> + >> + intel_commit_cursor_plane(plane, &state); >> + >> + return 0; >> } >> >> static const struct drm_plane_funcs intel_cursor_plane_funcs = { >> >
On Fri, Nov 28, 2014 at 02:15:08PM +0200, Ander Conselvan de Oliveira wrote: > On 11/24/2014 09:53 PM, Matt Roper wrote: > >Primary and sprite planes have already been refactored to include a > >'prepare' step which handles all the commit-time operations that could > >fail (i.e., pinning buffers and such). Refactor the cursor commit in a > >similar manner. > > > >For simplicity and consistency with other plane types, we also switch to > >using intel_pin_and_fence_fb_obj() to perform our pinning for > >non-physical cursors. This will allow us to more easily migrate the > >code into the atomic 'begin' handler in a plane-agnostic manner in a > >future patchset. > > > >Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > >--- > > drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++--------------------- > > 1 file changed, 44 insertions(+), 70 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >index 17788f8..ff94e43 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane, > > } > > > > static int > >+intel_prepare_cursor_plane(struct drm_plane *plane, > >+ struct intel_plane_state *state) > >+{ > >+ struct drm_device *dev = plane->dev; > >+ struct drm_framebuffer *fb = state->fb; > >+ struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc); > >+ struct drm_i915_gem_object *obj = intel_fb_obj(fb); > >+ struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); > >+ enum pipe pipe = intel_crtc->pipe; > >+ int ret = 0; > >+ > >+ if (old_obj != obj) { > >+ /* we only need to pin inside GTT if cursor is non-phy */ > >+ mutex_lock(&dev->struct_mutex); > >+ if (!INTEL_INFO(dev)->cursor_needs_physical) { > >+ if (obj) > >+ ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > >+ if (ret == 0) > >+ i915_gem_track_fb(intel_crtc->cursor_bo, obj, > >+ INTEL_FRONTBUFFER_CURSOR(pipe)); > > Shouldn't the frontbuffer bits be updated in the else case too? At least > they were, prior to this patch. gem_track_fb must be called always when you exchange plane buffer (even when the plane is disabled). It won't hurt too badly since the affected platforms don't support fbc/psr or anything which needs frontbuffer tracking. Well more correctly the hw can do fbc, but I don't think we'll ever enable it. But that's aside, still a bug. > >+ } else { > >+ int align = IS_I830(dev) ? 16 * 1024 : 256; > >+ if (obj) > >+ ret = i915_gem_object_attach_phys(obj, align); > >+ if (ret) > >+ DRM_DEBUG_KMS("failed to attach phys object\n"); > >+ } > >+ mutex_unlock(&dev->struct_mutex); > >+ } > >+ > >+ return ret; > >+} > >+ > >+static void > > 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 intel_crtc *intel_crtc = to_intel_crtc(crtc); > > struct intel_plane *intel_plane = to_intel_plane(plane); > > struct drm_i915_gem_object *obj = intel_fb_obj(state->fb); > > enum pipe pipe = intel_crtc->pipe; > > unsigned old_width; > > uint32_t addr; > >- int ret; > > > > crtc->cursor_x = state->orig_dst.x1; > > crtc->cursor_y = state->orig_dst.y1; > >@@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane, > > 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"); > >+ if (!obj) > > 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; > >- } > >- > >+ else if (!INTEL_INFO(dev)->cursor_needs_physical) > > 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; > >- } > >+ else > > 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); > > This is now being called without holding dev->struct_mutex. I don't know if > that is necessary or not. unpin requires dev->struct_mutex. Same for gem_track_fb actually. -Daniel > > > Ander > > > } > > > >- 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: > >@@ -12013,13 +11988,6 @@ 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); > >- return ret; > > } > > > > static int > >@@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, > > if (ret) > > return ret; > > > >- return intel_commit_cursor_plane(plane, &state); > >+ ret = intel_prepare_cursor_plane(plane, &state); > >+ if (ret) > >+ return ret; > >+ > >+ intel_commit_cursor_plane(plane, &state); > >+ > >+ return 0; > > } > > > > static const struct drm_plane_funcs intel_cursor_plane_funcs = { > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 11/24/2014 09:53 PM, Matt Roper wrote: > Primary and sprite planes have already been refactored to include a > 'prepare' step which handles all the commit-time operations that could > fail (i.e., pinning buffers and such). Refactor the cursor commit in a > similar manner. > > For simplicity and consistency with other plane types, we also switch to > using intel_pin_and_fence_fb_obj() to perform our pinning for > non-physical cursors. This will allow us to more easily migrate the > code into the atomic 'begin' handler in a plane-agnostic manner in a > future patchset. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++--------------------- > 1 file changed, 44 insertions(+), 70 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 17788f8..ff94e43 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane, > } > > static int > +intel_prepare_cursor_plane(struct drm_plane *plane, > + struct intel_plane_state *state) > +{ > + struct drm_device *dev = plane->dev; > + struct drm_framebuffer *fb = state->fb; > + struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc); > + struct drm_i915_gem_object *obj = intel_fb_obj(fb); > + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); I don't see where plane->fb is being updated. I think this needs to be done in intel_commit_cursor_plane(). Ander > + enum pipe pipe = intel_crtc->pipe; > + int ret = 0; > + > + if (old_obj != obj) { > + /* we only need to pin inside GTT if cursor is non-phy */ > + mutex_lock(&dev->struct_mutex); > + if (!INTEL_INFO(dev)->cursor_needs_physical) { > + if (obj) > + ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); > + if (ret == 0) > + i915_gem_track_fb(intel_crtc->cursor_bo, obj, > + INTEL_FRONTBUFFER_CURSOR(pipe)); > + } else { > + int align = IS_I830(dev) ? 16 * 1024 : 256; > + if (obj) > + ret = i915_gem_object_attach_phys(obj, align); > + if (ret) > + DRM_DEBUG_KMS("failed to attach phys object\n"); > + } > + mutex_unlock(&dev->struct_mutex); > + } > + > + return ret; > +} > + > +static void > 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 intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_i915_gem_object *obj = intel_fb_obj(state->fb); > enum pipe pipe = intel_crtc->pipe; > unsigned old_width; > uint32_t addr; > - int ret; > > crtc->cursor_x = state->orig_dst.x1; > crtc->cursor_y = state->orig_dst.y1; > @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane, > 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"); > + if (!obj) > 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; > - } > - > + else if (!INTEL_INFO(dev)->cursor_needs_physical) > 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; > - } > + else > 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); > - > intel_crtc->cursor_addr = addr; > intel_crtc->cursor_bo = obj; > update: > @@ -12013,13 +11988,6 @@ 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); > - return ret; > } > > static int > @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, > if (ret) > return ret; > > - return intel_commit_cursor_plane(plane, &state); > + ret = intel_prepare_cursor_plane(plane, &state); > + if (ret) > + return ret; > + > + intel_commit_cursor_plane(plane, &state); > + > + return 0; > } > > static const struct drm_plane_funcs intel_cursor_plane_funcs = { >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 17788f8..ff94e43 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11899,19 +11899,51 @@ intel_check_cursor_plane(struct drm_plane *plane, } static int +intel_prepare_cursor_plane(struct drm_plane *plane, + struct intel_plane_state *state) +{ + struct drm_device *dev = plane->dev; + struct drm_framebuffer *fb = state->fb; + struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc); + struct drm_i915_gem_object *obj = intel_fb_obj(fb); + struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb); + enum pipe pipe = intel_crtc->pipe; + int ret = 0; + + if (old_obj != obj) { + /* we only need to pin inside GTT if cursor is non-phy */ + mutex_lock(&dev->struct_mutex); + if (!INTEL_INFO(dev)->cursor_needs_physical) { + if (obj) + ret = intel_pin_and_fence_fb_obj(plane, fb, NULL); + if (ret == 0) + i915_gem_track_fb(intel_crtc->cursor_bo, obj, + INTEL_FRONTBUFFER_CURSOR(pipe)); + } else { + int align = IS_I830(dev) ? 16 * 1024 : 256; + if (obj) + ret = i915_gem_object_attach_phys(obj, align); + if (ret) + DRM_DEBUG_KMS("failed to attach phys object\n"); + } + mutex_unlock(&dev->struct_mutex); + } + + return ret; +} + +static void 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 intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_i915_gem_object *obj = intel_fb_obj(state->fb); enum pipe pipe = intel_crtc->pipe; unsigned old_width; uint32_t addr; - int ret; crtc->cursor_x = state->orig_dst.x1; crtc->cursor_y = state->orig_dst.y1; @@ -11929,75 +11961,18 @@ intel_commit_cursor_plane(struct drm_plane *plane, 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"); + if (!obj) 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; - } - + else if (!INTEL_INFO(dev)->cursor_needs_physical) 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; - } + else 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); - intel_crtc->cursor_addr = addr; intel_crtc->cursor_bo = obj; update: @@ -12013,13 +11988,6 @@ 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); - return ret; } static int @@ -12060,7 +12028,13 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, if (ret) return ret; - return intel_commit_cursor_plane(plane, &state); + ret = intel_prepare_cursor_plane(plane, &state); + if (ret) + return ret; + + intel_commit_cursor_plane(plane, &state); + + return 0; } static const struct drm_plane_funcs intel_cursor_plane_funcs = {
Primary and sprite planes have already been refactored to include a 'prepare' step which handles all the commit-time operations that could fail (i.e., pinning buffers and such). Refactor the cursor commit in a similar manner. For simplicity and consistency with other plane types, we also switch to using intel_pin_and_fence_fb_obj() to perform our pinning for non-physical cursors. This will allow us to more easily migrate the code into the atomic 'begin' handler in a plane-agnostic manner in a future patchset. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 114 ++++++++++++++--------------------- 1 file changed, 44 insertions(+), 70 deletions(-)