Re: [PATCH 1/3] drm/dp: extract drm_dp_dpcd_poll_act_handled()

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 20 Nov 2024, Imre Deak <imre.deak@intel.com> 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 <lyude@redhat.com>
>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  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 <linux/backlight.h>
>>  #include <linux/delay.h>
>> +#include <linux/dynamic_debug.h>
>>  #include <linux/errno.h>
>>  #include <linux/i2c.h>
>>  #include <linux/init.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/sched.h>
>>  #include <linux/seq_file.h>
>>  #include <linux/string_helpers.h>
>> -#include <linux/dynamic_debug.h>
>>  
>>  #include <drm/display/drm_dp_helper.h>
>>  #include <drm/display/drm_dp_mst_helper.h>
>> @@ -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 <imre.deak@intel.com>
>
>> +	/* 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 <linux/random.h>
>>  #include <linux/sched.h>
>>  #include <linux/seq_file.h>
>> -#include <linux/iopoll.h>
>>  
>>  #if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
>>  #include <linux/stacktrace.h>
>> @@ -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 < 0,
>> -				 200, timeout_ms * USEC_PER_MSEC);
>> -	if (ret < 0 && status >= 0) {
>> -		drm_err(mgr->dev, "Failed to get ACT after %dms, 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(mgr->dev, "Failed to read payload table status: %d\n", status);
>> -		return status;
>> -	}
>> -
>> -	return 0;
>> +	return drm_dp_dpcd_poll_act_handled(mgr->aux, 3000);
>>  }
>>  EXPORT_SYMBOL(drm_dp_check_act_status);
>>  
>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
>> index 279624833ea9..38eea21d1082 100644
>> --- a/include/drm/display/drm_dp_helper.h
>> +++ b/include/drm/display/drm_dp_helper.h
>> @@ -567,6 +567,8 @@ int drm_dp_dpcd_read_phy_link_status(struct drm_dp_aux *aux,
>>  				     enum drm_dp_phy dp_phy,
>>  				     u8 link_status[DP_LINK_STATUS_SIZE]);
>>  
>> +int drm_dp_dpcd_poll_act_handled(struct drm_dp_aux *aux, int timeout_ms);
>> +
>>  bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
>>  				    u8 real_edid_checksum);
>>  
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel