| Message ID | 20240501084109.v3.4.Id15fae80582bc74a0d4f1338987fa375738f45b9@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mipi-dsi: Reduce bloat and add funcs for cleaner init seqs | expand |
On 01/05/2024 17:41, 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. 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> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > 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() > + * 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); > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(mipi_dsi_generic_write_chatty); > + > /** > * mipi_dsi_generic_read() - receive data using a generic read packet > * @dsi: DSI peripheral device > @@ -852,6 +880,34 @@ ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, > } > EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer); > > +/** > + * mipi_dsi_dcs_write_buffer_chatty - mipi_dsi_dcs_write_buffer() w/ an error log > + * @dsi: DSI peripheral device > + * @data: buffer containing data to be transmitted > + * @len: size of transmission buffer > + * > + * Like mipi_dsi_dcs_write_buffer() but includes a 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_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi, > + const void *data, size_t len) > +{ > + struct device *dev = &dsi->dev; > + ssize_t ret; > + > + ret = mipi_dsi_dcs_write_buffer(dsi, data, len); > + if (ret < 0) { > + dev_err(dev, "sending dcs data %*ph failed: %zd\n", > + (int)len, data, ret); > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer_chatty); > + > /** > * mipi_dsi_dcs_write() - send DCS write command > * @dsi: DSI peripheral device > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index 67967be48dbd..6d68d9927f46 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -256,6 +256,8 @@ int mipi_dsi_picture_parameter_set(struct mipi_dsi_device *dsi, > > ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload, > size_t size); > +int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi, > + const void *payload, size_t size); > ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, > size_t num_params, void *data, size_t size); > > @@ -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); > +int 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, > @@ -311,38 +315,39 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > > /** > * mipi_dsi_generic_write_seq - transmit data using a generic write packet > + * > + * This macro will print errors for you and will RETURN FROM THE CALLING > + * FUNCTION (yes this is non-intuitive) upon error. > + * > * @dsi: DSI peripheral device > * @seq: buffer containing the payload > */ > -#define mipi_dsi_generic_write_seq(dsi, seq...) \ > - do { \ > - static const u8 d[] = { seq }; \ > - struct device *dev = &dsi->dev; \ > - ssize_t ret; \ > - ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d)); \ > - if (ret < 0) { \ > - dev_err(dev, "transmit data failed: %zd\n", ret); \ > - return ret; \ > - } \ > +#define mipi_dsi_generic_write_seq(dsi, seq...) \ > + do { \ > + static const u8 d[] = { seq }; \ > + int ret; \ > + ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d)); \ > + if (ret < 0) \ > + return ret; \ > } while (0) > > /** > * mipi_dsi_dcs_write_seq - transmit a DCS command with payload > + * > + * This macro will print errors for you and will RETURN FROM THE CALLING > + * FUNCTION (yes this is non-intuitive) upon error. > + * > * @dsi: DSI peripheral device > * @cmd: Command > * @seq: buffer containing data to be transmitted > */ > -#define mipi_dsi_dcs_write_seq(dsi, cmd, seq...) \ > - do { \ > - static const u8 d[] = { cmd, seq }; \ > - struct device *dev = &dsi->dev; \ > - ssize_t ret; \ > - ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > - if (ret < 0) { \ > - dev_err(dev, "sending command %#02x failed: %zd\n", \ > - cmd, ret); \ > - return ret; \ > - } \ > +#define mipi_dsi_dcs_write_seq(dsi, cmd, seq...) \ > + do { \ > + static const u8 d[] = { cmd, seq }; \ > + int ret; \ > + ret = mipi_dsi_dcs_write_buffer_chatty(dsi, d, ARRAY_SIZE(d)); \ > + if (ret < 0) \ > + return ret; \ > } while (0) > > /** Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
On Wed, May 1, 2024 at 5:43 PM Douglas Anderson <dianders@chromium.org> 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. This doesn't really explain why this takes so much less space. Please add some explanation like that "the macro gets inlined and thus the error report string gets 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 string in the chatty function and a function call at the invoking site". With some explanation like that +/- added in: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi, On Thu, May 2, 2024 at 1:23 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, May 1, 2024 at 5:43 PM Douglas Anderson <dianders@chromium.org> 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. > > This doesn't really explain why this takes so much less space. > > Please add some explanation like that "the macro gets inlined > and thus the error report string gets 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 > string in the chatty function and a function call at the invoking > site". > > With some explanation like that +/- added in: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Sure. I'll plan on not sending out a v4 (unless I need to for some other reason) and I can just add your wording in when applying. Yell if you want me to do something different. -Doug
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() + * 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); + return ret; + } + + return 0; +} +EXPORT_SYMBOL(mipi_dsi_generic_write_chatty); + /** * mipi_dsi_generic_read() - receive data using a generic read packet * @dsi: DSI peripheral device @@ -852,6 +880,34 @@ ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi, } EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer); +/** + * mipi_dsi_dcs_write_buffer_chatty - mipi_dsi_dcs_write_buffer() w/ an error log + * @dsi: DSI peripheral device + * @data: buffer containing data to be transmitted + * @len: size of transmission buffer + * + * Like mipi_dsi_dcs_write_buffer() but includes a 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_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi, + const void *data, size_t len) +{ + struct device *dev = &dsi->dev; + ssize_t ret; + + ret = mipi_dsi_dcs_write_buffer(dsi, data, len); + if (ret < 0) { + dev_err(dev, "sending dcs data %*ph failed: %zd\n", + (int)len, data, ret); + return ret; + } + + return 0; +} +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer_chatty); + /** * mipi_dsi_dcs_write() - send DCS write command * @dsi: DSI peripheral device diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 67967be48dbd..6d68d9927f46 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -256,6 +256,8 @@ int mipi_dsi_picture_parameter_set(struct mipi_dsi_device *dsi, ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload, size_t size); +int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi, + const void *payload, size_t size); ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, size_t num_params, void *data, size_t size); @@ -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); +int 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, @@ -311,38 +315,39 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, /** * mipi_dsi_generic_write_seq - transmit data using a generic write packet + * + * This macro will print errors for you and will RETURN FROM THE CALLING + * FUNCTION (yes this is non-intuitive) upon error. + * * @dsi: DSI peripheral device * @seq: buffer containing the payload */ -#define mipi_dsi_generic_write_seq(dsi, seq...) \ - do { \ - static const u8 d[] = { seq }; \ - struct device *dev = &dsi->dev; \ - ssize_t ret; \ - ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d)); \ - if (ret < 0) { \ - dev_err(dev, "transmit data failed: %zd\n", ret); \ - return ret; \ - } \ +#define mipi_dsi_generic_write_seq(dsi, seq...) \ + do { \ + static const u8 d[] = { seq }; \ + int ret; \ + ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d)); \ + if (ret < 0) \ + return ret; \ } while (0) /** * mipi_dsi_dcs_write_seq - transmit a DCS command with payload + * + * This macro will print errors for you and will RETURN FROM THE CALLING + * FUNCTION (yes this is non-intuitive) upon error. + * * @dsi: DSI peripheral device * @cmd: Command * @seq: buffer containing data to be transmitted */ -#define mipi_dsi_dcs_write_seq(dsi, cmd, seq...) \ - do { \ - static const u8 d[] = { cmd, seq }; \ - struct device *dev = &dsi->dev; \ - ssize_t ret; \ - ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ - if (ret < 0) { \ - dev_err(dev, "sending command %#02x failed: %zd\n", \ - cmd, ret); \ - return ret; \ - } \ +#define mipi_dsi_dcs_write_seq(dsi, cmd, seq...) \ + do { \ + static const u8 d[] = { cmd, seq }; \ + int ret; \ + ret = mipi_dsi_dcs_write_buffer_chatty(dsi, d, ARRAY_SIZE(d)); \ + if (ret < 0) \ + return ret; \ } while (0) /**