[PATCH] drm/i915: Hold runtime PM during plane commit

[PATCH] drm/i915: Hold runtime PM during plane commit

[Matt Roper]

During plane operations, we read/write some registers that only operate
properly if we're not runtime suspended.  At the moment we're not
holding the runtime PM reference across the whole plane operation, so
there's a potential for problems.

This issue was already partially addressed by commit

        commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
        Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
        Date:   Fri Aug 15 15:59:32 2014 -0300

            drm/i915: fix plane/cursor handling when runtime suspended

which took care of holding the runtime PM reference during the pin and
fence operations for plane updates.  However there are still a few
actual plane registers that we also need to hold the runtime PM
reference for.  Recent refactoring patches in preparation for atomic
have rearranged the code and made it increasingly likely that the
hardware will have time to suspend between the pin/fence operation and
the actual register writes.

The solution here grabs the runtime PM reference around the 'commit'
operation for planes, which should cover all the relevant register
reads/writes.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3044af5..a0ddce5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11863,6 +11863,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_framebuffer *old_fb = plane->fb;
 	struct intel_plane_state state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -11902,7 +11903,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			return ret;
 	}
 
+	intel_runtime_pm_get(dev_priv);
 	intel_plane->commit_plane(plane, &state);
+	intel_runtime_pm_put(dev_priv);
 
 	if (fb != old_fb && old_fb) {
 		if (intel_crtc->active)
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Paulo Zanoni]

2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>:
> During plane operations, we read/write some registers that only operate
> properly if we're not runtime suspended.  At the moment we're not
> holding the runtime PM reference across the whole plane operation, so
> there's a potential for problems.
>
> This issue was already partially addressed by commit
>
>         commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
>         Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>         Date:   Fri Aug 15 15:59:32 2014 -0300
>
>             drm/i915: fix plane/cursor handling when runtime suspended
>
> which took care of holding the runtime PM reference during the pin and
> fence operations for plane updates.  However there are still a few
> actual plane registers that we also need to hold the runtime PM
> reference for.  Recent refactoring patches in preparation for atomic
> have rearranged the code and made it increasingly likely that the
> hardware will have time to suspend between the pin/fence operation and
> the actual register writes.
>
> The solution here grabs the runtime PM reference around the 'commit'
> operation for planes, which should cover all the relevant register
> reads/writes.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

I see we're in the middle of a very big rework on how all these
prepare/commit functions are called, so I don't think it makes sense
to spend too much time trying to find the very-best-perfect spot for
the get/put calls, since they're likely to be changed later. So I
guess that for now it's important to fix the current "regression"
reported by QA:

Testcase: igt/pm-rpm/legacy-planes
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3044af5..a0ddce5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11863,6 +11863,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>                    uint32_t src_w, uint32_t src_h)
>  {
>         struct drm_device *dev = plane->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
>         struct drm_framebuffer *old_fb = plane->fb;
>         struct intel_plane_state state;
>         struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -11902,7 +11903,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>                         return ret;
>         }
>
> +       intel_runtime_pm_get(dev_priv);
>         intel_plane->commit_plane(plane, &state);
> +       intel_runtime_pm_put(dev_priv);
>
>         if (fb != old_fb && old_fb) {
>                 if (intel_crtc->active)
> --
> 1.8.5.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Daniel Vetter]

On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote:
> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>:
> > During plane operations, we read/write some registers that only operate
> > properly if we're not runtime suspended.  At the moment we're not
> > holding the runtime PM reference across the whole plane operation, so
> > there's a potential for problems.
> >
> > This issue was already partially addressed by commit
> >
> >         commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
> >         Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >         Date:   Fri Aug 15 15:59:32 2014 -0300
> >
> >             drm/i915: fix plane/cursor handling when runtime suspended
> >
> > which took care of holding the runtime PM reference during the pin and
> > fence operations for plane updates.  However there are still a few
> > actual plane registers that we also need to hold the runtime PM
> > reference for.  Recent refactoring patches in preparation for atomic
> > have rearranged the code and made it increasingly likely that the
> > hardware will have time to suspend between the pin/fence operation and
> > the actual register writes.

Which kind of registers? If this is just in the system agent then a rpm
ref is enough, but if this is also about plane registers then we'd need a
reference of the plane power domain. Which would indicate some failure to
check for crtc->active somewhere I think.

> > The solution here grabs the runtime PM reference around the 'commit'
> > operation for planes, which should cover all the relevant register
> > reads/writes.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I see we're in the middle of a very big rework on how all these
> prepare/commit functions are called, so I don't think it makes sense
> to spend too much time trying to find the very-best-perfect spot for
> the get/put calls, since they're likely to be changed later. So I
> guess that for now it's important to fix the current "regression"
> reported by QA:
> 
> Testcase: igt/pm-rpm/legacy-planes
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Jani Nikula]

On Tue, 16 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote:
>> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>:
>> > During plane operations, we read/write some registers that only operate
>> > properly if we're not runtime suspended.  At the moment we're not
>> > holding the runtime PM reference across the whole plane operation, so
>> > there's a potential for problems.
>> >
>> > This issue was already partially addressed by commit
>> >
>> >         commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
>> >         Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >         Date:   Fri Aug 15 15:59:32 2014 -0300
>> >
>> >             drm/i915: fix plane/cursor handling when runtime suspended
>> >
>> > which took care of holding the runtime PM reference during the pin and
>> > fence operations for plane updates.  However there are still a few
>> > actual plane registers that we also need to hold the runtime PM
>> > reference for.  Recent refactoring patches in preparation for atomic
>> > have rearranged the code and made it increasingly likely that the
>> > hardware will have time to suspend between the pin/fence operation and
>> > the actual register writes.
>
> Which kind of registers? If this is just in the system agent then a rpm
> ref is enough, but if this is also about plane registers then we'd need a
> reference of the plane power domain. Which would indicate some failure to
> check for crtc->active somewhere I think.
>
>> > The solution here grabs the runtime PM reference around the 'commit'
>> > operation for planes, which should cover all the relevant register
>> > reads/writes.
>> >
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> 
>> I see we're in the middle of a very big rework on how all these
>> prepare/commit functions are called, so I don't think it makes sense
>> to spend too much time trying to find the very-best-perfect spot for
>> the get/put calls, since they're likely to be changed later. So I
>> guess that for now it's important to fix the current "regression"
>> reported by QA:
>> 
>> Testcase: igt/pm-rpm/legacy-planes
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq?

At least the partial fix referenced is in 3.17.

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Jani Nikula]

On Tue, 16 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 16 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote:
>>> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>:
>>> > During plane operations, we read/write some registers that only operate
>>> > properly if we're not runtime suspended.  At the moment we're not
>>> > holding the runtime PM reference across the whole plane operation, so
>>> > there's a potential for problems.
>>> >
>>> > This issue was already partially addressed by commit
>>> >
>>> >         commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
>>> >         Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> >         Date:   Fri Aug 15 15:59:32 2014 -0300
>>> >
>>> >             drm/i915: fix plane/cursor handling when runtime suspended
>>> >
>>> > which took care of holding the runtime PM reference during the pin and
>>> > fence operations for plane updates.  However there are still a few
>>> > actual plane registers that we also need to hold the runtime PM
>>> > reference for.  Recent refactoring patches in preparation for atomic
>>> > have rearranged the code and made it increasingly likely that the
>>> > hardware will have time to suspend between the pin/fence operation and
>>> > the actual register writes.
>>
>> Which kind of registers? If this is just in the system agent then a rpm
>> ref is enough, but if this is also about plane registers then we'd need a
>> reference of the plane power domain. Which would indicate some failure to
>> check for crtc->active somewhere I think.
>>
>>> > The solution here grabs the runtime PM reference around the 'commit'
>>> > operation for planes, which should cover all the relevant register
>>> > reads/writes.
>>> >
>>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
>>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>> 
>>> I see we're in the middle of a very big rework on how all these
>>> prepare/commit functions are called, so I don't think it makes sense
>>> to spend too much time trying to find the very-best-perfect spot for
>>> the get/put calls, since they're likely to be changed later. So I
>>> guess that for now it's important to fix the current "regression"
>>> reported by QA:
>>> 
>>> Testcase: igt/pm-rpm/legacy-planes
>>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq?
>
> At least the partial fix referenced is in 3.17.

Ugh, but it conflicts badly, there's no ->commit_plane in
drm-intel-next-fixes.

Daniel, pick this up for dinq, and if someone wants this to stable, it
needs a backported version.

BR,
Jani.


>
>> -Daniel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Daniel Vetter]

On Tue, Dec 16, 2014 at 03:29:56PM +0200, Jani Nikula wrote:
> On Tue, 16 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Tue, 16 Dec 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote:
> >>> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>:
> >>> > During plane operations, we read/write some registers that only operate
> >>> > properly if we're not runtime suspended.  At the moment we're not
> >>> > holding the runtime PM reference across the whole plane operation, so
> >>> > there's a potential for problems.
> >>> >
> >>> > This issue was already partially addressed by commit
> >>> >
> >>> >         commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
> >>> >         Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> >         Date:   Fri Aug 15 15:59:32 2014 -0300
> >>> >
> >>> >             drm/i915: fix plane/cursor handling when runtime suspended
> >>> >
> >>> > which took care of holding the runtime PM reference during the pin and
> >>> > fence operations for plane updates.  However there are still a few
> >>> > actual plane registers that we also need to hold the runtime PM
> >>> > reference for.  Recent refactoring patches in preparation for atomic
> >>> > have rearranged the code and made it increasingly likely that the
> >>> > hardware will have time to suspend between the pin/fence operation and
> >>> > the actual register writes.
> >>
> >> Which kind of registers? If this is just in the system agent then a rpm
> >> ref is enough, but if this is also about plane registers then we'd need a
> >> reference of the plane power domain. Which would indicate some failure to
> >> check for crtc->active somewhere I think.
> >>
> >>> > The solution here grabs the runtime PM reference around the 'commit'
> >>> > operation for planes, which should cover all the relevant register
> >>> > reads/writes.
> >>> >
> >>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
> >>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >>> 
> >>> I see we're in the middle of a very big rework on how all these
> >>> prepare/commit functions are called, so I don't think it makes sense
> >>> to spend too much time trying to find the very-best-perfect spot for
> >>> the get/put calls, since they're likely to be changed later. So I
> >>> guess that for now it's important to fix the current "regression"
> >>> reported by QA:
> >>> 
> >>> Testcase: igt/pm-rpm/legacy-planes
> >>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq?
> >
> > At least the partial fix referenced is in 3.17.
> 
> Ugh, but it conflicts badly, there's no ->commit_plane in
> drm-intel-next-fixes.
> 
> Daniel, pick this up for dinq, and if someone wants this to stable, it
> needs a backported version.

Done, and thanks to Paulo for the additional clarification. I've augmented
the commit message with that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[Paulo Zanoni]

2014-12-16 7:42 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Dec 15, 2014 at 04:30:44PM -0200, Paulo Zanoni wrote:
>> 2014-12-15 16:11 GMT-02:00 Matt Roper <matthew.d.roper@intel.com>:
>> > During plane operations, we read/write some registers that only operate
>> > properly if we're not runtime suspended.  At the moment we're not
>> > holding the runtime PM reference across the whole plane operation, so
>> > there's a potential for problems.
>> >
>> > This issue was already partially addressed by commit
>> >
>> >         commit d6dd6843ff4a57c662dbc378b9f99a9c034b0956
>> >         Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >         Date:   Fri Aug 15 15:59:32 2014 -0300
>> >
>> >             drm/i915: fix plane/cursor handling when runtime suspended
>> >
>> > which took care of holding the runtime PM reference during the pin and
>> > fence operations for plane updates.  However there are still a few
>> > actual plane registers that we also need to hold the runtime PM
>> > reference for.  Recent refactoring patches in preparation for atomic
>> > have rearranged the code and made it increasingly likely that the
>> > hardware will have time to suspend between the pin/fence operation and
>> > the actual register writes.
>
> Which kind of registers?

Some of the regs touched by ivb_get_colorkey().

> If this is just in the system agent then a rpm
> ref is enough, but if this is also about plane registers then we'd need a
> reference of the plane power domain. Which would indicate some failure to
> check for crtc->active somewhere I think.
>
>> > The solution here grabs the runtime PM reference around the 'commit'
>> > operation for planes, which should cover all the relevant register
>> > reads/writes.
>> >
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87180
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>
>> I see we're in the middle of a very big rework on how all these
>> prepare/commit functions are called, so I don't think it makes sense
>> to spend too much time trying to find the very-best-perfect spot for
>> the get/put calls, since they're likely to be changed later. So I
>> guess that for now it's important to fix the current "regression"
>> reported by QA:
>>
>> Testcase: igt/pm-rpm/legacy-planes
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Also Cc: stable@vger.kernel.org I guess? Or is this only for dinq?

I would suggest only dinq, since it's the only thing that can
reproduce the WARN, which got visible after:

commit 6beb8c23ebcc3d3287d8a247d11b73d7d0eaa475
drm/i915: Consolidate plane 'prepare' functions (v2)



> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Hold runtime PM during plane commit

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +5                 360/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_drv_suspend_fence-restore-untiled      DMESG_WARN(1, M26)PASS(5, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(5, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_busy-flip-interruptible      DMESG_WARN(1, M26)PASS(5, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_flip-vs-rmfb-interruptible      DMESG_WARN(1, M26)PASS(5, M37M26)      PASS(1, M37)
 ILK  igt_kms_flip_rcs-flip-vs-dpms      DMESG_WARN(1, M26)PASS(4, M37M26)      PASS(1, M37)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx