Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2014-08-11 8:32 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> If we're runtime suspended and try to use the plane interfaces, we
>> will get a lot of WARNs saying we did the wrong thing.
>>
>> For intel_crtc_update_cursor(), all we need to do is return if the
>> CRTC is not active, since writing the registers won't really have any
>> effect if the screen is not visible, and we will write the registers
>> later when enabling the screen.
>>
>> For all the other cases, we need to get runtime PM references to
>> pin/unpin the objects, and to change the fences. The pin/unpin
>> functions are the ideal places for this, but
>> intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
>> get/put calls inside it. There is no problem if we runtime suspend
>> right after these functions are finished, because the registers
>> weitten are forwarded to system memory.
>>
>> v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
>> v3: - Make get/put also surround the fence and unpin calls (Daniel and
>>       Ville).
>>     - Merge all the plane changes into a single patch since they're
>>       the same fix.
>>     - Add the comment requested by Daniel.
>>
>> Testcase: igt/pm_rpm/cursor
>> Testcase: igt/pm_rpm/cursor-dpms
>> Testcase: igt/pm_rpm/legacy-planes
>> Testcase: igt/pm_rpm/legacy-planes-dpms
>> Testcase: igt/pm_rpm/universal-planes
>> Testcase: igt/pm_rpm/universal-planes-dpms
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4f659eb..a86d67c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
>>       if (need_vtd_wa(dev) && alignment < 256 * 1024)
>>               alignment = 256 * 1024;
>>
>> +     /*
>> +      * 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);
>> +
>>       dev_priv->mm.interruptible = false;
>>       ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
>>       if (ret)
>> @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
>>       i915_gem_object_pin_fence(obj);
>>
>>       dev_priv->mm.interruptible = true;
>> +     intel_runtime_pm_put(dev_priv);
>>       return 0;
>>
>>  err_unpin:
>>       i915_gem_object_unpin_from_display_plane(obj);
>>  err_interruptible:
>>       dev_priv->mm.interruptible = true;
>> +     intel_runtime_pm_put(dev_priv);
>>       return ret;
>>  }
>>
>>  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
>>  {
>> -     WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>> +     struct drm_device *dev = obj->base.dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>> +
>> +     intel_runtime_pm_get(dev_priv);
>>
>>       i915_gem_object_unpin_fence(obj);
>>       i915_gem_object_unpin_from_display_plane(obj);
>> +
>> +     intel_runtime_pm_put(dev_priv);
>
> I don't think we touch the hardware during unpin so these aren't
> strictly needed.
>

Daniel requested them.


>>  }
>>
>>  /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
>> @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>>       if (base == 0 && intel_crtc->cursor_base == 0)
>>               return;
>>
>> +     if (!intel_crtc->active)
>> +             return;
>> +
>
> Did you actually manage to get by the base==0 check above with a
> disabled pipe? I don't think this should happen.

Yes, since we enabled runtime suspend during DPMS. Remember that
crtc->active != crtc->enabled.

To hit this return, just run "sudo ./pm_rpm --run-subtest cursor-dpms".


>
>>       I915_WRITE(CURPOS(pipe), pos);
>>
>>       if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
>> @@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>>
>>       /* we only need to pin inside GTT if cursor is non-phy */
>>       mutex_lock(&dev->struct_mutex);
>> +
>> +     /*
>> +      * 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);
>> +
>>       if (!INTEL_INFO(dev)->cursor_needs_physical) {
>>               unsigned alignment;
>>
>> +
>
> Spurious whitespace

Oops... Will fix.


>
> And now that I look at our cursor code I see we're doing dubious stuff
> like unpinning the old bo before even writing the cursor registers to
> use the new bo (really we should wait for vblank before unpinning). But
> that's just something else we have to fix at some point.

Yeah, these should be in other patches.

Thanks,
Paulo

>
>>               if (obj->tiling_mode) {
>>                       DRM_DEBUG_KMS("cursor cannot be tiled\n");
>>                       ret = -EINVAL;
>> @@ -8392,6 +8424,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>>
>>       i915_gem_track_fb(intel_crtc->cursor_bo, obj,
>>                         INTEL_FRONTBUFFER_CURSOR(pipe));
>> +
>> +     if (obj)
>> +             intel_runtime_pm_put(dev_priv);
>> +
>>       mutex_unlock(&dev->struct_mutex);
>>
>>       old_width = intel_crtc->cursor_width;
>> @@ -8413,6 +8449,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>>  fail_unpin:
>>       i915_gem_object_unpin_from_display_plane(obj);
>>  fail_locked:
>> +     intel_runtime_pm_put(dev_priv);
>>       mutex_unlock(&dev->struct_mutex);
>>  fail:
>>       drm_gem_object_unreference_unlocked(&obj->base);
>> --
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]