* [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it [not found] <20200616141855.746-1-imre.deak@intel.com> @ 2020-06-16 14:18 ` Imre Deak 2020-06-16 15:45 ` Ville Syrjälä 2020-06-16 21:11 ` [PATCH v2 " Imre Deak 0 siblings, 2 replies; 9+ messages in thread From: Imre Deak @ 2020-06-16 14:18 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel Atm, we clear the ACT sent flag in the sink's DPCD before updating the sink's payload table, along clearing the payload table updated flag. The sink is supposed to set this flag once it detects that the source has completed the ACT sequence (after detecting the 4 required ACT MTPH symbols sent by the source). As opposed to this 2 DELL monitors I have set the flag already along the payload table updated flag, which is not quite correct. To be sure that the sink has detected the ACT MTPH symbols before continuing enabling the encoder, clear the ACT sent flag before enabling or disabling the transcoder VC payload allocation (which is what starts the ACT sequence). Cc: Lyude Paul <lyude@redhat.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 31 +++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ include/drm/drm_dp_mst_helper.h | 2 ++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index b2f5a84b4cfb..e3bf8c9c8267 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4377,6 +4377,34 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, } EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); +/** + * drm_dp_clear_payload_status() - Clears the payload table status flags + * @mgr: manager to use + * + * Clears the payload table ACT handled and table updated flags in the MST hub's + * DPCD. This function must be called before updating the payload table or + * starting the ACT sequence and waiting for the corresponding flags to get + * set by the hub. + * + * Returns: + * 0 if the flag got cleared successfully, otherwise a negative error code. + */ +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) +{ + int ret; + + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, + DP_PAYLOAD_ACT_HANDLED); + if (ret < 0) { + DRM_DEBUG_DRIVER("Can't clear the ACT sent flag (%d)\n", ret); + return ret; + } + WARN_ON(ret != 1); + + return 0; +} +EXPORT_SYMBOL(drm_dp_clear_payload_status); + static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, int id, struct drm_dp_payload *payload) { @@ -4384,8 +4412,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, int ret; int retries = 0; - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, - DP_PAYLOAD_TABLE_UPDATED); + drm_dp_clear_payload_status(mgr); payload_alloc[0] = id; payload_alloc[1] = payload->start_slot; diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 9308b5920780..3c4b0fb10d8b 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) intel_de_write(i915, intel_dp->regs.dp_tp_status, DP_TP_STATUS_ACT_SENT); + + drm_dp_clear_payload_status(&intel_dp->mst_mgr); } static void wait_for_act_sent(struct intel_dp *intel_dp) diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 8b9eb4db3381..2facb87624bf 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, int pbn); +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); + int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); -- 2.23.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 14:18 ` [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it Imre Deak @ 2020-06-16 15:45 ` Ville Syrjälä 2020-06-16 15:54 ` Imre Deak 2020-06-16 21:11 ` [PATCH v2 " Imre Deak 1 sibling, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2020-06-16 15:45 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, dri-devel On Tue, Jun 16, 2020 at 05:18:55PM +0300, Imre Deak wrote: > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > sink's payload table, along clearing the payload table updated flag. > The sink is supposed to set this flag once it detects that the source > has completed the ACT sequence (after detecting the 4 required ACT MTPH > symbols sent by the source). As opposed to this 2 DELL monitors I have > set the flag already along the payload table updated flag, which is not > quite correct. > > To be sure that the sink has detected the ACT MTPH symbols before > continuing enabling the encoder, clear the ACT sent flag before enabling > or disabling the transcoder VC payload allocation (which is what starts > the ACT sequence). > > Cc: Lyude Paul <lyude@redhat.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 31 +++++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > include/drm/drm_dp_mst_helper.h | 2 ++ > 3 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index b2f5a84b4cfb..e3bf8c9c8267 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4377,6 +4377,34 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > } > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > +/** > + * drm_dp_clear_payload_status() - Clears the payload table status flags > + * @mgr: manager to use > + * > + * Clears the payload table ACT handled and table updated flags in the MST hub's > + * DPCD. This function must be called before updating the payload table or > + * starting the ACT sequence and waiting for the corresponding flags to get > + * set by the hub. > + * > + * Returns: > + * 0 if the flag got cleared successfully, otherwise a negative error code. > + */ > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > +{ > + int ret; > + > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > + DP_PAYLOAD_ACT_HANDLED); > + if (ret < 0) { > + DRM_DEBUG_DRIVER("Can't clear the ACT sent flag (%d)\n", ret); > + return ret; > + } > + WARN_ON(ret != 1); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > + > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > int id, struct drm_dp_payload *payload) > { > @@ -4384,8 +4412,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > int ret; > int retries = 0; > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > - DP_PAYLOAD_TABLE_UPDATED); We used to clear DP_PAYLOAD_TABLE_UPDATED but now we clear DP_PAYLOAD_ACT_HANDLED ? > + drm_dp_clear_payload_status(mgr); > > payload_alloc[0] = id; > payload_alloc[1] = payload->start_slot; > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 9308b5920780..3c4b0fb10d8b 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > DP_TP_STATUS_ACT_SENT); > + > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > } > > static void wait_for_act_sent(struct intel_dp *intel_dp) > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 8b9eb4db3381..2facb87624bf 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > int pbn); > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > + > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > > -- > 2.23.1 -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 15:45 ` Ville Syrjälä @ 2020-06-16 15:54 ` Imre Deak 2020-06-16 16:23 ` Ville Syrjälä 0 siblings, 1 reply; 9+ messages in thread From: Imre Deak @ 2020-06-16 15:54 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, dri-devel On Tue, Jun 16, 2020 at 06:45:46PM +0300, Ville Syrjälä wrote: > On Tue, Jun 16, 2020 at 05:18:55PM +0300, Imre Deak wrote: > > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > > sink's payload table, along clearing the payload table updated flag. > > The sink is supposed to set this flag once it detects that the source > > has completed the ACT sequence (after detecting the 4 required ACT MTPH > > symbols sent by the source). As opposed to this 2 DELL monitors I have > > set the flag already along the payload table updated flag, which is not > > quite correct. > > > > To be sure that the sink has detected the ACT MTPH symbols before > > continuing enabling the encoder, clear the ACT sent flag before enabling > > or disabling the transcoder VC payload allocation (which is what starts > > the ACT sequence). > > > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 31 +++++++++++++++++++-- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > > include/drm/drm_dp_mst_helper.h | 2 ++ > > 3 files changed, 33 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > index b2f5a84b4cfb..e3bf8c9c8267 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -4377,6 +4377,34 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > > } > > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > > > +/** > > + * drm_dp_clear_payload_status() - Clears the payload table status flags > > + * @mgr: manager to use > > + * > > + * Clears the payload table ACT handled and table updated flags in the MST hub's > > + * DPCD. This function must be called before updating the payload table or > > + * starting the ACT sequence and waiting for the corresponding flags to get > > + * set by the hub. > > + * > > + * Returns: > > + * 0 if the flag got cleared successfully, otherwise a negative error code. > > + */ > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > > +{ > > + int ret; > > + > > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > + DP_PAYLOAD_ACT_HANDLED); > > + if (ret < 0) { > > + DRM_DEBUG_DRIVER("Can't clear the ACT sent flag (%d)\n", ret); > > + return ret; > > + } > > + WARN_ON(ret != 1); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > > + > > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > int id, struct drm_dp_payload *payload) > > { > > @@ -4384,8 +4412,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > int ret; > > int retries = 0; > > > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > - DP_PAYLOAD_TABLE_UPDATED); > > We used to clear DP_PAYLOAD_TABLE_UPDATED but now we clear > DP_PAYLOAD_ACT_HANDLED ? Eek. We should write DP_PAYLOAD_TABLE_UPDATED which is the only way to clear both the act-handled and the table-updated flags. I tested things that way but managed to send an old version. Thanks for catching it. > > > + drm_dp_clear_payload_status(mgr); > > > > payload_alloc[0] = id; > > payload_alloc[1] = payload->start_slot; > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index 9308b5920780..3c4b0fb10d8b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > > DP_TP_STATUS_ACT_SENT); > > + > > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > > } > > > > static void wait_for_act_sent(struct intel_dp *intel_dp) > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > > index 8b9eb4db3381..2facb87624bf 100644 > > --- a/include/drm/drm_dp_mst_helper.h > > +++ b/include/drm/drm_dp_mst_helper.h > > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > > int pbn); > > > > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > > + > > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > > > > > -- > > 2.23.1 > > -- > Ville Syrjälä > Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 15:54 ` Imre Deak @ 2020-06-16 16:23 ` Ville Syrjälä 2020-06-16 16:40 ` Ville Syrjälä 0 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2020-06-16 16:23 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, dri-devel On Tue, Jun 16, 2020 at 06:54:41PM +0300, Imre Deak wrote: > On Tue, Jun 16, 2020 at 06:45:46PM +0300, Ville Syrjälä wrote: > > On Tue, Jun 16, 2020 at 05:18:55PM +0300, Imre Deak wrote: > > > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > > > sink's payload table, along clearing the payload table updated flag. > > > The sink is supposed to set this flag once it detects that the source > > > has completed the ACT sequence (after detecting the 4 required ACT MTPH > > > symbols sent by the source). As opposed to this 2 DELL monitors I have > > > set the flag already along the payload table updated flag, which is not > > > quite correct. > > > > > > To be sure that the sink has detected the ACT MTPH symbols before > > > continuing enabling the encoder, clear the ACT sent flag before enabling > > > or disabling the transcoder VC payload allocation (which is what starts > > > the ACT sequence). > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/drm_dp_mst_topology.c | 31 +++++++++++++++++++-- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > > > include/drm/drm_dp_mst_helper.h | 2 ++ > > > 3 files changed, 33 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > > index b2f5a84b4cfb..e3bf8c9c8267 100644 > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > > @@ -4377,6 +4377,34 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > > > } > > > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > > > > > +/** > > > + * drm_dp_clear_payload_status() - Clears the payload table status flags > > > + * @mgr: manager to use > > > + * > > > + * Clears the payload table ACT handled and table updated flags in the MST hub's > > > + * DPCD. This function must be called before updating the payload table or > > > + * starting the ACT sequence and waiting for the corresponding flags to get > > > + * set by the hub. > > > + * > > > + * Returns: > > > + * 0 if the flag got cleared successfully, otherwise a negative error code. > > > + */ > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > > > +{ > > > + int ret; > > > + > > > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > > + DP_PAYLOAD_ACT_HANDLED); > > > + if (ret < 0) { > > > + DRM_DEBUG_DRIVER("Can't clear the ACT sent flag (%d)\n", ret); > > > + return ret; > > > + } > > > + WARN_ON(ret != 1); > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > > > + > > > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > > int id, struct drm_dp_payload *payload) > > > { > > > @@ -4384,8 +4412,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > > int ret; > > > int retries = 0; > > > > > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > > - DP_PAYLOAD_TABLE_UPDATED); > > > > We used to clear DP_PAYLOAD_TABLE_UPDATED but now we clear > > DP_PAYLOAD_ACT_HANDLED ? > > Eek. We should write DP_PAYLOAD_TABLE_UPDATED which is the only way to > clear both the act-handled and the table-updated flags. Huh. That's a bit crazy. But it is what the spec says. > I tested things > that way but managed to send an old version. Thanks for catching it. > > > > > > + drm_dp_clear_payload_status(mgr); > > > > > > payload_alloc[0] = id; > > > payload_alloc[1] = payload->start_slot; > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index 9308b5920780..3c4b0fb10d8b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > > > > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > > > DP_TP_STATUS_ACT_SENT); > > > + > > > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > > > } > > > > > > static void wait_for_act_sent(struct intel_dp *intel_dp) > > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > > > index 8b9eb4db3381..2facb87624bf 100644 > > > --- a/include/drm/drm_dp_mst_helper.h > > > +++ b/include/drm/drm_dp_mst_helper.h > > > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > > > int pbn); > > > > > > > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > > > + > > > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > > > > > > > > -- > > > 2.23.1 > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 16:23 ` Ville Syrjälä @ 2020-06-16 16:40 ` Ville Syrjälä 2020-06-16 16:47 ` Imre Deak 0 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2020-06-16 16:40 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx, dri-devel On Tue, Jun 16, 2020 at 07:23:21PM +0300, Ville Syrjälä wrote: > On Tue, Jun 16, 2020 at 06:54:41PM +0300, Imre Deak wrote: > > On Tue, Jun 16, 2020 at 06:45:46PM +0300, Ville Syrjälä wrote: > > > On Tue, Jun 16, 2020 at 05:18:55PM +0300, Imre Deak wrote: > > > > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > > > > sink's payload table, along clearing the payload table updated flag. > > > > The sink is supposed to set this flag once it detects that the source > > > > has completed the ACT sequence (after detecting the 4 required ACT MTPH > > > > symbols sent by the source). As opposed to this 2 DELL monitors I have > > > > set the flag already along the payload table updated flag, which is not > > > > quite correct. > > > > > > > > To be sure that the sink has detected the ACT MTPH symbols before > > > > continuing enabling the encoder, clear the ACT sent flag before enabling > > > > or disabling the transcoder VC payload allocation (which is what starts > > > > the ACT sequence). > > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_dp_mst_topology.c | 31 +++++++++++++++++++-- > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > > > > include/drm/drm_dp_mst_helper.h | 2 ++ > > > > 3 files changed, 33 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > index b2f5a84b4cfb..e3bf8c9c8267 100644 > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > @@ -4377,6 +4377,34 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > > > > } > > > > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > > > > > > > +/** > > > > + * drm_dp_clear_payload_status() - Clears the payload table status flags > > > > + * @mgr: manager to use > > > > + * > > > > + * Clears the payload table ACT handled and table updated flags in the MST hub's > > > > + * DPCD. This function must be called before updating the payload table or > > > > + * starting the ACT sequence and waiting for the corresponding flags to get > > > > + * set by the hub. > > > > + * > > > > + * Returns: > > > > + * 0 if the flag got cleared successfully, otherwise a negative error code. > > > > + */ > > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > > > + DP_PAYLOAD_ACT_HANDLED); > > > > + if (ret < 0) { > > > > + DRM_DEBUG_DRIVER("Can't clear the ACT sent flag (%d)\n", ret); > > > > + return ret; > > > > + } > > > > + WARN_ON(ret != 1); > > > > + > > > > + return 0; > > > > +} > > > > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > > > > + > > > > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > > > int id, struct drm_dp_payload *payload) > > > > { > > > > @@ -4384,8 +4412,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > > > int ret; > > > > int retries = 0; > > > > > > > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > > > - DP_PAYLOAD_TABLE_UPDATED); > > > > > > We used to clear DP_PAYLOAD_TABLE_UPDATED but now we clear > > > DP_PAYLOAD_ACT_HANDLED ? > > > > Eek. We should write DP_PAYLOAD_TABLE_UPDATED which is the only way to > > clear both the act-handled and the table-updated flags. > > Huh. That's a bit crazy. But it is what the spec says. In fact, I'd suggest adding a comment explaining this crazyness so that the next person doesn't have to wonder why we're never clearing the ACT bit. > > > I tested things > > that way but managed to send an old version. Thanks for catching it. > > > > > > > > > + drm_dp_clear_payload_status(mgr); > > > > > > > > payload_alloc[0] = id; > > > > payload_alloc[1] = payload->start_slot; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > index 9308b5920780..3c4b0fb10d8b 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > > > > > > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > > > > DP_TP_STATUS_ACT_SENT); > > > > + > > > > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > > > > } > > > > > > > > static void wait_for_act_sent(struct intel_dp *intel_dp) > > > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > > > > index 8b9eb4db3381..2facb87624bf 100644 > > > > --- a/include/drm/drm_dp_mst_helper.h > > > > +++ b/include/drm/drm_dp_mst_helper.h > > > > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > > > > int pbn); > > > > > > > > > > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > > > > + > > > > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > > > > > > > > > > > -- > > > > 2.23.1 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 16:40 ` Ville Syrjälä @ 2020-06-16 16:47 ` Imre Deak 0 siblings, 0 replies; 9+ messages in thread From: Imre Deak @ 2020-06-16 16:47 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, dri-devel On Tue, Jun 16, 2020 at 07:40:47PM +0300, Ville Syrjälä wrote: > On Tue, Jun 16, 2020 at 07:23:21PM +0300, Ville Syrjälä wrote: > > On Tue, Jun 16, 2020 at 06:54:41PM +0300, Imre Deak wrote: > > > On Tue, Jun 16, 2020 at 06:45:46PM +0300, Ville Syrjälä wrote: > > > > On Tue, Jun 16, 2020 at 05:18:55PM +0300, Imre Deak wrote: > > > > > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > > > > > sink's payload table, along clearing the payload table updated flag. > > > > > The sink is supposed to set this flag once it detects that the source > > > > > has completed the ACT sequence (after detecting the 4 required ACT MTPH > > > > > symbols sent by the source). As opposed to this 2 DELL monitors I have > > > > > set the flag already along the payload table updated flag, which is not > > > > > quite correct. > > > > > > > > > > To be sure that the sink has detected the ACT MTPH symbols before > > > > > continuing enabling the encoder, clear the ACT sent flag before enabling > > > > > or disabling the transcoder VC payload allocation (which is what starts > > > > > the ACT sequence). > > > > > > > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/drm_dp_mst_topology.c | 31 +++++++++++++++++++-- > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > > > > > include/drm/drm_dp_mst_helper.h | 2 ++ > > > > > 3 files changed, 33 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > index b2f5a84b4cfb..e3bf8c9c8267 100644 > > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > > > > @@ -4377,6 +4377,34 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > > > > > } > > > > > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > > > > > > > > > +/** > > > > > + * drm_dp_clear_payload_status() - Clears the payload table status flags > > > > > + * @mgr: manager to use > > > > > + * > > > > > + * Clears the payload table ACT handled and table updated flags in the MST hub's > > > > > + * DPCD. This function must be called before updating the payload table or > > > > > + * starting the ACT sequence and waiting for the corresponding flags to get > > > > > + * set by the hub. > > > > > + * > > > > > + * Returns: > > > > > + * 0 if the flag got cleared successfully, otherwise a negative error code. > > > > > + */ > > > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > > > > + DP_PAYLOAD_ACT_HANDLED); > > > > > + if (ret < 0) { > > > > > + DRM_DEBUG_DRIVER("Can't clear the ACT sent flag (%d)\n", ret); > > > > > + return ret; > > > > > + } > > > > > + WARN_ON(ret != 1); > > > > > + > > > > > + return 0; > > > > > +} > > > > > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > > > > > + > > > > > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > > > > int id, struct drm_dp_payload *payload) > > > > > { > > > > > @@ -4384,8 +4412,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > > > > > int ret; > > > > > int retries = 0; > > > > > > > > > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > > > > > - DP_PAYLOAD_TABLE_UPDATED); > > > > > > > > We used to clear DP_PAYLOAD_TABLE_UPDATED but now we clear > > > > DP_PAYLOAD_ACT_HANDLED ? > > > > > > Eek. We should write DP_PAYLOAD_TABLE_UPDATED which is the only way to > > > clear both the act-handled and the table-updated flags. > > > > Huh. That's a bit crazy. But it is what the spec says. > > In fact, I'd suggest adding a comment explaining this crazyness > so that the next person doesn't have to wonder why we're never > clearing the ACT bit. Ok. > > > > > > I tested things > > > that way but managed to send an old version. Thanks for catching it. > > > > > > > > > > > > + drm_dp_clear_payload_status(mgr); > > > > > > > > > > payload_alloc[0] = id; > > > > > payload_alloc[1] = payload->start_slot; > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > index 9308b5920780..3c4b0fb10d8b 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > > > > > > > > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > > > > > DP_TP_STATUS_ACT_SENT); > > > > > + > > > > > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > > > > > } > > > > > > > > > > static void wait_for_act_sent(struct intel_dp *intel_dp) > > > > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > > > > > index 8b9eb4db3381..2facb87624bf 100644 > > > > > --- a/include/drm/drm_dp_mst_helper.h > > > > > +++ b/include/drm/drm_dp_mst_helper.h > > > > > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > > > > > int pbn); > > > > > > > > > > > > > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > > > > > + > > > > > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > > > > > > > > > > > > > > -- > > > > > 2.23.1 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 14:18 ` [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it Imre Deak 2020-06-16 15:45 ` Ville Syrjälä @ 2020-06-16 21:11 ` Imre Deak 2020-06-17 15:27 ` Lyude Paul 2020-06-23 7:30 ` Imre Deak 1 sibling, 2 replies; 9+ messages in thread From: Imre Deak @ 2020-06-16 21:11 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel Atm, we clear the ACT sent flag in the sink's DPCD before updating the sink's payload table, along clearing the payload table updated flag. The sink is supposed to set this flag once it detects that the source has completed the ACT sequence (after detecting the 4 required ACT MTPH symbols sent by the source). As opposed to this 2 DELL monitors I have set the flag already along the payload table updated flag, which is not quite correct. To be sure that the sink has detected the ACT MTPH symbols before continuing enabling the encoder, clear the ACT sent flag before enabling or disabling the transcoder VC payload allocation (which is what starts the ACT sequence). v2 (Ville): - Use the correct bit to clear the flags. - Add code comment explaining the clearing semantics of the ACT handled flag. Cc: Lyude Paul <lyude@redhat.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 38 +++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ include/drm/drm_dp_mst_helper.h | 2 ++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index b2f5a84b4cfb..1f5d14128c1a 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4377,6 +4377,41 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, } EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); +/** + * drm_dp_clear_payload_status() - Clears the payload table status flags + * @mgr: manager to use + * + * Clears the payload table ACT handled and table updated flags in the MST hub's + * DPCD. This function must be called before updating the payload table or + * starting the ACT sequence and waiting for the corresponding flags to get + * set by the hub. + * + * Returns: + * 0 if the flags got cleared successfully, otherwise a negative error code. + */ +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) +{ + int ret; + + /* + * Note that the following is based on the DP Standard stating that + * writing the DP_PAYLOAD_TABLE_UPDATED bit alone will clear both the + * DP_PAYLOAD_TABLE_UPDATED and the DP_PAYLOAD_ACT_HANDLED flags. This + * seems to be also the only way to clear DP_PAYLOAD_ACT_HANDLED. + */ + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, + DP_PAYLOAD_TABLE_UPDATED); + if (ret < 0) { + DRM_DEBUG_DRIVER("Can't clear the ACT handled/table updated flags (%d)\n", + ret); + return ret; + } + WARN_ON(ret != 1); + + return 0; +} +EXPORT_SYMBOL(drm_dp_clear_payload_status); + static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, int id, struct drm_dp_payload *payload) { @@ -4384,8 +4419,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, int ret; int retries = 0; - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, - DP_PAYLOAD_TABLE_UPDATED); + drm_dp_clear_payload_status(mgr); payload_alloc[0] = id; payload_alloc[1] = payload->start_slot; diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 9308b5920780..3c4b0fb10d8b 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) intel_de_write(i915, intel_dp->regs.dp_tp_status, DP_TP_STATUS_ACT_SENT); + + drm_dp_clear_payload_status(&intel_dp->mst_mgr); } static void wait_for_act_sent(struct intel_dp *intel_dp) diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 8b9eb4db3381..2facb87624bf 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, int pbn); +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); + int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); -- 2.23.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 21:11 ` [PATCH v2 " Imre Deak @ 2020-06-17 15:27 ` Lyude Paul 2020-06-23 7:30 ` Imre Deak 1 sibling, 0 replies; 9+ messages in thread From: Lyude Paul @ 2020-06-17 15:27 UTC (permalink / raw) To: Imre Deak, intel-gfx; +Cc: dri-devel Reviewed-by: Lyude Paul <lyude@redhat.com> Thanks for all the subtle fixes for broken MST displays, these are always my favorite to find :) On Wed, 2020-06-17 at 00:11 +0300, Imre Deak wrote: > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > sink's payload table, along clearing the payload table updated flag. > The sink is supposed to set this flag once it detects that the source > has completed the ACT sequence (after detecting the 4 required ACT MTPH > symbols sent by the source). As opposed to this 2 DELL monitors I have > set the flag already along the payload table updated flag, which is not > quite correct. > > To be sure that the sink has detected the ACT MTPH symbols before > continuing enabling the encoder, clear the ACT sent flag before enabling > or disabling the transcoder VC payload allocation (which is what starts > the ACT sequence). > > v2 (Ville): > - Use the correct bit to clear the flags. > - Add code comment explaining the clearing semantics of the ACT handled > flag. > > Cc: Lyude Paul <lyude@redhat.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 38 +++++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > include/drm/drm_dp_mst_helper.h | 2 ++ > 3 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index b2f5a84b4cfb..1f5d14128c1a 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4377,6 +4377,41 @@ void drm_dp_mst_deallocate_vcpi(struct > drm_dp_mst_topology_mgr *mgr, > } > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > +/** > + * drm_dp_clear_payload_status() - Clears the payload table status flags > + * @mgr: manager to use > + * > + * Clears the payload table ACT handled and table updated flags in the MST > hub's > + * DPCD. This function must be called before updating the payload table or > + * starting the ACT sequence and waiting for the corresponding flags to get > + * set by the hub. > + * > + * Returns: > + * 0 if the flags got cleared successfully, otherwise a negative error > code. > + */ > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > +{ > + int ret; > + > + /* > + * Note that the following is based on the DP Standard stating that > + * writing the DP_PAYLOAD_TABLE_UPDATED bit alone will clear both the > + * DP_PAYLOAD_TABLE_UPDATED and the DP_PAYLOAD_ACT_HANDLED flags. This > + * seems to be also the only way to clear DP_PAYLOAD_ACT_HANDLED. > + */ > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > + DP_PAYLOAD_TABLE_UPDATED); > + if (ret < 0) { > + DRM_DEBUG_DRIVER("Can't clear the ACT handled/table updated > flags (%d)\n", > + ret); > + return ret; > + } > + WARN_ON(ret != 1); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > + > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > int id, struct drm_dp_payload *payload) > { > @@ -4384,8 +4419,7 @@ static int drm_dp_dpcd_write_payload(struct > drm_dp_mst_topology_mgr *mgr, > int ret; > int retries = 0; > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > - DP_PAYLOAD_TABLE_UPDATED); > + drm_dp_clear_payload_status(mgr); > > payload_alloc[0] = id; > payload_alloc[1] = payload->start_slot; > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 9308b5920780..3c4b0fb10d8b 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > DP_TP_STATUS_ACT_SENT); > + > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > } > > static void wait_for_act_sent(struct intel_dp *intel_dp) > diff --git a/include/drm/drm_dp_mst_helper.h > b/include/drm/drm_dp_mst_helper.h > index 8b9eb4db3381..2facb87624bf 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct > drm_dp_mst_topology_mgr *mgr, > int pbn); > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > + > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > -- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it 2020-06-16 21:11 ` [PATCH v2 " Imre Deak 2020-06-17 15:27 ` Lyude Paul @ 2020-06-23 7:30 ` Imre Deak 1 sibling, 0 replies; 9+ messages in thread From: Imre Deak @ 2020-06-23 7:30 UTC (permalink / raw) To: intel-gfx, Lyude Paul; +Cc: dri-devel On Wed, Jun 17, 2020 at 12:11:46AM +0300, Imre Deak wrote: > Atm, we clear the ACT sent flag in the sink's DPCD before updating the > sink's payload table, along clearing the payload table updated flag. > The sink is supposed to set this flag once it detects that the source > has completed the ACT sequence (after detecting the 4 required ACT MTPH > symbols sent by the source). As opposed to this 2 DELL monitors I have > set the flag already along the payload table updated flag, which is not > quite correct. > > To be sure that the sink has detected the ACT MTPH symbols before > continuing enabling the encoder, clear the ACT sent flag before enabling > or disabling the transcoder VC payload allocation (which is what starts > the ACT sequence). > > v2 (Ville): > - Use the correct bit to clear the flags. > - Add code comment explaining the clearing semantics of the ACT handled > flag. > > Cc: Lyude Paul <lyude@redhat.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Imre Deak <imre.deak@intel.com> Dropping this patch because clearing the ACT handled flag from DPCD causes a problem for some sinks, which set this flag only once when the VC payload table is updated and do not set it when the ACT symbols are actually sent by the source. > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 38 +++++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++ > include/drm/drm_dp_mst_helper.h | 2 ++ > 3 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index b2f5a84b4cfb..1f5d14128c1a 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4377,6 +4377,41 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, > } > EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); > > +/** > + * drm_dp_clear_payload_status() - Clears the payload table status flags > + * @mgr: manager to use > + * > + * Clears the payload table ACT handled and table updated flags in the MST hub's > + * DPCD. This function must be called before updating the payload table or > + * starting the ACT sequence and waiting for the corresponding flags to get > + * set by the hub. > + * > + * Returns: > + * 0 if the flags got cleared successfully, otherwise a negative error code. > + */ > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr) > +{ > + int ret; > + > + /* > + * Note that the following is based on the DP Standard stating that > + * writing the DP_PAYLOAD_TABLE_UPDATED bit alone will clear both the > + * DP_PAYLOAD_TABLE_UPDATED and the DP_PAYLOAD_ACT_HANDLED flags. This > + * seems to be also the only way to clear DP_PAYLOAD_ACT_HANDLED. > + */ > + ret = drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > + DP_PAYLOAD_TABLE_UPDATED); > + if (ret < 0) { > + DRM_DEBUG_DRIVER("Can't clear the ACT handled/table updated flags (%d)\n", > + ret); > + return ret; > + } > + WARN_ON(ret != 1); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_dp_clear_payload_status); > + > static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > int id, struct drm_dp_payload *payload) > { > @@ -4384,8 +4419,7 @@ static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr, > int ret; > int retries = 0; > > - drm_dp_dpcd_writeb(mgr->aux, DP_PAYLOAD_TABLE_UPDATE_STATUS, > - DP_PAYLOAD_TABLE_UPDATED); > + drm_dp_clear_payload_status(mgr); > > payload_alloc[0] = id; > payload_alloc[1] = payload->start_slot; > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 9308b5920780..3c4b0fb10d8b 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -323,6 +323,8 @@ static void clear_act_sent(struct intel_dp *intel_dp) > > intel_de_write(i915, intel_dp->regs.dp_tp_status, > DP_TP_STATUS_ACT_SENT); > + > + drm_dp_clear_payload_status(&intel_dp->mst_mgr); > } > > static void wait_for_act_sent(struct intel_dp *intel_dp) > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 8b9eb4db3381..2facb87624bf 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -763,6 +763,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, > int pbn); > > > +int drm_dp_clear_payload_status(struct drm_dp_mst_topology_mgr *mgr); > + > int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr); > > > -- > 2.23.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-23 7:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200616141855.746-1-imre.deak@intel.com>
2020-06-16 14:18 ` [PATCH 6/6] drm/i915/dp_mst: Ensure the DPCD ACT sent flag is cleared before waiting for it Imre Deak
2020-06-16 15:45 ` Ville Syrjälä
2020-06-16 15:54 ` Imre Deak
2020-06-16 16:23 ` Ville Syrjälä
2020-06-16 16:40 ` Ville Syrjälä
2020-06-16 16:47 ` Imre Deak
2020-06-16 21:11 ` [PATCH v2 " Imre Deak
2020-06-17 15:27 ` Lyude Paul
2020-06-23 7:30 ` Imre Deak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).