* Radeon regression fix @ 2011-09-29 14:21 Brad Campbell 2011-09-29 14:36 ` Alex Deucher 0 siblings, 1 reply; 8+ messages in thread From: Brad Campbell @ 2011-09-29 14:21 UTC (permalink / raw) To: alexdeucher; +Cc: airlied, dri-devel, linux-kernel This patch fixes a regression introduced between 2.6.39 & 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER response. Signed-off-by: Brad Campbell <brad@fnarfbargle.com> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..b8450f4 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -60,11 +60,13 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, int index = GetIndexIntoMasterTable(COMMAND, ProcessAuxChannelTransaction); unsigned char *base; int recv_bytes; + int retry_count = 0; memset(&args, 0, sizeof(args)); base = (unsigned char *)rdev->mode_info.atom_context->scratch; +retry: memcpy(base, send, send_bytes); args.v1.lpAuxRequest = 0; @@ -79,6 +81,16 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, *ack = args.v1.ucReplyStatus; + /* defer */ + if (args.v1.ucReplyStatus == 0x20){ + DRM_DEBUG_KMS("dp_aux_ch defer\n"); + /* 10 is an arbitrary value from the pre-regression patch + in practice I've never seen more than one */ + if (retry_count++ < 10) + goto retry; + return -EBUSY; + } + /* timeout */ if (args.v1.ucReplyStatus == 1) { DRM_DEBUG_KMS("dp_aux_ch timeout\n"); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-29 14:21 Radeon regression fix Brad Campbell @ 2011-09-29 14:36 ` Alex Deucher 2011-09-29 15:10 ` Brad Campbell 2011-09-29 15:21 ` Brad Campbell 0 siblings, 2 replies; 8+ messages in thread From: Alex Deucher @ 2011-09-29 14:36 UTC (permalink / raw) To: Brad Campbell; +Cc: airlied, dri-devel, linux-kernel On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbell <brad@fnarfbargle.com> wrote: > This patch fixes a regression introduced between 2.6.39 & 3.1-rc1 whereby > the displayport AUX channel stopped re-trying commands that elicited a DEFER > response. > It should still be retrying, just restructured slightly. The retry logic just moved into radeon_dp_i2c_aux_ch(), radeon_dp_aux_native_write(), and radeon_dp_aux_native_read(), e.g., else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) udelay(400); Perhaps the delay is causing a problem. Does removing the udelay(400); help? Alex > Signed-off-by: Brad Campbell <brad@fnarfbargle.com> > > diff --git a/drivers/gpu/drm/radeon/atombios_dp.c > b/drivers/gpu/drm/radeon/atombios_dp.c > index 7ad43c6..b8450f4 100644 > --- a/drivers/gpu/drm/radeon/atombios_dp.c > +++ b/drivers/gpu/drm/radeon/atombios_dp.c > @@ -60,11 +60,13 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan > *chan, > int index = GetIndexIntoMasterTable(COMMAND, > ProcessAuxChannelTransaction); > unsigned char *base; > int recv_bytes; > + int retry_count = 0; > > memset(&args, 0, sizeof(args)); > > base = (unsigned char *)rdev->mode_info.atom_context->scratch; > > +retry: > memcpy(base, send, send_bytes); > > args.v1.lpAuxRequest = 0; > @@ -79,6 +81,16 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan > *chan, > > *ack = args.v1.ucReplyStatus; > > + /* defer */ > + if (args.v1.ucReplyStatus == 0x20){ > + DRM_DEBUG_KMS("dp_aux_ch defer\n"); > + /* 10 is an arbitrary value from the pre-regression > patch > + in practice I've never seen more than one */ > + if (retry_count++ < 10) > + goto retry; > + return -EBUSY; > + } > + > /* timeout */ > if (args.v1.ucReplyStatus == 1) { > DRM_DEBUG_KMS("dp_aux_ch timeout\n"); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-29 14:36 ` Alex Deucher @ 2011-09-29 15:10 ` Brad Campbell 2011-09-29 15:21 ` Brad Campbell 1 sibling, 0 replies; 8+ messages in thread From: Brad Campbell @ 2011-09-29 15:10 UTC (permalink / raw) To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel On 29/09/11 22:36, Alex Deucher wrote: > On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbell<brad@fnarfbargle.com> wrote: >> This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby >> the displayport AUX channel stopped re-trying commands that elicited a DEFER >> response. >> > It should still be retrying, just restructured slightly. The retry > logic just moved into radeon_dp_i2c_aux_ch(), > radeon_dp_aux_native_write(), and radeon_dp_aux_native_read(), e.g., > > else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) > udelay(400); One problem with that logic I'm afraid. if (ret == 0) return -EPROTO; if (ret < 0) return ret; if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) return ret; else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) udelay(400); else return -EIO; } ret == 0 with a defer as there is no data in the packet. It never even gets past the first hurdle. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-29 14:36 ` Alex Deucher 2011-09-29 15:10 ` Brad Campbell @ 2011-09-29 15:21 ` Brad Campbell 2011-09-30 0:23 ` Brad Campbell 1 sibling, 1 reply; 8+ messages in thread From: Brad Campbell @ 2011-09-29 15:21 UTC (permalink / raw) To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel On 29/09/11 22:36, Alex Deucher wrote: > On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbell<brad@fnarfbargle.com> wrote: >> This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby >> the displayport AUX channel stopped re-trying commands that elicited a DEFER >> response. >> > > It should still be retrying, just restructured slightly. The retry > logic just moved into radeon_dp_i2c_aux_ch(), > radeon_dp_aux_native_write(), and radeon_dp_aux_native_read(), e.g., > > else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) > udelay(400); > > Perhaps the delay is causing a problem. Does removing the udelay(400); help? How's this look ? diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..aa5624a 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -128,12 +128,14 @@ static int radeon_dp_aux_native_write(struct radeon_connector *radeon_connector, while (1) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, NULL, 0, delay, &ack); + if (ret == 0 && ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)){ + udelay(400); + continue; + } if (ret < 0) return ret; if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) break; - else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) - udelay(400); else return -EIO; } @@ -158,14 +160,16 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector, while (1) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay, &ack); + if (ret == 0 && ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)){ + udelay(400); + continue; + } if (ret == 0) return -EPROTO; if (ret < 0) return ret; if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) return ret; - else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) - udelay(400); else return -EIO; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-29 15:21 ` Brad Campbell @ 2011-09-30 0:23 ` Brad Campbell 2011-09-30 4:59 ` Alex Deucher 0 siblings, 1 reply; 8+ messages in thread From: Brad Campbell @ 2011-09-30 0:23 UTC (permalink / raw) To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel On 29/09/11 23:21, Brad Campbell wrote: > On 29/09/11 22:36, Alex Deucher wrote: >> On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbell<brad@fnarfbargle.com> >> wrote: >>> This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby >>> the displayport AUX channel stopped re-trying commands that elicited >>> a DEFER >>> response. >>> >> >> It should still be retrying, just restructured slightly. The retry >> logic just moved into radeon_dp_i2c_aux_ch(), >> radeon_dp_aux_native_write(), and radeon_dp_aux_native_read(), e.g., >> >> else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) >> udelay(400); >> >> Perhaps the delay is causing a problem. Does removing the udelay(400); >> help? Looking at it with a nights sleep, it's obvious the code path in aux_native_write is ok. Is this a bit cleaner than the last patch? diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..aacc97d 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -158,14 +158,17 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector, while (1) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay, &ack); - if (ret == 0) + if (ret == 0){ + if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER){ + udelay(400); + continue; + } return -EPROTO; + } if (ret < 0) return ret; if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) return ret; - else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) - udelay(400); else return -EIO; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-30 0:23 ` Brad Campbell @ 2011-09-30 4:59 ` Alex Deucher 2011-09-30 5:14 ` Brad Campbell 0 siblings, 1 reply; 8+ messages in thread From: Alex Deucher @ 2011-09-30 4:59 UTC (permalink / raw) To: Brad Campbell; +Cc: airlied, dri-devel, linux-kernel On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell <lists2009@fnarfbargle.com> wrote: > On 29/09/11 23:21, Brad Campbell wrote: >> >> On 29/09/11 22:36, Alex Deucher wrote: >>> >>> On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbell<brad@fnarfbargle.com> >>> wrote: >>>> >>>> This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby >>>> the displayport AUX channel stopped re-trying commands that elicited >>>> a DEFER >>>> response. >>>> >>> >>> It should still be retrying, just restructured slightly. The retry >>> logic just moved into radeon_dp_i2c_aux_ch(), >>> radeon_dp_aux_native_write(), and radeon_dp_aux_native_read(), e.g., >>> >>> else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) >>> udelay(400); >>> >>> Perhaps the delay is causing a problem. Does removing the udelay(400); >>> help? > > Looking at it with a nights sleep, it's obvious the code path in > aux_native_write is ok. Is this a bit cleaner than the last patch? Looks pretty good. I was thinking of something more like this (sorry for the lack of a patch, I'm away from my source trees at the moment): while (1) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay, &ack); if (ret < 0) return ret; if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) return ret; else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) udelay(400); else if (ret == 0) return -EPROTO; else return -EIO; } Thanks for tracking this down. Alex > > diff --git a/drivers/gpu/drm/radeon/atombios_dp.c > b/drivers/gpu/drm/radeon/atombios_dp.c > index 7ad43c6..aacc97d 100644 > --- a/drivers/gpu/drm/radeon/atombios_dp.c > +++ b/drivers/gpu/drm/radeon/atombios_dp.c > @@ -158,14 +158,17 @@ static int radeon_dp_aux_native_read(struct > radeon_connector *radeon_connector, > while (1) { > ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, > msg, msg_bytes, recv, recv_bytes, > delay, &ack); > - if (ret == 0) > + if (ret == 0){ > + if ((ack & AUX_NATIVE_REPLY_MASK) == > AUX_NATIVE_REPLY_DEFER){ > + udelay(400); > + continue; > + } > return -EPROTO; > + } > if (ret < 0) > return ret; > if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) > return ret; > - else if ((ack & AUX_NATIVE_REPLY_MASK) == > AUX_NATIVE_REPLY_DEFER) > - udelay(400); > else > return -EIO; > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-30 4:59 ` Alex Deucher @ 2011-09-30 5:14 ` Brad Campbell 2011-09-30 5:39 ` Alex Deucher 0 siblings, 1 reply; 8+ messages in thread From: Brad Campbell @ 2011-09-30 5:14 UTC (permalink / raw) To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel On 30/09/11 12:59, Alex Deucher wrote: > On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell >> Looking at it with a nights sleep, it's obvious the code path in >> aux_native_write is ok. Is this a bit cleaner than the last patch? > > Looks pretty good. I was thinking of something more like this (sorry > for the lack of a patch, I'm away from my source trees at the moment): > > while (1) { > ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, > msg, msg_bytes, recv, recv_bytes, delay,&ack); > > if (ret< 0) > return ret; > if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) > return ret; > else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) > udelay(400); > else if (ret == 0) > return -EPROTO; > else > return -EIO; > } Yep, that looks cleaner. My only thought was the pre-3.0 code had a limit to the number of retries. Was that for a specific reason or is it ok to attempt to retry indefinitely if we receive a DEFER ? > > Thanks for tracking this down. No worries. I learned quite a bit about some kernel internals and more than a few quirks about apple hardware while muddling around. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix 2011-09-30 5:14 ` Brad Campbell @ 2011-09-30 5:39 ` Alex Deucher 0 siblings, 0 replies; 8+ messages in thread From: Alex Deucher @ 2011-09-30 5:39 UTC (permalink / raw) To: Brad Campbell; +Cc: airlied, dri-devel, linux-kernel On Fri, Sep 30, 2011 at 1:14 AM, Brad Campbell <lists2009@fnarfbargle.com> wrote: > On 30/09/11 12:59, Alex Deucher wrote: >> >> On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell > >>> Looking at it with a nights sleep, it's obvious the code path in >>> aux_native_write is ok. Is this a bit cleaner than the last patch? >> >> Looks pretty good. I was thinking of something more like this (sorry >> for the lack of a patch, I'm away from my source trees at the moment): >> >> while (1) { >> ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, >> msg, msg_bytes, recv, >> recv_bytes, delay,&ack); >> >> if (ret< 0) >> return ret; >> if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) >> return ret; >> else if ((ack& AUX_NATIVE_REPLY_MASK) == >> AUX_NATIVE_REPLY_DEFER) >> udelay(400); >> else if (ret == 0) >> return -EPROTO; >> else >> return -EIO; >> } > > Yep, that looks cleaner. > > My only thought was the pre-3.0 code had a limit to the number of retries. > Was that for a specific reason or is it ok to attempt to retry indefinitely > if we receive a DEFER ? I need to double check the DP spec, but I think it's 4. The while (1) loops in radeon_dp_aux_native_write() and radeon_dp_aux_native_read() should probably be changed to: for (retry = 0; retry < 4; retry++) to match what we do in radeon_dp_i2c_aux_ch(). Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-09-30 5:39 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-29 14:21 Radeon regression fix Brad Campbell 2011-09-29 14:36 ` Alex Deucher 2011-09-29 15:10 ` Brad Campbell 2011-09-29 15:21 ` Brad Campbell 2011-09-30 0:23 ` Brad Campbell 2011-09-30 4:59 ` Alex Deucher 2011-09-30 5:14 ` Brad Campbell 2011-09-30 5:39 ` Alex Deucher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox