| Message ID | 3d91f7b6639960fe688eb6ae0236254adae3e82d.1731942780.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 > >
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 >>
On Fri, Nov 22, 2024 at 02:22:44PM +0200, Jani Nikula wrote: > 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. 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@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
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(-)