On Wed, Dec 09, 2015 at 12:26:11PM -0500, Rob Clark wrote:
> On Wed, Dec 9, 2015 at 11:25 AM, Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Wed, Dec 09, 2015 at 05:51:43PM +0200, Jani Nikula wrote:
> >> On Wed, 09 Dec 2015, Rob Clark <robdclark@xxxxxxxxx> wrote:
> >> > On Fri, Oct 16, 2015 at 2:54 PM, Rob Clark <robdclark@xxxxxxxxx> 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
Yeah, in practice the spec is more what you call guidelines than actual
rules :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.
Yep. We'd need something to lock out the i2c at least. And if we anyway
need this for native AUX as well, there's little point in adding any i2c
specific locks.
> Although I don't think that was
> happening in this particular case. It was looking more like aux+i2c
> interleaving.
I'm curious if bumping the retry counts to some ridiculously high number
would help with your problematic setup. Did you try that by any chance?
Although I suppose it's possible that the sink just drops any ongoing
i2c transfer to the floor when you bang on the door.
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
