Re: [PATCH] drm/i915/bxt: Save/restore HOTPLUG_CTL during suspend/resume.

From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/bxt: Save/restore HOTPLUG_CTL during suspend/resume.
Date: Mon, 15 Feb 2016 17:41:29 +0100	[thread overview]
Message-ID: <20160215164129.GS11240@phenom.ffwll.local> (raw)
In-Reply-To: <87powc3m0y.fsf@intel.com>

On Thu, Feb 04, 2016 at 10:41:33AM +0200, Jani Nikula wrote:
> On Thu, 04 Feb 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Thu, Feb 04, 2016 at 07:17:08AM +0530, Thulasimani, Sivakumar wrote:
> >> 
> >> 
> >> On 2/4/2016 6:19 AM, Matt Roper wrote:
> >> >From: Bob Paauwe <bob.j.paauwe@intel.com>
> >> >
> >> >Broxton has some additional bits in the HOTPLUG_CTL register that
> >> >indicate whether the HPD sense lines need to be inverted or not for the
> >> >current platform.  The BIOS sets these bits to an appropriate value at
> >> >boot time, but the value is lost across suspend/resume.  We need to save
> >> >and restore the register so that hotplug and display detect works on
> >> >resume.
> >> i have a patch that is about to be upstreamed that will read and write these
> >> values based on vbt. Shuhangi did the basic testing last week so will
> >> ask her to send to mail list today.  i would prefer that patch where
> >> we know how and when to set these bits rather than just save
> >> & restore.
> >
> > Sure, sounds good.  Can your patch handle cases where there is no VBT by
> > falling back to a save/restore?  Quite often in the embedded world, we
> > have very specialized boot firmware that doesn't resemble vbios/gop and
> > doesn't have any VBT info.
> 
> Please no blind save/restore of registers anywhere.
> 
> We can (and in many places do) read the BIOS set values for defaults at
> driver load, but preferably should also survive without. If there's
> anything you really need enabled, it should be stored in the appropriate
> driver data structures, and the registers should be fully initialized
> when needed.
> 
> Looks like there's still plenty of read-modify-write in
> *_hpd_irq_setup(), but the right approach would be to ensure
> bxt_hpd_irq_setup() does the right thing for you. Either based on sane
> defaults or VBT.

Fully concurred. The safe/restore stuff for byt/chv/bxt is really not a
part I like, and if we already have code to set up hpd bits (hpd_setup et
al), then please don't proliferate safe/restore hacks. They're way too
fragile, it's pretty much impossible to get ordering right and as soon as
they need to interact with other setup functions you're landing in rmw
hell.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx