Re: [PATCH] drm/i915: Use dpcd read wake for sink crc calls.

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"vivijim@rdvivi-budapest.jf.intel.com"
	<vivijim@rdvivi-budapest.jf.intel.com>
Subject: Re: [PATCH] drm/i915: Use dpcd read wake for sink crc	calls.
Date: Thu, 27 Aug 2015 13:45:56 +0300	[thread overview]
Message-ID: <87zj1d6mcb.fsf@intel.com> (raw)
In-Reply-To: <CAKMK7uGSnQjDwK_dJtDnE_s9MwUPhpjXCJb66q45HETzRjfMhA@mail.gmail.com>

On Thu, 27 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 26, 2015 at 6:41 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
>> On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote:
>>> On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote:
>>> > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
>>> >
>>> > Let's use a native read with retry as suggested per spec to
>>> > fix Sink CRC on SKL when PSR is enabled.
>>> >
>>> > With PSR enabled panel is probably taking more time to wake
>>> > and dpcd read is faling.
>>> >
>>> > Cc: Sonika Jindal <sonika.jindal@intel.com>
>>> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com>
>>>
>>> Seems like we should just move the trickery we do in our own version
>>> into
>>> the dp helpers in the core if this is needed all over the place?
>>
>> I've wondered this, but I thought there was a good reason to let this
>> trick separated.
>
> I think in general you can assume that if i915 dp sink handling is
> special it's because we have more testing on various broken hw out
> there.

In truth our inconsistent use of wake vs. non-wake can be mostly
attributed to the fact that we're clueless about sink sleep states, and
we've just added more wakes here and there to paper over it.

BR,
Jani.

>
>>> At least in i915 we use it everywhere and it doesn't seem actively
>>> harmful
>>> really ... Maybe the only exception would be the i2c-over-dp_aux
>>> code.
>>
>> Why this would be the exception? Maybe this was the good reason?
>
> I'd be fairly easy to keep an internal __drm_dp_aux_read (need it
> anyway to implement this trick) and use that in i2c. At least that's
> what I'd do without any evidence that we need to make this wake dance
> also for i2c transactions. i2c uses a special dp-aux mode on the wire,
> so makes some sense if it's different. See also the recent work from
> Ville to tune the i2c dp-aux timeouts and retries, it really seems to
> be a world of its own a bit.
> -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