[RFC] drm/dp: move hw_mutex up the call stack

[Rob Clark]

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