[RFC] drm/dp: move hw_mutex up the call stack
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Dec 9 08:25:55 PST 2015
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.
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 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