[Intel-gfx] [PATCH 4/9] drm/i915: Introduce intel_prepare_cursor_plane()
Ander Conselvan de Oliveira
conselvan2 at gmail.com
Fri Nov 28 14:12:40 CET 2014
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 at 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 = {
>>
>
More information about the Intel-gfx
mailing list