| Message ID | 20240508135148.v4.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 |
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. Maxime
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
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) /**