| Message ID | 3d91f7b6639960fe688eb6ae0236254adae3e82d.1731942780.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/dp: extract payload helpers | expand |
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 <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. > +{ > + 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 >
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 <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. > > > +{ > > + 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 <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 > >
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) +{ + int ret, status; + + /* 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);
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. 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(-)