Messages in this thread | | | From | Brian Norris <> | Date | Tue, 11 Jan 2022 11:17:04 -0800 | Subject | Re: [PATCH v2] drm/bridge: analogix_dp: Grab runtime PM reference for DP-AUX |
| |
Hi Andrzej,
On Tue, Jan 11, 2022 at 5:26 AM Andrzej Hajda <andrzej.hajda@intel.com> wrote: > I am not DP specialist so CC-ed people working with DP
Thanks for the review regardless! I'll also not claim to be a DP specialist -- although I've had to learn my fair share to debug a good handful of issues on an SoC using this driver.
> On 01.10.2021 23:42, Brian Norris wrote: > > If the display is not enable()d, then we aren't holding a runtime PM > > reference here. Thus, it's easy to accidentally cause a hang, if user > > space is poking around at /dev/drm_dp_aux0 at the "wrong" time. > > > > Let's get the panel and PM state right before trying to talk AUX. > > > > Fixes: 0d97ad03f422 ("drm/bridge: analogix_dp: Remove duplicated code") > > Cc: <stable@vger.kernel.org> > > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > Few questions/issues here: > > 1. If it is just to avoid accidental 'hangs' it would be better to just > check if the panel is working before transfer, if not, return error > code. If there is better reason for this pm dance, please provide it in > description.
I'm not that familiar with DP-AUX, but I believe it can potentially provide a variety of useful information (e.g., EDID?) to users without the display and primary video link being active. So it doesn't sound like a good idea to me to purposely leave this interface uninitialized (and emitting errors) even when the user is asking for communication (via /dev/drm_dp_aux<N>). Do you want me to document what /dev/drm_dp_aux<N> does, and why someone would use it, in the commit message?
> 2. Again I see an assumption that panel-prepare enables power for > something different than video transmission, accidentally it is true for > most devices, but devices having more fine grained power management will > break, or at least will be used inefficiently - but maybe in case of dp > it is OK ???
For this part, I'm less sure -- I wasn't sure what the general needs are for AUX communication, and whether we need the panel enabled or not. It seems logical that we need something powered, and I don't know of anything besides "prepare()" that ensures that for DP panels.
(NB: the key to _my_ problem is the PM runtime reference. It's absolutely essential that we don't try to utilize the DP hardware without powering it up. The panel power state is less critical.)
> 3. More general issue - I am not sure if this should not be handled > uniformly for all drm_dp devices.
I'm not sure what precisely you mean by #3. But FWIW, this is at least partially documented ("make sure it's been properly enabled"):
/** * @transfer: transfers a message representing a single AUX * transaction. * * This is a hardware-specific implementation of how * transactions are executed that the drivers must provide. ... * Also note that this callback can be called no matter the * state @dev is in. Drivers that need that device to be powered * to perform this operation will first need to make sure it's * been properly enabled. */ ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg);
But maybe the definition of "properly enabled" is what you're unsure about? (I'm also a little unsure.)
Regards, Brian
| |