Re: [PATCH 1/2] drm/panel: Remove redundant checks in multiple panels

[Emilio Mendoza Reyes <emendoz@g.clemson.edu>]

[-- Attachment #1: Type: text/plain, Size: 3990 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]