On Wed, Dec 9, 2015 at 10:53 AM, Jani Nikula
<jani.nikula at linux.intel.com> wrote:
> On Wed, 09 Dec 2015, Jani Nikula <jani.nikula at linux.intel.com> 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.
>>
>> 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.
>
> Doh, you say it right below. This smells like cc: stable material, so
> would you mind respinning on top of current upstream?
Yeah, will swap the order and resend later today (so dpcd logging
patch is on top of this one)..
I did send a v2 of the 'dpcd logging' patch a while back, but don't
remember seeing any comments.. I think that would be a useful thing to
get merged too (although perhaps not really stable material.. but I
found it quite useful for debugging). IIRC you did have one comment
about driver's implementation of transfer fxn.. which I kind of
ignored that (since the point was to separate dpcd logging from other
debug msgs, so we could debug tricky monitors/hubs/etc).
BR,
-R
>>
>>
>> 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
