Re: [PATCH v9 16/21] drm/mediatek: Support "Pre-multiplied" blending in OVL

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

Hi, Jason:

On Tue, 2024-06-18 at 17:00 +0000, Jason-JH Lin (林睿祥) wrote:
> Hi CK,
> 
> On Fri, 2024-06-14 at 05:42 +0000, CK Hu (胡俊光) wrote:
> > On Fri, 2024-06-14 at 10:46 +0800, Shawn Sung wrote:
> > > 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>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 42 ++++++++++++++++++++-
> > > ----
> > >  1 file changed, 34 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > index 6567806cf4e2..47d0b039a616 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > @@ -52,8 +52,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)
> > > @@ -61,6 +65,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)
> > 
> > #define OVL_CON_CLRFMT_PARGB8888 (OVL_CON_CLRFMT_RGBA8888 |
> > OVL_CON_CLRFMT_MAN)
> > 
> > But I'm confused with the naming.
> > 
> 
> We found that original naming is not correct, so we fix it with the
> patch:
> 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240616-mediatek-drm-next-v1-11-7e8f9cf785d8@mediatek.com/
> 
> PARGB8888 is not ARGB8888 + MAN.
> 
> If DISP_REG_OVL_CON open the MAN bit, some original color format
> settings will be changed, for example:
> 
> When MAN = 0:
> RGB565 = 0000 in bit[15:12]
> RGB888 = 0001 in bit[15:12]
> ARGB8888 = 0010 in bit[15:12]
> RGBA8888 = 0011 in bit[15:12]
> UYVY = 0100 in bit[15:12]
> YUYV = 0101 in bit[15:12]
> 
> When MAN = 1:
> RGB888 = 0000 in bit[15:12]
> RGB565 = 0001 in bit[15:12]
> BGRA
> 8888 = 0010 in bit[15:12]
> PARGB8888 = 0011 in bit[15:12]BGRA4444 = 0100
> in bit[15:12]
> BGRA5551 = 0101 in bit[15:12]
> YUYV = 1000 in bit[15:12]
> UYVY
> = 1001 in bit[15:12]
> YVYU = 1010 in bit[15:12]
> VYUY = 1011 in bit[15:12]

I think you could define premultiplied format as:

#define PRGB888		(0 << 12) | OVL_CON_CLRFMT_MAN
#define PRGB565		(1 << 12) | OVL_CON_CLRFMT_MAN
#define PBGRA8888	(2 << 12) | OVL_CON_CLRFMT_MAN
#define PARGB8888	(3 << 12) | OVL_CON_CLRFMT_MAN
#define PBGRA4444	(4 << 12) | OVL_CON_CLRFMT_MAN
#define PBGRA5551	(5 << 12) | OVL_CON_CLRFMT_MAN
#define PYUYV		(8 << 12) | OVL_CON_CLRFMT_MAN
#define PVYUY		(9 << 12) | OVL_CON_CLRFMT_MAN

> 
> > 
> > > +#define OVL_CON_CLRFMT_PABGR8888 (OVL_CON_CLRFMT_PARGB8888 |
> > > OVL_CON_RGB_SWAP)
> > 
> > #define OVL_CON_CLRFMT_PABGR8888 (OVL_CON_CLRFMT_ABGR8888 |
> > OVL_CON_CLRFMT_MAN)
> > 
> > > +#define OVL_CON_CLRFMT_PBGRA8888 (OVL_CON_CLRFMT_PARGB8888 |
> > > OVL_CON_BYTE_SWAP)
> > 
> > #define OVL_CON_CLRFMT_PBGRA8888 (OVL_CON_CLRFMT_BGRA8888 |
> > OVL_CON_CLRFMT_MAN)
> > 
> > > +#define OVL_CON_CLRFMT_PRGBA8888 (OVL_CON_CLRFMT_PABGR8888 |
> > > OVL_CON_BYTE_SWAP)
> > >  #define OVL_CON_CLRFMT_RGB565(ovl)	((ovl)->data->fmt_rgb565_is_0 ?
> > > \
> > >  					0 : OVL_CON_CLRFMT_RGB)
> > >  #define OVL_CON_CLRFMT_RGB888(ovl)	((ovl)->data->fmt_rgb565_is_0 ?
> > > \
> > > @@ -74,6 +83,8 @@
> > >  #define	OVL_CON_VIRT_FLIP	BIT(9)
> > >  #define	OVL_CON_HORZ_FLIP	BIT(10)
> > >  
> > > +#define OVL_COLOR_ALPHA		GENMASK(31, 24)
> > > +
> > >  static inline bool is_10bit_rgb(u32 fmt)
> > >  {
> > >  	switch (fmt) {
> > > @@ -298,7 +309,13 @@ void mtk_ovl_config(struct device *dev,
> > > unsigned int w,
> > >  	if (w != 0 && h != 0)
> > >  		mtk_ddp_write_relaxed(cmdq_pkt, h << 16 | w, &ovl-
> > > > cmdq_reg, ovl->regs,
> > > 
> > >  				      DISP_REG_OVL_ROI_SIZE);
> > > -	mtk_ddp_write_relaxed(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs, 
> > > DISP_REG_OVL_ROI_BGCLR);
> > > +
> > > +	/*
> > > +	 * The background color must be opaque black (ARGB),
> > > +	 * otherwise the alpha blending will have no effect
> > > +	 */
> > > +	mtk_ddp_write_relaxed(cmdq_pkt, OVL_COLOR_ALPHA, &ovl-
> > > > cmdq_reg,
> > > 
> > > +			      ovl->regs, DISP_REG_OVL_ROI_BGCLR);
> > 
> > The coverage mode formula is:
> > dst.RGB = src.RGB * src.A + dst.RGB * (1 - src.A)
> > 
> > The pre-multiplied mode formula is:
> > dst.RGB = src.RGB + dst.RGB * (1 - src.A)
> > 
> > Both formula has no destination alpha (I think background color is
> > last destination),
> > why coverage mode work fine but pre-multiplied mode has something
> > wrong?
> > 
> 
> alpha of coverage mode formula is:
> dst.a = dst.a x (0xff - src.a x SCA/0xff)/0xff + src.a x SCA/0xff;
> 
> alpha of pre-multiplied mode is:
> dst.a = round( {{dst.a x [0xff - round((src.a x SCA)/0xff)]} / 0xff} +
> round((src.a x SCA)/0xff) );
> 
> * SCA is constant alpha
> 
> If SCA = 0xff, then the formula will be:
> dst.a = dst.a x (0xff - src.a) + src.a
> 
> If dst.a is 0, then dst.a will be src.a
> If dst.a is 0xff, then dst.a will be 0xff

I'm confused that dst.a exist in both side of '='. Let me name them as dst.a.new and dst.a.old.
So calculate dst.a.new first, then calculate dst.RGB according to dst.a.new not dst.a.old?

If so, dst.a.new is influenced by SCA either in coverage mode and premultiplied mode.
This is to fix the bug already exist in coverage mode.

Regards,
CK

> 
> 
> Regards,
> Jason-JH Lin
> 
> > Regards,
> > CK
> > 
> > >  
> > >  	mtk_ddp_write(cmdq_pkt, 0x1, &ovl->cmdq_reg, ovl->regs,
> > > DISP_REG_OVL_RST);
> > >  	mtk_ddp_write(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs,
> > > DISP_REG_OVL_RST);
> > > @@ -374,7 +391,8 @@ void mtk_ovl_layer_off(struct device *dev,
> > > unsigned int idx,
> > >  		      DISP_REG_OVL_RDMA_CTRL(idx));
> > >  }
> > >  
> > > -static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl,
> > > unsigned int fmt)
> > > +static unsigned int ovl_fmt_convert(struct mtk_disp_ovl *ovl,
> > > unsigned int fmt,
> > > +				    unsigned int blend_mode)
> > >  {
> > >  	/* The return value in switch "MEM_MODE_INPUT_FORMAT_XXX"
> > >  	 * is defined in mediatek HW data sheet.
> > > @@ -395,22 +413,30 @@ static unsigned int ovl_fmt_convert(struct
> > > mtk_disp_ovl *ovl, unsigned int fmt)
> > >  	case DRM_FORMAT_RGBA8888:
> > >  	case DRM_FORMAT_RGBX1010102:
> > >  	case DRM_FORMAT_RGBA1010102:
> > > -		return OVL_CON_CLRFMT_RGBA8888;
> > > +		return blend_mode == DRM_MODE_BLEND_COVERAGE ?
> > > +		       OVL_CON_CLRFMT_RGBA8888 :
> > > +		       OVL_CON_CLRFMT_PRGBA8888;
> > >  	case DRM_FORMAT_BGRX8888:
> > >  	case DRM_FORMAT_BGRA8888:
> > >  	case DRM_FORMAT_BGRX1010102:
> > >  	case DRM_FORMAT_BGRA1010102:
> > > -		return OVL_CON_CLRFMT_BGRA8888;
> > > +		return blend_mode == DRM_MODE_BLEND_COVERAGE ?
> > > +		       OVL_CON_CLRFMT_BGRA8888 :
> > > +		       OVL_CON_CLRFMT_PBGRA8888;
> > >  	case DRM_FORMAT_XRGB8888:
> > >  	case DRM_FORMAT_ARGB8888:
> > >  	case DRM_FORMAT_XRGB2101010:
> > >  	case DRM_FORMAT_ARGB2101010:
> > > -		return OVL_CON_CLRFMT_ARGB8888;
> > > +		return blend_mode == DRM_MODE_BLEND_COVERAGE ?
> > > +		       OVL_CON_CLRFMT_ARGB8888 :
> > > +		       OVL_CON_CLRFMT_PARGB8888;
> > >  	case DRM_FORMAT_XBGR8888:
> > >  	case DRM_FORMAT_ABGR8888:
> > >  	case DRM_FORMAT_XBGR2101010:
> > >  	case DRM_FORMAT_ABGR2101010:
> > > -		return OVL_CON_CLRFMT_ABGR8888;
> > > +		return blend_mode == DRM_MODE_BLEND_COVERAGE ?
> > > +		       OVL_CON_CLRFMT_ABGR8888 :
> > > +		       OVL_CON_CLRFMT_PABGR8888;
> > >  	case DRM_FORMAT_UYVY:
> > >  		return OVL_CON_CLRFMT_UYVY | OVL_CON_MTX_YUV_TO_RGB;
> > >  	case DRM_FORMAT_YUYV:
> > > @@ -450,7 +476,7 @@ void mtk_ovl_layer_config(struct device *dev,
> > > unsigned int idx,
> > >  		return;
> > >  	}
> > >  
> > > -	con = ovl_fmt_convert(ovl, fmt);
> > > +	con = ovl_fmt_convert(ovl, fmt, blend_mode);
> > >  	if (state->base.fb) {
> > >  		con |= OVL_CON_AEN;
> > >  		con |= state->base.alpha & OVL_CON_ALPHA;