On Fri, Nov 22, 2024 at 02:22:44PM +0200, Jani Nikula wrote:
> On Wed, 20 Nov 2024, Imre Deak <imre.deak@xxxxxxxxx> 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 <lyude@xxxxxxxxxx>
> >
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx>
> >
> >> + /* 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
