* [RFC] drm/dp: move hw_mutex up the call stack @ 2015-10-16 18:54 Rob Clark 2015-12-09 15:34 ` Rob Clark 0 siblings, 1 reply; 9+ messages in thread From: Rob Clark @ 2015-10-16 18:54 UTC (permalink / raw) To: dri-devel 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. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-10-16 18:54 [RFC] drm/dp: move hw_mutex up the call stack Rob Clark @ 2015-12-09 15:34 ` Rob Clark 2015-12-09 15:51 ` Jani Nikula 0 siblings, 1 reply; 9+ messages in thread From: Rob Clark @ 2015-12-09 15:34 UTC (permalink / raw) To: dri-devel@lists.freedesktop.org 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. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 15:34 ` Rob Clark @ 2015-12-09 15:51 ` Jani Nikula 2015-12-09 15:53 ` Jani Nikula 2015-12-09 16:25 ` Ville Syrjälä 0 siblings, 2 replies; 9+ messages in thread From: Jani Nikula @ 2015-12-09 15:51 UTC (permalink / raw) To: Rob Clark, dri-devel@lists.freedesktop.org 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. 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 15:51 ` Jani Nikula @ 2015-12-09 15:53 ` Jani Nikula 2015-12-09 16:00 ` Rob Clark 2015-12-09 16:25 ` Ville Syrjälä 1 sibling, 1 reply; 9+ messages in thread From: Jani Nikula @ 2015-12-09 15:53 UTC (permalink / raw) To: Rob Clark, dri-devel@lists.freedesktop.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 15:53 ` Jani Nikula @ 2015-12-09 16:00 ` Rob Clark 0 siblings, 0 replies; 9+ messages in thread From: Rob Clark @ 2015-12-09 16:00 UTC (permalink / raw) To: Jani Nikula; +Cc: dri-devel@lists.freedesktop.org On Wed, Dec 9, 2015 at 10:53 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > 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? 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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 15:51 ` Jani Nikula 2015-12-09 15:53 ` Jani Nikula @ 2015-12-09 16:25 ` Ville Syrjälä 2015-12-09 17:26 ` Rob Clark 1 sibling, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2015-12-09 16:25 UTC (permalink / raw) To: Jani Nikula; +Cc: dri-devel@lists.freedesktop.org On Wed, Dec 09, 2015 at 05:51:43PM +0200, Jani Nikula 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. 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@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 -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 16:25 ` Ville Syrjälä @ 2015-12-09 17:26 ` Rob Clark 2015-12-09 19:37 ` Ville Syrjälä 0 siblings, 1 reply; 9+ messages in thread From: Rob Clark @ 2015-12-09 17:26 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel@lists.freedesktop.org On Wed, Dec 9, 2015 at 11:25 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Dec 09, 2015 at 05:51:43PM +0200, Jani Nikula 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. > > 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 > 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. Although I don't think that was happening in this particular case. It was looking more like aux+i2c interleaving. 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@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 > > -- > Ville Syrjälä > Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 17:26 ` Rob Clark @ 2015-12-09 19:37 ` Ville Syrjälä 2015-12-09 20:03 ` Rob Clark 0 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2015-12-09 19:37 UTC (permalink / raw) To: Rob Clark; +Cc: dri-devel@lists.freedesktop.org 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@linux.intel.com> wrote: > > On Wed, Dec 09, 2015 at 05:51:43PM +0200, Jani Nikula 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. > > > > 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@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 > > > > -- > > Ville Syrjälä > > Intel OTC -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm/dp: move hw_mutex up the call stack 2015-12-09 19:37 ` Ville Syrjälä @ 2015-12-09 20:03 ` Rob Clark 0 siblings, 0 replies; 9+ messages in thread From: Rob Clark @ 2015-12-09 20:03 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel@lists.freedesktop.org On Wed, Dec 9, 2015 at 2:37 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > 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@linux.intel.com> wrote: >> > On Wed, Dec 09, 2015 at 05:51:43PM +0200, Jani Nikula 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. >> > >> > 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? well, on a *different* monitor (when I originally wrote the dpcd logging and this patch), the end result of higher retry count was that it would just take longer to give up (and meanwhile the UI was hung, and from user perspective the machine was locked up) (that said, that particular monitor eventually started working after a power outage.. so I guess it just needed to be unplugged long enough for it's internal state to reset.. did I mention I have little confidence that sinks are actually compliant, beyond what is required to make the windows driver work?) BR, -R > 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@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 >> > >> > -- >> > Ville Syrjälä >> > Intel OTC > > -- > Ville Syrjälä > Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-12-09 20:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-16 18:54 [RFC] drm/dp: move hw_mutex up the call stack Rob Clark 2015-12-09 15:34 ` Rob Clark 2015-12-09 15:51 ` Jani Nikula 2015-12-09 15:53 ` Jani Nikula 2015-12-09 16:00 ` Rob Clark 2015-12-09 16:25 ` Ville Syrjälä 2015-12-09 17:26 ` Rob Clark 2015-12-09 19:37 ` Ville Syrjälä 2015-12-09 20:03 ` Rob Clark
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.