Date: Mon, 25 Mar 2024 22:47:57 -0400
From: Emilio Mendoza Reyes <emendoz@...lemson.edu>
To: Doug Anderson <dianders@...omium.org>,
 Doug Anderson <dianders@...omium.org>
Cc: neil.armstrong@...aro.org, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, 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, I'd like to first thank you for your time and for looking at my patch.

On Monday, March 25, 2024 8:21:20 PM 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. 

> 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.  

> 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. 

> 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. 

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

> 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. 

> 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
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists