Received-SPF: pass (google.com: domain of linux-kernel+bounces-118313-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Emilio Mendoza Reyes <emendoz@g.clemson.edu>
To: Doug Anderson <dianders@chromium.org>,
 Doug Anderson <dianders@chromium.org>
Cc: neil.armstrong@linaro.org, linux-kernel@vger.kernel.org,
 dri-devel@lists.freedesktop.org, neil.armstrong@linaro.org,
 linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/panel: Remove redundant checks in multiple panels
Date: Mon, 25 Mar 2024 22:47:57 -0400
Message-ID: <3454518.e9J7NaK4W3@mephi-laptop>
In-Reply-To:
 <CAD=FV=X8N0g0P1d85P0nJHHSkb9xZ-hxJATYD2+b_nNrwqfsUg@mail.gmail.com>
References:
 <12472820.O9o76ZdvQC@mephi-laptop>
 <CAD=FV=X8N0g0P1d85P0nJHHSkb9xZ-hxJATYD2+b_nNrwqfsUg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart3807596.aeNJFYEL58";
 micalg="pgp-sha512"; protocol="application/pgp-signature"

--nextPart3807596.aeNJFYEL58
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"; protected-headers="v1"
From: Emilio Mendoza Reyes <emendoz@g.clemson.edu>
Date: Mon, 25 Mar 2024 22:47:30 -0400
Message-ID: <3454518.e9J7NaK4W3@mephi-laptop>
MIME-Version: 1.0

Hi, I'd like to first thank you for your time and for looking at my patch.

On Monday, March 25, 2024 8:21:20=E2=80=AFPM EDT Doug Anderson wrote:
> 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:
Yeah so sorry about the PGP stuff. Didn't realize my mail client did
inline until after it sent. Whoops.=20

> 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.
Yeah I didn't notice that. Sorry, I am very new to kernel development.

> 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].
I see, yeah I noticed some of these panels also checked for prepared/
enabled in other functions that don't have to do with preparing/enabling
them so those would probs be some that would have to keep the check or
would need more thorough refactoring. =20

> 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(...);
Alright I will look into that.=20

> 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).
Thanks I will start with that then.=20

> 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.
>=20
> 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.
Alright I'll keep this in mind.=20

> 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.
Yeah, that makes sense. I can see how making it a full WARN would result
in a bunch of bug reports and wasted time with the current state of the
panels.=20

> Hopefully that's not too overwhelming.
We'll see. I'll see what I can do. Thanks again for all this usefull info.
Hopefully, I'll have a draft of a more complete patch soon.

Thanks,
EMR
--nextPart3807596.aeNJFYEL58
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTcaZbABwnECYA0cZI0LAN3EqikFgUCZgI3QgAKCRA0LAN3Eqik
FhXFAP4o225efxCCysV0GZmUPS2Q6rfuH4B+TKszU8o3WpKvaAD+J/qOGqAeIs4J
u0DgW5XLyj22Jwyb79TZ9WR9SYLfZwg=
=8iip
-----END PGP SIGNATURE-----

--nextPart3807596.aeNJFYEL58--