From: Doug Anderson <dianders@chromium.org>
To: Maxime Ripard <mripard@kernel.org>
Cc: dri-devel@lists.freedesktop.org,
lvzhaoxiong@huaqin.corp-partner.google.com,
Jani Nikula <jani.nikula@linux.intel.com>,
Hsin-Yi Wang <hsinyi@google.com>,
Linus Walleij <linus.walleij@linaro.org>,
Javier Martinez Canillas <javierm@redhat.com>,
Neil Armstrong <neil.armstrong@linaro.org>,
Cong Yang <yangcong5@huaqin.corp-partner.google.com>,
Sam Ravnborg <sam@ravnborg.org>,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Joel Selvaraj <jo@jsfamily.in>,
Brian Norris <briannorris@chromium.org>,
Daniel Vetter <daniel@ffwll.ch>,
David Airlie <airlied@gmail.com>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Thomas Zimmermann <tzimmermann@suse.de>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 4/9] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
Date: Mon, 13 May 2024 08:48:19 -0700 [thread overview]
Message-ID: <CAD=FV=UWV7O-FkO56af5-ae5qdb40p_x_yUQ3NDuJKRKzF5BWA@mail.gmail.com> (raw)
In-Reply-To: <20240513-solemn-yak-of-refinement-cb778a@penduick>
Hi,
On Mon, May 13, 2024 at 2:30 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Wed, May 08, 2024 at 01:51:46PM -0700, Douglas Anderson wrote:
> > Through a cooperative effort between Hsin-Yi Wang and Dmitry
> > Baryshkov, we have realized the dev_err() in the
> > mipi_dsi_*_write_seq() macros was causing quite a bit of bloat to the
> > kernel. Let's hoist this call into drm_mipi_dsi.c by adding a "chatty"
> > version of the functions that includes the print. While doing this,
> > add a bit more comments to these macros making it clear that they
> > print errors and also that they return out of _the caller's_ function.
> >
> > Without any changes to clients this gives a nice savings. Specifically
> > the macro was inlined and thus the error report call was inlined into
> > every call to mipi_dsi_dcs_write_seq() and
> > mipi_dsi_generic_write_seq(). By using a call to a "chatty" function,
> > the usage is reduced to one call in the chatty function and a function
> > call at the invoking site.
> >
> > Building with my build system shows one example:
> >
> > $ scripts/bloat-o-meter \
> > .../before/panel-novatek-nt36672e.ko \
> > .../after/panel-novatek-nt36672e.ko
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-4404 (-4404)
> > Function old new delta
> > nt36672e_1080x2408_60hz_init 10640 6236 -4404
> > Total: Before=15055, After=10651, chg -29.25%
> >
> > Note that given the change in location of the print it's harder to
> > include the "cmd" in the printout for mipi_dsi_dcs_write_seq() since,
> > theoretically, someone could call the new chatty function with a
> > zero-size array and it would be illegal to dereference data[0].
> > There's a printk format to print the whole buffer and this is probably
> > more useful for debugging anyway. Given that we're doing this for
> > mipi_dsi_dcs_write_seq(), let's also print the buffer for
> > mipi_dsi_generic_write_seq() in the error case.
> >
> > It should be noted that the current consensus of DRM folks is that the
> > mipi_dsi_*_write_seq() should be deprecated due to the non-intuitive
> > return behavior. A future patch will formally mark them as deprecated
> > and provide an alternative.
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v4:
> > - Update wording as per Linus W.
> >
> > Changes in v3:
> > - Rebased upon patch to remove ratelimit of prints.
> >
> > Changes in v2:
> > - Add some comments to the macros about printing and returning.
> > - Change the way err value is handled in prep for next patch.
> > - Modify commit message now that this is part of a series.
> > - Rebased upon patches to avoid theoretical int overflow.
> >
> > drivers/gpu/drm/drm_mipi_dsi.c | 56 ++++++++++++++++++++++++++++++++++
> > include/drm/drm_mipi_dsi.h | 47 +++++++++++++++-------------
> > 2 files changed, 82 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index 795001bb7ff1..8593d9ed5891 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -764,6 +764,34 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
> > }
> > EXPORT_SYMBOL(mipi_dsi_generic_write);
> >
> > +/**
> > + * mipi_dsi_generic_write_chatty() - mipi_dsi_generic_write() w/ an error log
> > + * @dsi: DSI peripheral device
> > + * @payload: buffer containing the payload
> > + * @size: size of payload buffer
> > + *
> > + * Like mipi_dsi_generic_write() but includes a dev_err_ratelimited()
>
> You mention in both functions that it's calling dev_err_ratelimited() ...
>
> > + * call for you and returns 0 upon success, not the number of bytes sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
> > + const void *payload, size_t size)
> > +{
> > + struct device *dev = &dsi->dev;
> > + ssize_t ret;
> > +
> > + ret = mipi_dsi_generic_write(dsi, payload, size);
> > + if (ret < 0) {
> > + dev_err(dev, "sending generic data %*ph failed: %zd\n",
> > + (int)size, payload, ret);
>
> ... but it doesn't.
Whoops, thanks for catching this! I'll plan to send a v5 tomorrow to
fix this and then I'll still plan to land the series on Thursday
unless anything major comes up.
-Doug
next prev parent reply other threads:[~2024-05-13 15:56 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 20:51 [PATCH v4 0/9] drm/mipi-dsi: Reduce bloat and add funcs for cleaner init seqs Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 1/9] drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_dcs_write_seq() Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 2/9] drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_generic_write_seq() Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 3/9] drm/mipi-dsi: mipi_dsi_*_write functions don't need to ratelimit prints Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 4/9] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq() Douglas Anderson
2024-05-13 9:29 ` Maxime Ripard
2024-05-13 15:48 ` Doug Anderson [this message]
2024-05-08 20:51 ` [PATCH v4 5/9] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi() Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 6/9] drm/panel: novatek-nt36672e: Switch to mipi_dsi_dcs_write_seq_multi() Douglas Anderson
2024-05-13 17:00 ` Dmitry Baryshkov
2024-05-13 19:46 ` Linus Walleij
2024-05-08 20:51 ` [PATCH v4 7/9] drm/panel: boe-tv101wum-nl6: Don't use a table for initting panels Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 8/9] drm/panel: ili9882t: " Douglas Anderson
2024-05-08 20:51 ` [PATCH v4 9/9] drm/panel: innolux-p079zca: " Douglas 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='CAD=FV=UWV7O-FkO56af5-ae5qdb40p_x_yUQ3NDuJKRKzF5BWA@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=airlied@gmail.com \
--cc=briannorris@chromium.org \
--cc=daniel@ffwll.ch \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=hsinyi@google.com \
--cc=jani.nikula@linux.intel.com \
--cc=javierm@redhat.com \
--cc=jo@jsfamily.in \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lvzhaoxiong@huaqin.corp-partner.google.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
--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).