Message-ID: <20240726-cerise-civet-of-reverence-ebeb9d@houat>
Date: Fri, 26 Jul 2024 11:15:17 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Doug Anderson <dianders@...omium.org>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>, 
	Tejas Vipin <tejasvipin76@...il.com>, maarten.lankhorst@...ux.intel.com, tzimmermann@...e.de, 
	airlied@...il.com, daniel@...ll.ch, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drm/mipi-dsi: Change multi functions to use quiet
 member of mipi_dsi_multi_context

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@...nel.org> wrote:
> >
> > On Wed, Jul 24, 2024 at 06:32:14PM GMT, Jani Nikula wrote:
> > > On Wed, 24 Jul 2024, Tejas Vipin <tejasvipin76@...il.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@...il.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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists