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:
- 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?
- 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.)
- 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