lkml.org 
[lkml]   [2024]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
    Hi,

    On Thu, Apr 25, 2024 at 1:19 AM Jani Nikula <jani.nikula@linux.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)
    >
    > 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.

    ---

    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

    \
     
     \ /
      Last update: 2024-05-27 18:04    [W:2.958 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site