Message-ID: <CAD=FV=WbXdnM4or3Ae+nYoQW1Sce0jP6FWtCHShsALuEFNhiww@mail.gmail.com>
Date: Thu, 25 Jul 2024 10:12:46 -0700
From: Doug Anderson <dianders@...omium.org>
To: Maxime Ripard <mripard@...nel.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 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.

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

Powered by blists - more mailing lists