Hi Brian,

I am not DP specialist so CC-ed people working with DP

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.

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

3. More general issue - I am not sure if this should not be handled uniformly for all drm_dp devices.

Regards

Andrzej

---

Changes in v2:

• Fix spelling in Subject

• DRM_DEV_ERROR() -> drm_err()

• Propagate errors from un-analogix_dp_prepare_panel()

  .../drm/bridge/analogix/analogix_dp_core.c | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b7d2e4449cfa..6fc46ac93ef8 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { struct analogix_dp_device *dp = to_dp(aux);

• int ret, ret2;

• return analogix_dp_transfer(dp, msg);

• ret = analogix_dp_prepare_panel(dp, true, false);

• if (ret) {

• drm_err(dp->drm_dev, "Failed to prepare panel (%d)\n", ret);
  

• return ret;
  

• }

• pm_runtime_get_sync(dp->dev);

• ret = analogix_dp_transfer(dp, msg);

• pm_runtime_put(dp->dev);

• ret2 = analogix_dp_prepare_panel(dp, false, false);

• if (ret2) {

• drm_err(dp->drm_dev, "Failed to unprepare panel (%d)\n", ret2);
  

• /* Prefer the analogix_dp_transfer() error, if it exists. */
  

• if (!ret)
  

• 	ret = ret2;
  

• }

• return ret; }

  struct analogix_dp_device *

Hi,

On Fri, Oct 1, 2021 at 2:50 PM Brian Norris briannorris@chromium.org 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

---

Changes in v2:

• Fix spelling in Subject
• DRM_DEV_ERROR() -> drm_err()
• Propagate errors from un-analogix_dp_prepare_panel()

.../drm/bridge/analogix/analogix_dp_core.c | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b7d2e4449cfa..6fc46ac93ef8 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1632,8 +1632,27 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { struct analogix_dp_device *dp = to_dp(aux);

•   int ret, ret2;
  
  

•   return analogix_dp_transfer(dp, msg);
  

•   ret = analogix_dp_prepare_panel(dp, true, false);
  

•   if (ret) {
  

•           drm_err(dp->drm_dev, "Failed to prepare panel (%d)\n", ret);
  

•           return ret;
  

•   }
  

•   pm_runtime_get_sync(dp->dev);
  

•   ret = analogix_dp_transfer(dp, msg);
  

•   pm_runtime_put(dp->dev);
  

I've spent an unfortunate amount of time digging around the DP AUX bus recently, so I can at least say that I have some experience and some opinions here.

IMO:

1. Don't power the panel on. If the panel isn't powered on then the DP AUX transfer will timeout. Tough nuggies. Think of yourself more like an i2c controller and of this as an i2c transfer implementation. The i2c controller isn't in charge of powering up the i2c devices on the bus. If userspace does an "i2c detect" on an i2c bus and some of the devices aren't powered then they won't be found. If you try to read/write from a powered off device that won't work either.

2. In theory if the DP driver can read HPD (I haven't looked through the analogix code to see how it handles it) then you can fail an AUX transfer right away if HPD isn't asserted instead of timing out. If this is hard, it's probably fine to just time out though.

3. Do the "pm_runtime" calls, but enable "autosuspend" with something ~1 second autosuspend delay. When using the AUX bus to read an EDID the underlying code will call your function 16 times in quick succession. If you're powering up and down constantly that'll be a bit of a waste.

The above will help set us up for when someone goes through and enables the "new" DP AUX bus and generic eDP panels on for analogix-edp. See commit aeb33699fc2c ("drm: Introduce the DP AUX bus"). In that case panels will actually be instantiated DP AUX Endpoints instead of platform devices. They'll be given the DP AUX bus and they'll be able to read the EDID over that. In this case, the panel code turns itself on (it knows how to turn itself on enough to read the EDID) and then calls the DP AUX transfer code. :-)

For a reference, you could look at `drivers/gpu/drm/bridge/ti-sn65dsi86.c`. Also `drivers/gpu/drm/bridge/parade-ps8640.c`

-Doug