[PATCH 0/2] Allow errors to be silenced in multi functions

[PATCH 0/2] Allow errors to be silenced in multi functions

[Tejas Vipin]

Multi functions so far always print errors when any function fails. This
may not always be desirable, so a new member of mipi_dsi_multi_context
is introduced to allow errors to be silenced.

The larger implication of this is that all the old non-multi functions
can be replaced entirely by the multi functions without any loss of
functionality once all the panels are changed to use multi functions.

Tejas Vipin (2):
  drm/mipi-dsi: Add quiet member to mipi_dsi_multi_context struct
  drm/mipi-dsi: Change multi functions to use quiet member of
    mipi_dsi_multi_context

 drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     | 10 ++++++++++
 2 files changed, 30 insertions(+)

-- 
2.45.2

[PATCH 1/2] drm/mipi-dsi: Add quiet member to mipi_dsi_multi_context struct

[Tejas Vipin]

A "quiet" member is added to mipi_dsi_multi_context which allows 
silencing all the errors printed by the multi functions.

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 include/drm/drm_mipi_dsi.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 0f520eeeaa8e..75855c1c7dae 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -217,6 +217,16 @@ struct mipi_dsi_multi_context {
 	 * end to see if any of them failed.
 	 */
 	int accum_err;
+
+	/**
+	 * @quiet: Controls if a function calls dev_err or not
+	 *
+	 * Init to 0. When the value of quiet is set to 0, the function
+	 * will  print error messages as required. If this is set to 1,
+	 * the function will not print error messages, but will still
+	 * change the value of accum_err.
+	 */
+	int quiet;
 };
 
 #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
-- 
2.45.2

Re: [PATCH 1/2] drm/mipi-dsi: Add quiet member to mipi_dsi_multi_context struct

[Jani Nikula]

On Wed, 24 Jul 2024, Tejas Vipin <tejasvipin76@gmail.com> wrote:
> A "quiet" member is added to mipi_dsi_multi_context which allows 
> silencing all the errors printed by the multi functions.
>
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
>  include/drm/drm_mipi_dsi.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 0f520eeeaa8e..75855c1c7dae 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -217,6 +217,16 @@ struct mipi_dsi_multi_context {
>  	 * end to see if any of them failed.
>  	 */
>  	int accum_err;
> +
> +	/**
> +	 * @quiet: Controls if a function calls dev_err or not
> +	 *
> +	 * Init to 0. When the value of quiet is set to 0, the function
> +	 * will  print error messages as required. If this is set to 1,
> +	 * the function will not print error messages, but will still
> +	 * change the value of accum_err.
> +	 */
> +	int quiet;

This is being used as a bool, why not make it a bool?

BR,
Jani.


>  };
>  
>  #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"

-- 
Jani Nikula, Intel

[PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context

[Tejas Vipin]

Changes all the multi functions to check if the current context requires
errors to be printed or not using the quiet member.

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a471c46f5ca6..cbb77342d201 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -814,6 +814,8 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
 	ret = mipi_dsi_generic_write(dsi, payload, size);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending generic data %*ph failed: %d\n",
 			(int)size, payload, ctx->accum_err);
 	}
@@ -958,6 +960,8 @@ void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
 	ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending dcs data %*ph failed: %d\n",
 			(int)len, data, ctx->accum_err);
 	}
@@ -1450,6 +1454,8 @@ void mipi_dsi_picture_parameter_set_multi(struct mipi_dsi_multi_context *ctx,
 	ret = mipi_dsi_picture_parameter_set(dsi, pps);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending PPS failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1481,6 +1487,8 @@ void mipi_dsi_compression_mode_ext_multi(struct mipi_dsi_multi_context *ctx,
 	ret = mipi_dsi_compression_mode_ext(dsi, enable, algo, pps_selector);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending COMPRESSION_MODE failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1506,6 +1514,8 @@ void mipi_dsi_dcs_nop_multi(struct mipi_dsi_multi_context *ctx)
 	ret = mipi_dsi_dcs_nop(dsi);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending DCS NOP failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1531,6 +1541,8 @@ void mipi_dsi_dcs_enter_sleep_mode_multi(struct mipi_dsi_multi_context *ctx)
 	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending DCS ENTER_SLEEP_MODE failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1556,6 +1568,8 @@ void mipi_dsi_dcs_exit_sleep_mode_multi(struct mipi_dsi_multi_context *ctx)
 	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending DCS EXIT_SLEEP_MODE failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1581,6 +1595,8 @@ void mipi_dsi_dcs_set_display_off_multi(struct mipi_dsi_multi_context *ctx)
 	ret = mipi_dsi_dcs_set_display_off(dsi);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending DCS SET_DISPLAY_OFF failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1606,6 +1622,8 @@ void mipi_dsi_dcs_set_display_on_multi(struct mipi_dsi_multi_context *ctx)
 	ret = mipi_dsi_dcs_set_display_on(dsi);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending DCS SET_DISPLAY_ON failed: %d\n",
 			ctx->accum_err);
 	}
@@ -1633,6 +1651,8 @@ void mipi_dsi_dcs_set_tear_on_multi(struct mipi_dsi_multi_context *ctx,
 	ret = mipi_dsi_dcs_set_tear_on(dsi, mode);
 	if (ret < 0) {
 		ctx->accum_err = ret;
+		if (ctx->quiet)
+			return;
 		dev_err(dev, "sending DCS SET_TEAR_ON failed: %d\n",
 			ctx->accum_err);
 	}
-- 
2.45.2

Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context

[Jani Nikula]

On Wed, 24 Jul 2024, Tejas Vipin <tejasvipin76@gmail.com> wrote:
> Changes all the multi functions to check if the current context requires
> errors to be printed or not using the quiet member.
>
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index a471c46f5ca6..cbb77342d201 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -814,6 +814,8 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
>  	ret = mipi_dsi_generic_write(dsi, payload, size);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending generic data %*ph failed: %d\n",
>  			(int)size, payload, ctx->accum_err);

A maintainability nitpick. Imagine someone wishing to add some more
error handling here. Or something else after the block.

IMO the dev_err() should be wrapped in if (!ctx->quiet) instead of
adding an early return.

Ditto everywhere.

BR,
Jani.


>  	}
> @@ -958,6 +960,8 @@ void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
>  	ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending dcs data %*ph failed: %d\n",
>  			(int)len, data, ctx->accum_err);
>  	}
> @@ -1450,6 +1454,8 @@ void mipi_dsi_picture_parameter_set_multi(struct mipi_dsi_multi_context *ctx,
>  	ret = mipi_dsi_picture_parameter_set(dsi, pps);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending PPS failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1481,6 +1487,8 @@ void mipi_dsi_compression_mode_ext_multi(struct mipi_dsi_multi_context *ctx,
>  	ret = mipi_dsi_compression_mode_ext(dsi, enable, algo, pps_selector);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending COMPRESSION_MODE failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1506,6 +1514,8 @@ void mipi_dsi_dcs_nop_multi(struct mipi_dsi_multi_context *ctx)
>  	ret = mipi_dsi_dcs_nop(dsi);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending DCS NOP failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1531,6 +1541,8 @@ void mipi_dsi_dcs_enter_sleep_mode_multi(struct mipi_dsi_multi_context *ctx)
>  	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending DCS ENTER_SLEEP_MODE failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1556,6 +1568,8 @@ void mipi_dsi_dcs_exit_sleep_mode_multi(struct mipi_dsi_multi_context *ctx)
>  	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending DCS EXIT_SLEEP_MODE failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1581,6 +1595,8 @@ void mipi_dsi_dcs_set_display_off_multi(struct mipi_dsi_multi_context *ctx)
>  	ret = mipi_dsi_dcs_set_display_off(dsi);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending DCS SET_DISPLAY_OFF failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1606,6 +1622,8 @@ void mipi_dsi_dcs_set_display_on_multi(struct mipi_dsi_multi_context *ctx)
>  	ret = mipi_dsi_dcs_set_display_on(dsi);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending DCS SET_DISPLAY_ON failed: %d\n",
>  			ctx->accum_err);
>  	}
> @@ -1633,6 +1651,8 @@ void mipi_dsi_dcs_set_tear_on_multi(struct mipi_dsi_multi_context *ctx,
>  	ret = mipi_dsi_dcs_set_tear_on(dsi, mode);
>  	if (ret < 0) {
>  		ctx->accum_err = ret;
> +		if (ctx->quiet)
> +			return;
>  		dev_err(dev, "sending DCS SET_TEAR_ON failed: %d\n",
>  			ctx->accum_err);
>  	}

-- 
Jani Nikula, Intel

Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

On Wed, Jul 24, 2024 at 06:32:14PM GMT, Jani Nikula wrote:
> On Wed, 24 Jul 2024, Tejas Vipin <tejasvipin76@gmail.com> wrote:
> > Changes all the multi functions to check if the current context requires
> > errors to be printed or not using the quiet member.
> >
> > Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > index a471c46f5ca6..cbb77342d201 100644
> > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > @@ -814,6 +814,8 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
> >  	ret = mipi_dsi_generic_write(dsi, payload, size);
> >  	if (ret < 0) {
> >  		ctx->accum_err = ret;
> > +		if (ctx->quiet)
> > +			return;
> >  		dev_err(dev, "sending generic data %*ph failed: %d\n",
> >  			(int)size, payload, ctx->accum_err);
> 
> A maintainability nitpick. Imagine someone wishing to add some more
> error handling here. Or something else after the block.
> 
> IMO the dev_err() should be wrapped in if (!ctx->quiet) instead of
> adding an early return.
> 
> Ditto everywhere.

I'm not even sure why we need that parameter in the first place.

Like, what's the expected use of that parameter? Is it supposed to be
set in users of that function?

If so, wouldn't it prevent any sort of meaningful debugging if some
drivers set it and some don't?

It looks to me like you're reimplementing drm.debug.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context

[Doug Anderson]

Hi,

On Thu, Jul 25, 2024 at 1:28 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Jul 24, 2024 at 06:32:14PM GMT, Jani Nikula wrote:
> > On Wed, 24 Jul 2024, Tejas Vipin <tejasvipin76@gmail.com> wrote:
> > > Changes all the multi functions to check if the current context requires
> > > errors to be printed or not using the quiet member.
> > >
> > > Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> > > ---
> > >  drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > > index a471c46f5ca6..cbb77342d201 100644
> > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > @@ -814,6 +814,8 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
> > >     ret = mipi_dsi_generic_write(dsi, payload, size);
> > >     if (ret < 0) {
> > >             ctx->accum_err = ret;
> > > +           if (ctx->quiet)
> > > +                   return;
> > >             dev_err(dev, "sending generic data %*ph failed: %d\n",
> > >                     (int)size, payload, ctx->accum_err);
> >
> > A maintainability nitpick. Imagine someone wishing to add some more
> > error handling here. Or something else after the block.
> >
> > IMO the dev_err() should be wrapped in if (!ctx->quiet) instead of
> > adding an early return.
> >
> > Ditto everywhere.
>
> I'm not even sure why we need that parameter in the first place.
>
> Like, what's the expected use of that parameter? Is it supposed to be
> set in users of that function?
>
> If so, wouldn't it prevent any sort of meaningful debugging if some
> drivers set it and some don't?
>
> It looks to me like you're reimplementing drm.debug.

I can explain how we got here and maybe you can explain how it should
be designed differently.

1. The majority of MIPI panels drivers just have a pile of commands
that need to be sent during panel init time. Each time you send a
command it technically can fail but it's very unlikely. In order to
make things debuggable in the unlikely case of failure, you want a
printout about which command failed. In order to avoid massive numbers
of printouts in each driver you want the printout in the core. This is
the impetus behind the "_multi" functions that were introduced
recently and I think most people who have looked at converted drivers
are pretty pleased. The functions are readable/non-bloated but still
check for errors and print messages in the right place.

2. As Tejas was adding more "_multi" variants things were starting to
feel a bit awkward. Dmitry proposed [1] that maybe the answer was that
we should work to get rid of the non-multi variants and then we don't
need these awkward wrappers.

3. The issue with telling everyone to use the "_multi" variants is
that they print the error message for you. While this is the correct
default behavior and the correct behavior for the vast majority of
users, I can imagine there being a legitimate case where a driver
might not want error messages printed. I don't personally know of a
case, but in my experience there's always some case where you're
dealing with weird hardware and the driver knows that a command has a
high chance of failure. Maybe the driver will retry or maybe it'll
detect /handle the error, but the idea is that the driver wouldn't
want a printout.

Said another way: I think of the errors of these MIPI initialization
functions a lot like errors allocating memory. By default kmalloc()
reports an error so all drivers calling kmalloc() don't need to print,
but if your driver specifically knows that an allocation failure is
likely and it knows how to handle it then it can pass a flag to tell
the core kernel not to print.


So I guess options are:

a) Accept what Tejas is doing here as reasonable.

b) Don't accept what Tejas is doing here and always keep the "_multi"
functions as wrappers.

c) Declare that, since there are no known cases where we want to
suppress the error printouts, that suppressing the error printouts is
a "tomorrow" problem. We transition everyone to _multi but don't
provide a way to suppress the printouts.

d) Declare that the _multi functions are terrible and undo all the
recent changes. Hopefully we don't do this. :-P


[1] https://lore.kernel.org/r/p7fahem6egnooi5org4lv3gtz2elafylvlnyily7buvzcpv2qh@vssvpfci3lwn

Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5240 bytes --]

Hi,

On Thu, Jul 25, 2024 at 10:12:46AM GMT, Doug Anderson wrote:
> On Thu, Jul 25, 2024 at 1:28 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Wed, Jul 24, 2024 at 06:32:14PM GMT, Jani Nikula wrote:
> > > On Wed, 24 Jul 2024, Tejas Vipin <tejasvipin76@gmail.com> wrote:
> > > > Changes all the multi functions to check if the current context requires
> > > > errors to be printed or not using the quiet member.
> > > >
> > > > Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_mipi_dsi.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> > > > index a471c46f5ca6..cbb77342d201 100644
> > > > --- a/drivers/gpu/drm/drm_mipi_dsi.c
> > > > +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> > > > @@ -814,6 +814,8 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
> > > >     ret = mipi_dsi_generic_write(dsi, payload, size);
> > > >     if (ret < 0) {
> > > >             ctx->accum_err = ret;
> > > > +           if (ctx->quiet)
> > > > +                   return;
> > > >             dev_err(dev, "sending generic data %*ph failed: %d\n",
> > > >                     (int)size, payload, ctx->accum_err);
> > >
> > > A maintainability nitpick. Imagine someone wishing to add some more
> > > error handling here. Or something else after the block.
> > >
> > > IMO the dev_err() should be wrapped in if (!ctx->quiet) instead of
> > > adding an early return.
> > >
> > > Ditto everywhere.
> >
> > I'm not even sure why we need that parameter in the first place.
> >
> > Like, what's the expected use of that parameter? Is it supposed to be
> > set in users of that function?
> >
> > If so, wouldn't it prevent any sort of meaningful debugging if some
> > drivers set it and some don't?
> >
> > It looks to me like you're reimplementing drm.debug.
> 
> I can explain how we got here and maybe you can explain how it should
> be designed differently.
> 
> 1. The majority of MIPI panels drivers just have a pile of commands
> that need to be sent during panel init time. Each time you send a
> command it technically can fail but it's very unlikely. In order to
> make things debuggable in the unlikely case of failure, you want a
> printout about which command failed. In order to avoid massive numbers
> of printouts in each driver you want the printout in the core. This is
> the impetus behind the "_multi" functions that were introduced
> recently and I think most people who have looked at converted drivers
> are pretty pleased. The functions are readable/non-bloated but still
> check for errors and print messages in the right place.
>
> 2. As Tejas was adding more "_multi" variants things were starting to
> feel a bit awkward. Dmitry proposed [1] that maybe the answer was that
> we should work to get rid of the non-multi variants and then we don't
> need these awkward wrappers.

Makes sense to me so far

> 3. The issue with telling everyone to use the "_multi" variants is
> that they print the error message for you. While this is the correct
> default behavior and the correct behavior for the vast majority of
> users, I can imagine there being a legitimate case where a driver
> might not want error messages printed.

That's the part I disagree with actually. I don't think that message
printing is a driver's decision, but a context one. Users might want to
increase / decrease the log levels, but drivers shouldn't and just
provide a consistent behaviour there.

> I don't personally know of a case, but in my experience there's always
> some case where you're dealing with weird hardware and the driver
> knows that a command has a high chance of failure. Maybe the driver
> will retry or maybe it'll detect /handle the error, but the idea is
> that the driver wouldn't want a printout.

Then we'll address it when the time comes and we're sure what we're
actually trying to fix. And even that theoretical scenario might just
disappear when the printk threaded printing work is done.

> Said another way: I think of the errors of these MIPI initialization
> functions a lot like errors allocating memory. By default kmalloc()
> reports an error so all drivers calling kmalloc() don't need to print,
> but if your driver specifically knows that an allocation failure is
> likely and it knows how to handle it then it can pass a flag to tell
> the core kernel not to print.
> 
> 
> So I guess options are:
> 
> a) Accept what Tejas is doing here as reasonable.

I don't think it's unreasonable, however I do think it's an API that has
no current users and will just lead to inconsistencies in the drivers,
without any benefit at the moment.

> b) Don't accept what Tejas is doing here and always keep the "_multi"
> functions as wrappers.
> 
> c) Declare that, since there are no known cases where we want to
> suppress the error printouts, that suppressing the error printouts is
> a "tomorrow" problem. We transition everyone to _multi but don't
> provide a way to suppress the printouts.

That's my preferred solution.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet member of mipi_dsi_multi_context

[Doug Anderson]

Hi,

On Fri, Jul 26, 2024 at 2:15 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > c) Declare that, since there are no known cases where we want to
> > suppress the error printouts, that suppressing the error printouts is
> > a "tomorrow" problem. We transition everyone to _multi but don't
> > provide a way to suppress the printouts.
>
> That's my preferred solution.

OK, fair enough. So I guess the transition plan would be:

1. Add a wrapper like we're doing today.

2. Transition everyone to the _multi variant.

3. Delete the non-multi variant which will cause us to delete the wrapper.


-Doug