| Message ID | 20240807100429.13260-2-lvzhaoxiong@huaqin.corp-partner.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Modify the method of sending "exit sleep | expand |
On 8/7/2024 3:04 AM, Zhaoxiong Lv wrote: > Move the "exit sleep mode" and "set display on" command from > enable() to init() function. > > As mentioned in the patch: > https://lore.kernel.org/all/20240624141926.5250-2-lvzhaoxiong@huaqin.corp-partner.google.com/ > > The Mediatek Soc DSI host has different modes in prepare() and > enable() functions, prepare() is in LP mode and enable() is in > HS mode. Since the "exit sleep mode" and "set display on" > command must also be sent in LP mode, so we also move "exit > sleep mode" and "set display on" command to the init() function. > > We have no other actions in the enable() function after moves > "exit sleep mode" and "set display on", and we checked the call > of the enable() function during the "startup" process. It seems > that only one judgment was made in drm_panel_enabel(). If the > panel does not define enable(), the judgment will skip the > enable() and continue execution. This does not seem to have > any other effect, and we found that some drivers also seem > to have no enable() function added, for example: > panel-asus-z00t-tm5p5-n35596 / panel-boe-himax8279d... > In addition, we briefly tested the kingdisplay_kd101ne3 panel and > melfas_lmfbx101117480 panel, and it seems that there is no garbage > on the panel, so we delete enable() function. > > After moving the "exit sleep mode" and "set display on" command > to the init() function, we no longer need additional delay > judgment, so we delete variables "exit_sleep_to_display_on_delay_ms" > and "display_on_delay_ms". > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > Changes between V3 and V2: > - 1. The code has not changed, Just modified the nit in the commit > - information mentioned by Doug. > v2: https://lore.kernel.org/all/20240806034015.11884-2-lvzhaoxiong@huaqin.corp-partner.google.com/ > > Changes between V2 and V1: > - 1. The code has not changed, Modify the commit information. > v1: https://lore.kernel.org/all/20240725083245.12253-2-lvzhaoxiong@huaqin.corp-partner.google.com/ > --- > .../gpu/drm/panel/panel-jadard-jd9365da-h3.c | 59 ++++++++++--------- > 1 file changed, 32 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > index 04d315d96bff..ce73e8cb1db5 100644 > --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c > @@ -31,8 +31,6 @@ struct jadard_panel_desc { > bool reset_before_power_off_vcioo; > unsigned int vcioo_to_lp11_delay_ms; > unsigned int lp11_to_reset_delay_ms; > - unsigned int exit_sleep_to_display_on_delay_ms; > - unsigned int display_on_delay_ms; > unsigned int backlight_off_to_display_off_delay_ms; > unsigned int display_off_to_enter_sleep_delay_ms; > unsigned int enter_sleep_to_reset_down_delay_ms; > @@ -66,26 +64,6 @@ static inline struct jadard *panel_to_jadard(struct drm_panel *panel) > return container_of(panel, struct jadard, panel); > } > > -static int jadard_enable(struct drm_panel *panel) > -{ > - struct jadard *jadard = panel_to_jadard(panel); > - struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi }; > - > - msleep(120); > - > - mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > - > - if (jadard->desc->exit_sleep_to_display_on_delay_ms) > - mipi_dsi_msleep(&dsi_ctx, jadard->desc->exit_sleep_to_display_on_delay_ms); > - > - mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); > - > - if (jadard->desc->display_on_delay_ms) > - mipi_dsi_msleep(&dsi_ctx, jadard->desc->display_on_delay_ms); > - > - return dsi_ctx.accum_err; > -} > - > static int jadard_disable(struct drm_panel *panel) > { > struct jadard *jadard = panel_to_jadard(panel); > @@ -202,7 +180,6 @@ static const struct drm_panel_funcs jadard_funcs = { > .disable = jadard_disable, > .unprepare = jadard_unprepare, > .prepare = jadard_prepare, > - .enable = jadard_enable, > .get_modes = jadard_get_modes, > .get_orientation = jadard_panel_get_orientation, > }; > @@ -382,6 +359,12 @@ static int radxa_display_8hd_ad002_init_cmds(struct jadard *jadard) > > jd9365da_switch_page(&dsi_ctx, 0x00); > > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > + > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); > + > return dsi_ctx.accum_err; > }; > > @@ -608,6 +591,12 @@ static int cz101b4001_init_cmds(struct jadard *jadard) > mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xE6, 0x02); > mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xE7, 0x0C); > > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > + > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); > + > return dsi_ctx.accum_err; > }; > > @@ -831,6 +820,16 @@ static int kingdisplay_kd101ne3_init_cmds(struct jadard *jadard) > > jd9365da_switch_page(&dsi_ctx, 0x00); > > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > + > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); > + > + mipi_dsi_msleep(&dsi_ctx, 20); > + > return dsi_ctx.accum_err; > }; > > @@ -859,8 +858,6 @@ static const struct jadard_panel_desc kingdisplay_kd101ne3_40ti_desc = { > .reset_before_power_off_vcioo = true, > .vcioo_to_lp11_delay_ms = 5, > .lp11_to_reset_delay_ms = 10, > - .exit_sleep_to_display_on_delay_ms = 120, > - .display_on_delay_ms = 20, > .backlight_off_to_display_off_delay_ms = 100, > .display_off_to_enter_sleep_delay_ms = 50, > .enter_sleep_to_reset_down_delay_ms = 100, > @@ -1074,6 +1071,16 @@ static int melfas_lmfbx101117480_init_cmds(struct jadard *jadard) > mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe6, 0x02); > mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe7, 0x06); > > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > + > + mipi_dsi_msleep(&dsi_ctx, 120); > + > + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); > + > + mipi_dsi_msleep(&dsi_ctx, 20); > + > return dsi_ctx.accum_err; > }; > > @@ -1102,8 +1109,6 @@ static const struct jadard_panel_desc melfas_lmfbx101117480_desc = { > .reset_before_power_off_vcioo = true, > .vcioo_to_lp11_delay_ms = 5, > .lp11_to_reset_delay_ms = 10, > - .exit_sleep_to_display_on_delay_ms = 120, > - .display_on_delay_ms = 20, > .backlight_off_to_display_off_delay_ms = 100, > .display_off_to_enter_sleep_delay_ms = 50, > .enter_sleep_to_reset_down_delay_ms = 100, > -- > 2.17.1 >
Jessica, On Thu, Aug 8, 2024 at 3:56 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > > > > On 8/7/2024 3:04 AM, Zhaoxiong Lv wrote: > > Move the "exit sleep mode" and "set display on" command from > > enable() to init() function. > > > > As mentioned in the patch: > > https://lore.kernel.org/all/20240624141926.5250-2-lvzhaoxiong@huaqin.corp-partner.google.com/ > > > > The Mediatek Soc DSI host has different modes in prepare() and > > enable() functions, prepare() is in LP mode and enable() is in > > HS mode. Since the "exit sleep mode" and "set display on" > > command must also be sent in LP mode, so we also move "exit > > sleep mode" and "set display on" command to the init() function. > > > > We have no other actions in the enable() function after moves > > "exit sleep mode" and "set display on", and we checked the call > > of the enable() function during the "startup" process. It seems > > that only one judgment was made in drm_panel_enabel(). If the > > panel does not define enable(), the judgment will skip the > > enable() and continue execution. This does not seem to have > > any other effect, and we found that some drivers also seem > > to have no enable() function added, for example: > > panel-asus-z00t-tm5p5-n35596 / panel-boe-himax8279d... > > In addition, we briefly tested the kingdisplay_kd101ne3 panel and > > melfas_lmfbx101117480 panel, and it seems that there is no garbage > > on the panel, so we delete enable() function. > > > > After moving the "exit sleep mode" and "set display on" command > > to the init() function, we no longer need additional delay > > judgment, so we delete variables "exit_sleep_to_display_on_delay_ms" > > and "display_on_delay_ms". > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Signed-off-by: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> > > Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com> Does this Ack mean you're confident enough about this patch that we should go ahead and merge it, or do you think we should wait on anything else (like Neil getting a chance to look at it)? As I mentioned in my reply to the cover letter [1] the patches look OK to me but I still don't consider myself to have a wonderful understanding of the intricacies of MIPI DSI. If you think this is OK from a MIPI DSI point of view then we can land it... [1] https://lore.kernel.org/r/CAD=FV=WCw6pAump-PUFCW0cgbRY+5_2tPNLe=hN3-dnXD=B6MA@mail.gmail.com Thanks! -Doug
On 12/08/2024 17:45, Doug Anderson wrote: > Jessica, > > On Thu, Aug 8, 2024 at 3:56 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote: >> >> >> >> On 8/7/2024 3:04 AM, Zhaoxiong Lv wrote: >>> Move the "exit sleep mode" and "set display on" command from >>> enable() to init() function. >>> >>> As mentioned in the patch: >>> https://lore.kernel.org/all/20240624141926.5250-2-lvzhaoxiong@huaqin.corp-partner.google.com/ >>> >>> The Mediatek Soc DSI host has different modes in prepare() and >>> enable() functions, prepare() is in LP mode and enable() is in >>> HS mode. Since the "exit sleep mode" and "set display on" >>> command must also be sent in LP mode, so we also move "exit >>> sleep mode" and "set display on" command to the init() function. >>> >>> We have no other actions in the enable() function after moves >>> "exit sleep mode" and "set display on", and we checked the call >>> of the enable() function during the "startup" process. It seems >>> that only one judgment was made in drm_panel_enabel(). If the >>> panel does not define enable(), the judgment will skip the >>> enable() and continue execution. This does not seem to have >>> any other effect, and we found that some drivers also seem >>> to have no enable() function added, for example: >>> panel-asus-z00t-tm5p5-n35596 / panel-boe-himax8279d... >>> In addition, we briefly tested the kingdisplay_kd101ne3 panel and >>> melfas_lmfbx101117480 panel, and it seems that there is no garbage >>> on the panel, so we delete enable() function. >>> >>> After moving the "exit sleep mode" and "set display on" command >>> to the init() function, we no longer need additional delay >>> judgment, so we delete variables "exit_sleep_to_display_on_delay_ms" >>> and "display_on_delay_ms". >>> >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>> Signed-off-by: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> >> >> Acked-by: Jessica Zhang <quic_jesszhan@quicinc.com> > > Does this Ack mean you're confident enough about this patch that we > should go ahead and merge it, or do you think we should wait on > anything else (like Neil getting a chance to look at it)? As I > mentioned in my reply to the cover letter [1] the patches look OK to > me but I still don't consider myself to have a wonderful understanding > of the intricacies of MIPI DSI. If you think this is OK from a MIPI > DSI point of view then we can land it... I don't have an advanced MIPI DSI knowledge, but they simple remove unneeded sleeps between dcs commands, so if they are confident they can be removed I agree... Neil > > [1] https://lore.kernel.org/r/CAD=FV=WCw6pAump-PUFCW0cgbRY+5_2tPNLe=hN3-dnXD=B6MA@mail.gmail.com > > Thanks! > > -Doug
diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c index 04d315d96bff..ce73e8cb1db5 100644 --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c @@ -31,8 +31,6 @@ struct jadard_panel_desc { bool reset_before_power_off_vcioo; unsigned int vcioo_to_lp11_delay_ms; unsigned int lp11_to_reset_delay_ms; - unsigned int exit_sleep_to_display_on_delay_ms; - unsigned int display_on_delay_ms; unsigned int backlight_off_to_display_off_delay_ms; unsigned int display_off_to_enter_sleep_delay_ms; unsigned int enter_sleep_to_reset_down_delay_ms; @@ -66,26 +64,6 @@ static inline struct jadard *panel_to_jadard(struct drm_panel *panel) return container_of(panel, struct jadard, panel); } -static int jadard_enable(struct drm_panel *panel) -{ - struct jadard *jadard = panel_to_jadard(panel); - struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi }; - - msleep(120); - - mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); - - if (jadard->desc->exit_sleep_to_display_on_delay_ms) - mipi_dsi_msleep(&dsi_ctx, jadard->desc->exit_sleep_to_display_on_delay_ms); - - mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); - - if (jadard->desc->display_on_delay_ms) - mipi_dsi_msleep(&dsi_ctx, jadard->desc->display_on_delay_ms); - - return dsi_ctx.accum_err; -} - static int jadard_disable(struct drm_panel *panel) { struct jadard *jadard = panel_to_jadard(panel); @@ -202,7 +180,6 @@ static const struct drm_panel_funcs jadard_funcs = { .disable = jadard_disable, .unprepare = jadard_unprepare, .prepare = jadard_prepare, - .enable = jadard_enable, .get_modes = jadard_get_modes, .get_orientation = jadard_panel_get_orientation, }; @@ -382,6 +359,12 @@ static int radxa_display_8hd_ad002_init_cmds(struct jadard *jadard) jd9365da_switch_page(&dsi_ctx, 0x00); + mipi_dsi_msleep(&dsi_ctx, 120); + + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); + + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); + return dsi_ctx.accum_err; }; @@ -608,6 +591,12 @@ static int cz101b4001_init_cmds(struct jadard *jadard) mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xE6, 0x02); mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xE7, 0x0C); + mipi_dsi_msleep(&dsi_ctx, 120); + + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); + + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); + return dsi_ctx.accum_err; }; @@ -831,6 +820,16 @@ static int kingdisplay_kd101ne3_init_cmds(struct jadard *jadard) jd9365da_switch_page(&dsi_ctx, 0x00); + mipi_dsi_msleep(&dsi_ctx, 120); + + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); + + mipi_dsi_msleep(&dsi_ctx, 120); + + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); + + mipi_dsi_msleep(&dsi_ctx, 20); + return dsi_ctx.accum_err; }; @@ -859,8 +858,6 @@ static const struct jadard_panel_desc kingdisplay_kd101ne3_40ti_desc = { .reset_before_power_off_vcioo = true, .vcioo_to_lp11_delay_ms = 5, .lp11_to_reset_delay_ms = 10, - .exit_sleep_to_display_on_delay_ms = 120, - .display_on_delay_ms = 20, .backlight_off_to_display_off_delay_ms = 100, .display_off_to_enter_sleep_delay_ms = 50, .enter_sleep_to_reset_down_delay_ms = 100, @@ -1074,6 +1071,16 @@ static int melfas_lmfbx101117480_init_cmds(struct jadard *jadard) mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe6, 0x02); mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xe7, 0x06); + mipi_dsi_msleep(&dsi_ctx, 120); + + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); + + mipi_dsi_msleep(&dsi_ctx, 120); + + mipi_dsi_dcs_set_display_on_multi(&dsi_ctx); + + mipi_dsi_msleep(&dsi_ctx, 20); + return dsi_ctx.accum_err; }; @@ -1102,8 +1109,6 @@ static const struct jadard_panel_desc melfas_lmfbx101117480_desc = { .reset_before_power_off_vcioo = true, .vcioo_to_lp11_delay_ms = 5, .lp11_to_reset_delay_ms = 10, - .exit_sleep_to_display_on_delay_ms = 120, - .display_on_delay_ms = 20, .backlight_off_to_display_off_delay_ms = 100, .display_off_to_enter_sleep_delay_ms = 50, .enter_sleep_to_reset_down_delay_ms = 100,