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 <ly...@redhat.com>

> Signed-off-by: Jani Nikula <jani.nik...@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.

> +{
> +     int ret, status;
> +

Extra w/s.

Regardless of the namespace comment:

Reviewed-by: Imre Deak <imre.d...@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
> 

Reply via email to