Re: [v3 2/3] drm/panel: ili9882t: Avoid blurred screen from fast sleep

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Fri, Oct 13, 2023 at 10:28 AM Doug Anderson <dianders@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, Oct 12, 2023 at 6:12 PM cong yang
> <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Thu, Oct 12, 2023 at 11:15 PM Doug Anderson <dianders@xxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Oct 12, 2023 at 5:10 AM Cong Yang
> > > <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> 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...

Thanks. It looks like the panel works fine after I disable the touch screen
device. So the panel may not depend on touch screen reset.
Need to continue investigating the root cause for current status.

>
>
> > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux