| Message ID | 20240724122447.284165-3-tejasvipin76@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow errors to be silenced in multi functions | expand |
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); > }
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
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
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
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
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); }
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(+)