| Message ID | 20240315143621.v2.1.I16aff881c9fe82b5e0fc06ca312da017aa7b5b3e@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dp: Improve DP AUX transfer vs. HPD interactions | expand |
On Fri, Mar 15, 2024 at 2:37 PM Douglas Anderson <dianders@chromium.org> wrote: > > As documented in the description of the transfer() function of > "struct drm_dp_aux", the transfer() function can be called at any time > regardless of the state of the DP port. Specifically if the kernel has > the DP AUX character device enabled and userspace accesses > "/dev/drm_dp_auxN" directly then the AUX transfer function will be > called regardless of whether a DP device is connected. > > For eDP panels we have a special rule where we wait (with a 5 second > timeout) for HPD to go high. This rule was important before all panels > drivers were converted to call wait_hpd_asserted() and actually can be > removed in a future commit. > > For external DP devices we never checked for HPD. That means that > trying to access the DP AUX character device (AKA `hexdump -C > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my > system: > $ time hexdump -C /dev/drm_dp_aux0 > hexdump: /dev/drm_dp_aux0: Connection timed out > real 0m8.200s > We want access to the drm_dp_auxN character device to fail faster than > 8 seconds when no DP cable is plugged in. > > Let's add a test to make transfers fail right away if a device isn't > plugged in. Rather than testing the HPD line directly, we have the > dp_display module tell us when AUX transfers should be enabled so we > can handle cases where HPD is signaled out of band like with Type C. > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> > --- > > Changes in v2: > - Don't look at the HPD line directly; have dp_display call us. > > drivers/gpu/drm/msm/dp/dp_aux.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_aux.h | 1 + > drivers/gpu/drm/msm/dp/dp_display.c | 4 ++++ > 3 files changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index 03f4951c49f4..e67a80d56948 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -35,6 +35,7 @@ struct dp_aux_private { > bool no_send_stop; > bool initted; > bool is_edp; > + bool enable_xfers; > u32 offset; > u32 segment; > > @@ -301,6 +302,17 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > goto exit; > } > > + /* > + * If we're using DP and an external display isn't connected then the > + * transfer won't succeed. Return right away. If we don't do this we > + * can end up with long timeouts if someone tries to access the DP AUX > + * character device when no DP device is connected. > + */ > + if (!aux->is_edp && !aux->enable_xfers) { > + ret = -ENXIO; > + goto exit; > + } > + > /* > * For eDP it's important to give a reasonably long wait here for HPD > * to be asserted. This is because the panel driver may have _just_ > @@ -433,6 +445,14 @@ irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux) > return IRQ_HANDLED; > } > > +void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled) > +{ > + struct dp_aux_private *aux; > + > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + aux->enable_xfers = enabled; > +} > + > void dp_aux_reconfig(struct drm_dp_aux *dp_aux) > { > struct dp_aux_private *aux; > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h > index 511305da4f66..f3052cb43306 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.h > +++ b/drivers/gpu/drm/msm/dp/dp_aux.h > @@ -12,6 +12,7 @@ > int dp_aux_register(struct drm_dp_aux *dp_aux); > void dp_aux_unregister(struct drm_dp_aux *dp_aux); > irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux); > +void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled); > void dp_aux_init(struct drm_dp_aux *dp_aux); > void dp_aux_deinit(struct drm_dp_aux *dp_aux); > void dp_aux_reconfig(struct drm_dp_aux *dp_aux); > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 4c72124ffb5d..b0f3e2ef5a6b 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -565,6 +565,8 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) > int ret; > struct platform_device *pdev = dp->dp_display.pdev; > > + dp_aux_enable_xfers(dp->aux, true); > + > mutex_lock(&dp->event_mutex); > > state = dp->hpd_state; > @@ -629,6 +631,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) > u32 state; > struct platform_device *pdev = dp->dp_display.pdev; > > + dp_aux_enable_xfers(dp->aux, false); > + > mutex_lock(&dp->event_mutex); > > state = dp->hpd_state; > -- > 2.44.0.291.gc1ea87d7ee-goog >
On 3/15/2024 2:36 PM, Douglas Anderson wrote: > As documented in the description of the transfer() function of > "struct drm_dp_aux", the transfer() function can be called at any time > regardless of the state of the DP port. Specifically if the kernel has > the DP AUX character device enabled and userspace accesses > "/dev/drm_dp_auxN" directly then the AUX transfer function will be > called regardless of whether a DP device is connected. > > For eDP panels we have a special rule where we wait (with a 5 second > timeout) for HPD to go high. This rule was important before all panels > drivers were converted to call wait_hpd_asserted() and actually can be > removed in a future commit. > > For external DP devices we never checked for HPD. That means that > trying to access the DP AUX character device (AKA `hexdump -C > /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my > system: > $ time hexdump -C /dev/drm_dp_aux0 > hexdump: /dev/drm_dp_aux0: Connection timed out > real 0m8.200s > We want access to the drm_dp_auxN character device to fail faster than > 8 seconds when no DP cable is plugged in. > > Let's add a test to make transfers fail right away if a device isn't > plugged in. Rather than testing the HPD line directly, we have the > dp_display module tell us when AUX transfers should be enabled so we > can handle cases where HPD is signaled out of band like with Type C. > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - Don't look at the HPD line directly; have dp_display call us. > > drivers/gpu/drm/msm/dp/dp_aux.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/msm/dp/dp_aux.h | 1 + > drivers/gpu/drm/msm/dp/dp_display.c | 4 ++++ > 3 files changed, 25 insertions(+) > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 03f4951c49f4..e67a80d56948 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -35,6 +35,7 @@ struct dp_aux_private { bool no_send_stop; bool initted; bool is_edp; + bool enable_xfers; u32 offset; u32 segment; @@ -301,6 +302,17 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, goto exit; } + /* + * If we're using DP and an external display isn't connected then the + * transfer won't succeed. Return right away. If we don't do this we + * can end up with long timeouts if someone tries to access the DP AUX + * character device when no DP device is connected. + */ + if (!aux->is_edp && !aux->enable_xfers) { + ret = -ENXIO; + goto exit; + } + /* * For eDP it's important to give a reasonably long wait here for HPD * to be asserted. This is because the panel driver may have _just_ @@ -433,6 +445,14 @@ irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux) return IRQ_HANDLED; } +void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled) +{ + struct dp_aux_private *aux; + + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); + aux->enable_xfers = enabled; +} + void dp_aux_reconfig(struct drm_dp_aux *dp_aux) { struct dp_aux_private *aux; diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h index 511305da4f66..f3052cb43306 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.h +++ b/drivers/gpu/drm/msm/dp/dp_aux.h @@ -12,6 +12,7 @@ int dp_aux_register(struct drm_dp_aux *dp_aux); void dp_aux_unregister(struct drm_dp_aux *dp_aux); irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux); +void dp_aux_enable_xfers(struct drm_dp_aux *dp_aux, bool enabled); void dp_aux_init(struct drm_dp_aux *dp_aux); void dp_aux_deinit(struct drm_dp_aux *dp_aux); void dp_aux_reconfig(struct drm_dp_aux *dp_aux); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 4c72124ffb5d..b0f3e2ef5a6b 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -565,6 +565,8 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) int ret; struct platform_device *pdev = dp->dp_display.pdev; + dp_aux_enable_xfers(dp->aux, true); + mutex_lock(&dp->event_mutex); state = dp->hpd_state; @@ -629,6 +631,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) u32 state; struct platform_device *pdev = dp->dp_display.pdev; + dp_aux_enable_xfers(dp->aux, false); + mutex_lock(&dp->event_mutex); state = dp->hpd_state;
As documented in the description of the transfer() function of "struct drm_dp_aux", the transfer() function can be called at any time regardless of the state of the DP port. Specifically if the kernel has the DP AUX character device enabled and userspace accesses "/dev/drm_dp_auxN" directly then the AUX transfer function will be called regardless of whether a DP device is connected. For eDP panels we have a special rule where we wait (with a 5 second timeout) for HPD to go high. This rule was important before all panels drivers were converted to call wait_hpd_asserted() and actually can be removed in a future commit. For external DP devices we never checked for HPD. That means that trying to access the DP AUX character device (AKA `hexdump -C /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my system: $ time hexdump -C /dev/drm_dp_aux0 hexdump: /dev/drm_dp_aux0: Connection timed out real 0m8.200s We want access to the drm_dp_auxN character device to fail faster than 8 seconds when no DP cable is plugged in. Let's add a test to make transfers fail right away if a device isn't plugged in. Rather than testing the HPD line directly, we have the dp_display module tell us when AUX transfers should be enabled so we can handle cases where HPD is signaled out of band like with Type C. Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Don't look at the HPD line directly; have dp_display call us. drivers/gpu/drm/msm/dp/dp_aux.c | 20 ++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_aux.h | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 4 ++++ 3 files changed, 25 insertions(+)