| Message ID | 20231007060639.725350-3-yangcong5@huaqin.corp-partner.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Break out as separate driver from boe-tv101wum-nl6 panel driver | expand |
Hi, On Fri, Oct 6, 2023 at 11:07 PM Cong Yang <yangcong5@huaqin.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, i2c_hid_core_suspend before ili9882t_disable. > This doesn't look very spec-compliant. Presumably you could be more spec compliant if we used "panel_follower" in this case? Would that be a better solution? > 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@huaqin.corp-partner.google.com> > --- > drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 21 ++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c > index bbfcffe65623..0a1dd987b204 100644 > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c > @@ -423,6 +423,23 @@ static inline struct ili9882t *to_ili9882t(struct drm_panel *panel) > return container_of(panel, struct ili9882t, base); > } > > +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page) > +{ > + u8 switch_cmd[] = {0x98, 0x82, 0x00}; Can't you just replace the last 0x00 above with "page" and get rid of the manual assignment below? > + int ret; > + > + switch_cmd[2] = page; > + > + ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3); Instead of hardcoding 3, should use ARRAY_SIZE(). > + if (ret) { > + dev_err(&dsi->dev, > + "error switching panel controller page (%d)\n", ret); > + return ret; > + } > + > + return 0; > +} optional: It feels like it would be nice to somehow use the "_INIT_SWITCH_PAGE_CMD" macro I suggested in patch #1 instead of having to hardcode 0x98, 0x82 again. In patch #1 I already suggested breaking out the function to send a sequence of commands. If you had that function take a pointer instead of hardcoding it to look at ->init_cmds then you could probably use the same function that you do at init time? > static int ili9882t_enter_sleep_mode(struct ili9882t *ili) > { > struct mipi_dsi_device *dsi = ili->dsi; > @@ -444,8 +461,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); > @@ -507,7 +526,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(40000, 50000); nit: use 40000, 41000 instead of 40000, 50000. Linux almost always uses the longer delay, so that'll save ~9 ms. The only reason for the range is to optimize kernel wakeups which is really not a concern here.
Hi, On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang > <yangcong5@huaqin.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, i2c_hid_core_suspend before ili9882t_disable. > > This doesn't look very spec-compliant. > > Presumably you could be more spec compliant if we used > "panel_follower" in this case? Would that be a better solution? In the "panel_follower" solution, the phenomenon is the same. The current order is ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare, ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before ili9882t_disable. > > > > 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@huaqin.corp-partner.google.com> > > --- > > drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 21 ++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c > > index bbfcffe65623..0a1dd987b204 100644 > > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c > > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c > > @@ -423,6 +423,23 @@ static inline struct ili9882t *to_ili9882t(struct drm_panel *panel) > > return container_of(panel, struct ili9882t, base); > > } > > > > +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page) > > +{ > > + u8 switch_cmd[] = {0x98, 0x82, 0x00}; > > Can't you just replace the last 0x00 above with "page" and get rid of > the manual assignment below? > > > > + int ret; > > + > > + switch_cmd[2] = page; > > + > > + ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3); > > Instead of hardcoding 3, should use ARRAY_SIZE(). > > > > + if (ret) { > > + dev_err(&dsi->dev, > > + "error switching panel controller page (%d)\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > optional: It feels like it would be nice to somehow use the > "_INIT_SWITCH_PAGE_CMD" macro I suggested in patch #1 instead of > having to hardcode 0x98, 0x82 again. In patch #1 I already suggested > breaking out the function to send a sequence of commands. If you had > that function take a pointer instead of hardcoding it to look at > ->init_cmds then you could probably use the same function that you do > at init time? > > > > static int ili9882t_enter_sleep_mode(struct ili9882t *ili) > > { > > struct mipi_dsi_device *dsi = ili->dsi; > > @@ -444,8 +461,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); > > @@ -507,7 +526,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(40000, 50000); > > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always > uses the longer delay, so that'll save ~9 ms. The only reason for the > range is to optimize kernel wakeups which is really not a concern > here. We need 50ms delay to meet the requirement.
Hi, On Tue, Oct 10, 2023 at 4:36 AM cong yang <yangcong5@huaqin.corp-partner.google.com> wrote: > > Hi, > > On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote: > > > > Hi, > > > > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang > > <yangcong5@huaqin.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, i2c_hid_core_suspend before ili9882t_disable. > > > This doesn't look very spec-compliant. > > > > Presumably you could be more spec compliant if we used > > "panel_follower" in this case? Would that be a better solution? > > In the "panel_follower" solution, the phenomenon is the same. > The current order is > ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare, > ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before > ili9882t_disable. Ugh, that's unfortunate. Though is there a reason why you couldn't just move the `ili9882t_enter_sleep_mode()` to `ili9882t_unprepare()`? That seems like it should be OK and even perhaps makes it more symmetric with thue enable? > > > @@ -507,7 +526,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(40000, 50000); > > > > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always > > uses the longer delay, so that'll save ~9 ms. The only reason for the > > range is to optimize kernel wakeups which is really not a concern > > here. > > We need 50ms delay to meet the requirement. I'll respond to your v2, but if you need 50 ms then your current delay is wrong. -Doug
Hi, On Wed, Oct 11, 2023 at 3:11 AM Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Tue, Oct 10, 2023 at 4:36 AM cong yang > <yangcong5@huaqin.corp-partner.google.com> wrote: > > > > Hi, > > > > On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote: > > > > > > Hi, > > > > > > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang > > > <yangcong5@huaqin.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, i2c_hid_core_suspend before ili9882t_disable. > > > > This doesn't look very spec-compliant. > > > > > > Presumably you could be more spec compliant if we used > > > "panel_follower" in this case? Would that be a better solution? > > > > In the "panel_follower" solution, the phenomenon is the same. > > The current order is > > ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare, > > ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before > > ili9882t_disable. > > Ugh, that's unfortunate. Though is there a reason why you couldn't > just move the `ili9882t_enter_sleep_mode()` to `ili9882t_unprepare()`? > That seems like it should be OK and even perhaps makes it more > symmetric with thue enable? Thank you for your suggestion. If the timing is met and there is no problem, I will implement it in V3. > > > > > > @@ -507,7 +526,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(40000, 50000); > > > > > > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always > > > uses the longer delay, so that'll save ~9 ms. The only reason for the > > > range is to optimize kernel wakeups which is really not a concern > > > here. > > > > We need 50ms delay to meet the requirement. > > I'll respond to your v2, but if you need 50 ms then your current delay is wrong. > > > -Doug
Hi, On Wed, Oct 11, 2023 at 3:11 AM Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Tue, Oct 10, 2023 at 4:36 AM cong yang > <yangcong5@huaqin.corp-partner.google.com> wrote: > > > > Hi, > > > > On Tue, Oct 10, 2023 at 4:44 AM Doug Anderson <dianders@google.com> wrote: > > > > > > Hi, > > > > > > On Fri, Oct 6, 2023 at 11:07 PM Cong Yang > > > <yangcong5@huaqin.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, i2c_hid_core_suspend before ili9882t_disable. > > > > This doesn't look very spec-compliant. > > > > > > Presumably you could be more spec compliant if we used > > > "panel_follower" in this case? Would that be a better solution? > > > > In the "panel_follower" solution, the phenomenon is the same. > > The current order is > > ili9882t_disable=>i2c_hid_core_suspend=>elan_i2c_hid_power_down=>ili9882t_unprepare, > > ili9882t need touchpanel stop scanning,i2c_hid_core_suspend before > > ili9882t_disable. > > Ugh, that's unfortunate. Though is there a reason why you couldn't > just move the `ili9882t_enter_sleep_mode()` to `ili9882t_unprepare()`? > That seems like it should be OK and even perhaps makes it more > symmetric with thue enable? It looks like the test will still fail, because the touchpanel reset was already pulled low before the panel enter sleep, which did not seem to meet the timing requirements. I will explain this in the V3 cover letter. Thanks. > > > > > > @@ -507,7 +526,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(40000, 50000); > > > > > > nit: use 40000, 41000 instead of 40000, 50000. Linux almost always > > > uses the longer delay, so that'll save ~9 ms. The only reason for the > > > range is to optimize kernel wakeups which is really not a concern > > > here. > > > > We need 50ms delay to meet the requirement. > > I'll respond to your v2, but if you need 50 ms then your current delay is wrong. > > > -Doug
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c index bbfcffe65623..0a1dd987b204 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9882t.c @@ -423,6 +423,23 @@ static inline struct ili9882t *to_ili9882t(struct drm_panel *panel) return container_of(panel, struct ili9882t, base); } +static int ili9882t_switch_page(struct mipi_dsi_device *dsi, u8 page) +{ + u8 switch_cmd[] = {0x98, 0x82, 0x00}; + int ret; + + switch_cmd[2] = page; + + ret = mipi_dsi_dcs_write(dsi, ILI9882T_DCS_SWITCH_PAGE, switch_cmd, 3); + 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; @@ -444,8 +461,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); @@ -507,7 +526,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(40000, 50000); gpiod_set_value(ili->enable_gpio, 1); usleep_range(6000, 10000);
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, i2c_hid_core_suspend before ili9882t_disable. This doesn't look very spec-compliant. 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@huaqin.corp-partner.google.com> --- drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)