Re: [Intel-gfx] [PATCH v2 3/6] drm/i915: Replace the VLV/CHV eDP reboot notifier with the .shutdown() hook

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 3/6] drm/i915: Replace the VLV/CHV eDP reboot notifier with the .shutdown() hook
Date: Tue, 6 Oct 2020 21:33:07 +0300	[thread overview]
Message-ID: <20201006183307.GY6112@intel.com> (raw)
In-Reply-To: <87zh4zi5o3.fsf@intel.com>

On Tue, Oct 06, 2020 at 09:13:32PM +0300, Jani Nikula wrote:
> On Tue, 06 Oct 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Oct 06, 2020 at 12:29:22PM +0300, Jani Nikula wrote:
> >> On Thu, 01 Oct 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > Currently VLV/CHV use a reboot notifier to make sure the panel
> >> > power cycle delay isn't violated across a system reboot. Replace
> >> > that with the new encoder .shutdown() hook.
> >> >
> >> > And let's also stop overriding the power cycle delay with the
> >> > max value. No idea why the current code does that. The already
> >> > programmed delay should be correct.
> >> 
> >> I kind of have a little uneasy feeling about conflating these two
> >> changes together. I think both are objectively good changes, just not
> >> necessarily at once.
> >> 
> >> ISTR setting the max delay was, perhaps, somehow related to the hardware
> >> losing its marbles after power is cut, effectively not ensuring any of
> >> the delays at power-on. So it's possible we set the max here to account
> >> for that. Maybe. ;)
> >> 
> >> Anyway,
> >> 
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >> 
> >> on the whole.
> >> 
> >> I'm leaving it up to you, but personally I'd lean towards switching
> >> edp_notify_handler() to use wait_panel_power_cycle(intel_dp) first in a
> >> separate patch, to help with potential bisect results, and then doing
> >> the rest.
> >
> > I don't think it would be quite that simple. We'd have to also toss
> > in some combination of panel_off() and vdd_off_sync() in there,
> > depending on whether the panel power is currently enabled or not.
> > Otherwise the bookkeeping needed by wait_panel_power_cycle() isn't
> > going to be up to date.
> 
> Oh? So the calls via encoder hooks actually ensure that?

1. drm_atomic_helper_shutdown()
   -> panel_off() as needed via the normal crtc disable sequence
2. mst suspend + interrupts off + cancel hpd stuff
   -> hopefully nothing can re-enable vdd after this point
3. encoder.suspend()
   -> vdd_off_sync() to really turn vdd off if it's still lingering
4. encoder.shutdown()
   -> wait_panel_power_cycle()

Hopefully it even works like that :P

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx