On Wed, 09 Dec 2015, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> 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. > > 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@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