Re: [PATCH v3 3/5] drm/mediatek: Support "Pre-multiplied" blending in OVL

["Shawn Sung (宋孝謙)" <Shawn.Sung@mediatek.com>]

Hi Angelo,

On Wed, 2024-07-10 at 14:02 +0200, AngeloGioacchino Del Regno wrote:
> Il 10/07/24 10:52, Hsiao Chien Sung via B4 Relay ha scritto:
> > From: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > 
> > Support "Pre-multiplied" alpha blending mode on in OVL.
> > Before this patch, only the "coverage" mode is supported.
> > 
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 32
> > +++++++++++++++++++++++++-------
> >   1 file changed, 25 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > index add671c38613..89b439dcf3a6 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > @@ -56,8 +56,12 @@
> >   #define GMC_THRESHOLD_HIGH	((1 << GMC_THRESHOLD_BITS) / 4)
> >   #define GMC_THRESHOLD_LOW	((1 << GMC_THRESHOLD_BITS) / 8)
> >   
> > +#define OVL_CON_CLRFMT_MAN	BIT(23)
> >   #define OVL_CON_BYTE_SWAP	BIT(24)
> > -#define OVL_CON_MTX_YUV_TO_RGB	(6 << 16)
> > +
> > +/* OVL_CON_RGB_SWAP works only if OVL_CON_CLRFMT_MAN is enabled */
> > +#define OVL_CON_RGB_SWAP	BIT(25)
> > +
> >   #define OVL_CON_CLRFMT_RGB	(1 << 12)
> >   #define OVL_CON_CLRFMT_ARGB8888	(2 << 12)
> >   #define OVL_CON_CLRFMT_RGBA8888	(3 << 12)
> > @@ -65,6 +69,11 @@
> >   #define OVL_CON_CLRFMT_BGRA8888	(OVL_CON_CLRFMT_ARGB8888 |
> > OVL_CON_BYTE_SWAP)
> >   #define OVL_CON_CLRFMT_UYVY	(4 << 12)
> >   #define OVL_CON_CLRFMT_YUYV	(5 << 12)
> > +#define OVL_CON_MTX_YUV_TO_RGB	(6 << 16)
> > +#define OVL_CON_CLRFMT_PARGB8888 ((3 << 12) | OVL_CON_CLRFMT_MAN)
> 
> 
> Shouldn't this be (OVL_CON_CLRFMT_RGBA8888 | OVL_CON_CLRFMT_MAN) ??
> 
> That's getting written to the same register, so I'd guess this is
> like that; but
> then, if it is like that, why is it PARGB888 and not PRGBA888?!
> 

Thank you for highlighting this important point. As whether
OVL_CON_CLRFMT_MAN bit is enabled, (3 << 12) means different formats in
the datasheet, and this function also confused us very much at first.

To prevent any similar misunderstandings going forward, instead of
reusing the MACRO you mentioned, we intetionally redefined
OVL_CON_CLRFMT_PARGB8888 using bit operation again, although this
approach seems to cause another confusion.

Regards,
Shawn