<pre>
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
> > >
>
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->