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@xxxxxxxxxxxxxxx> 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
