[PATCH] drm: rcar-du: Add setting to PnALPHAR register on Gen3

[Laurent Pinchart]

Hi Biju,

On Sun, Apr 24, 2022 at 04:12:08PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH] drm: rcar-du: Add setting to PnALPHAR register on Gen3
> > On Sat, Apr 23, 2022 at 08:37:28AM +0100, Biju Das wrote:
> > > From: LUU HOAI <hoai.luu.ub at renesas.com>
> > >
> > > In Gen3, when Alpha blend is enabled in the PnMR register, depending
> > > on the initial value of the PnALPHAR register, either channel of DU
> > > might be black screen.
> > > Therefore, this patch prevents the black screen by setting the
> > > PnALPHAR register to all 0.
> > >
> > > In addition, PnALPHAR register will be released in the R-Car Gen3
> > > Hardware Manual Rev 2.4 (Sep. 2021).
> > >
> > > Signed-off-by: LUU HOAI <hoai.luu.ub at renesas.com>
> > > Signed-off-by: Biju Das <biju.das.jz at bp.renesas.com>
> > > ---
> > > This patch is based on [1]
> > > [1]
> > >
> > > Not sure this patches has to go with Fixes tag for stable??
> > >
> > > Tested the changes on RZ/G2M board
> > >
> > > root at hihope-rzg2m:/cip-test-scripts#  modetest -M rcar-du -w
> > > 54:alpha:55555 root at hihope-rzg2m:/cip-test-scripts# modetest -M rcar-du
> > -s "93 at 90:1024x768 at AR24" -d -P "54 at 90:400x300+200+200 at XR24"
> > > setting mode 1024x768-75Hz at AR24 on connectors 93, crtc 90 testing
> > > 400x300 at XR24 overlay plane 54
> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > > b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > > index 5c1c7bb04f3f..aff39b9253f8 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > > @@ -510,6 +510,12 @@ static void
> > > rcar_du_plane_setup_format_gen3(struct rcar_du_group *rgrp,
> > >
> > >  	rcar_du_plane_write(rgrp, index, PnDDCR4,
> > >  			    state->format->edf | PnDDCR4_CODE);
> > > +
> > > +	/* In Gen3, PnALPHAR register need to be set to 0
> > > +	 * to avoid black screen issue when alpha blend is enable
> > > +	 * on DU module
> > > +	 */
> > 
> > Comments should start with /* on a line of its own, and you can also
> > reflow the text to 80 columns:
> 
> OK.
> 
> > 	/*
> > 	 * In Gen3, PnALPHAR register need to be set to 0 to avoid black screen
> > 	 * issue when alpha blend is enable on DU module.
> > 	 */
> > 
> > It would however be nicer to document the exact behaviour, but the latest
> > version of the documentation I have access to is rev 2.3 and it lists
> > PnALPHAR as not available on Gen3.
> 
> I don't have access to rev 2.4, but I got access to 
> "R-Car-Gen3_Common_OPC_Customer_Notifications_V30.1.pdf" 
> where it is mentioned about this issue and solution for fix
> which is inline with the patch from R-Car BSP.
> 
> "The reason is that a register is not initialized by reset.
> This could lead to output wrong image data of other plane or 
> wrong color set from BPOR (Background plane output register)."
> 
> > Furthermore, is this really the right fix, shouldn't we instead avoid
> > enabling alpha-blending in PnMR on Gen3 ?
> 
> Avoid enabling alpha-blending in PnMR on Gen3, will it help here?

It's hard to tell without knowing the exact cause of the issue. Clearing
PnALPHAR probably makes sense on Gen3 if the register exists,
independently from disabling alpha blending in PnMR. It would be nice if
the commit messsage could reference the issue described in
R-Car-Gen3_Common_OPC_Customer_Notifications_V30.1.pdf. I would also
expand the comment a little bit:

 /*
  * On Gen3, some DU channels have two planes, each being wired to a separate
  * VSPD instance. The DU can then blend two two planes. While this feature
  * isn't used by the driver, issues related to alpha blending (such as
  * incorrect colors or planes being invisible) may still occur if the PnALPHAR
  * register has a stale value. Set the register to 0 to avoid this.
  */

> Here the issue they mentioned as "register is not initialized by reset"
> 
> > > +	rcar_du_plane_write(rgrp, index, PnALPHAR, 0x00000000);

I'd write 0 instead of 0x00000000 to match the rest of the driver.

Would you mind sending a v2 with these changed and an expanded commit
message ?

> > >  }
> > >
> > >  static void rcar_du_plane_setup_format(struct rcar_du_group *rgrp,

-- 
Regards,

Laurent Pinchart