Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH] drm: rcar-du: Add setting to PnALPHAR register on > Gen3 > > 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@xxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxx> > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > > > --- > > > > 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@hihope-rzg2m:/cip-test-scripts# modetest -M rcar-du -w > > > > 54:alpha:55555 root@hihope-rzg2m:/cip-test-scripts# modetest -M > > > > rcar-du > > > -s "93@90:1024x768@AR24" -d -P "54@90:400x300+200+200@XR24" > > > > setting mode 1024x768-75Hz@AR24 on connectors 93, crtc 90 testing > > > > 400x300@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. > */ OK. > > > 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 ? OK, Will send v2 with these changes. Cheers, Biju > > > > > } > > > > > > > > static void rcar_du_plane_setup_format(struct rcar_du_group > > > > *rgrp, > > -- > Regards, > > Laurent Pinchart
