[PATCH v2 7/7] drm/tilcdc: Load palette at the end of mode_set_nofb()

[Jyri Sarha]

On 11/18/16 18:10, Bartosz Golaszewski wrote:
> 2016-11-18 16:34 GMT+01:00 Bartosz Golaszewski <bgolaszewski at baylibre.com>:
>> 2016-11-16 13:41 GMT+01:00 Jyri Sarha <jsarha at ti.com>:
>>> 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 <jsarha at ti.com>
>>> ---
>>>  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(&tilcdc_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(&tilcdc_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