[PATCH v5 2/2] drm: tilcdc: clear the sync lost bit in crtc isr

[Karl Beldan]

Hi,

On Mon, Oct 31, 2016 at 2:19 PM, Bartosz Golaszewski
 wrote:
> The frame synchronization error happens when the DMA engine attempts
> to read what it believes to be the first word of the video buffer but
> it cannot be recognized as such or when the LCDC is starved of data
> due to insufficient bandwidth of the system interconnect.
>

This also happens when the framebuffer boundaries are "incorrect", eg
when flipping while the crtc is enabled. The driver doesn't handle it yet,
so working around it is made via toggling LCDC_RASTER_ENABLE as
you put in this change, it is worth noting because that's how modetest
for example can "seem" to work with v1 when handling SYNC_LOST
and is partly why the "vsynced page flipping" of modetest won't work
either with v1 ATM. I haven't looked at the different trees since early
september but my guess would be that nobody has reworked this yet.

> On some SoCs (notably: da850) the memory settings do not meet the
> LCDC throughput requirements even after increasing the memory
> controller command re-ordering and the LDCD master peripheral
> priority, sometimes causing the sync lost error (typically when
> changing the resolution).
>
> When the sync lost error occurs, simply reset the input FIFO in the
> DMA controller unless a sync lost flood is detected in which case
> disable the interrupt.
>
> Signed-off-by: Bartosz Golaszewski 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 50 
> ++--
>  drivers/gpu/drm/tilcdc/tilcdc_regs.h |  1 +
>  2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 937697d..c4c6323 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -163,7 +163,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device 
> *dev)
>
> if (priv->rev == 1) {
> tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
> -   LCDC_V1_UNDERFLOW_INT_ENA);
> +   LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_SYNC_LOST_ENA);
> tilcdc_set(dev, LCDC_DMA_CTRL_REG,
> LCDC_V1_END_OF_FRAME_INT_ENA);
> } else {
> @@ -181,7 +181,9 @@ static void tilcdc_crtc_disable_irqs(struct drm_device 
> *dev)
> /* disable irqs that we might have enabled: */
> if (priv->rev == 1) {
> tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
> -   LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA);
> +LCDC_V1_UNDERFLOW_INT_ENA |
> +LCDC_V1_PL_INT_ENA |
> +LCDC_V1_SYNC_LOST_ENA);
> tilcdc_clear(dev, LCDC_DMA_CTRL_REG,
> LCDC_V1_END_OF_FRAME_INT_ENA);
> } else {
> @@ -885,24 +887,44 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
> wake_up(&tilcdc_crtc->frame_done_wq);
> }
>
> -   if (stat & LCDC_SYNC_LOST) {
> -   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, disabling the interrupt", __func__, stat);
> -   tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
> -LCDC_SYNC_LOST);
> -   }
> -   }
> -
> /* Indicate to LCDC that the interrupt service routine has
>  * completed, see 13.3.6.1.6 in AM335x TRM.
>  */
> tilcdc_write(dev, LCDC_END_OF_INT_IND_REG, 0);
> }
>
> +   if (stat & LCDC_SYNC_LOST) {
> +   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, 
> disabling the interrupt",
> +   __func__, stat);
> +   if (priv->rev == 2)
> +   tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
> +LCDC_SYNC_LOST);
> +   else if (priv->rev == 1)
> +   tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
> +LCDC_V1_SYNC_LOST_ENA);
> +   }
> +
> +   if (priv->rev == 2) {
> +   /*
> +* Indicate to LCDC that the interrupt service routine

[PATCH 0/2] ARM: davinci: initial infrastructure for LCDC

[Karl Beldan]

On Wed, Oct 05, 2016 at 03:05:30PM +0200, Bartosz Golaszewski wrote:
> After discussing the matter with Laurent Pinchart it turned out that
> using ti,tilcdc,panel was wrong and we should go with the new
> simple-vga-dac driver proposed by Maxime Ripard and currently being
> reviewed.
> 
> The da850-lcdk board on which I'm working has a THS8135 video DAC for
> which the new driver seems to be best suited and we'll be able to
> query the connected display for supported modes instead of hardcoding
> them in the dt as is needed for the panel driver.
> 

I meant to point to these new changes but then it slipped my mind, it is
clearly the way to go.

Regards, 
Karl


> In the meantime I'm posting two patches based on Karl Beldan's
> previous work that can already be merged.
> 
> The first one adds OF_DEV_AUXDATA entry to da8xx-dt.c. I changed the
> compatible string to the new one we're introducing in the tilcdc
> driver.
> 
> The second adds the lcd pins and the display node to da850.dtsi. As
> suggested by Sekhar: I moved the pins node, which was previously in
> da850-lcdk.dts, to da850.dtsi. I also squashed Karl's two patches and
> removed the panel node.
> 
> Tested on a da850-lcdk with an LCD display connected over VGA with
> two patches already posted to the drm mailing list:
> 
>   drm: tilcdc: add a da850-specific compatible string
>   drm: tilcdc: add a workaround for failed clk_set_rate()
> 
> and some additional work-in-progress/hacks on top of that.
> 
> Karl Beldan (2):
>   ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entry for lcdc
>   ARM: dts: da850: add a node for the LCD controller
> 
>  arch/arm/boot/dts/da850.dtsi | 29 +
>  arch/arm/mach-davinci/da8xx-dt.c |  1 +
>  2 files changed, 30 insertions(+)
> 
> -- 
> 2.9.3
> 

[PATCH 0/3] drm/tilcdc: Some fixes for LCDC rev1

[Karl Beldan]

Hi,

On Tue, Aug 23, 2016 at 07:24:42PM +0300, Jyri Sarha wrote:
> Thanks a lot!
> This is very helpful as I do not have LCDC rev1 HW my self, but only
> am335x based boards.
> 
> On 08/23/16 15:56, Karl Beldan wrote:
> > Hi,
> > 
> > I found some missing bits for rev1 of the LCDC and here are some of the
> > changes I am using to use the DRM driver on an LCDCK (which has a rev1).
> > 1/3 seems required by rev1 of the IP and without it my the LCDC on my
> > LCDK keeps can't sync, 2/3 is required by the driver logic, while 3/3
> > is less of a requirement.
> 
> I'll drop 3/3, because my recent patches should take care of that:
> http://www.spinics.net/lists/devicetree/msg138885.html
> 

They do indeed.

> However, there will be v2 round of those patches. Only the DT binding
> and its default value should change.
> 
> > Although 1,2/3 would have fitted drm-fixes I based them on drm-next as
> > 2/3 would conflict with the recent changes in drm-next.
> > 
> 
> I'll pick these two up for my next pull request. Thanks!
> 
> > Another thing which is also an absolute requirement for the rev1 but 
> > with which rev2 seems to cope fine is the palette loading (even if the DRM
> > uses >= 16bpp formats), dixit the TRM:
> > "12-, 16-, and 24-BPP modes do not need a palette; i.e., the pixel data is
> > the desired RGB value. However, the first 32 bytes are still considered a
> > palette. The first entry should be 4000h (bit 14 is 1) while the remaining
> > entries must be filled with 0.".
> > ATM I am using a local crude way of loading the palette to verify the
> > missing bits to get the DRM driver properly working on the LCDK (as I
> > haven't seen how things work in the DRM subsystem I can't post any proper
> > change for that).
> > 
> 
> I see. Reading from the TRM, that should be done for rev2 too. I'll try
> to come up with a patch for that at some point, unless you send me one
> first.
> 

That would be nice. I didn't look at how to integrate my local changes
but instead I tried to see what prevented modetest to work, (with the
posted changes and the palette loading the framebuffer was operational
but not modetest). It appears that when modetesting the LCDC loses its
sync, and I guess that's because the framebuffer addresses are changed
while the raster is still on, which seems inappropriate. Since the v2s
seem to cope with framebuffer ceiling boundaries that are off, my first
guess would be they don't use it and directly use the raster timings
and the bpp format for the boundary and sees the framebuffer addresses
changes (base + ceiling) as atomic. My local changes try to cope with
that by handling the sync loss irq and it gives me a working modetest,
however it is not enough to get modetest -v (ie. with vsynced page
flipping) working.
My impression would be that to work with v1, the following would be in
order (in addition to the palette):
- make changes to the controller with the raster disabled
- handle FRAME_DONE and SYNC_LOST with v1 as well (the driver assumes it
  is only valid for v2, which is false)

> > The posted changes and the above mentioned palette loading missing bit 
> > are specific to the DRM driver.
> > 
> > However there are other shortcomings to the proper functioning of the 
> > LCDC on some platforms (I am currently focusing on the LCDK but I guess 
> > it should be true for all da850 based systems), which are not inherent to
> > the DRM driver driver but which strongly relate to it:
> > 
> > - The pixel clock rate setting (eg. currently the davinci clk can't cope
> > properly with the DRM driver way of setting the clock and doesn't use
> > the common clock framework)
> > - The memory bandwidth/latency requirements for the LCDC are not met 
> > with the default priority settings (apparently there is a fix in
> > da850_lcd_hw_init of: 
> > http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=blob;f=arch/arm/mach-davinci/board-omapl138-lcdk.c;h=689b4f304011e09e262782cf8c4b96eba00ac28b;hb=HEAD).
> > Of course this one affects both the DRM and framebuffer systems.
> > 
> 
> There is also memory bandwidth issues on am335x and we are still looking
> for a proper system level fix.
> 
> > Hence to test the LCDK my crude local changes include (ie. on top of the
> > posted changes and some dts or sync_lost recovery bits):
> > - palette loading
> > - tweak of the pixel clock to cope with davinci clk
> 
> If you have proper a fixes for these, please send a patch. The palette
> loading is something I can probably do myself too, but the clock thing
> is something t

[PATCH 3/3] drm/tilcdc: Advertise the DRM_FORMATs according to the IP revision

[Karl Beldan]

ATM the driver unconditionally advertises support for some 24bpp and
32bpp formats while version 1 of the IP only supports up to 16bpp.

Signed-off-by: Karl Beldan 
---
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c 
b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
index 41911e3..11285f6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -114,12 +114,17 @@ static const struct drm_plane_helper_funcs 
plane_helper_funcs = {
 int tilcdc_plane_init(struct drm_device *dev,
  struct drm_plane *plane)
 {
+   struct tilcdc_drm_private *priv = dev->dev_private;
+   unsigned int num_formats = ARRAY_SIZE(tilcdc_formats);
int ret;

+   if (priv->rev == 1)
+   num_formats = 1;
+
ret = drm_plane_init(dev, plane, 1,
 &tilcdc_plane_funcs,
 tilcdc_formats,
-ARRAY_SIZE(tilcdc_formats),
+num_formats,
 true);
if (ret) {
dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
-- 
2.9.2

[PATCH 2/3] drm/tilcdc: Enable EOF interrupts for v1 LCDC

[Karl Beldan]

This got accidentally dropped in the fixed commit and is required for
the driver to properly work on the rev1 IP, such as found on the LCDK.

Fixes: 2b2080d7e9ae ("drm/tilcdc: Get rid of complex ping-pong mechanism")
Signed-off-by: Karl Beldan 
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 89d6916..163f111 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -98,6 +98,8 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
if (priv->rev == 1) {
tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
LCDC_V1_UNDERFLOW_INT_ENA);
+   tilcdc_set(dev, LCDC_DMA_CTRL_REG,
+   LCDC_V1_END_OF_FRAME_INT_ENA);
} else {
tilcdc_write(dev, LCDC_INT_ENABLE_SET_REG,
LCDC_V2_UNDERFLOW_INT_ENA |
-- 
2.9.2

[PATCH 1/3] drm/tilcdc: Adjust the FB_CEILING address

[Karl Beldan]

The LCDC seems to expect its framebuffer ceiling address pointer to be
an inclusive bound.  The IP rev2 seems to cope with that but rev1 (as
found on the LCDK) don't.
Also note that this is what the framebuffer code does in da8xx-fb.c.

Since, as the TRM puts it, "The 2 LSBs are hardwired to 00b", the
dma_addr_t can be decremented without cast.
I tested it with a v2 (AM335x, rev  0x4F201000) and an LCDK (v1).

Signed-off-by: Karl Beldan 
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 25d6b22..89d6916 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -80,7 +80,7 @@ static void set_scanout(struct drm_crtc *crtc, struct 
drm_framebuffer *fb)
end = start + (crtc->mode.vdisplay * fb->pitches[0]);

tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, start);
-   tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, end);
+   tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, end - 1);

if (tilcdc_crtc->curr_fb)
drm_flip_work_queue(&tilcdc_crtc->unref_work,
-- 
2.9.2

[PATCH 0/3] drm/tilcdc: Some fixes for LCDC rev1

[Karl Beldan]

Hi,

I found some missing bits for rev1 of the LCDC and here are some of the
changes I am using to use the DRM driver on an LCDCK (which has a rev1).
1/3 seems required by rev1 of the IP and without it my the LCDC on my
LCDK keeps can't sync, 2/3 is required by the driver logic, while 3/3
is less of a requirement.
Although 1,2/3 would have fitted drm-fixes I based them on drm-next as
2/3 would conflict with the recent changes in drm-next.

Another thing which is also an absolute requirement for the rev1 but 
with which rev2 seems to cope fine is the palette loading (even if the DRM
uses >= 16bpp formats), dixit the TRM:
"12-, 16-, and 24-BPP modes do not need a palette; i.e., the pixel data is
the desired RGB value. However, the first 32 bytes are still considered a
palette. The first entry should be 4000h (bit 14 is 1) while the remaining
entries must be filled with 0.".
ATM I am using a local crude way of loading the palette to verify the
missing bits to get the DRM driver properly working on the LCDK (as I
haven't seen how things work in the DRM subsystem I can't post any proper
change for that).

The posted changes and the above mentioned palette loading missing bit 
are specific to the DRM driver.

However there are other shortcomings to the proper functioning of the 
LCDC on some platforms (I am currently focusing on the LCDK but I guess 
it should be true for all da850 based systems), which are not inherent to
the DRM driver driver but which strongly relate to it:

- The pixel clock rate setting (eg. currently the davinci clk can't cope
properly with the DRM driver way of setting the clock and doesn't use
the common clock framework)
- The memory bandwidth/latency requirements for the LCDC are not met 
with the default priority settings (apparently there is a fix in
da850_lcd_hw_init of: 
http://arago-project.org/git/projects/linux-davinci.git?p=projects/linux-davinci.git;a=blob;f=arch/arm/mach-davinci/board-omapl138-lcdk.c;h=689b4f304011e09e262782cf8c4b96eba00ac28b;hb=HEAD).
Of course this one affects both the DRM and framebuffer systems.

Hence to test the LCDK my crude local changes include (ie. on top of the
posted changes and some dts or sync_lost recovery bits):
- palette loading
- tweak of the pixel clock to cope with davinci clk
- adjustment of the memory master priorities


BTW, with the recent changes of tilcdc from drm-next, I see this warning
when loading the module:
"drm:drm_helper_disable_unused_functions [drm_kms_helper]] *ERROR* Called for 
atomic driver, this is not what you want."


Regards, 
Karl


Karl Beldan (3):
  drm/tilcdc: Adjust the FB_CEILING address
  drm/tilcdc: Enable EOF interrupts for v1 LCDC
  drm/tilcdc: Advertise the DRM_FORMATs according to the IP revision

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 4 +++-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 7 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.9.2