Re: [PATCH] drm/i915/skl: implement DP Aux Mutex framework

From: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
To: "airlied@gmail.com" <airlied@gmail.com>,
	"jani.nikula@linux.intel.com" <jani.nikula@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915/skl: implement DP Aux Mutex framework
Date: Tue, 10 Nov 2015 15:31:56 +0000	[thread overview]
Message-ID: <1447169534.1401.17.camel@intel.com> (raw)
In-Reply-To: <CAPM=9tyZ=ooEyTYh-inpq2p=rcaQ_jcwwNdg1QGY=+FZq7erRw@mail.gmail.com>

When PSR is enablabled the Hardware takes control on AUX and use it to
control panel exit/entry states.

When validating PSR with automated tests grabbing CRC from sink we were
facing strange aux communication issues where aux were returning
message size read equal 0. And 0 is a forbidden message.

So, by using the HW mutex you block HW from using aux when we are going
to use here on the automated PSR tests.

There was a bug opened at fd.o regarding sink_crc failing on SKL when
PSR was enabled but I couldn't find it. But we have 3 different
solutions for this case where we are trying to read sink crc and aux
being used by Hardware:

1. Use intel_dp_dpcd_read_wake for all dpcd reads at sink_crc. 

2. Return -EBUSY when message size is zeroed so drm takes care of
reties.

3. Use HW mutex so we let HW know we are going to use the aux.

Either one is fine for me as long as we have a reliable sink crc on SKL
and KBL.

Well, regarding this patch itself it seems that maintainers would need
a better description, but for the content I'm sure it is right and I
tested it here so feel free to use:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Tue, 2015-11-10 at 21:14 +1000, Dave Airlie wrote:
> On 10 November 2015 at 20:51, Jani Nikula <
> jani.nikula@linux.intel.com> wrote:
> > On Tue, 10 Nov 2015, Wayne Boyer <wayne.boyer@intel.com> wrote:
> > > Beginning with SKL the DP Aux channel communication can be 
> > > protected
> > > using a built in H/W mutex.
> > > 
> > > This patch provides an initial implementation for using that 
> > > mutex.
> > > The use is currently limited to protecting the sink crc request 
> > > based
> > > on feedback from the H/W designers indicating that using the 
> > > mutex
> > > for all aux channel communication is not recommended.
> > 
> > Please explain in the commit message what benefits this brings, and 
> > why
> > it's a good idea. Just that something *can* be done in hardware 
> > does not
> > mean that we should do it.
> 
> I also have trouble understanding a mutex that only some things use. 
> Seems
> like a mutex needs to be used everywhere to provide locking.
> 
> 
> Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx