Re: [PATCH 04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.

From: Sunil Kamath <sunil.kamath@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: "Vetter, Daniel" <daniel.vetter@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.
Date: Tue, 28 Jul 2015 15:58:07 +0530	[thread overview]
Message-ID: <55B75937.5060700@intel.com> (raw)
In-Reply-To: <20150728075719.GA16722@phenom.ffwll.local>

On Tuesday 28 July 2015 01:27 PM, Daniel Vetter wrote:
> On Mon, Jul 27, 2015 at 10:48:16AM +0200, Daniel Vetter wrote:
>> On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
>>> Grabbing a runtime pm reference with intel_runtime_pm_get will only
>>> prevent device D3. But dmc firmware is required even earlier (namely
>>> for the skl power well 1). DMC is responsible to save the status of
>>> power well 1 and shut off the power well when panel is self refresh
>>> mode of display is completely off.
>>>
>>> Another interesting criteria to work dmc as expected is pw1 to be
>>> enabled by driver and dmc will shut it off in its execution
>>> sequence. If already disabled by driver dmc will get confuse and
>>> behave differently than expected found during pc10 entry issue
>>> for skl.
>>>
>>> So berfore we disable power-well 1, added check if dmc firmware is
>>> present and driver will not disable power well 1, but for any reason
>>> if firmware is not present of failed to load we can shut off the
>>> power well 1 which will save some power.
>>>
>>> As skl is currently fully dependent on dmc to go in lowest possible
>>> power state (dc6) but the same is not applicable for bxt. Display
>>> engine can enter into dc9 without dmc, hence unblocking disable call.
>>>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> Please use the approach I've laid out in my original patch series with
>> "drm/i915: use correct power domain for csr loading" and "drm/i915: Only
>> allow rpm on gen9+ with dmc loaded". Your approach here requires a
>> flush_work in the runtime pm callbacks which can deadlock.
>>
>> If you want to make dmc optional on bxt then you need to adjust the 2nd
>> patch a bit to no longer leak the runtime pm reference. Your approach here
>> is an inversion of control and that doesn't work well since control
>> inversions very easily lead to locking inversions.
> Summary of what we just discussed offline:
>
> Ok I was confused here with the intel_csr_load_status_get() check. If we
> apply the above to patch from me first then we don't need that check any
> more, and the patch itself looks good as a bugfix for skl dmc firmware
> assumptions.
> -Daniel
Thanks alot Damien.

Animesh,
As discussed in same call, bug fix should go initial patches in this series.

also its good to add the check in skl_set_power_well function itself 
than intel_display_power_put

- Sunil

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