[RFC] drm/dp: move hw_mutex up the call stack
Rob Clark
robdclark at gmail.com
Wed Dec 9 09:26:11 PST 2015
On Wed, Dec 9, 2015 at 11:25 AM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Wed, Dec 09, 2015 at 05:51:43PM +0200, Jani Nikula wrote:
>> On Wed, 09 Dec 2015, Rob Clark <robdclark at gmail.com> wrote:
>> > On Fri, Oct 16, 2015 at 2:54 PM, Rob Clark <robdclark at gmail.com> wrote:
>> >> 1) don't let other threads trying to bang on aux channel interrupt the
>> >> defer timeout/logic
>> >> 2) don't let other threads interrupt the i2c over aux logic
>> >>
>> >> ---
>> >> This wasn't actually fixing things w/ problematic monitor, but seems
>> >> like generally a good idea. At least AFAIU you shouldn't allow the
>> >> sequence of transfers for i2c over aux be interrupted but unrelated
>> >> chatter.
>> >
>> > So, I got confirmation today that this patch actually does fix things
>> > w/ a different problematic Dell monitor. So I think it actually is
>> > the right thing to do.
>>
>> Looking at drm_dp_dpcd_access and drm_dp_i2c_do_msg it seems clear to me
>> that not holding the mutex will screw up the delays in native and
>> i2c-over-aux defer handling by letting another caller on the aux.
>
> What do you mean? The retry counts are on the stack, so each caller
> has its own. There's no specified delay between retries in the spec.
>
> I think the DP spec is pretty clear that AUX messages can be interleaved
> any which way by the policy maker. So I don't think the patch is really
> correct as far as the spec goes. But I suppose in practice it's possible
> the sink gets confused by the constant ping-pong and in the end can't
> reply in time. Eg. maybe it might reset some internal counter on every
> AUX request and doesn't reply with success until the counter has cleared,
> which it never does since the other guy caused it to reset again. Or
> maybe the sink just keeps one transfer open at a time and keeps deferring
> the other one(s) until the first one has finished, meaning our retry
> counts would have to account for the fact that there's already some
> other message(s) still pending.
ok, I don't actually have access to the DP spec, so was unsure if this
was our bug or a sink bug. I think I should describe the patch as a
workaround for buggy sinks.
In the end, I think it is safer to assume the sink's are the product
of hw designers, and not trust them too much. :-P
> As far as i2c goes, I think there we can't intermingle i2c transactions
> from multiple sources since that might end up signalling STOPs on the
> i2c bus when it wasn't intended, or maybe the data would get mixed up
> between the two, etc. As far as the AUX transfers go, I think mixing in
> native transfers between the i2c-over-aux ones should be fine. They're
> all just AUX transfers so the same rules should apply as for native vs.
> native. But again, the sink might not like it anyway.
I think without moving the locking, there is the potential to
interleave i2c transactions. Although I don't think that was
happening in this particular case. It was looking more like aux+i2c
interleaving.
BR,
-R
> I guess real world wins in the end, and if the patch helps with stuff,
> we should go with it.
>
>>
>> However this is missing some aux->transfer to aux_transfer abstraction
>> prep patch; has it been sent to the list? With that addressed, r-b
>> follows.
>>
>>
>> BR,
>> Jani.
>>
>>
>>
>> >
>> > BR,
>> > -R
>> >
>> >> This applies on top of the DPCD logging patch.
>> >>
>> >> drivers/gpu/drm/drm_dp_helper.c | 27 +++++++++++++++++----------
>> >> 1 file changed, 17 insertions(+), 10 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> >> index ccb1f8a..e409b5f 100644
>> >> --- a/drivers/gpu/drm/drm_dp_helper.c
>> >> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> >> @@ -163,8 +163,6 @@ static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> >> {
>> >> ssize_t ret;
>> >>
>> >> - mutex_lock(&aux->hw_mutex);
>> >> -
>> >> DRM_DEBUG_DPCD("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
>> >> msg->request, msg->address, msg->size);
>> >>
>> >> @@ -196,8 +194,6 @@ static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> >> }
>> >> }
>> >>
>> >> - mutex_unlock(&aux->hw_mutex);
>> >> -
>> >> return ret;
>> >> }
>> >>
>> >> @@ -220,7 +216,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> {
>> >> struct drm_dp_aux_msg msg;
>> >> unsigned int retry;
>> >> - int err;
>> >> + int err = 0;
>> >>
>> >> memset(&msg, 0, sizeof(msg));
>> >> msg.address = offset;
>> >> @@ -228,6 +224,8 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> msg.buffer = buffer;
>> >> msg.size = size;
>> >>
>> >> + mutex_lock(&aux->hw_mutex);
>> >> +
>> >> /*
>> >> * The specification doesn't give any recommendation on how often to
>> >> * retry native transactions. We used to retry 7 times like for
>> >> @@ -241,18 +239,19 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> if (err == -EBUSY)
>> >> continue;
>> >>
>> >> - return err;
>> >> + goto unlock;
>> >> }
>> >>
>> >> switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
>> >> case DP_AUX_NATIVE_REPLY_ACK:
>> >> if (err < size)
>> >> - return -EPROTO;
>> >> - return err;
>> >> + err = -EPROTO;
>> >> + goto unlock;
>> >>
>> >> case DP_AUX_NATIVE_REPLY_NACK:
>> >> DRM_DEBUG_DPCD("native nack (result=%d, size=%zu)\n", err, msg.size);
>> >> - return -EIO;
>> >> + err = -EIO;
>> >> + goto unlock;
>> >>
>> >> case DP_AUX_NATIVE_REPLY_DEFER:
>> >> DRM_DEBUG_DPCD("native defer\n");
>> >> @@ -262,7 +261,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> }
>> >>
>> >> DRM_ERROR("DPCD: too many retries, giving up!\n");
>> >> - return -EIO;
>> >> + err = -EIO;
>> >> +
>> >> +unlock:
>> >> + mutex_unlock(&aux->hw_mutex);
>> >> + return err;
>> >> }
>> >>
>> >> /**
>> >> @@ -698,6 +701,8 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>> >>
>> >> memset(&msg, 0, sizeof(msg));
>> >>
>> >> + mutex_lock(&aux->hw_mutex);
>> >> +
>> >> for (i = 0; i < num; i++) {
>> >> msg.address = msgs[i].addr;
>> >> msg.request = (msgs[i].flags & I2C_M_RD) ?
>> >> @@ -741,6 +746,8 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>> >> msg.size = 0;
>> >> (void)drm_dp_i2c_do_msg(aux, &msg);
>> >>
>> >> + mutex_unlock(&aux->hw_mutex);
>> >> +
>> >> return err;
>> >> }
>> >>
>> >> --
>> >> 2.5.0
>> >>
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
More information about the dri-devel
mailing list