[PATCH v2 7/7] drm/tilcdc: Load palette at the end of mode_set_nofb()
On 11/18/16 23:53, Jyri Sarha wrote: >> Ok, got it. You need to set the palette loading mode back to 'palette >> > and data' before returning. Just add this at the end: >> > >> > tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG, >> > LCDC_PALETTE_LOAD_MODE(PALETTE_AND_DATA), >> > LCDC_PALETTE_LOAD_MODE_MASK); > Really? I wonder why, because we anyway set it to data only when we turn > the display on. The raster is not turned on before that so the register > value should not matter. I need to investigate what really happens. > > However, for now I think I should just add it. There should not be any > harm in doing that. > Now I got it. The problem is of course the wrong usage of tilcdc_set() for the palette load mode bit-field in tilcdc_crtc_enable(). I'll change: tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_PALETTE_LOAD_MODE(DATA_ONLY)); to tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG, LCDC_PALETTE_LOAD_MODE(DATA_ONLY), LCDC_PALETTE_LOAD_MODE_MASK); in tilcdc_crtc_enable(). That should fix the problem. Cheers, Jyri
[PATCH v2 7/7] drm/tilcdc: Load palette at the end of mode_set_nofb()
On 11/18/16 18:10, Bartosz Golaszewski wrote:
> 2016-11-18 16:34 GMT+01:00 Bartosz Golaszewski :
>> 2016-11-16 13:41 GMT+01:00 Jyri Sarha :
>>> Load palette at the end of mode_set_nofb() and only if the palette has
>>> not been loaded since last runtime resume. Moving the palette loading
>>> to mode_set_nofb() saves us from storing and restoring of LCDC dma
>>> addresses that were just recently updated.
>>>
>>> Signed-off-by: Jyri Sarha
>>> ---
>>> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +
>>> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 12
>>> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 +
>>> 3 files changed, 26 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>>> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>>> index 1590c42..f3be171 100644
>>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>>> @@ -113,6 +113,13 @@ static void set_scanout(struct drm_crtc *crtc, struct
>>> drm_framebuffer *fb)
>>> tilcdc_crtc->curr_fb = fb;
>>> }
>>>
>>> +void tilcdc_crtc_reload_palette(struct drm_crtc *crtc)
>>> +{
>>> + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>>> +
>>> + reinit_completion(_crtc->palette_loaded);
>>> +}
>>> +
>>> /*
>>> * The driver currently only supports only true color formats. For
>>> * true color the palette block is bypassed, but a 32 byte palette
>>> @@ -121,14 +128,12 @@ static void set_scanout(struct drm_crtc *crtc, struct
>>> drm_framebuffer *fb)
>>> */
>>> static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
>>> {
>>> - u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
>>> struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>>> struct drm_device *dev = crtc->dev;
>>> struct tilcdc_drm_private *priv = dev->dev_private;
>>>
>>> - dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
>>> - dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);
>>> - raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
>>> + if (completion_done(_crtc->palette_loaded))
>>> + return;
>>>
>>> /* Tell the LCDC where the palette is located. */
>>> tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
>>> @@ -160,11 +165,6 @@ static void tilcdc_crtc_load_palette(struct drm_crtc
>>> *crtc)
>>> tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
>>> else
>>> tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
>>> LCDC_V2_PL_INT_ENA);
>>> -
>>> - /* Restore the registers. */
>>> - tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
>>> - tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
>>> - tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);
>>> }
>>>
>>
>> Hi Jyri,
>>
>> I don't know exactly why, but not restoring the RASTER CTRL register
>> here messes up simple modetest - the image is shifted vertically. The
>> rest of the patch seems fine.
>>
>> Thanks,
>> Bartosz Golaszewski
>
> Ok, got it. You need to set the palette loading mode back to 'palette
> and data' before returning. Just add this at the end:
>
> tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,
> LCDC_PALETTE_LOAD_MODE(PALETTE_AND_DATA),
> LCDC_PALETTE_LOAD_MODE_MASK);
Really? I wonder why, because we anyway set it to data only when we turn
the display on. The raster is not turned on before that so the register
value should not matter. I need to investigate what really happens.
However, for now I think I should just add it. There should not be any
harm in doing that.
Thanks,
Jyri
[PATCH v2 7/7] drm/tilcdc: Load palette at the end of mode_set_nofb()
2016-11-18 16:34 GMT+01:00 Bartosz Golaszewski :
> 2016-11-16 13:41 GMT+01:00 Jyri Sarha :
>> Load palette at the end of mode_set_nofb() and only if the palette has
>> not been loaded since last runtime resume. Moving the palette loading
>> to mode_set_nofb() saves us from storing and restoring of LCDC dma
>> addresses that were just recently updated.
>>
>> Signed-off-by: Jyri Sarha
>> ---
>> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +
>> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 12
>> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 +
>> 3 files changed, 26 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 1590c42..f3be171 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -113,6 +113,13 @@ static void set_scanout(struct drm_crtc *crtc, struct
>> drm_framebuffer *fb)
>> tilcdc_crtc->curr_fb = fb;
>> }
>>
>> +void tilcdc_crtc_reload_palette(struct drm_crtc *crtc)
>> +{
>> + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>> +
>> + reinit_completion(_crtc->palette_loaded);
>> +}
>> +
>> /*
>> * The driver currently only supports only true color formats. For
>> * true color the palette block is bypassed, but a 32 byte palette
>> @@ -121,14 +128,12 @@ static void set_scanout(struct drm_crtc *crtc, struct
>> drm_framebuffer *fb)
>> */
>> static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
>> {
>> - u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
>> struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>> struct drm_device *dev = crtc->dev;
>> struct tilcdc_drm_private *priv = dev->dev_private;
>>
>> - dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
>> - dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);
>> - raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
>> + if (completion_done(_crtc->palette_loaded))
>> + return;
>>
>> /* Tell the LCDC where the palette is located. */
>> tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
>> @@ -160,11 +165,6 @@ static void tilcdc_crtc_load_palette(struct drm_crtc
>> *crtc)
>> tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
>> else
>> tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
>> LCDC_V2_PL_INT_ENA);
>> -
>> - /* Restore the registers. */
>> - tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
>> - tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
>> - tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);
>> }
>>
>
> Hi Jyri,
>
> I don't know exactly why, but not restoring the RASTER CTRL register
> here messes up simple modetest - the image is shifted vertically. The
> rest of the patch seems fine.
>
> Thanks,
> Bartosz Golaszewski
Ok, got it. You need to set the palette loading mode back to 'palette
and data' before returning. Just add this at the end:
tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,
LCDC_PALETTE_LOAD_MODE(PALETTE_AND_DATA),
LCDC_PALETTE_LOAD_MODE_MASK);
Thanks,
Bartosz Golaszewski
[PATCH v2 7/7] drm/tilcdc: Load palette at the end of mode_set_nofb()
2016-11-16 13:41 GMT+01:00 Jyri Sarha :
> Load palette at the end of mode_set_nofb() and only if the palette has
> not been loaded since last runtime resume. Moving the palette loading
> to mode_set_nofb() saves us from storing and restoring of LCDC dma
> addresses that were just recently updated.
>
> Signed-off-by: Jyri Sarha
> ---
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 12
> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 +
> 3 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 1590c42..f3be171 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -113,6 +113,13 @@ static void set_scanout(struct drm_crtc *crtc, struct
> drm_framebuffer *fb)
> tilcdc_crtc->curr_fb = fb;
> }
>
> +void tilcdc_crtc_reload_palette(struct drm_crtc *crtc)
> +{
> + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> +
> + reinit_completion(_crtc->palette_loaded);
> +}
> +
> /*
> * The driver currently only supports only true color formats. For
> * true color the palette block is bypassed, but a 32 byte palette
> @@ -121,14 +128,12 @@ static void set_scanout(struct drm_crtc *crtc, struct
> drm_framebuffer *fb)
> */
> static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
> {
> - u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
> struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> struct drm_device *dev = crtc->dev;
> struct tilcdc_drm_private *priv = dev->dev_private;
>
> - dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
> - dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);
> - raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
> + if (completion_done(_crtc->palette_loaded))
> + return;
>
> /* Tell the LCDC where the palette is located. */
> tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
> @@ -160,11 +165,6 @@ static void tilcdc_crtc_load_palette(struct drm_crtc
> *crtc)
> tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
> else
> tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
> LCDC_V2_PL_INT_ENA);
> -
> - /* Restore the registers. */
> - tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
> - tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
> - tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);
> }
>
Hi Jyri,
I don't know exactly why, but not restoring the RASTER CTRL register
here messes up simple modetest - the image is shifted vertically. The
rest of the patch seems fine.
Thanks,
Bartosz Golaszewski
[PATCH v2 7/7] drm/tilcdc: Load palette at the end of mode_set_nofb()
Load palette at the end of mode_set_nofb() and only if the palette has
not been loaded since last runtime resume. Moving the palette loading
to mode_set_nofb() saves us from storing and restoring of LCDC dma
addresses that were just recently updated.
Signed-off-by: Jyri Sarha
---
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +
drivers/gpu/drm/tilcdc/tilcdc_drv.c | 12
drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 +
3 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 1590c42..f3be171 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -113,6 +113,13 @@ static void set_scanout(struct drm_crtc *crtc, struct
drm_framebuffer *fb)
tilcdc_crtc->curr_fb = fb;
}
+void tilcdc_crtc_reload_palette(struct drm_crtc *crtc)
+{
+ struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+
+ reinit_completion(_crtc->palette_loaded);
+}
+
/*
* The driver currently only supports only true color formats. For
* true color the palette block is bypassed, but a 32 byte palette
@@ -121,14 +128,12 @@ static void set_scanout(struct drm_crtc *crtc, struct
drm_framebuffer *fb)
*/
static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
{
- u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
struct drm_device *dev = crtc->dev;
struct tilcdc_drm_private *priv = dev->dev_private;
- dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
- dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);
- raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
+ if (completion_done(_crtc->palette_loaded))
+ return;
/* Tell the LCDC where the palette is located. */
tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
@@ -160,11 +165,6 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc)
tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
else
tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, LCDC_V2_PL_INT_ENA);
-
- /* Restore the registers. */
- tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
- tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
- tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);
}
static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
@@ -234,9 +234,6 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
reset(crtc);
- if (!completion_done(_crtc->palette_loaded))
- tilcdc_crtc_load_palette(crtc);
-
tilcdc_crtc_enable_irqs(dev);
tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE);
@@ -278,12 +275,6 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool
shutdown)
__func__);
}
- /*
-* LCDC will not retain the palette when reset. Make sure it gets
-* reloaded on tilcdc_crtc_enable().
-*/
- reinit_completion(_crtc->palette_loaded);
-
drm_crtc_vblank_off(crtc);
tilcdc_crtc_disable_irqs(dev);
@@ -675,10 +666,12 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc
*crtc)
drm_framebuffer_reference(fb);
- set_scanout(crtc, fb);
-
tilcdc_crtc_set_clk(crtc);
+ tilcdc_crtc_load_palette(crtc);
+
+ set_scanout(crtc, fb);
+
crtc->hwmode = crtc->state->adjusted_mode;
}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 28e97d5..a7c91f7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -637,8 +637,20 @@ static int tilcdc_pm_resume(struct device *dev)
}
#endif
+static int tilcdc_pm_runtime_resume(struct device *dev)
+{
+ struct drm_device *ddev = dev_get_drvdata(dev);
+ struct tilcdc_drm_private *priv = ddev->dev_private;
+
+ if (priv->crtc)
+ tilcdc_crtc_reload_palette(priv->crtc);
+
+ return 0;
+}
+
static const struct dev_pm_ops tilcdc_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(tilcdc_pm_suspend, tilcdc_pm_resume)
+ .runtime_resume = tilcdc_pm_runtime_resume,
};
/*
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 7913b0e..5803848 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -180,6 +180,7 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc
*crtc,
int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event);
+void tilcdc_crtc_reload_palette(struct drm_crtc *crtc);
int tilcdc_plane_init(struct drm_device *dev, struct drm_plane *plane);
--
1.9.1
