Message-ID: <CAD=FV=VmiS6G25JyAj4UGjU5Q9f80GcjLq1xwU4gdVyzDdk8LQ@mail.gmail.com>
Date:   Thu, 12 Oct 2023 19:28:11 -0700
From:   Doug Anderson <dianders@...gle.com>
To:     cong yang <yangcong5@...qin.corp-partner.google.com>
Cc:     sam@...nborg.org, neil.armstrong@...aro.org, daniel@...ll.ch,
        hsinyi@...gle.com, linus.walleij@...aro.org, swboyd@...omium.org,
        airlied@...il.com, catalin.marinas@....com, will@...nel.org,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [v3 2/3] drm/panel: ili9882t: Avoid blurred screen from fast sleep

Hi,

On Thu, Oct 12, 2023 at 6:12 PM cong yang
<yangcong5@...qin.corp-partner.google.com> wrote:
>
> Hi,
>
> On Thu, Oct 12, 2023 at 11:15 PM Doug Anderson <dianders@...gle.com> wrote:
> >
> > Hi,
> >
> > On Thu, Oct 12, 2023 at 5:10 AM Cong Yang
> > <yangcong5@...qin.corp-partner.google.com> wrote:
> > >
> > > At present, we have found that there may be a problem of blurred
> > > screen during fast sleep/resume. The direct cause of the blurred
> > > screen is that the IC does not receive 0x28/0x10. Because of the
> > > particularity of the IC, before the panel enters sleep hid must
> > > stop scanning, as i2c_hid_core_suspend before ili9882t_disable.
> > > If move the ili9882t_enter_sleep_mode function to ili9882t_unprepare,
> > > touch reset will pull low before panel entersleep, which does not meet
> > > the timing requirements..
> >
> > The above makes me believe that the reset GPIO should be moved out of
> > the input driver and into the panel driver. I could just imagine that
> > the kernel might have some reason it wants to suspend the i2c hid
> > device. If that causes the panel to suddenly start failing then that
> > would be bad... I think we should fix this.
>
> Thanks, I will confirm with ilitek in further analysis and use "move
> the ili9882t_enter_sleep_mode
> function to ili9882t_unprepare".  Is the test failure really because
> the touch reset timing
> does not match? There is also a separate reset GPIO on the panel.
> Shouldn't touch reset not
> affect the panel?
>
> If we find a better solution I will continue upstream,。 So is it
> possible to apply this plan now?

I wouldn't be too upset at applying the current code as long as you're
going to continue to investigate. We can always continue to iterate on
it and having something working reasonably well is better than nothing
at all. However, I probably would wait at least 1 week before applying
any patch from you just simply out of courtesy to give others on the
mailing list time to express their comments. ...presumably we could
get to the bottom of the problem in that 1 week time anyway...

I'm not trying to be an obstinate pain here--I'm merely trying to make
sure that whatever we land will continue to work across kernel uprevs,
even if driver probe order / timing changes in the kernel. If the
panel is really so tied to the touchscreen device's reset GPIO timing
then it worries me. What happens, for instance, if you disable the
touchscreen CONFIG in the kernel? Does the panel still work, or is
that extra reset GPIO totally critical to the functioning of the
panel. If it's totally critical then it probably makes sense to move
to the panel driver given that the touchscreen is a panel follower
anyway...


> > > So in order to solve this problem, the IC
> > > can handle it through the exception mechanism when it cannot receive
> > > 0x28/0x10 command. Handling exceptions requires a reset 50ms delay.
> > > Refer to vendor detailed analysis [1].
> > >
> > > Ilitek vendor also suggested switching the page before entering sleep to
> > > avoid panel IC not receiving 0x28/0x10 command.
> > >
> > > Note: 0x28 is display off, 0x10 is sleep in.
> > >
> > > [1]: https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence
> > >
> > > Signed-off-by: Cong Yang <yangcong5@...qin.corp-partner.google.com>
> > > ---
> > >  drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 22 ++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > > index 93a40c2f1483..54ff1efb94aa 100644
> > > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c
> > > @@ -463,6 +463,24 @@ static int ili9882t_init_dcs_cmd(struct ili9882t *ili)
> > >         return 0;
> > >  }
> > >
> > > +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page)
> > > +{
> > > +       int ret;
> > > +       const struct panel_init_cmd cmd = _INIT_SWITCH_PAGE_CMD(page);
> > > +
> > > +       ret = mipi_dsi_dcs_write(dsi, cmd.data[0],
> > > +                                cmd.len <= 1 ? NULL :
> > > +                                &cmd.data[1],
> > > +                                cmd.len - 1);
> > > +       if (ret) {
> > > +               dev_err(&dsi->dev,
> > > +                       "error switching panel controller page (%d)\n", ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
> > >  {
> > >         struct mipi_dsi_device *dsi = ili->dsi;
> > > @@ -484,8 +502,10 @@ static int ili9882t_enter_sleep_mode(struct ili9882t *ili)
> > >  static int ili9882t_disable(struct drm_panel *panel)
> > >  {
> > >         struct ili9882t *ili = to_ili9882t(panel);
> > > +       struct mipi_dsi_device *dsi = ili->dsi;
> > >         int ret;
> > >
> > > +       ili9882t_switch_page(dsi, 0x00);
> > >         ret = ili9882t_enter_sleep_mode(ili);
> > >         if (ret < 0) {
> > >                 dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> > > @@ -546,7 +566,7 @@ static int ili9882t_prepare(struct drm_panel *panel)
> > >         gpiod_set_value(ili->enable_gpio, 1);
> > >         usleep_range(1000, 2000);
> > >         gpiod_set_value(ili->enable_gpio, 0);
> > > -       usleep_range(1000, 2000);
> > > +       usleep_range(50000, 51000);
> >
> > From my previous response, I think the above is better as msleep(50).
>
> Sorry. Will be corrected in V4.

Thanks! It's not a huge deal, but it's nice to fix.

-Doug

Powered by blists - more mailing lists