* [PATCH] drm: tilcdc: simplify the recovery from sync lost error on rev1
@ 2016-12-13 15:07 Bartosz Golaszewski
2016-12-19 13:09 ` Jyri Sarha
0 siblings, 1 reply; 4+ messages in thread
From: Bartosz Golaszewski @ 2016-12-13 15:07 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori
Cc: LKML, linux-drm, Peter Ujfalusi, arm-soc, Bartosz Golaszewski
Revision 2 of LCDC suffers from an issue where a SYNC_LOST error
caused by limited memory bandwidth may leave the picture shifted a
couple pixels to the right.
This issue has not been observed on revision 1, while the recovery
mechanism introduces a different issue, where the END_OF_FRAME
interrupt doesn't fire while drm is waiting for vblanks.
On rev1: recover from sync lost errors by simply clearing the
RASTER_ENABLE bit in the RASTER_CTRL register and re-enabling it
again as is suggested by the datasheet.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 9942b05..70e57a7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -921,17 +921,23 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",
__func__, stat);
tilcdc_crtc->frame_intact = false;
- if (tilcdc_crtc->sync_lost_count++ >
- SYNC_LOST_COUNT_LIMIT) {
- dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);
- queue_work(system_wq, &tilcdc_crtc->recover_work);
- if (priv->rev == 1)
- tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
- LCDC_V1_SYNC_LOST_INT_ENA);
- else
+ if (priv->rev == 1) {
+ tilcdc_clear(dev,
+ LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+ tilcdc_set(dev,
+ LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+ } else {
+ if (tilcdc_crtc->sync_lost_count++ >
+ SYNC_LOST_COUNT_LIMIT) {
+ dev_err(dev->dev,
+ "%s(0x%08x): Sync lost flood detected, recovering",
+ __func__, stat);
+ queue_work(system_wq,
+ &tilcdc_crtc->recover_work);
tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
LCDC_SYNC_LOST);
- tilcdc_crtc->sync_lost_count = 0;
+ tilcdc_crtc->sync_lost_count = 0;
+ }
}
}
--
2.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: tilcdc: simplify the recovery from sync lost error on rev1
2016-12-13 15:07 [PATCH] drm: tilcdc: simplify the recovery from sync lost error on rev1 Bartosz Golaszewski
@ 2016-12-19 13:09 ` Jyri Sarha
2016-12-19 14:19 ` Bartosz Golaszewski
0 siblings, 1 reply; 4+ messages in thread
From: Jyri Sarha @ 2016-12-19 13:09 UTC (permalink / raw)
To: Bartosz Golaszewski, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori
Cc: LKML, linux-drm, Peter Ujfalusi, arm-soc
One comment bellow.
On 12/13/16 17:07, Bartosz Golaszewski wrote:
> Revision 2 of LCDC suffers from an issue where a SYNC_LOST error
> caused by limited memory bandwidth may leave the picture shifted a
> couple pixels to the right.
>
> This issue has not been observed on revision 1, while the recovery
> mechanism introduces a different issue, where the END_OF_FRAME
> interrupt doesn't fire while drm is waiting for vblanks.
>
> On rev1: recover from sync lost errors by simply clearing the
> RASTER_ENABLE bit in the RASTER_CTRL register and re-enabling it
> again as is suggested by the datasheet.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 9942b05..70e57a7 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -921,17 +921,23 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
> dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",
> __func__, stat);
> tilcdc_crtc->frame_intact = false;
> - if (tilcdc_crtc->sync_lost_count++ >
> - SYNC_LOST_COUNT_LIMIT) {
> - dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);
> - queue_work(system_wq, &tilcdc_crtc->recover_work);
> - if (priv->rev == 1)
> - tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
> - LCDC_V1_SYNC_LOST_INT_ENA);
> - else
> + if (priv->rev == 1) {
I would add here:
+ if ((tilcdc_read(dev, LCDC_RASTER_CTRL_REG) &
+ LCDC_RASTER_ENABLE)) {
> + tilcdc_clear(dev,
> + LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
> + tilcdc_set(dev,
> + LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+ }
Just in case the interrupt is for some reason handled right after the
crtc is disabled.
With this addition I could send a pull request for this fix still today,
if you agree with the change.
> + } else {
> + if (tilcdc_crtc->sync_lost_count++ >
> + SYNC_LOST_COUNT_LIMIT) {
> + dev_err(dev->dev,
> + "%s(0x%08x): Sync lost flood detected, recovering",
> + __func__, stat);
> + queue_work(system_wq,
> + &tilcdc_crtc->recover_work);
> tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
> LCDC_SYNC_LOST);
> - tilcdc_crtc->sync_lost_count = 0;
> + tilcdc_crtc->sync_lost_count = 0;
> + }
> }
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: tilcdc: simplify the recovery from sync lost error on rev1
2016-12-19 13:09 ` Jyri Sarha
@ 2016-12-19 14:19 ` Bartosz Golaszewski
2016-12-19 16:50 ` Jyri Sarha
0 siblings, 1 reply; 4+ messages in thread
From: Bartosz Golaszewski @ 2016-12-19 14:19 UTC (permalink / raw)
To: Jyri Sarha
Cc: Tomi Valkeinen, David Airlie, Kevin Hilman, Michael Turquette,
Sekhar Nori, LKML, linux-drm, Peter Ujfalusi, arm-soc
2016-12-19 14:09 GMT+01:00 Jyri Sarha <jsarha@ti.com>:
> One comment bellow.
>
> On 12/13/16 17:07, Bartosz Golaszewski wrote:
>> Revision 2 of LCDC suffers from an issue where a SYNC_LOST error
>> caused by limited memory bandwidth may leave the picture shifted a
>> couple pixels to the right.
>>
>> This issue has not been observed on revision 1, while the recovery
>> mechanism introduces a different issue, where the END_OF_FRAME
>> interrupt doesn't fire while drm is waiting for vblanks.
>>
>> On rev1: recover from sync lost errors by simply clearing the
>> RASTER_ENABLE bit in the RASTER_CTRL register and re-enabling it
>> again as is suggested by the datasheet.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 9942b05..70e57a7 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -921,17 +921,23 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>> dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",
>> __func__, stat);
>> tilcdc_crtc->frame_intact = false;
>> - if (tilcdc_crtc->sync_lost_count++ >
>> - SYNC_LOST_COUNT_LIMIT) {
>> - dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);
>> - queue_work(system_wq, &tilcdc_crtc->recover_work);
>> - if (priv->rev == 1)
>> - tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
>> - LCDC_V1_SYNC_LOST_INT_ENA);
>> - else
>> + if (priv->rev == 1) {
>
> I would add here:
> + if ((tilcdc_read(dev, LCDC_RASTER_CTRL_REG) &
> + LCDC_RASTER_ENABLE)) {
>
>> + tilcdc_clear(dev,
>> + LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
>> + tilcdc_set(dev,
>> + LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
> + }
>
> Just in case the interrupt is for some reason handled right after the
> crtc is disabled.
>
> With this addition I could send a pull request for this fix still today,
> if you agree with the change.
I'm not sure this can really happen, but it won't hurt either, so I'll
send a v2.
Thanks,
Bartosz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: tilcdc: simplify the recovery from sync lost error on rev1
2016-12-19 14:19 ` Bartosz Golaszewski
@ 2016-12-19 16:50 ` Jyri Sarha
0 siblings, 0 replies; 4+ messages in thread
From: Jyri Sarha @ 2016-12-19 16:50 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Tomi Valkeinen, David Airlie, Kevin Hilman, Michael Turquette,
Sekhar Nori, LKML, linux-drm, Peter Ujfalusi, arm-soc
On 12/19/16 16:19, Bartosz Golaszewski wrote:
>> I would add here:
>> + if ((tilcdc_read(dev, LCDC_RASTER_CTRL_REG) &
>> + LCDC_RASTER_ENABLE)) {
>>
>>> + tilcdc_clear(dev,
>>> + LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
>>> + tilcdc_set(dev,
>>> + LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
>> + }
>>
>> Just in case the interrupt is for some reason handled right after the
>> crtc is disabled.
>>
>> With this addition I could send a pull request for this fix still today,
>> if you agree with the change.
> I'm not sure this can really happen, but it won't hurt either, so I'll
> send a v2.
Well, in theory it is quite possible. If the driver clears the raster
enable at the same time when sync lost interrupt is generated, then the
irq routine handles the irq before the interrupts are disabled. Then it
happens that rastere remains on but the driver thinks it has already
turned it off.
In practice this is of course terribly unlikely, but a race is a race
and should be avoided.
Anyway, my vacation has already been started so I won't send a pull
request now. I do not like the idea of doing something like that and
then vanishing for two weeks. I hope they still take fixes for 4.10 in
4th Jan.
Best regards,
Jyri
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-19 16:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13 15:07 [PATCH] drm: tilcdc: simplify the recovery from sync lost error on rev1 Bartosz Golaszewski
2016-12-19 13:09 ` Jyri Sarha
2016-12-19 14:19 ` Bartosz Golaszewski
2016-12-19 16:50 ` Jyri Sarha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).