linux-kernel - Re: [PATCH v4 1/4] drm/panel: jd9365da: Modify the method of sending commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 12:33:31 -0700
From: Doug Anderson <dianders@...gle.com>
To: Zhaoxiong Lv <lvzhaoxiong@...qin.corp-partner.google.com>
Cc: dmitry.torokhov@...il.com, robh@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, jikos@...nel.org, 
	benjamin.tissoires@...hat.co, hsinyi@...gle.com, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Jagan Teki <jagan@...eble.ai>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Subject: Re: [PATCH v4 1/4] drm/panel: jd9365da: Modify the method of sending commands

Hi,

On Thu, Jun 20, 2024 at 1:05 AM Zhaoxiong Lv
<lvzhaoxiong@...qin.corp-partner.google.com> wrote:
>
> Currently, the init_code of the jd9365da driver is placed
> in the enable() function and sent, but this seems to take
> a long time. It takes 17ms to send each instruction (an init
> code consists of about 200 instructions), so it takes
> about 3.5s to send the init_code. So we moved the sending
> of the inti_code to the prepare() function, and each
> instruction seemed to take only 25μs.
>
> We checked the DSI host and found that the difference in
> command sending time is caused by the different modes of
> the DSI host in prepare() and enable() functions.
> Our DSI Host only supports sending cmd in LP mode, The
> prepare() function can directly send init_code (LP->cmd)
> in LP mode, but the enable() function is in HS mode and
> needs to switch to LP mode before sending init code
> (HS->LP->cmd->HS). Therefore, it takes longer to send
> the command.
>
> Signed-off-by: Zhaoxiong Lv <lvzhaoxiong@...qin.corp-partner.google.com>
> ---
>
> Changes between V4 and V3:
> - 1. Only move mipi_dsi_dcs_write_buffer from enable() function to prepare() function,
> -    and no longer use mipi_dsi_dcs_write_seq_multi.
>
> V3:https://lore.kernel.org/all/20240614145510.22965-2-lvzhaoxiong@huaqin.corp-partner.google.com/
>
> ---
>  .../gpu/drm/panel/panel-jadard-jd9365da-h3.c  | 24 +++++++++----------
>  1 file changed, 11 insertions(+), 13 deletions(-)

This seems reasonable to me, but I'd prefer someone with more MIPI
experience than me to actually give it a good review to make sure that
the argument about things being in prepare() vs enable() makes sense
to them.

Actually, that makes me look at your CC list, which seems to be
missing some important people. You should have run
`./scripts/get_maintainer.pl` and used that to help you make your CC
list, but you're missing some important people including the
maintainer of this driver and the panel maintainer. I've added a few
of those people to this patch. If you send future versions please
include folks that get_maintainer points out.

> @@ -117,7 +107,15 @@ static int jadard_prepare(struct drm_panel *panel)
>         msleep(10);
>
>         gpiod_set_value(jadard->reset, 1);
> -       msleep(120);
> +       msleep(130);
> +
> +       for (i = 0; i < desc->num_init_cmds; i++) {
> +               const struct jadard_init_cmd *cmd = &desc->init_cmds[i];
> +
> +               ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, JD9365DA_INIT_CMD_LEN);

In general people don't like the table-based approach for DSI init.
For this patch it's probably OK, but before the later patch where you
add a new panel you'll probably need to transition to a separate init
per panel and then use mipi_dsi_dcs_write_seq_multi() in there. I'll
comment more on the later patch. In other words, one possible v5
should be a 5-patch series:

Patch #1: this patch
Patch #2: DT binding
Patch #3: switch jd9365da to use mipi_dsi_dcs_write_seq_multi() but no
functional changes.
Patch #4: add your new panel.
Patch #5: orientation.

-Doug