Message-ID: <beqsovvdkvn63prt3c6b3epb6tachff35vpaf62dfkwof7kwht@u3p7bkv7owro>
Date: Fri, 26 Apr 2024 06:03:07 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Doug Anderson <dianders@...omium.org>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>, 
	dri-devel@...ts.freedesktop.org, Javier Martinez Canillas <javierm@...hat.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, linus.walleij@...aro.org, 
	Cong Yang <yangcong5@...qin.corp-partner.google.com>, lvzhaoxiong@...qin.corp-partner.google.com, 
	Hsin-Yi Wang <hsinyi@...gle.com>, Sam Ravnborg <sam@...nborg.org>, Daniel Vetter <daniel@...ll.ch>, 
	David Airlie <airlied@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of
 mipi_dsi_*_write_seq()

On Thu, Apr 25, 2024 at 10:04:49AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 25, 2024 at 1:19 AM Jani Nikula <jani.nikula@...ux.intel.com> wrote:
> >
> > > @@ -279,6 +281,8 @@ enum mipi_dsi_dcs_tear_mode {
> > >
> > >  ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> > >                                 const void *data, size_t len);
> > > +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
> > > +                                      const void *data, size_t len);
> > >  ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> > >                          const void *data, size_t len);
> > >  ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> > > @@ -317,14 +321,10 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> > >  #define mipi_dsi_generic_write_seq(dsi, seq...)                                \
> > >       do {                                                                   \
> > >               static const u8 d[] = { seq };                                 \
> > > -             struct device *dev = &dsi->dev;                                \
> > >               int ret;                                                       \
> > > -             ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));           \
> > > -             if (ret < 0) {                                                 \
> > > -                     dev_err_ratelimited(dev, "transmit data failed: %d\n", \
> > > -                                         ret);                              \
> > > +             ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d));    \
> > > +             if (ret < 0)                                                   \
> > >                       return ret;                                            \
> > > -             }                                                              \
> > >       } while (0)


Reading the thread makes me wonder whether we should be going into
slightly other direction:

Add __must_check() to mipi_dsi_ writing functions,

#define mipi_dsi_dcs_whatever_write(dsi, cmd, seq...)	\
	({						\
		static const u8 d[] = { cmd, seq };     \
                mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));    \
	})

Then in panel drivers we actually have to explicitly handle the return
code (either by dropping to the error label or by just returning an
error).


> >
> > The one thing that I've always disliked about these macros (even if I've
> > never actually used them myself) is that they hide control flow from the
> > caller, i.e. return directly. You don't see that in the code, it's not
> > documented, and if you wanted to do better error handling yourself,
> > you're out of luck.
> 
> Yeah, I agree that it's not the cleanest. That being said, it is
> existing code and making the existing code less bloated seems worth
> doing.
> 
> I'd also say that it feels worth it to have _some_ solution so that
> the caller doesn't need to write error handling after every single cmd
> sent. If we get rid of / discourage these macros that's either going
> to end us up with ugly/verbose code or it's going to encourage people
> to totally skip error handling. IMO neither of those are wonderful
> solutions.
> 
> While thinking about this there were a few ideas I came up with. None
> of them are amazing, but probably they are better than the hidden
> "return" like this. Perhaps we could mark the current function as
> "deprecated" and pick one of these depending on what others opinions
> are:
> 
> 1. Use "goto" and force the caller to give a goto target for error handling.
> 
> This is based on an idea that Dmitry came up with, but made a little
> more explicit. Example usage:
> 
> int ret;
> 
> ret = 0;
> mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETSPCCMD, 0xcd,
>                             some_cmd_failed);
> mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETMIPI, 0x84,
>                             some_cmd_failed);
> mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETSPCCMD, 0x3f,
>                             some_cmd_failed);
> mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETVDC, 0x1b, 0x04,
>                             some_cmd_failed);
> 
> ...
> 
> some_cmd_failed:
>   pr_err("Commands failed to write: %d", ret);
>   return ret;
> }
> 
> One downside here is that you can't easily tell which command failed
> to put it in the error message. A variant of this idea (1a?) could be
> to hoist the print back into the write command. I'd want to pick one
> or the other. I guess my preference would be to hoist the print into
> the write command and if someone really doesn't want the print then
> they call mipi_dsi_dcs_write_buffer() directly.

Do we really care, which command has failed? I mean, usually either all
DSI transfers work, or we have an issue with the DSI host.

> 
> ---
> 
> 2. Accept that a slightly less efficient handling of the error case
> and perhaps a less intuitive API, but avoid the goto.
> 
> Essentially you could pass in "ret" and have the function be a no-op
> if an error is already present. Something like this:
> 
> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi,
> const void *data, size_t len, int *accum_ret)
> {
>   if (*accum_ret)
>     return;
> 
>   *accum_ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
> }
> 
> ...and then the caller:
> 
> int ret;
> 
> ret = 0;
> mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0xcd, &ret);
> mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETMIPI, 0x84, &ret);
> mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0x3f, &ret);
> mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETVDC, 0x1b, 0x04, &ret);
> if (ret)
>   goto some_cmd_failed;
> 
> This has similar properties to solution #1.
> 
> ---
> 
> 3. Accept that callers don't want to error handling but just need a print.
> 
> I'm not 100% sure we want to encourage this. On the one hand it's
> unlikely anyone is really going to be able to reliably recover super
> properly from an error midway through a big long command sequence. On
> the other hand, this means we can't pass the error back to the caller.
> In theory the caller _could_ try to handle errors by resetting / power
> cycling things, so that's a real downside.
> 
> Example usage:
> 
> mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETSPCCMD, 0xcd);
> mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETMIPI, 0x84);
> mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETSPCCMD, 0x3f);
> mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETVDC, 0x1b, 0x04);
> 
> ---
> 
> I think I'd lean towards #1a (user passes goto label and we include
> the error print in the helper), but I'd personally be happy with any
> of #1 or #2. I don't love #3.
> 
> > Be that as it may, the combo of ratelimited error printing and return on
> > errors does not make much sense to me. If there's something to print,
> > you bail out, that's it. I suspect we never hit the ratelimit.
> 
> Yeah, I'm in favor of removing the ratelimit.
> 
> 
> > You might even want to try *only* changing the ratelimited printing to a
> > regular error message, and see if the compiler can combine the logging
> > to a single exit point in the callers. Ratelimited it obviously can't
> > because every single one of them is unique.
> 
> It wasn't quite as good. Comparing the "after" solution (AKA applying
> $SUBJECT patch) vs. _not_ taking $SUBJECT patch and instead changing
> dev_err_ratelimited() to dev_err().
> 
> $ scripts/bloat-o-meter \
>    .../after/panel-novatek-nt36672e.ko \
>   .../noratelimit/panel-novatek-nt36672e.ko
> add/remove: 0/0 grow/shrink: 1/0 up/down: 3404/0 (3404)
> Function                                     old     new   delta
> nt36672e_1080x2408_60hz_init                7260   10664   +3404
> Total: Before=11669, After=15073, chg +29.17%
> 
> ...so $SUBJECT patch is still better.
> 
> ---
> 
> Where does that leave us? IMO:
> 
> a) If others agree, we should land $SUBJECT patch. It doesn't change
> the behavior at all and gives big savings. It adds an extra function
> hop, but presumably the fact that we have to fetch _a lot_ less stuff
> from RAM might mean we still get better performance (likely it doesn't
> matter anyway since this is not hotpath code).
> 
> b) Atop this patch, we should consider changing dev_err_ratelimited()
> to dev_err(). It doesn't seem to make lots of sense to me to ratelimit
> this error.
> 
> c) Atop this patch, we should consider making the two existing macros
> "deprecated" in favor of a new macro that makes the control flow more
> obvious.
> 
> How does that sound to folks?
> 
> -Doug

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists