Message-ID: <CAD=FV=X8N0g0P1d85P0nJHHSkb9xZ-hxJATYD2+b_nNrwqfsUg@mail.gmail.com>
Date: Mon, 25 Mar 2024 17:21:20 -0700
From: Doug Anderson <dianders@...omium.org>
To: Emilio Mendoza Reyes <emendoz@...lemson.edu>
Cc: neil.armstrong@...aro.org, linux-kernel@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 1/2] drm/panel: Remove redundant checks in multiple panels

Hi,

On Sat, Mar 23, 2024 at 7:05 PM Emilio Mendoza Reyes
<emendoz@...lemson.edu> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> From: Emilio Mendoza Reyes <emendoz@...mson.edu>
>
> The patch ("drm/panel: Check for already prepared/enabled in drm_panel")
> moved checking for (en/dis)abled and [un]prepared panels before specific
> function calls to drm_panel.c.Those checks that still exist within the
> panels are redundant. This patch removes those redundant checks.
>
> Removing those checks was/is also a todo in the kernel docs
> Link: https://www.kernel.org/doc/html/v6.8/gpu/todo.html#clean-up-checks-for-already-prepared-enabled-in-panels
>
> Signed-off-by: Emilio Mendoza Reyes <emendoz@...mson.edu>
> - ---
>  drivers/gpu/drm/panel/panel-boe-himax8279d.c     | 12 ------------
>  drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c   |  6 ------
>  drivers/gpu/drm/panel/panel-edp.c                | 14 --------------
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c    | 12 ------------
>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c   | 12 ------------
>  drivers/gpu/drm/panel/panel-khadas-ts050.c       |  9 ---------
>  .../gpu/drm/panel/panel-kingdisplay-kd097d04.c   | 12 ------------
>  .../gpu/drm/panel/panel-leadtek-ltk050h3146w.c   |  6 ------
>  .../gpu/drm/panel/panel-leadtek-ltk500hd1829.c   |  6 ------
>  drivers/gpu/drm/panel/panel-novatek-nt36672a.c   |  6 ------
>  .../gpu/drm/panel/panel-olimex-lcd-olinuxino.c   | 12 ------------
>  .../gpu/drm/panel/panel-osd-osd101t2587-53ts.c   | 12 ------------
>  .../gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 12 ------------
>  drivers/gpu/drm/panel/panel-raydium-rm67191.c    | 12 ------------
>  drivers/gpu/drm/panel/panel-raydium-rm692e5.c    |  6 ------
>  drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 16 ----------------
>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c      | 12 ------------
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c  | 12 ------------
>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c  |  6 ------
>  drivers/gpu/drm/panel/panel-simple.c             | 14 --------------
>  drivers/gpu/drm/panel/panel-sitronix-st7703.c    |  6 ------
>  drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c     |  6 ------
>  drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c |  6 ------
>  23 files changed, 227 deletions(-)

Aside from the formatting issues (several lines start with an extra
"-" and there is the PGP stuff), there are a few high-level issues
here:

1. In general, you need to be a little more careful here because not
all these panels are going through the code path that the new code
covers. For instance, look at the first panel here
("panel-boe-himax8279d.c"). The panel_shutdown() function in that
driver _directly_ calls boe_panel_disable() and boe_panel_unprepare()
rather than calling the drm_panel equivalent function. That means that
they _won't_ get the benefit of the checking I added to drm_panel.c.
What that means is that if someone using the "panel-boe-himax8279d.c"
panel shuts down / reboots while their panel is off you'll probably
get a bunch of errors. I _think_ this is as simple as just changing
all those panels to call the drm_panel equivalent function.

2. As much as possible, not only should you be removing the checks,
but you should also be removing all the code that tracks the state of
prepared/enabled in the panel drivers and then removing the "prepared"
/ "enabled" members from the structs. If the panel driver needs to
track prepared/enabled for something else, you might need to keep it
though. One example would be sony-acx565akm, as per my previous
attempt [1].

In general, maybe a good approach would be to actually start with
patches #5 - #9 in my previous series [2] but instead of calling
drm_panel_helper_shutdown() just do:
  drm_panel_disable(...);
  drm_panel_unprepare(...);

Feel free to take my patches, change them, and post them. If you want,
you could add a Co-Developed-by (see submitting-patches.rst).

For some panels the above would be just leaving what's already there.
For some panels that might convert them from their static function to
the drm_panel equivalent.

Leaving the drm_panel_disable() / drm_panel_unprepare() in the panel
driver shutdown/remove code is not an ideal long term solution, but it
at least moves us on the right path and at least lets us get rid of
most of the prepared / enabled tracking. IMO that should be landable,
though perhaps others would have different opinions.

After doing all that, then you could submit patches that simply get
rid of the drm_panel_disable() / drm_panel_unprepare() for any panel
drivers if you know that those panels are only used by DRM drivers
that properly call the DRM shutdown code. See my series that tried to
add that to a bunch of DRM drivers [3]. Not everything landed, but
quite a bit of it did. As Maxime and I talked about [4] that _should_
be as simple as tracking down the panel's compatible string, seeing
which device trees use it, and then seeing if the associated DRM
driver is properly shutting things down.

Finally, after you've removed the calls to drm_panel_disable() /
drm_panel_unprepare() from the majority of the panel drivers then you
could go forward with your patch #2 where you change this to a WARN().
Personally, I'd be a little hesitant to land that change, though,
until at least panel-simple and panel-edp got rid of the calls since
that would add WARN for A LOT of people. Unfortunately, those two
panels drivers are also among the hardest to validate since they're
used with nearly all DRM drivers out there. However, IMO even if we
aren't ready to convert to a full WARN all the rest of the stuff I've
talked about here is worth doing.

Hopefully that's not too overwhelming.


[1] https://lore.kernel.org/lkml/20230804140605.RFC.9.I6a51b36831a5c7b2b82bccf8c550cf0d076aa541@changeid/
[2] https://lore.kernel.org/lkml/20230804210644.1862287-1-dianders@chromiumorg/
[3] https://lore.kernel.org/lkml/20230921192749.1542462-1-dianders@chromiumorg/
[4] https://lore.kernel.org/lkml/c6kwqxz2xgl64qb6dzetjjh6j2a6hj7mvbkeg57f5ulfs2hrib@ocjjsoxw3ns6/

Powered by blists - more mailing lists