Re: [PATCH v4 05/11] drm/mediatek: gamma: Enable the Gamma LUT table only after programming

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

Hi, Angelo:

On Mon, 2023-05-22 at 14:25 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 22/05/23 12:00, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Thu, 2023-05-18 at 12:48 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > Move the write to DISP_GAMMA_CFG to enable the Gamma LUT to after
> > > programming the actual table to avoid potential visual glitches
> > > during
> > > table modification.
> > 
> > I think user could update the lut table frequently, so when do you
> > disable the gamma function before next update? In addition, if we
> > really care the glitches, update the register in vblank period
> > which
> > should use cmdq to update the register. But now, I think we do not
> > care
> > the glitches. You may skip this patch, or fix the problem I
> > mention.
> > 
> 
> If you disable the GAMMA function (either set RELAY mode or disable
> the
> GAMMA_LUT_EN bit in GAMMA_CFG), there will be glitches during setting
> via
> the GNOME Night Mode color temperature slider.
> 
> This commit prevents a glitch in the case in which the GAMMA LUT
> registers
> are not zeroed before setting the expected LUT, for which reason I
> disagree
> about skipping this patch.
> 
> Please note that, while I agree about updating the GAMMA LUT through
> CMDQ
> setting and between vblanks, this requires a lot more effort to
> implement
> and it's out of scope for this specific series; depending on my
> bandwidth,
> this may come later and it would in any case require this patch to
> move
> the LUT enablement to after LUT registers setting. Besides, I have
> already
> tried to enable this through CMDQ, but didn't work as expected and
> since I
> had no time to dig further, I deemed this to be essential for at
> least an
> initial functionality implementation for MT8195, and a cleanup of
> this
> driver.
> Obviously, the *only* way to fix *all* of the corner cases of the
> problem
> that you mentioned is to use CMDQ and trying to implement this purely
> with
> cpu writes will in any case be prone to at least some glitches.
> 
> In any case, while your concern is valid, I'm sure that you agree
> with me
> on the fact that enabling the LUT before actually programming it is
> something
> that should not happen in principle. For this reason, and for the
> others that
> I've just mentioned, I think that this patch is totally valid.

OK. So please add these comment to patch description so that we could
know there is something to do.

Regards,
CK

> 
> Regards,
> Angelo
> 
> > Regards,
> > CK
> > 
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > Reviewed-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > ---
> > >   drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 13 ++++++++-----
> > >   1 file changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > index 60ccea8c1e1a..1592614b6de7 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
> > > @@ -71,12 +71,12 @@ unsigned int mtk_gamma_get_lut_size(struct
> > > device
> > > *dev)
> > >   void mtk_gamma_set_common(struct device *dev, void __iomem
> > > *regs,
> > > struct drm_crtc_state *state)
> > >   {
> > >          struct mtk_disp_gamma *gamma = dev_get_drvdata(dev);
> > > -       unsigned int i, reg;
> > > +       unsigned int i;
> > >          struct drm_color_lut *lut;
> > >          void __iomem *lut_base;
> > >          bool lut_diff;
> > >          u16 lut_size;
> > > -       u32 word;
> > > +       u32 cfg_val, word;
> > > 
> > >          /* If there's no gamma lut there's nothing to do here.
> > > */
> > >          if (!state->gamma_lut)
> > > @@ -90,9 +90,7 @@ void mtk_gamma_set_common(struct device *dev,
> > > void
> > > __iomem *regs, struct drm_crt
> > >                  lut_size = LUT_SIZE_DEFAULT;
> > >          }
> > > 
> > > -       reg = readl(regs + DISP_GAMMA_CFG);
> > > -       reg = reg | GAMMA_LUT_EN;
> > > -       writel(reg, regs + DISP_GAMMA_CFG);
> > > +       cfg_val = readl(regs + DISP_GAMMA_CFG);
> > >          lut_base = regs + DISP_GAMMA_LUT;
> > >          lut = (struct drm_color_lut *)state->gamma_lut->data;
> > >          for (i = 0; i < lut_size; i++) {
> > > @@ -122,6 +120,11 @@ void mtk_gamma_set_common(struct device
> > > *dev,
> > > void __iomem *regs, struct drm_crt
> > >                  }
> > >                  writel(word, (lut_base + i * 4));
> > >          }
> > > +
> > > +       /* Enable the gamma table */
> > > +       cfg_val = cfg_val | GAMMA_LUT_EN;
> > > +
> > > +       writel(cfg_val, regs + DISP_GAMMA_CFG);
> > >   }
> > > 
> > >   void mtk_gamma_set(struct device *dev, struct drm_crtc_state
> > > *state)
> > > --
> > > 2.40.1
> > > 
> 
>