On Wed, Apr 7, 2021 at 12:11 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx> wrote: > > Il 07/04/21 20:19, abhinavk@xxxxxxxxxxxxxx ha scritto: > > Hi Marijn > > > > On 2021-04-06 14:47, Marijn Suijten wrote: > >> Leaving this at a close-to-maximum register value 0xFFF0 means it takes > >> very long for the MDSS to generate a software vsync interrupt when the > >> hardware TE interrupt doesn't arrive. Configuring this to double the > >> vtotal (like some downstream kernels) leads to a frame to take at most > >> twice before the vsync signal, until hardware TE comes up. > >> > >> In this case the hardware interrupt responsible for providing this > >> signal - "disp-te" gpio - is not hooked up to the mdp5 vsync/pp logic at > >> all. This solves severe panel update issues observed on at least the > >> Xperia Loire and Tone series, until said gpio is properly hooked up to > >> an irq. > > > > The reason the CONFIG_HEIGHT was at such a high value is to make sure that > > we always get the TE only from the panel vsync and not false positives > > coming > > from the tear check logic itself. > > > > When you say that disp-te gpio is not hooked up, is it something > > incorrect with > > the schematic OR panel is not generating the TE correctly? > > > > Sometimes, some panels aren't getting correctly configured by the > OEM/ODM in the first place: especially when porting devices from > downstream to upstream, developers often get in a situation in which > their TE line is either misconfigured or the DriverIC is not configured > to raise V-Sync interrupts. > Please remember: some DDICs need a "commands sequence" to enable > generating the TE interrupts, sometimes this is not standard, and > sometimes OEMs/ODMs are not even doing that in their downstream code > (but instead they work around it in creative ways "for reasons", even > though their DDIC supports indeed sending TE events). > > This mostly happens when bringing up devices that have autorefresh > enabled from the bootloader (when the bootloader sets up the splash > screen) by using simple-panel as a (hopefully) temporary solution to get > through the initial stages of porting. > > We are not trying to cover cases related to incorrect schematics or > hardware mistakes here, as the fix for that - as you know - is to just > fix your hardware. > What we're trying to do here is to stop freezes and, in some cases, > lockups, other than false positives making the developer go offroad when > the platform shows that something is wrong during early porting. > > Also, sometimes, some DDICs will not generate TE interrupts when > expected... in these cases we get a PP timeout and a MDP5 recovery: this > is totally avoidable if we rely on the 2*vtotal, as we wouldn't get > through the very time consuming task of recovering the entire MDP. > > Of course, if something is wrong in the MDP and the block really needs > recovery, this "trick" won't save anyone and the recovery will anyway be > triggered, as the PP-done will anyway timeout. So, is this (mostly) a workaround due to TE not wired up? In which case I think it is ok, but maybe should have a comment about the interaction with TE? Currently I have this patch in msm-next-staging but I guess we need to decide in the next day or so whether to drop it or smash in a comment? BR, -R > >> > >> Suggested-by: AngeloGioacchino Del Regno > >> <angelogioacchino.delregno@xxxxxxxxxxxxxx> > >> Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> > >> Reviewed-by: AngeloGioacchino Del Regno > >> <angelogioacchino.delregno@xxxxxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > >> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > >> index ff2c1d583c79..2d5ac03dbc17 100644 > >> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > >> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > >> @@ -51,7 +51,7 @@ static int pingpong_tearcheck_setup(struct > >> drm_encoder *encoder, > >> > >> mdp5_write(mdp5_kms, REG_MDP5_PP_SYNC_CONFIG_VSYNC(pp_id), cfg); > >> mdp5_write(mdp5_kms, > >> - REG_MDP5_PP_SYNC_CONFIG_HEIGHT(pp_id), 0xfff0); > >> + REG_MDP5_PP_SYNC_CONFIG_HEIGHT(pp_id), (2 * mode->vtotal)); > >> mdp5_write(mdp5_kms, > >> REG_MDP5_PP_VSYNC_INIT_VAL(pp_id), mode->vdisplay); > >> mdp5_write(mdp5_kms, REG_MDP5_PP_RD_PTR_IRQ(pp_id), > >> mode->vdisplay + 1); >
