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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 09 Dec 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 09 Dec 2015, Rob Clark <robdclark@gmail.com> wrote:
>> On Fri, Oct 16, 2015 at 2:54 PM, Rob Clark <robdclark@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?

>
>
> 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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel