From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Doug Anderson <dianders@google.com>
Cc: Hsin-Yi Wang <hsinyi@google.com>,
lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>,
mripard@kernel.org, airlied@gmail.com, daniel@ffwll.ch,
robh@kernel.org, krzysztof.kozlowski+dt@linaro.org,
conor+dt@kernel.org, dri-devel@lists.freedesktop.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
cong yang <yangcong5@huaqin.corp-partner.google.com>
Subject: Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
Date: Thu, 25 Apr 2024 00:49:08 +0300 [thread overview]
Message-ID: <CAA8EJprv3qBd1hfdWHrfhY=S0w2O70dZnYb6TVsS6AGRPxsYdw@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=VQ8rbwKk4WpHRER9p4cZp7UrrHRpgnErqbQxyxp4sg5w@mail.gmail.com>
On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > >
> > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > +
> > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > >
> > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > >
> > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > >
> > > > > > Similar discussion in here:
> > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > >
> > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > >
> > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > >
> > > > > Yes, let's get a framework done in a useful way.
> > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > used instead (and it's up to the developer to select correct delay
> > > > > function).
> > > > >
> > > > > >
> > > > > > > > +
> > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > >
> > > > > [skipped the body of the table]
> > > > >
> > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > +
> > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > >
> > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > >
> > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > >
> > > > > > > > + /* T6: 120ms */
> > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > >
> > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > >
> > > > > Having a single table enourages people to put known commands into the
> > > > > table, the practice that must be frowned upon and forbidden.
> > > > >
> > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > instead of adding a single-table based approach we can improve
> > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > error handling to a common part of enable() / prepare() function.
> > > > >
> > > >
> > > > For this panel, I think it can also refer to how
> > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > looping through the table.
> > >
> > > Even more similar discussion:
> > >
> > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> >
> > It seems I skipped that thread.
> >
> > I'd still suggest a code-based solution compared to table-based one, for
> > the reasons I've outlined before. Having a tables puts a pressure on the
> > developer to put commands there for which we already have a
> > command-specific function.
>
> The problem is that with these panels that need big init sequences the
> code based solution is _a lot_ bigger. If it were a few bytes or a
> 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> from a table to code it was 100K difference in code [1]. I would also
> say that having these long init sequences done as separate commands
> encourages people to skip checking the return values of each of the
> transfer functions and I don't love that idea.
>
> It would be ideal if these panels didn't need these long init
> sequences, but I don't have any inside knowledge here saying that they
> could be removed. So assume we can't get rid of the init sequences it
> feels like we have to find some way to make the tables work for at
> least the large chunks of init code and encourage people to make the
> tables readable...
I did a quick check on the boe-tv101wum-nl6 driver by converting the
writes to use the following macro:
#define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
\
do { \
static const u8 d[] = { cmd, seq }; \
ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
if (ret < 0) \
goto err; \
} while (0)
And then at the end of the init funciton having
err:
dev_err(panel->dev,
"failed to write command %d\n", ret);
return ret;
}
Size comparison:
text data bss dec hex filename
before
34109 10410 18 44537 adf9
../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
making init data const
44359 184 0 44543 adff
../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
with new macros
44353 184 0 44537 adf9
../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
As you can see, there is literally no size difference with this macro in place.
The only drawback is that the init stops on the first write rather
than going through the sequence.
WDYT? I can turn this into a proper patch if you think this makes sense.
>
>
> [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com
--
With best wishes
Dmitry
next prev parent reply other threads:[~2024-04-24 21:49 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong
2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong
2024-04-18 22:59 ` Krzysztof Kozlowski
2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong
2024-04-18 11:46 ` Dmitry Baryshkov
2024-04-18 13:11 ` Hsin-Yi Wang
2024-04-18 14:11 ` Dmitry Baryshkov
2024-04-23 18:10 ` Hsin-Yi Wang
2024-04-23 20:41 ` Doug Anderson
2024-04-23 21:20 ` Dmitry Baryshkov
2024-04-24 21:04 ` Doug Anderson
2024-04-24 21:18 ` Hsin-Yi Wang
2024-04-24 21:49 ` Dmitry Baryshkov [this message]
2024-04-24 22:15 ` Hsin-Yi Wang
2024-04-24 22:50 ` Dmitry Baryshkov
2024-04-24 23:25 ` Doug Anderson
2024-04-24 23:37 ` Dmitry Baryshkov
2024-04-25 0:21 ` Doug Anderson
2024-04-24 22:27 ` Doug Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAA8EJprv3qBd1hfdWHrfhY=S0w2O70dZnYb6TVsS6AGRPxsYdw@mail.gmail.com' \
--to=dmitry.baryshkov@linaro.org \
--cc=airlied@gmail.com \
--cc=conor+dt@kernel.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dianders@google.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hsinyi@google.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lvzhaoxiong@huaqin.corp-partner.google.com \
--cc=mripard@kernel.org \
--cc=robh@kernel.org \
--cc=yangcong5@huaqin.corp-partner.google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).