Re: [PATCH v2 4/4] drm/dp: Allow registering AUX channels as I2C busses

From: Thierry Reding <thierry.reding@gmail.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 4/4] drm/dp: Allow registering AUX channels as I2C busses
Date: Fri, 20 Dec 2013 10:13:20 +0100	[thread overview]
Message-ID: <20131220091319.GB27787@ulmo.nvidia.com> (raw)
In-Reply-To: <87fvpr1jye.fsf@intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 2243 bytes --]

On Tue, Dec 17, 2013 at 07:11:21PM +0200, Jani Nikula wrote:
> On Tue, 17 Dec 2013, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > +		case DP_AUX_NATIVE_REPLY_DEFER:
> > +			/*
> > +			 * We could check for I2C bitrate capabilities and if
> > +			 * available adjust this interval. We could also be
> > +			 * more careful with DP-to-legacy adapters where a
> > +			 * long legacy cable may forc very low I2C bit rates.
>                                                  ^^^^ force

Fixed.

> > +			 * For now just defer for long enough to hopefully be
> > +			 * safe for all use-cases.
> > +			 */
> > +			usleep_range(500, 600);
> 
> I've banged my head against the wall a bit with this (the original i915
> comment similar to the above was mine) and I'd be hesitant to switch to
> this as-is in i915.
> 
> First, I'd like to retain the DRM_DEBUG_KMS messages all around, as
> otherwise issues here will be hard to debug. It's a pain to have to ask
> to patch the kernel just get the debug info. At least we've had a fair
> share of DP aux issues in our driver.

Okay, I'll add those back in. I'll assume the same goes for DRM_ERROR
messages.

> Second, I fear one size does not fit all wrt the delays. What would you
> say about making all the DP_AUX_NATIVE_REPLY_DEFER and
> DP_AUX_I2C_REPLY_DEFER checks do callbacks if defined, and fallback to
> the default delays otherwise? I think there'll be some more churn in the
> error handling, particularly for the more exotic sinks, and IMO having
> them per driver would make development and bug fixing faster. I'm not
> saying you need to do this now, it can be a follow-up; just asking what
> you'd think of it.

Sure, we should be able to easily do that. One possible alternative to
this would be to handle in in the drivers by handling DEFER explicitly
and then returning -EBUSY. -EBUSY will cause native AUX transactions to
be retried and I just noticed that radeon does the same for I2C-over-AUX
transactions, so I'll add that special case there as well.

The benefit of doing that would be that the helper stays relatively
simple. On the downside it may not be as explicit as a driver-specific
callback.

Thanks,
Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel