Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()
On Fri, Nov 22, 2024 at 02:22:44PM +0200, Jani Nikula wrote:
> On Wed, 20 Nov 2024, Imre Deak wrote:
> > On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
> >> SST with 128b/132b channel coding needs this too. Extract to a separate
> >> helper, independent of MST.
> >>
> >> Pass timeout in as a parameter, anticipating that we can reduce the
> >> timeout for SST.
> >
> > I wish there was a DP Standard section making the above clear,
> > but I suppose we just deduct that except of the side-band messaging,
> > every other payload programming and ACT signaling is required for
> > 128b/132b SST.
The closest I found explaing the above is 5.7.5. Allocation Change
Trigger in DP Standard v2.1, maybe worth mentioning it in the commit
log.
> Ping. Okay to merge, or do we want to mull over the the timeout?
My R-b still applies, if that's enough for merging.
> I'm primarily trying to do a non-functional change. I could omit the
> timeout parameter, but for non-hubs three seconds seems excessive, and
> reducing it for hubs too is a can of worms I prefer keeping the lid on
> top.
>
> > Cc: Lyude Paul
> >
> >> Signed-off-by: Jani Nikula
> >> ---
> >> drivers/gpu/drm/display/drm_dp_helper.c | 54 ++-
> >> drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +
> >> include/drm/display/drm_dp_helper.h | 2 +
> >> 3 files changed, 57 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> >> b/drivers/gpu/drm/display/drm_dp_helper.c
> >> index 6ee51003de3c..b7e03bf02cd8 100644
> >> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >> @@ -22,15 +22,16 @@
> >>
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> -#include
> >>
> >> #include
> >> #include
> >> @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct
> >> drm_dp_aux *aux,
> >> }
> >> EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
> >>
> >> +static int read_payload_update_status(struct drm_dp_aux *aux)
> >> +{
> >> + int ret;
> >> + u8 status;
> >> +
> >> + ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return status;
> >> +}
> >> +
> >> +/**
> >> + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
> >> + * @aux: DisplayPort AUX channel
> >> + * @timeout_ms: Timeout in ms
> >> + *
> >> + * Tries waiting for the sink to finish updating its payload table by
> >> polling
> >> + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting
> >> to
> >> + * 3000 ms if 0.
> >> + *
> >> + * Returns:
> >> + * 0 if the ACT was handled in time, negative error code on failure.
> >> + */
> >> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
> >
> > I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.
>
> Honestly I think there's already enough of an acronym jumble in the
> name. At least "drm_dp_dpcd" is common for most functions around here.
>
>
> BR,
> Jani.
>
> >
> >> +{
> >> + int ret, status;
> >> +
> >
> > Extra w/s.
> >
> > Regardless of the namespace comment:
> >
> > Reviewed-by: Imre Deak
> >
> >> + /* default to 3 seconds, this is arbitrary */
> >> + timeout_ms = timeout_ms ?: 3000;
> >> +
> >> + ret = readx_poll_timeout(read_payload_update_status, aux, status,
> >> + status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> >> + 200, timeout_ms * USEC_PER_MSEC);
> >> + if (ret < 0 && status >= 0) {
> >> + drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last
> >> status: %02x\n",
> >> + timeout_ms, status);
> >> + return -EINVAL;
> >> + } else if (status < 0) {
> >> + /*
> >> + * Failure here isn't unexpected - the hub may have
> >> + * just been unplugged
> >> + */
> >> + drm_dbg_kms(aux->drm_dev, "Failed to read payload table status:
> >> %d\n", status);
> >> + return status;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
> >> +
> >> static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
> >> {
> >>/* FIXME: get rid of drm_edid_raw() */
> >> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> index ac90118b9e7a..2bdbc1eb282b 100644
> >> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> >> @@ -29,7 +29,6 @@
> >> #include
> >> #include
> >> #include
> >> -#include
> >>
> >> #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> >> #include
> >> @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct
> >> drm_dp_mst_to
Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()
On Wed, 20 Nov 2024, Imre Deak wrote:
> On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
>> SST with 128b/132b channel coding needs this too. Extract to a separate
>> helper, independent of MST.
>>
>> Pass timeout in as a parameter, anticipating that we can reduce the
>> timeout for SST.
>
> I wish there was a DP Standard section making the above clear,
> but I suppose we just deduct that except of the side-band messaging,
> every other payload programming and ACT signaling is required for
> 128b/132b SST.
Ping. Okay to merge, or do we want to mull over the the timeout?
I'm primarily trying to do a non-functional change. I could omit the
timeout parameter, but for non-hubs three seconds seems excessive, and
reducing it for hubs too is a can of worms I prefer keeping the lid on
top.
> Cc: Lyude Paul
>
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/display/drm_dp_helper.c | 54 ++-
>> drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +
>> include/drm/display/drm_dp_helper.h | 2 +
>> 3 files changed, 57 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
>> b/drivers/gpu/drm/display/drm_dp_helper.c
>> index 6ee51003de3c..b7e03bf02cd8 100644
>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
>> @@ -22,15 +22,16 @@
>>
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> #include
>> #include
>> -#include
>>
>> #include
>> #include
>> @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux
>> *aux,
>> }
>> EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
>>
>> +static int read_payload_update_status(struct drm_dp_aux *aux)
>> +{
>> +int ret;
>> +u8 status;
>> +
>> +ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
>> +if (ret < 0)
>> +return ret;
>> +
>> +return status;
>> +}
>> +
>> +/**
>> + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
>> + * @aux: DisplayPort AUX channel
>> + * @timeout_ms: Timeout in ms
>> + *
>> + * Tries waiting for the sink to finish updating its payload table by
>> polling
>> + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
>> + * 3000 ms if 0.
>> + *
>> + * Returns:
>> + * 0 if the ACT was handled in time, negative error code on failure.
>> + */
>> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
>
> I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.
Honestly I think there's already enough of an acronym jumble in the
name. At least "drm_dp_dpcd" is common for most functions around here.
BR,
Jani.
>
>> +{
>> +int ret, status;
>> +
>
> Extra w/s.
>
> Regardless of the namespace comment:
>
> Reviewed-by: Imre Deak
>
>> +/* default to 3 seconds, this is arbitrary */
>> +timeout_ms = timeout_ms ?: 3000;
>> +
>> +ret = readx_poll_timeout(read_payload_update_status, aux, status,
>> + status & DP_PAYLOAD_ACT_HANDLED || status < 0,
>> + 200, timeout_ms * USEC_PER_MSEC);
>> +if (ret < 0 && status >= 0) {
>> +drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last
>> status: %02x\n",
>> +timeout_ms, status);
>> +return -EINVAL;
>> +} else if (status < 0) {
>> +/*
>> + * Failure here isn't unexpected - the hub may have
>> + * just been unplugged
>> + */
>> +drm_dbg_kms(aux->drm_dev, "Failed to read payload table status:
>> %d\n", status);
>> +return status;
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
>> +
>> static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
>> {
>> /* FIXME: get rid of drm_edid_raw() */
>> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> index ac90118b9e7a..2bdbc1eb282b 100644
>> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> @@ -29,7 +29,6 @@
>> #include
>> #include
>> #include
>> -#include
>>
>> #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
>> #include
>> @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct
>> drm_dp_mst_topology_mgr *mgr,
>> return ret;
>> }
>>
>> -static int do_get_act_status(struct drm_dp_aux *aux)
>> -{
>> -int ret;
>> -u8 status;
>> -
>> -ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
>> -if (ret < 0)
>> -return ret;
>> -
>> -return status;
>> -}
>> -
>> /**
>> * drm_dp_check_act_status() - Polls for ACT handled status.
>> * @mgr: manager to use
>> @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct
>> drm_dp_mst_topo
Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()
On Wed, Nov 20, 2024 at 04:59:43PM +0200, Imre Deak wrote:
> On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
> > SST with 128b/132b channel coding needs this too. Extract to a separate
> > helper, independent of MST.
> >
> > Pass timeout in as a parameter, anticipating that we can reduce the
> > timeout for SST.
>
> I wish there was a DP Standard section making the above clear,
> but I suppose we just deduct that except of the side-band messaging,
> every other payload programming and ACT signaling is required for
> 128b/132b SST.
>
> >
>
> Cc: Lyude Paul
>
> > Signed-off-by: Jani Nikula
> > ---
> > drivers/gpu/drm/display/drm_dp_helper.c | 54 ++-
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +
> > include/drm/display/drm_dp_helper.h | 2 +
> > 3 files changed, 57 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> > b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 6ee51003de3c..b7e03bf02cd8 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -22,15 +22,16 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > #include
> > #include
> > -#include
> >
> > #include
> > #include
> > @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux
> > *aux,
> > }
> > EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
> >
> > +static int read_payload_update_status(struct drm_dp_aux *aux)
> > +{
> > + int ret;
> > + u8 status;
> > +
> > + ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return status;
> > +}
> > +
> > +/**
> > + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
> > + * @aux: DisplayPort AUX channel
> > + * @timeout_ms: Timeout in ms
> > + *
> > + * Tries waiting for the sink to finish updating its payload table by
> > polling
> > + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting
> > to
> > + * 3000 ms if 0.
> > + *
> > + * Returns:
> > + * 0 if the ACT was handled in time, negative error code on failure.
> > + */
> > +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
>
> I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.
>
> > +{
> > + int ret, status;
> > +
>
> Extra w/s.
Ah nvm, timeout_ms is a function param so new is needed.
>
> Regardless of the namespace comment:
>
> Reviewed-by: Imre Deak
>
> > + /* default to 3 seconds, this is arbitrary */
> > + timeout_ms = timeout_ms ?: 3000;
> > +
> > + ret = readx_poll_timeout(read_payload_update_status, aux, status,
> > +status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> > +200, timeout_ms * USEC_PER_MSEC);
> > + if (ret < 0 && status >= 0) {
> > + drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last
> > status: %02x\n",
> > + timeout_ms, status);
> > + return -EINVAL;
> > + } else if (status < 0) {
> > + /*
> > +* Failure here isn't unexpected - the hub may have
> > +* just been unplugged
> > +*/
> > + drm_dbg_kms(aux->drm_dev, "Failed to read payload table status:
> > %d\n", status);
> > + return status;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
> > +
> > static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
> > {
> > /* FIXME: get rid of drm_edid_raw() */
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index ac90118b9e7a..2bdbc1eb282b 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -29,7 +29,6 @@
> > #include
> > #include
> > #include
> > -#include
> >
> > #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> > #include
> > @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct
> > drm_dp_mst_topology_mgr *mgr,
> > return ret;
> > }
> >
> > -static int do_get_act_status(struct drm_dp_aux *aux)
> > -{
> > - int ret;
> > - u8 status;
> > -
> > - ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> > - if (ret < 0)
> > - return ret;
> > -
> > - return status;
> > -}
> > -
> > /**
> > * drm_dp_check_act_status() - Polls for ACT handled status.
> > * @mgr: manager to use
> > @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct
> > drm_dp_mst_topology_mgr *mgr)
> > * There doesn't seem to be any recommended retry count or timeout in
> > * the MST specification. Since some hubs have been observed to take
> > * over 1 second to update their payload allocations unde
Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()
On Mon, Nov 18, 2024 at 05:14:52PM +0200, Jani Nikula wrote:
> SST with 128b/132b channel coding needs this too. Extract to a separate
> helper, independent of MST.
>
> Pass timeout in as a parameter, anticipating that we can reduce the
> timeout for SST.
I wish there was a DP Standard section making the above clear,
but I suppose we just deduct that except of the side-band messaging,
every other payload programming and ACT signaling is required for
128b/132b SST.
>
Cc: Lyude Paul
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/display/drm_dp_helper.c | 54 ++-
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 36 +
> include/drm/display/drm_dp_helper.h | 2 +
> 3 files changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index 6ee51003de3c..b7e03bf02cd8 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -22,15 +22,16 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> -#include
>
> #include
> #include
> @@ -779,6 +780,57 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux
> *aux,
> }
> EXPORT_SYMBOL(drm_dp_dpcd_read_phy_link_status);
>
> +static int read_payload_update_status(struct drm_dp_aux *aux)
> +{
> + int ret;
> + u8 status;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> + if (ret < 0)
> + return ret;
> +
> + return status;
> +}
> +
> +/**
> + * drm_dp_dpcd_poll_act_handled() - Polls for ACT handled status.
> + * @aux: DisplayPort AUX channel
> + * @timeout_ms: Timeout in ms
> + *
> + * Tries waiting for the sink to finish updating its payload table by polling
> + * for the ACT handled bit for up to @timeout_ms milliseconds, defaulting to
> + * 3000 ms if 0.
> + *
> + * Returns:
> + * 0 if the ACT was handled in time, negative error code on failure.
> + */
> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms)
I wonder if it'd make sense to namespace these helpers using ll_mtp or mtp.
> +{
> + int ret, status;
> +
Extra w/s.
Regardless of the namespace comment:
Reviewed-by: Imre Deak
> + /* default to 3 seconds, this is arbitrary */
> + timeout_ms = timeout_ms ?: 3000;
> +
> + ret = readx_poll_timeout(read_payload_update_status, aux, status,
> + status & DP_PAYLOAD_ACT_HANDLED || status < 0,
> + 200, timeout_ms * USEC_PER_MSEC);
> + if (ret < 0 && status >= 0) {
> + drm_err(aux->drm_dev, "Failed to get ACT after %d ms, last
> status: %02x\n",
> + timeout_ms, status);
> + return -EINVAL;
> + } else if (status < 0) {
> + /*
> + * Failure here isn't unexpected - the hub may have
> + * just been unplugged
> + */
> + drm_dbg_kms(aux->drm_dev, "Failed to read payload table status:
> %d\n", status);
> + return status;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_poll_act_handled);
> +
> static bool is_edid_digital_input_dp(const struct drm_edid *drm_edid)
> {
> /* FIXME: get rid of drm_edid_raw() */
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index ac90118b9e7a..2bdbc1eb282b 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -29,7 +29,6 @@
> #include
> #include
> #include
> -#include
>
> #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> #include
> @@ -4723,18 +4722,6 @@ static int drm_dp_dpcd_write_payload(struct
> drm_dp_mst_topology_mgr *mgr,
> return ret;
> }
>
> -static int do_get_act_status(struct drm_dp_aux *aux)
> -{
> - int ret;
> - u8 status;
> -
> - ret = drm_dp_dpcd_readb(aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, &status);
> - if (ret < 0)
> - return ret;
> -
> - return status;
> -}
> -
> /**
> * drm_dp_check_act_status() - Polls for ACT handled status.
> * @mgr: manager to use
> @@ -4752,28 +4739,9 @@ int drm_dp_check_act_status(struct
> drm_dp_mst_topology_mgr *mgr)
>* There doesn't seem to be any recommended retry count or timeout in
>* the MST specification. Since some hubs have been observed to take
>* over 1 second to update their payload allocations under certain
> - * conditions, we use a rather large timeout value.
> + * conditions, we use a rather large timeout value of 3 seconds.
>*/
> - const int timeout_ms = 3000;
> - int ret, status;
> -
> - ret = readx_poll_timeout(do_get_act_status, mgr->aux, status,
> - status & DP_PAYLOAD_ACT_HANDLED || status
