* [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
@ 2024-04-25 0:20 Douglas Anderson
2024-04-25 7:55 ` Neil Armstrong
2024-04-25 8:19 ` Jani Nikula
0 siblings, 2 replies; 10+ messages in thread
From: Douglas Anderson @ 2024-04-25 0:20 UTC (permalink / raw)
To: dri-devel
Cc: Javier Martinez Canillas, Neil Armstrong, Dmitry Baryshkov,
linus.walleij, Cong Yang, lvzhaoxiong, Hsin-Yi Wang, Sam Ravnborg,
Douglas Anderson, Daniel Vetter, David Airlie, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, linux-kernel
The consensus of many DRM folks is that we want to move away from DSI
drivers defining tables of init commands. Instead, we want to move to
init functions that can use common DRM functions. The issue thus far
has been that using the macros mipi_dsi_generic_write_seq() and
mipi_dsi_dcs_write_seq() bloats the driver using them.
Through a cooperative effort between Hsin-Yi Wang and Dmitry
Baryshkov, we have realized that the majority of the bloat is the fact
that we have the dev_err_ratelimited() directly in the macros. Let's
hoist this call into drm_mipi_dsi.c by adding a "chatty" version of
the functions that includes the print.
Without any changes to clients this gives a dramatic 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/1 grow/shrink: 1/1 up/down: 6/-19652 (-19646)
Function old new delta
__UNIQUE_ID_vermagic520 64 70 +6
nt36672e_1080x2408_60hz_init 16592 7260 -9332
nt36672e_1080x2408_60hz_init._rs 10320 - -10320
Total: Before=31503, After=11857, chg -62.36%
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.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
The MIPI device I have in front of me (wormdingler) hasn't been
converted to use these functions yet, so this is just compile
tested. It's about my end of day so I'm sending this out since it's
pretty straightforward. Hopefully others can confirm it works well for
them and also saves space under their compilers.
drivers/gpu/drm/drm_mipi_dsi.c | 54 ++++++++++++++++++++++++++++++++++
include/drm/drm_mipi_dsi.h | 31 ++++++++-----------
2 files changed, 67 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 795001bb7ff1..5ded6aef38ed 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -764,6 +764,33 @@ 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
+ *
+ * Just like mipi_dsi_generic_write() but includes a dev_err_ratelimited()
+ * call for you.
+ *
+ * Return: The number of bytes transmitted on success or a negative error code
+ * on failure.
+ */
+ssize_t mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
+ const void *payload, size_t size)
+{
+ struct device *dev = &dsi->dev;
+ int ret;
+
+ ret = mipi_dsi_generic_write(dsi, payload, size);
+ if (ret < 0)
+ dev_err_ratelimited(dev, "sending generic data %*ph failed: %d\n",
+ (int)size, payload, ret);
+
+ return ret;
+}
+EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
+
/**
* mipi_dsi_generic_read() - receive data using a generic read packet
* @dsi: DSI peripheral device
@@ -852,6 +879,33 @@ 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
+ *
+ * Just like mipi_dsi_dcs_write_buffer() but includes a dev_err_ratelimited()
+ * call for you.
+ *
+ * Return: The number of bytes successfully transmitted or a negative error
+ * code on failure.
+ */
+ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
+ const void *data, size_t len)
+{
+ struct device *dev = &dsi->dev;
+ int ret;
+
+ ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
+ if (ret < 0)
+ dev_err_ratelimited(dev, "sending dcs data %*ph failed: %d\n",
+ (int)len, data, ret);
+
+ return ret;
+}
+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 82b1cc434ea3..784e425dc4c8 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);
+ssize_t 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);
+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)
/**
@@ -333,18 +333,13 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
* @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; \
- int ret; \
- ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
- if (ret < 0) { \
- dev_err_ratelimited( \
- dev, "sending command %#02x failed: %d\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)
/**
--
2.44.0.769.g3c40516874-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-25 0:20 [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq() Douglas Anderson
@ 2024-04-25 7:55 ` Neil Armstrong
2024-04-25 8:19 ` Jani Nikula
1 sibling, 0 replies; 10+ messages in thread
From: Neil Armstrong @ 2024-04-25 7:55 UTC (permalink / raw)
To: Douglas Anderson, dri-devel
Cc: Javier Martinez Canillas, Dmitry Baryshkov, linus.walleij,
Cong Yang, lvzhaoxiong, Hsin-Yi Wang, Sam Ravnborg, Daniel Vetter,
David Airlie, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
linux-kernel
On 25/04/2024 02:20, Douglas Anderson wrote:
> The consensus of many DRM folks is that we want to move away from DSI
> drivers defining tables of init commands. Instead, we want to move to
> init functions that can use common DRM functions. The issue thus far
> has been that using the macros mipi_dsi_generic_write_seq() and
> mipi_dsi_dcs_write_seq() bloats the driver using them.
>
> Through a cooperative effort between Hsin-Yi Wang and Dmitry
> Baryshkov, we have realized that the majority of the bloat is the fact
> that we have the dev_err_ratelimited() directly in the macros. Let's
> hoist this call into drm_mipi_dsi.c by adding a "chatty" version of
> the functions that includes the print.
>
> Without any changes to clients this gives a dramatic 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/1 grow/shrink: 1/1 up/down: 6/-19652 (-19646)
> Function old new delta
> __UNIQUE_ID_vermagic520 64 70 +6
> nt36672e_1080x2408_60hz_init 16592 7260 -9332
> nt36672e_1080x2408_60hz_init._rs 10320 - -10320
> Total: Before=31503, After=11857, chg -62.36%
>
> 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.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> The MIPI device I have in front of me (wormdingler) hasn't been
> converted to use these functions yet, so this is just compile
> tested. It's about my end of day so I'm sending this out since it's
> pretty straightforward. Hopefully others can confirm it works well for
> them and also saves space under their compilers.
>
> drivers/gpu/drm/drm_mipi_dsi.c | 54 ++++++++++++++++++++++++++++++++++
> include/drm/drm_mipi_dsi.h | 31 ++++++++-----------
> 2 files changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 795001bb7ff1..5ded6aef38ed 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -764,6 +764,33 @@ 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
> + *
> + * Just like mipi_dsi_generic_write() but includes a dev_err_ratelimited()
> + * call for you.
> + *
> + * Return: The number of bytes transmitted on success or a negative error code
> + * on failure.
> + */
> +ssize_t mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
> + const void *payload, size_t size)
> +{
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + ret = mipi_dsi_generic_write(dsi, payload, size);
> + if (ret < 0)
> + dev_err_ratelimited(dev, "sending generic data %*ph failed: %d\n",
> + (int)size, payload, ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
> +
> /**
> * mipi_dsi_generic_read() - receive data using a generic read packet
> * @dsi: DSI peripheral device
> @@ -852,6 +879,33 @@ 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
> + *
> + * Just like mipi_dsi_dcs_write_buffer() but includes a dev_err_ratelimited()
> + * call for you.
> + *
> + * Return: The number of bytes successfully transmitted or a negative error
> + * code on failure.
> + */
> +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
> + const void *data, size_t len)
> +{
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
> + if (ret < 0)
> + dev_err_ratelimited(dev, "sending dcs data %*ph failed: %d\n",
> + (int)len, data, ret);
> +
> + return ret;
> +}
> +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 82b1cc434ea3..784e425dc4c8 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);
> +ssize_t 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);
> +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)
>
> /**
> @@ -333,18 +333,13 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> * @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; \
> - int ret; \
> - ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> - if (ret < 0) { \
> - dev_err_ratelimited( \
> - dev, "sending command %#02x failed: %d\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)
>
> /**
At first sight it looks o for me, it's less efficient since it adds a function call
for each sequence, but the saving if very significant.
I'll run a test on the vtdr6130 to be sure.
Neil
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-25 0:20 [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq() Douglas Anderson
2024-04-25 7:55 ` Neil Armstrong
@ 2024-04-25 8:19 ` Jani Nikula
2024-04-25 17:04 ` Doug Anderson
1 sibling, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2024-04-25 8:19 UTC (permalink / raw)
To: Douglas Anderson, dri-devel
Cc: Javier Martinez Canillas, Neil Armstrong, Dmitry Baryshkov,
linus.walleij, Cong Yang, lvzhaoxiong, Hsin-Yi Wang, Sam Ravnborg,
Douglas Anderson, Daniel Vetter, David Airlie, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, linux-kernel
On Wed, 24 Apr 2024, Douglas Anderson <dianders@chromium.org> wrote:
> The consensus of many DRM folks is that we want to move away from DSI
> drivers defining tables of init commands. Instead, we want to move to
> init functions that can use common DRM functions. The issue thus far
> has been that using the macros mipi_dsi_generic_write_seq() and
> mipi_dsi_dcs_write_seq() bloats the driver using them.
>
> Through a cooperative effort between Hsin-Yi Wang and Dmitry
> Baryshkov, we have realized that the majority of the bloat is the fact
> that we have the dev_err_ratelimited() directly in the macros. Let's
> hoist this call into drm_mipi_dsi.c by adding a "chatty" version of
> the functions that includes the print.
>
> Without any changes to clients this gives a dramatic 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/1 grow/shrink: 1/1 up/down: 6/-19652 (-19646)
> Function old new delta
> __UNIQUE_ID_vermagic520 64 70 +6
> nt36672e_1080x2408_60hz_init 16592 7260 -9332
> nt36672e_1080x2408_60hz_init._rs 10320 - -10320
> Total: Before=31503, After=11857, chg -62.36%
>
> 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.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> The MIPI device I have in front of me (wormdingler) hasn't been
> converted to use these functions yet, so this is just compile
> tested. It's about my end of day so I'm sending this out since it's
> pretty straightforward. Hopefully others can confirm it works well for
> them and also saves space under their compilers.
>
> drivers/gpu/drm/drm_mipi_dsi.c | 54 ++++++++++++++++++++++++++++++++++
> include/drm/drm_mipi_dsi.h | 31 ++++++++-----------
> 2 files changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 795001bb7ff1..5ded6aef38ed 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -764,6 +764,33 @@ 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
> + *
> + * Just like mipi_dsi_generic_write() but includes a dev_err_ratelimited()
> + * call for you.
> + *
> + * Return: The number of bytes transmitted on success or a negative error code
> + * on failure.
> + */
> +ssize_t mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
> + const void *payload, size_t size)
> +{
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + ret = mipi_dsi_generic_write(dsi, payload, size);
> + if (ret < 0)
> + dev_err_ratelimited(dev, "sending generic data %*ph failed: %d\n",
> + (int)size, payload, ret);
Why does this even need to be ratelimited? See below.
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
> +
> /**
> * mipi_dsi_generic_read() - receive data using a generic read packet
> * @dsi: DSI peripheral device
> @@ -852,6 +879,33 @@ 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
> + *
> + * Just like mipi_dsi_dcs_write_buffer() but includes a dev_err_ratelimited()
> + * call for you.
> + *
> + * Return: The number of bytes successfully transmitted or a negative error
> + * code on failure.
> + */
> +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
> + const void *data, size_t len)
> +{
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
> + if (ret < 0)
> + dev_err_ratelimited(dev, "sending dcs data %*ph failed: %d\n",
> + (int)len, data, ret);
> +
> + return ret;
> +}
> +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 82b1cc434ea3..784e425dc4c8 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);
> +ssize_t 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);
> +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.
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.
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.
BR,
Jani.
>
> /**
> @@ -333,18 +333,13 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> * @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; \
> - int ret; \
> - ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> - if (ret < 0) { \
> - dev_err_ratelimited( \
> - dev, "sending command %#02x failed: %d\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)
>
> /**
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-25 8:19 ` Jani Nikula
@ 2024-04-25 17:04 ` Doug Anderson
2024-04-26 3:03 ` Dmitry Baryshkov
2024-04-26 10:08 ` Jani Nikula
0 siblings, 2 replies; 10+ messages in thread
From: Doug Anderson @ 2024-04-25 17:04 UTC (permalink / raw)
To: Jani Nikula
Cc: dri-devel, Javier Martinez Canillas, Neil Armstrong,
Dmitry Baryshkov, linus.walleij, Cong Yang, lvzhaoxiong,
Hsin-Yi Wang, Sam Ravnborg, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-25 17:04 ` Doug Anderson
@ 2024-04-26 3:03 ` Dmitry Baryshkov
2024-04-26 15:35 ` Doug Anderson
2024-04-26 10:08 ` Jani Nikula
1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Baryshkov @ 2024-04-26 3:03 UTC (permalink / raw)
To: Doug Anderson
Cc: Jani Nikula, dri-devel, Javier Martinez Canillas, Neil Armstrong,
linus.walleij, Cong Yang, lvzhaoxiong, Hsin-Yi Wang, Sam Ravnborg,
Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, linux-kernel
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@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)
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-25 17:04 ` Doug Anderson
2024-04-26 3:03 ` Dmitry Baryshkov
@ 2024-04-26 10:08 ` Jani Nikula
2024-04-26 15:28 ` Doug Anderson
1 sibling, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2024-04-26 10:08 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, Javier Martinez Canillas, Neil Armstrong,
Dmitry Baryshkov, linus.walleij, Cong Yang, lvzhaoxiong,
Hsin-Yi Wang, Sam Ravnborg, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, linux-kernel
On Thu, 25 Apr 2024, Doug Anderson <dianders@chromium.org> wrote:
> 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);
No reason you couldn't do error logging here
if (*accum_ret)
dev_err(...)
> }
>
> ...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.
I like this option the best, for the simple reason that the caller side
is aware of what's going on, there's no magic control flow happening,
and they can add error handling in the middle if they so choose.
I don't find this unintuitive, but if it helps, you could conceivably
add a context parameter:
struct mipi_dsi_seq_context context = {
.dsi = dsi,
};
mipi_dsi_dcs_write_seq(&context, HX83102_SETSPCCMD, 0xcd);
...
if (context.ret)
...
And even have further control in the context whether to log or keep
going or whatever.
I don't think the efficiency in error cases is a problem worth thinking
about, but you could address that by turning this into a macro so
there's no extra calls on errors.
BR,
Jani.
>
> ---
>
> 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
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-26 10:08 ` Jani Nikula
@ 2024-04-26 15:28 ` Doug Anderson
2024-04-27 0:01 ` Doug Anderson
0 siblings, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2024-04-26 15:28 UTC (permalink / raw)
To: Jani Nikula
Cc: dri-devel, Javier Martinez Canillas, Neil Armstrong,
Dmitry Baryshkov, linus.walleij, Cong Yang, lvzhaoxiong,
Hsin-Yi Wang, Sam Ravnborg, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, linux-kernel
Hi,
On Fri, Apr 26, 2024 at 3:09 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> > 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);
>
> No reason you couldn't do error logging here
>
> if (*accum_ret)
> dev_err(...)
Yup, exactly. This is probably best.
> > }
> >
> > ...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.
>
> I like this option the best, for the simple reason that the caller side
> is aware of what's going on, there's no magic control flow happening,
> and they can add error handling in the middle if they so choose.
Sounds good to me. I went back and forth a bit between solution #1 and
this and I see the benefits of both. If folks like this one I think we
should run with it. Certainly it's better than the current hidden
return.
> I don't find this unintuitive, but if it helps, you could conceivably
> add a context parameter:
>
> struct mipi_dsi_seq_context context = {
> .dsi = dsi,
> };
>
> mipi_dsi_dcs_write_seq(&context, HX83102_SETSPCCMD, 0xcd);
> ...
>
> if (context.ret)
> ...
>
> And even have further control in the context whether to log or keep
> going or whatever.
I agree there are some benefits of adding the extra "context"
abstraction and we can go that way if you want, but I lean towards the
simplicity of just passing in the accumulated return value like I did
in my example.
I'll try to write up patches and see if I can post them later today.
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-26 3:03 ` Dmitry Baryshkov
@ 2024-04-26 15:35 ` Doug Anderson
2024-04-26 15:42 ` Dmitry Baryshkov
0 siblings, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2024-04-26 15:35 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Jani Nikula, dri-devel, Javier Martinez Canillas, Neil Armstrong,
linus.walleij, Cong Yang, lvzhaoxiong, Hsin-Yi Wang, Sam Ravnborg,
Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, linux-kernel
Hi,
On Thu, Apr 25, 2024 at 8:03 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> 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@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)
>
>
> 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).
Given the sheer number of init commands needed by some panels (see
j606f_boe_init_sequence() for instance) I'm still convinced that we
want something that allows people to write their init code in a way
that's not quite so verbose. It sounds as if Jani is OK w/ the
proposal of using the "accumulated return value" (proposal #2 I had).
I'm hoping you're OK w/ that too...
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-26 15:35 ` Doug Anderson
@ 2024-04-26 15:42 ` Dmitry Baryshkov
0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Baryshkov @ 2024-04-26 15:42 UTC (permalink / raw)
To: Doug Anderson
Cc: Jani Nikula, dri-devel, Javier Martinez Canillas, Neil Armstrong,
linus.walleij, Cong Yang, lvzhaoxiong, Hsin-Yi Wang, Sam Ravnborg,
Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, linux-kernel
On Fri, 26 Apr 2024 at 18:41, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 25, 2024 at 8:03 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > 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@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)
> >
> >
> > 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).
>
> Given the sheer number of init commands needed by some panels (see
> j606f_boe_init_sequence() for instance) I'm still convinced that we
> want something that allows people to write their init code in a way
> that's not quite so verbose. It sounds as if Jani is OK w/ the
> proposal of using the "accumulated return value" (proposal #2 I had).
> I'm hoping you're OK w/ that too...
Yes, I'm fine with that.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq()
2024-04-26 15:28 ` Doug Anderson
@ 2024-04-27 0:01 ` Doug Anderson
0 siblings, 0 replies; 10+ messages in thread
From: Doug Anderson @ 2024-04-27 0:01 UTC (permalink / raw)
To: Jani Nikula
Cc: dri-devel, Javier Martinez Canillas, Neil Armstrong,
Dmitry Baryshkov, linus.walleij, Cong Yang, lvzhaoxiong,
Hsin-Yi Wang, Sam Ravnborg, Daniel Vetter, David Airlie,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, linux-kernel
Hi,
On Fri, Apr 26, 2024 at 8:28 AM Doug Anderson <dianders@chromium.org> wrote:
>
> > I don't find this unintuitive, but if it helps, you could conceivably
> > add a context parameter:
> >
> > struct mipi_dsi_seq_context context = {
> > .dsi = dsi,
> > };
> >
> > mipi_dsi_dcs_write_seq(&context, HX83102_SETSPCCMD, 0xcd);
> > ...
> >
> > if (context.ret)
> > ...
> >
> > And even have further control in the context whether to log or keep
> > going or whatever.
>
> I agree there are some benefits of adding the extra "context"
> abstraction and we can go that way if you want, but I lean towards the
> simplicity of just passing in the accumulated return value like I did
> in my example.
>
>
> I'll try to write up patches and see if I can post them later today.
FWIW, I went with your "context" idea. In the end, I liked how it
looked and the icing on the cake was that it generated even smaller
code! :-)
My v2 series (now 8 patches long) is at:
https://lore.kernel.org/r/20240426235857.3870424-1-dianders@chromium.org
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-27 0:01 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 0:20 [PATCH] drm/mipi-dsi: Reduce driver bloat of mipi_dsi_*_write_seq() Douglas Anderson
2024-04-25 7:55 ` Neil Armstrong
2024-04-25 8:19 ` Jani Nikula
2024-04-25 17:04 ` Doug Anderson
2024-04-26 3:03 ` Dmitry Baryshkov
2024-04-26 15:35 ` Doug Anderson
2024-04-26 15:42 ` Dmitry Baryshkov
2024-04-26 10:08 ` Jani Nikula
2024-04-26 15:28 ` Doug Anderson
2024-04-27 0:01 ` Doug Anderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).