Messages in this thread | | | Date | Wed, 13 Mar 2024 13:41:16 -0700 | Subject | Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected | From | Abhinav Kumar <> |
| |
On 3/12/2024 5:13 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. >
I do see
" * Also note that this callback can be called no matter the * state @dev is in and also no matter what state the panel is * in. It's expected: "
I understand about the host state that we need to allow the transfers by powering on if the host was off.
But I wonder why we should allow the transfer if the sink is not connected because it will anyway timeout.
Does it make sense to have get_hpd_status() from the aux dev and not issue the transfers if the sink was not connected?
This is more of questioning the intent of drm_dp_helpers to allow transfers without checking the sink status.
> 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 >
IIUC, we want to timeout faster by not bailing out if not connected right?
> Let's add a check for HPD to avoid the slow timeout. This matches > what, for instance, the intel_dp_aux_xfer() function does when it > calls intel_tc_port_connected_locked(). That call has a document by it > explaining that it's important to avoid the long timeouts. > > Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/msm/dp/dp_aux.c | 8 +++++++- > drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++++++++++ > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index 03f4951c49f4..de0b0eabced9 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > * turned on the panel and then tried to do an AUX transfer. The panel > * driver has no way of knowing when the panel is ready, so it's up > * to us to wait. For DP we never get into this situation so let's > - * avoid ever doing the extra long wait for DP. > + * avoid ever doing the extra long wait for DP and just query HPD > + * directly. > */ > if (aux->is_edp) { > ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, > DRM_DEBUG_DP("Panel not ready for aux transactions\n"); > goto exit; > } > + } else { > + if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) { > + ret = -ENXIO; > + goto exit; > + } > } > > dp_aux_update_offset_and_segment(aux, msg); > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index 5142aeb705a4..93e2d413a1e7 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) > 2000, 500000); > } > > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog) > +{ > + struct dp_catalog_private *catalog = container_of(dp_catalog, > + struct dp_catalog_private, dp_catalog); > + > + /* poll for hpd connected status every 2ms and timeout after 500ms */ > + return readl(catalog->io->dp_controller.aux.base + REG_DP_DP_HPD_INT_STATUS) & > + DP_DP_HPD_STATE_STATUS_CONNECTED; > +}
This method of checking HPD status works for devices which use internal HPD block to control the HPD (like sc7180/sc7280) but not for devices where HPD is controlled outside the MSM DP controller like sc8280xp, sc835-/sm8450 etc etc which use pmic_glink and DP driver only receives the hpd status using the dp_bridge_hpd_notify() callback.
If we want to make this generic, we have to do something like:
dp_hpd_unplug_handle() notifies the dp_aux.c module that status is disconncted and we should bail out
dp_hpd_plug_handle() notifies dp_aux.c module that status is connected again and we allow the aux transfers.
> + > static void dump_regs(void __iomem *base, int len) > { > int i; > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h > index 38786e855b51..1694040c530f 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.h > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h > @@ -86,6 +86,7 @@ void dp_catalog_aux_reset(struct dp_catalog *dp_catalog); > void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable); > void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog); > int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog); > +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog); > u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog); > > /* DP Controller APIs */
| |