diff mbox series

[1/3] drm/msm/mdp5: Configure PP_SYNC_HEIGHT to double the vtotal

Message ID 20210406214726.131534-2-marijn.suijten@somainline.org (mailing list archive)
State New
Headers show
Series drm/msm/mdp5: Emit vsync signal often enough | expand

Commit Message

Marijn Suijten April 6, 2021, 9:47 p.m. UTC
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.

Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Abhinav Kumar April 7, 2021, 6:19 p.m. UTC | #1
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?

> 
> Suggested-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org>
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org>
> ---
>  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);
AngeloGioacchino Del Regno April 7, 2021, 7:11 p.m. UTC | #2
Il 07/04/21 20:19, abhinavk@codeaurora.org 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.

>>
>> Suggested-by: AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@somainline.org>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> Reviewed-by: AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@somainline.org>
>> ---
>>  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);
Rob Clark April 8, 2021, 7:05 p.m. UTC | #3
On Wed, Apr 7, 2021 at 12:11 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 07/04/21 20:19, abhinavk@codeaurora.org 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@somainline.org>
> >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> Reviewed-by: AngeloGioacchino Del Regno
> >> <angelogioacchino.delregno@somainline.org>
> >> ---
> >>  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);
>
Rob Clark April 9, 2021, 12:08 a.m. UTC | #4
On Thu, Apr 8, 2021 at 4:16 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
>
> Il gio 8 apr 2021, 21:05 Rob Clark <robdclark@gmail.com> ha scritto:
>>
>> On Wed, Apr 7, 2021 at 12:11 PM AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@somainline.org> wrote:
>> >
>> > Il 07/04/21 20:19, abhinavk@codeaurora.org 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?
>
>
> Mostly, yes.
>
>>
>> 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
>
>
> Marijn, can you please urgently throw a comment in, reminding that these timers are interacting with TE and send a fast V2?
>

Or just reply on list w/ a comment to smash in, if that is easier

BR,
-R
Marijn Suijten April 9, 2021, 8:22 a.m. UTC | #5
Hi Abhinav, Angelo, Rob,

On 4/9/21 2:08 AM, Rob Clark wrote:
> On Thu, Apr 8, 2021 at 4:16 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>>
>>
>> Il gio 8 apr 2021, 21:05 Rob Clark <robdclark@gmail.com> ha scritto:
>>>
>>> On Wed, Apr 7, 2021 at 12:11 PM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@somainline.org> wrote:
>>>>
>>>> Il 07/04/21 20:19, abhinavk@codeaurora.org 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.


Correct, most CAF kernels mention such behaviour in comments, with
fallbacks to vtotal*2 for safety or vtotal when an emulated panel does
not support hardware TE at all.  We don't seem to (need to) support the 
latter case but one might at some point want to configure the tearcheck 
logic to emit a signal for every frame, by means of a DT property or 
automatically when disp-te is not defined.

>>>>>
>>>>> 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?
>>>>>


The GPIO is currently not used at all by this kernel driver besides a
call to devm_gpiod_get_optional.  As mentioned in the cover letter we
have patches in progress to hook up this interrupt line to the pp_done
infrastructure and complete vsync requests that way instead of waiting
for this "simulated" interrupt from the MDP.

>>> 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?
>>
>> Marijn, can you please urgently throw a comment in, reminding that these timers are interacting with TE and send a fast V2?
> 
> Or just reply on list w/ a comment to smash in, if that is easier

How about a comment along the lines of:

Tearcheck emits a blanking signal every vclks_line * vtotal * 2 ticks on 
the vsync_clk equating to roughly half the desired panel refresh rate. 
This is only necessary as stability fallback if interrupts from the 
panel arrive too late or not at all, but is currently used by default 
because these panel interrupts are not wired up yet.

I'd place this comment right above REG_MDP5_PP_SYNC_CONFIG_VSYNC, and 
perhaps add a newline after REG_MDP5_PP_SYNC_CONFIG_HEIGHT to make it 
clear this applies to those two registers specifically.  We can also 
involve MDP5_PP_SYNC_CONFIG_VSYNC_COUNT(vclks_line) in the mix.

Then, when the panel TE is wired up we can be smarter about the 
situation and print warnings when the user has disp-te hooked up but is 
receiving interrupts from the MDSS block instead of the panel directly 
(except if incidentally).  This likely means that SET_TEAR_ON was not 
sent to the panel or the GPIO is wrong.  In that sense this patch 
(together with x100 removal) is concealing configuration mistakes, but 
we might also revert back to 0xfff0 if the GPIO is specified in DT and 
accept the timeout+recovery which did not seem to hamper devices on 
downstream kernels anyway.

- Marijn
diff mbox series

Patch

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);