| Message ID | 1447288860-2498-1-git-send-email-wayne.boyer@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"aux mutex framework" in the title sounds a bit grandiose, to be honest. "add support for DP AUX hardware mutex" is what I would have used. Further comments inline. On Thu, 12 Nov 2015, Wayne Boyer <wayne.boyer@intel.com> wrote: > From: "Boyer, Wayne" <wayne.boyer@intel.com> > > Beginning with SKL the DP Aux channel communication can be protected > using a built in HW mutex. > > When PSR is enabled the HW takes control on AUX and uses it to > control panel exit/entry states. > > When validating PSR with automated tests, grabbing CRC from sink > revealed strange aux communication issues. Aux reads were returning > a message read size equal to 0 and 0 is a forbidden message. > > By using the HW mutex the HW is blocked from using aux when running > the automated PSR tests. > > 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. > > v2: Improved commit message to explain the case where the HW mutex is > helpful. Also added bug reference. > v3: Fix typos in commit message. > > Signed-off-by: Wayne Boyer <wayne.boyer@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91437 > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 5 ++++ > drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index d5cf30b..ac7ed0d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2585,6 +2585,7 @@ struct drm_i915_cmd_table { > > #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi) > #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) > +#define HAS_AUX_MUTEX(dev) (INTEL_INFO(dev)->gen >= 9) > #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ > IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \ > IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 8bd2699..f9ee874 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4288,6 +4288,11 @@ enum skl_disp_power_wells { > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5) > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1) > > +#define DP_AUX_MUTEX_A 0x6402C > +#define DP_AUX_MUTEX_B 0x6412C Please add all of these at once, using an underscore prefix, and add DP_AUX_MUTEX(port) to pick the right one. Plenty of examples in the file. And then Ville doesn't have to clean it up afterwards. > +#define DP_AUX_MUTEX_ENABLE (1 << 31) > +#define DP_AUX_MUTEX_STATUS (1 << 30) > + > /* > * Computing GMCH M and N values for the Display Port link > * > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index da02ed7..b3c7d82 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -781,6 +781,47 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > } > > +static bool skl_aux_mutex(struct intel_dp *intel_dp, bool get) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + uint32_t aux_ch_mutex, status; > + int count = 0; > + > + if (!HAS_AUX_MUTEX(dev)) > + return false; > + > + /* > + * FIXME: determine actual aux channel > + * Hard coded to channel A for now to protect sink crc requests on eDP. > + */ If you want to leave it like this for now, the please add "|| !is_edp()" alongside !HAS_AUX_MUTEX() for an early return. > + aux_ch_mutex = DP_AUX_MUTEX_A; > + > + if (!get) { > + I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE | DP_AUX_MUTEX_STATUS); > + return false; > + } > + > + /* > + * The Bspec specifies waiting 500us between attempts to acquire the > + * mutex. Ten retries should be adequate to balance successfully > + * acquirng the mutex and spending too much time trying. > + */ > + while (count++ < 10) { Please use a for loop whenever you can use a for loop. The reader has to pause for a second to ensure "while (count++ < 10)" does the right thing and count is initialized properly; "for (i = 0; i < 10; i++)" comes from the spine. > + I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE); > + status = I915_READ(aux_ch_mutex); > + if (!(status & DP_AUX_MUTEX_STATUS)) > + return true; > + udelay(500); > + } You should probably add an error or a debug message here. I observe that the caller has no way of knowing whether the lock was not acquired because the platform doesn't support it, or because we failed to acquire it. I would imagine the callers might want to opt for not proceeding to mess with the hardware if they can't ensure exclusive access. That's kind of the point of locking. > + > + return false; > +} > + > +#define skl_aux_mutex_get(dev) skl_aux_mutex(dev, true) > +#define skl_aux_mutex_put(dev) skl_aux_mutex(dev, false) Please have two separate functions instead of this indirection. Name them _lock and _unlock; _get and _put imply support for refcounting/nesting, which isn't there. > + > static int > intel_dp_aux_ch(struct intel_dp *intel_dp, > const uint8_t *send, int send_bytes, > @@ -3927,10 +3968,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > u8 buf; > int count, ret; > int attempts = 6; > + bool aux_mutex_acquired = false; Unnecessary initialization. > + > + aux_mutex_acquired = skl_aux_mutex_get(intel_dp); > > ret = intel_dp_sink_crc_start(intel_dp); > + Unwanted whitespace change. > if (ret) > - return ret; > + goto release; > > do { > intel_wait_for_vblank(dev, intel_crtc->pipe); > @@ -3957,6 +4002,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > > stop: > intel_dp_sink_crc_stop(intel_dp); > + > +release: > + if (aux_mutex_acquired) > + aux_mutex_acquired = skl_aux_mutex_put(intel_dp); No need to do an assignment here. BR, Jani. > + > return ret; > }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d5cf30b..ac7ed0d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2585,6 +2585,7 @@ struct drm_i915_cmd_table { #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi) #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg) +#define HAS_AUX_MUTEX(dev) (INTEL_INFO(dev)->gen >= 9) #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \ IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8bd2699..f9ee874 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4288,6 +4288,11 @@ enum skl_disp_power_wells { #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5) #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1) +#define DP_AUX_MUTEX_A 0x6402C +#define DP_AUX_MUTEX_B 0x6412C +#define DP_AUX_MUTEX_ENABLE (1 << 31) +#define DP_AUX_MUTEX_STATUS (1 << 30) + /* * Computing GMCH M and N values for the Display Port link * diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index da02ed7..b3c7d82 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -781,6 +781,47 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); } +static bool skl_aux_mutex(struct intel_dp *intel_dp, bool get) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + uint32_t aux_ch_mutex, status; + int count = 0; + + if (!HAS_AUX_MUTEX(dev)) + return false; + + /* + * FIXME: determine actual aux channel + * Hard coded to channel A for now to protect sink crc requests on eDP. + */ + aux_ch_mutex = DP_AUX_MUTEX_A; + + if (!get) { + I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE | DP_AUX_MUTEX_STATUS); + return false; + } + + /* + * The Bspec specifies waiting 500us between attempts to acquire the + * mutex. Ten retries should be adequate to balance successfully + * acquirng the mutex and spending too much time trying. + */ + while (count++ < 10) { + I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE); + status = I915_READ(aux_ch_mutex); + if (!(status & DP_AUX_MUTEX_STATUS)) + return true; + udelay(500); + } + + return false; +} + +#define skl_aux_mutex_get(dev) skl_aux_mutex(dev, true) +#define skl_aux_mutex_put(dev) skl_aux_mutex(dev, false) + static int intel_dp_aux_ch(struct intel_dp *intel_dp, const uint8_t *send, int send_bytes, @@ -3927,10 +3968,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) u8 buf; int count, ret; int attempts = 6; + bool aux_mutex_acquired = false; + + aux_mutex_acquired = skl_aux_mutex_get(intel_dp); ret = intel_dp_sink_crc_start(intel_dp); + if (ret) - return ret; + goto release; do { intel_wait_for_vblank(dev, intel_crtc->pipe); @@ -3957,6 +4002,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) stop: intel_dp_sink_crc_stop(intel_dp); + +release: + if (aux_mutex_acquired) + aux_mutex_acquired = skl_aux_mutex_put(intel_dp); + return ret; }