Re: [PATCH 4/5] drm/i915: Check live status before reading edid

From: Daniel Vetter <daniel@ffwll.ch>
To: Daniel Vetter <daniel@ffwll.ch>,
	Sonika Jindal <sonika.jindal@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 4/5] drm/i915: Check live status before reading edid
Date: Wed, 13 Jul 2016 13:59:52 +0200	[thread overview]
Message-ID: <20160713115952.GF23520@phenom.ffwll.local> (raw)
In-Reply-To: <20160712113947.GB27260@boom>

On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote:
> On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote:
> > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote:
> > > Adding this for SKL onwards.
> > > 
> > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > 
> > I think this is the critical piece really, and I'd like to roll it out for
> > all platforms. git has the code for all the old ones, so no big deal to
> > digg that out. Also we need your fix for the interrupt handling first ofc,
> > otherwise this won't work.
> > 
> > Then we can apply this and give it some good testing before we start
> > relying on it with caching hdmi probe status. I know this means that
> > things will be slower, but I've been burned a bit too much the last few
> > times we've tried this. And I really think we need the most amount of
> > testing we can get (hence rolling this out for all platforms). If your
> > hpd irq handling bugfix is indeed the bug that will be confirmed quickly,
> > otherwise it means back to debugging.
> 
> "things will be slower" is a bit of an understatement, sadly.
> On machines with no external display connected (the typical usecase for
> any device with an eDP, such as a laptop, tablet, etc.), the approach to
> repeat live status reads until buggy displays can be bothered to reply
> makes us spend twice as long as needed on resume.
> 
> While it's nice that we can support buggy hardware, I think that we
> also, at the very least, should allow for skipping said those
> workarounds when not needed. Either by allow for disabling the
> lvie status reads (proven to work on older platforms since that was
> the default behaviour for a long, long time, and tested to work
> on at least Broadwell & Skylake ThinkPads) or by making the number of
> LSR reads configurable.

Nack on making probing configurable, that just plain doesn't work. It's
the same hole war I've been fighting against adding the live status check
until vpg realized that they have this hack to make broken sinks work.

> The former would be the simplest solution, the latter would meet the
> letter of the spec, and allow for more precise tuning of behaviour;
> people who have a display that needs -- say -- for LSR reads to work
> reliably shouldn't have to wait for 9.
> 
> While allowing for unusual use-cases / buggy hardware is great,
> we shouldn't miss out on the benefits that  working hardware can
> give to the common use-cases.
> 
> Just my 2¢.
> 
> I'm feeling sorely tempted to treat this as a proper regression,
> and simply submit a revert patch, seeing as it slows down resume by
> something like 200ms, but seeing as there was mention of a case where
> incorrect EDID-information might end up being read after resume on some
> Chromebooks, I'll just try to open a discussion instead.

+1 on reverts from me on principle. No opinion on this case specifically,
but given that we fail the suspend/resume limits since forever I'm
inclined to say "meh" here.

Probably we should add this as a basic/BAT performance metric, but atm
igt/BAT/Jenkins aren't set up to track performance measurements and
automatically report/bisect regressions.

I think the proper solution should be to make all the probing and fbdev
restoring async. As soon as we have working async atomic commit for
modeset we should be able to do all that.
-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