| Message ID | 20180831174739.30387-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp_mst: Fix enabling pipe clock for all streams | expand |
On Fri, Aug 31, 2018 at 08:47:39PM +0300, Imre Deak wrote: > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > inadvertently stopped enabling the pipe clock for any DP-MST stream > after the first one. It also rearranged the pipe clock enabling wrt. > initial MST payload allocation step (which may or may not be a > problem, but it's contrary to the spec.). > > Fix things by making the above commit truly a non-functional change. > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107365 > Reported-by: Lyude Paul <lyude@redhat.com> > Reported-by: dmummenschanz@web.de > Tested-by: dmummenschanz@web.de > Cc: Lyude Paul <lyude@redhat.com> > Cc: dmummenschanz@web.de > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index f3b115ce4029..dcb1a98d624d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > icl_enable_phy_clock_gating(dig_port); > > - intel_ddi_enable_pipe_clock(crtc_state); > + if (!is_mst) > + intel_ddi_enable_pipe_clock(crtc_state); > } > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > bool is_mst = intel_crtc_has_type(old_crtc_state, > INTEL_OUTPUT_DP_MST); > > - intel_ddi_disable_pipe_clock(old_crtc_state); > - > - /* > - * Power down sink before disabling the port, otherwise we end > - * up getting interrupts from the sink on detecting link loss. > - */ > - if (!is_mst) > + if (!is_mst) { > + intel_ddi_disable_pipe_clock(old_crtc_state); > + /* > + * Power down sink before disabling the port, otherwise we end > + * up getting interrupts from the sink on detecting link loss. > + */ > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + } > > intel_disable_ddi_buf(encoder); > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 352e5216cc65..77920f1a3da1 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > struct intel_connector *connector = > to_intel_connector(old_conn_state->connector); > > + intel_ddi_disable_pipe_clock(old_crtc_state); > + > /* this can fail */ > drm_dp_check_act_status(&intel_dp->mst_mgr); > /* and this can also fail */ > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, > I915_WRITE(DP_TP_STATUS(port), temp); > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > + > + intel_ddi_enable_pipe_clock(pipe_config); > } > > static void intel_mst_enable_dp(struct intel_encoder *encoder, > -- > 2.13.2
Tested on my T450s, fixes the regression with MST! Consider this: Tested-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Lyude Paul <lyude@redhat.com> On Fri, 2018-08-31 at 20:47 +0300, Imre Deak wrote: > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > inadvertently stopped enabling the pipe clock for any DP-MST stream > after the first one. It also rearranged the pipe clock enabling wrt. > initial MST payload allocation step (which may or may not be a > problem, but it's contrary to the spec.). > > Fix things by making the above commit truly a non-functional change. > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to > encoders") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107365 > Reported-by: Lyude Paul <lyude@redhat.com> > Reported-by: dmummenschanz@web.de > Tested-by: dmummenschanz@web.de > Cc: Lyude Paul <lyude@redhat.com> > Cc: dmummenschanz@web.de > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index f3b115ce4029..dcb1a98d624d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct > intel_encoder *encoder, > > icl_enable_phy_clock_gating(dig_port); > > - intel_ddi_enable_pipe_clock(crtc_state); > + if (!is_mst) > + intel_ddi_enable_pipe_clock(crtc_state); > } > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct > intel_encoder *encoder, > bool is_mst = intel_crtc_has_type(old_crtc_state, > INTEL_OUTPUT_DP_MST); > > - intel_ddi_disable_pipe_clock(old_crtc_state); > - > - /* > - * Power down sink before disabling the port, otherwise we end > - * up getting interrupts from the sink on detecting link loss. > - */ > - if (!is_mst) > + if (!is_mst) { > + intel_ddi_disable_pipe_clock(old_crtc_state); > + /* > + * Power down sink before disabling the port, otherwise we end > + * up getting interrupts from the sink on detecting link loss. > + */ > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + } > > intel_disable_ddi_buf(encoder); > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > b/drivers/gpu/drm/i915/intel_dp_mst.c > index 352e5216cc65..77920f1a3da1 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct > intel_encoder *encoder, > struct intel_connector *connector = > to_intel_connector(old_conn_state->connector); > > + intel_ddi_disable_pipe_clock(old_crtc_state); > + > /* this can fail */ > drm_dp_check_act_status(&intel_dp->mst_mgr); > /* and this can also fail */ > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder > *encoder, > I915_WRITE(DP_TP_STATUS(port), temp); > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > + > + intel_ddi_enable_pipe_clock(pipe_config); > } > > static void intel_mst_enable_dp(struct intel_encoder *encoder,
On Fri, Aug 31, 2018 at 08:47:39PM +0300, Imre Deak wrote: > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > inadvertently stopped enabling the pipe clock for any DP-MST stream > after the first one. It also rearranged the pipe clock enabling wrt. > initial MST payload allocation step (which may or may not be a > problem, but it's contrary to the spec.). > > Fix things by making the above commit truly a non-functional change. > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107365 > Reported-by: Lyude Paul <lyude@redhat.com> > Reported-by: dmummenschanz@web.de > Tested-by: dmummenschanz@web.de > Cc: Lyude Paul <lyude@redhat.com> > Cc: dmummenschanz@web.de > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index f3b115ce4029..dcb1a98d624d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > icl_enable_phy_clock_gating(dig_port); > > - intel_ddi_enable_pipe_clock(crtc_state); > + if (!is_mst) > + intel_ddi_enable_pipe_clock(crtc_state); > } > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > bool is_mst = intel_crtc_has_type(old_crtc_state, > INTEL_OUTPUT_DP_MST); > > - intel_ddi_disable_pipe_clock(old_crtc_state); > - > - /* > - * Power down sink before disabling the port, otherwise we end > - * up getting interrupts from the sink on detecting link loss. > - */ > - if (!is_mst) > + if (!is_mst) { > + intel_ddi_disable_pipe_clock(old_crtc_state); > + /* > + * Power down sink before disabling the port, otherwise we end > + * up getting interrupts from the sink on detecting link loss. > + */ > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + } > > intel_disable_ddi_buf(encoder); > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 352e5216cc65..77920f1a3da1 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > struct intel_connector *connector = > to_intel_connector(old_conn_state->connector); > > + intel_ddi_disable_pipe_clock(old_crtc_state); > + > /* this can fail */ > drm_dp_check_act_status(&intel_dp->mst_mgr); > /* and this can also fail */ > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, > I915_WRITE(DP_TP_STATUS(port), temp); > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > + > + intel_ddi_enable_pipe_clock(pipe_config); > } > > static void intel_mst_enable_dp(struct intel_encoder *encoder, > -- > 2.13.2 >
On Sat, Sep 01, 2018 at 04:38:23AM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915/dp_mst: Fix enabling pipe clock for all streams > URL : https://patchwork.freedesktop.org/series/49025/ > State : success Pushed to -dinq, thanks for the report, review and testing. > > == Summary == > > = CI Bug Log - changes from CI_DRM_4750_full -> Patchwork_10063_full = > > == Summary - SUCCESS == > > No regressions found. > > > > == Known issues == > > Here are the changes found in Patchwork_10063_full that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@drv_suspend@shrink: > shard-hsw: PASS -> INCOMPLETE (fdo#103540, fdo#106886) > > > ==== Possible fixes ==== > > igt@gem_exec_await@wide-contexts: > shard-kbl: FAIL (fdo#105900) -> PASS > > igt@gem_exec_suspend@basic-s3-devices: > shard-snb: INCOMPLETE (fdo#105411) -> PASS > > igt@kms_frontbuffer_tracking@fbc-suspend: > shard-kbl: INCOMPLETE (fdo#103665, fdo#105959) -> PASS > > igt@perf@blocking: > shard-hsw: FAIL (fdo#102252) -> PASS > > > fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 > fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540 > fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665 > fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411 > fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900 > fdo#105959 https://bugs.freedesktop.org/show_bug.cgi?id=105959 > fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886 > > > == Participating hosts (5 -> 5) == > > No changes in participating hosts > > > == Build changes == > > * Linux: CI_DRM_4750 -> Patchwork_10063 > > CI_DRM_4750: ef9613f5ddd35f2bd2834489b6d96e54c0cae8c6 @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4618: 9d83154c898b5acc8b462d17104df50cfd71e9a0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_10063: bb1f39c1120a86a6d33ca3c3fe8679e33a73f133 @ git://anongit.freedesktop.org/gfx-ci/linux > piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10063/shards.html
On Fri, 31 Aug 2018, Imre Deak <imre.deak@intel.com> wrote: > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > inadvertently stopped enabling the pipe clock for any DP-MST stream > after the first one. It also rearranged the pipe clock enabling wrt. > initial MST payload allocation step (which may or may not be a > problem, but it's contrary to the spec.). > > Fix things by making the above commit truly a non-functional change. What kind of MST setups do we have in CI? Why didn't they catch this? BR, Jani. > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107365 > Reported-by: Lyude Paul <lyude@redhat.com> > Reported-by: dmummenschanz@web.de > Tested-by: dmummenschanz@web.de > Cc: Lyude Paul <lyude@redhat.com> > Cc: dmummenschanz@web.de > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > 2 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index f3b115ce4029..dcb1a98d624d 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > icl_enable_phy_clock_gating(dig_port); > > - intel_ddi_enable_pipe_clock(crtc_state); > + if (!is_mst) > + intel_ddi_enable_pipe_clock(crtc_state); > } > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > bool is_mst = intel_crtc_has_type(old_crtc_state, > INTEL_OUTPUT_DP_MST); > > - intel_ddi_disable_pipe_clock(old_crtc_state); > - > - /* > - * Power down sink before disabling the port, otherwise we end > - * up getting interrupts from the sink on detecting link loss. > - */ > - if (!is_mst) > + if (!is_mst) { > + intel_ddi_disable_pipe_clock(old_crtc_state); > + /* > + * Power down sink before disabling the port, otherwise we end > + * up getting interrupts from the sink on detecting link loss. > + */ > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + } > > intel_disable_ddi_buf(encoder); > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 352e5216cc65..77920f1a3da1 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > struct intel_connector *connector = > to_intel_connector(old_conn_state->connector); > > + intel_ddi_disable_pipe_clock(old_crtc_state); > + > /* this can fail */ > drm_dp_check_act_status(&intel_dp->mst_mgr); > /* and this can also fail */ > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, > I915_WRITE(DP_TP_STATUS(port), temp); > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > + > + intel_ddi_enable_pipe_clock(pipe_config); > } > > static void intel_mst_enable_dp(struct intel_encoder *encoder,
On Tue, Sep 04, 2018 at 03:08:16PM +0300, Jani Nikula wrote: > On Fri, 31 Aug 2018, Imre Deak <imre.deak@intel.com> wrote: > > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > > inadvertently stopped enabling the pipe clock for any DP-MST stream > > after the first one. It also rearranged the pipe clock enabling wrt. > > initial MST payload allocation step (which may or may not be a > > problem, but it's contrary to the spec.). > > > > Fix things by making the above commit truly a non-functional change. > > What kind of MST setups do we have in CI? Why didn't they catch this? What we'd need is a dock/other branch device with an DP-MST input and two outputs. That would exercise the case that broke here. > > BR, > Jani. > > > > > > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107365 > > Reported-by: Lyude Paul <lyude@redhat.com> > > Reported-by: dmummenschanz@web.de > > Tested-by: dmummenschanz@web.de > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: dmummenschanz@web.de > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > > 2 files changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index f3b115ce4029..dcb1a98d624d 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > > > icl_enable_phy_clock_gating(dig_port); > > > > - intel_ddi_enable_pipe_clock(crtc_state); > > + if (!is_mst) > > + intel_ddi_enable_pipe_clock(crtc_state); > > } > > > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > > bool is_mst = intel_crtc_has_type(old_crtc_state, > > INTEL_OUTPUT_DP_MST); > > > > - intel_ddi_disable_pipe_clock(old_crtc_state); > > - > > - /* > > - * Power down sink before disabling the port, otherwise we end > > - * up getting interrupts from the sink on detecting link loss. > > - */ > > - if (!is_mst) > > + if (!is_mst) { > > + intel_ddi_disable_pipe_clock(old_crtc_state); > > + /* > > + * Power down sink before disabling the port, otherwise we end > > + * up getting interrupts from the sink on detecting link loss. > > + */ > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > + } > > > > intel_disable_ddi_buf(encoder); > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 352e5216cc65..77920f1a3da1 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > > struct intel_connector *connector = > > to_intel_connector(old_conn_state->connector); > > > > + intel_ddi_disable_pipe_clock(old_crtc_state); > > + > > /* this can fail */ > > drm_dp_check_act_status(&intel_dp->mst_mgr); > > /* and this can also fail */ > > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, > > I915_WRITE(DP_TP_STATUS(port), temp); > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > + > > + intel_ddi_enable_pipe_clock(pipe_config); > > } > > > > static void intel_mst_enable_dp(struct intel_encoder *encoder, > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tuesday, September 4, 2018 5:54:16 AM PDT Imre Deak wrote: > On Tue, Sep 04, 2018 at 03:08:16PM +0300, Jani Nikula wrote: > > On Fri, 31 Aug 2018, Imre Deak <imre.deak@intel.com> wrote: > > > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to > > > encoders") > > > inadvertently stopped enabling the pipe clock for any DP-MST stream > > > after the first one. It also rearranged the pipe clock enabling wrt. > > > initial MST payload allocation step (which may or may not be a > > > problem, but it's contrary to the spec.). > > > > > > Fix things by making the above commit truly a non-functional change. > > > > What kind of MST setups do we have in CI? Why didn't they catch this? > > What we'd need is a dock/other branch device with an DP-MST input and > two outputs. That would exercise the case that broke here. > I had the same question. We have these two in CI, but both have only one external display attached. fi-kbl-7560u Dell XPS 13 Kaby Lake / i7-7560u / Iris Plus Graphics 640 GT3e eDP, DELL TB16->TB->DP-MST fi-cfl-s3 Intel Coffee Lake-S RVP Coffee Lake eDP-PSR, DP-MST (HDMI) Tomi, Is it possible to have another external display connected to one of these? > > BR, > > Jani. > > > > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to > > > encoders") Bugzilla: > > > https://bugs.freedesktop.org/show_bug.cgi?id=107365 > > > Reported-by: Lyude Paul <lyude@redhat.com> > > > Reported-by: dmummenschanz@web.de > > > Tested-by: dmummenschanz@web.de > > > Cc: Lyude Paul <lyude@redhat.com> > > > Cc: dmummenschanz@web.de > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > > > > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > > > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > > > 2 files changed, 13 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > b/drivers/gpu/drm/i915/intel_ddi.c index f3b115ce4029..dcb1a98d624d > > > 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct > > > intel_encoder *encoder,> > > > > icl_enable_phy_clock_gating(dig_port); > > > > > > - intel_ddi_enable_pipe_clock(crtc_state); > > > + if (!is_mst) > > > + intel_ddi_enable_pipe_clock(crtc_state); > > > > > > } > > > > > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > > > > > > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct > > > intel_encoder *encoder,> > > > > bool is_mst = intel_crtc_has_type(old_crtc_state, > > > > > > INTEL_OUTPUT_DP_MST); > > > > > > - intel_ddi_disable_pipe_clock(old_crtc_state); > > > - > > > - /* > > > - * Power down sink before disabling the port, otherwise we end > > > - * up getting interrupts from the sink on detecting link loss. > > > - */ > > > - if (!is_mst) > > > + if (!is_mst) { > > > + intel_ddi_disable_pipe_clock(old_crtc_state); > > > + /* > > > + * Power down sink before disabling the port, otherwise we end > > > + * up getting interrupts from the sink on detecting link loss. > > > + */ > > > > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > > > > > + } > > > > > > intel_disable_ddi_buf(encoder); > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/intel_dp_mst.c index 352e5216cc65..77920f1a3da1 > > > 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct > > > intel_encoder *encoder,> > > > > struct intel_connector *connector = > > > > > > to_intel_connector(old_conn_state->connector); > > > > > > + intel_ddi_disable_pipe_clock(old_crtc_state); > > > + > > > > > > /* this can fail */ > > > drm_dp_check_act_status(&intel_dp->mst_mgr); > > > /* and this can also fail */ > > > > > > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct > > > intel_encoder *encoder,> > > > > I915_WRITE(DP_TP_STATUS(port), temp); > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > > > > + > > > + intel_ddi_enable_pipe_clock(pipe_config); > > > > > > } > > > > > > static void intel_mst_enable_dp(struct intel_encoder *encoder, > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 04, 2018 at 03:53:51PM -0700, Dhinakaran Pandiyan wrote: > On Tuesday, September 4, 2018 5:54:16 AM PDT Imre Deak wrote: > > On Tue, Sep 04, 2018 at 03:08:16PM +0300, Jani Nikula wrote: > > > On Fri, 31 Aug 2018, Imre Deak <imre.deak@intel.com> wrote: > > > > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to > > > > encoders") > > > > inadvertently stopped enabling the pipe clock for any DP-MST stream > > > > after the first one. It also rearranged the pipe clock enabling wrt. > > > > initial MST payload allocation step (which may or may not be a > > > > problem, but it's contrary to the spec.). > > > > > > > > Fix things by making the above commit truly a non-functional change. > > > > > > What kind of MST setups do we have in CI? Why didn't they catch this? > > > > What we'd need is a dock/other branch device with an DP-MST input and > > two outputs. That would exercise the case that broke here. > > > I had the same question. We have these two in CI, but both have only one > external display attached. > fi-kbl-7560u Dell XPS 13 Kaby Lake / i7-7560u / Iris Plus Graphics 640 GT3e > eDP, DELL TB16->TB->DP-MST > fi-cfl-s3 Intel Coffee Lake-S RVP Coffee Lake eDP-PSR, DP-MST (HDMI) > > Tomi, > Is it possible to have another external display connected to one of these? or to both of them?! I'm seeing MST related regressions since 4.16 so I'd like to have few different configuration if possible with more than one monitor. at least one kind of dock and one kind of chain both with 2 monitors plugged in would be good. > > > > > BR, > > > Jani. > > > > > > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to > > > > encoders") Bugzilla: > > > > https://bugs.freedesktop.org/show_bug.cgi?id=107365 > > > > Reported-by: Lyude Paul <lyude@redhat.com> > > > > Reported-by: dmummenschanz@web.de > > > > Tested-by: dmummenschanz@web.de > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Cc: dmummenschanz@web.de > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > > > > > drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ > > > > 2 files changed, 13 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > b/drivers/gpu/drm/i915/intel_ddi.c index f3b115ce4029..dcb1a98d624d > > > > 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct > > > > intel_encoder *encoder,> > > > > > icl_enable_phy_clock_gating(dig_port); > > > > > > > > - intel_ddi_enable_pipe_clock(crtc_state); > > > > + if (!is_mst) > > > > + intel_ddi_enable_pipe_clock(crtc_state); > > > > > > > > } > > > > > > > > static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, > > > > > > > > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct > > > > intel_encoder *encoder,> > > > > > bool is_mst = intel_crtc_has_type(old_crtc_state, > > > > > > > > INTEL_OUTPUT_DP_MST); > > > > > > > > - intel_ddi_disable_pipe_clock(old_crtc_state); > > > > - > > > > - /* > > > > - * Power down sink before disabling the port, otherwise we end > > > > - * up getting interrupts from the sink on detecting link loss. > > > > - */ > > > > - if (!is_mst) > > > > + if (!is_mst) { > > > > + intel_ddi_disable_pipe_clock(old_crtc_state); > > > > + /* > > > > + * Power down sink before disabling the port, otherwise we end > > > > + * up getting interrupts from the sink on detecting link loss. > > > > + */ > > > > > > > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > > > > > > > + } > > > > > > > > intel_disable_ddi_buf(encoder); > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > b/drivers/gpu/drm/i915/intel_dp_mst.c index 352e5216cc65..77920f1a3da1 > > > > 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct > > > > intel_encoder *encoder,> > > > > > struct intel_connector *connector = > > > > > > > > to_intel_connector(old_conn_state->connector); > > > > > > > > + intel_ddi_disable_pipe_clock(old_crtc_state); > > > > + > > > > > > > > /* this can fail */ > > > > drm_dp_check_act_status(&intel_dp->mst_mgr); > > > > /* and this can also fail */ > > > > > > > > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct > > > > intel_encoder *encoder,> > > > > > I915_WRITE(DP_TP_STATUS(port), temp); > > > > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); > > > > > > > > + > > > > + intel_ddi_enable_pipe_clock(pipe_config); > > > > > > > > } > > > > > > > > static void intel_mst_enable_dp(struct intel_encoder *encoder, > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >
On Tue, 2018-09-04 at 16:19 -0700, Rodrigo Vivi wrote: > On Tue, Sep 04, 2018 at 03:53:51PM -0700, Dhinakaran Pandiyan wrote: > > On Tuesday, September 4, 2018 5:54:16 AM PDT Imre Deak wrote: > > > On Tue, Sep 04, 2018 at 03:08:16PM +0300, Jani Nikula wrote: > > > > On Fri, 31 Aug 2018, Imre Deak <imre.deak@intel.com> wrote: > > > > > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling > > > > > to > > > > > encoders") > > > > > inadvertently stopped enabling the pipe clock for any DP-MST > > > > > stream > > > > > after the first one. It also rearranged the pipe clock > > > > > enabling wrt. > > > > > initial MST payload allocation step (which may or may not be > > > > > a > > > > > problem, but it's contrary to the spec.). > > > > > > > > > > Fix things by making the above commit truly a non-functional > > > > > change. > > > > > > > > What kind of MST setups do we have in CI? Why didn't they catch > > > > this? > > > > > > What we'd need is a dock/other branch device with an DP-MST input > > > and > > > two outputs. That would exercise the case that broke here. > > > > > > > I had the same question. We have these two in CI, but both have > > only one > > external display attached. > > fi-kbl-7560u Dell XPS 13 Kaby Lake / i7-7560u / Iris Plus > > Graphics 640 GT3e > > eDP, DELL TB16->TB->DP-MST > > fi-cfl-s3 Intel Coffee Lake-S RVP Coffee Lake eDP-PSR, DP- > > MST (HDMI) > > > > Tomi, > > Is it possible to have another external display connected to one of > > these? > > or to both of them?! > :) Not sure why I wanted only one of them. > I'm seeing MST related regressions since 4.16 so I'd like to have few > different configuration if possible with more than one monitor. A 4K@60Hz monitor along with a full-HD (or higher) monitor will be useful to test the link BW code too. Together, they should be exceeding the 63 vcpi slot limit. We can then write new IGT's to verify that. > > at least one kind of dock and one kind of chain both with 2 monitors > plugged in would be good.
On Tue, 04 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > On Tue, 2018-09-04 at 16:19 -0700, Rodrigo Vivi wrote: >> On Tue, Sep 04, 2018 at 03:53:51PM -0700, Dhinakaran Pandiyan wrote: >> > Is it possible to have another external display connected to one of >> > these? >> >> or to both of them?! >> > :) Not sure why I wanted only one of them. Heh, well, we did have a bug in the past where we failed if the device was booted with no displays. Diversify, don't maximize. ;) BR, Jani.
On Wed, Sep 05, 2018 at 01:01:58PM +0300, Jani Nikula wrote: 1;5202;0c> On Tue, 04 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote: > > On Tue, 2018-09-04 at 16:19 -0700, Rodrigo Vivi wrote: > >> On Tue, Sep 04, 2018 at 03:53:51PM -0700, Dhinakaran Pandiyan wrote: > >> > Is it possible to have another external display connected to one of > >> > these? > >> > >> or to both of them?! > >> > > :) Not sure why I wanted only one of them. > > Heh, well, we did have a bug in the past where we failed if the device > was booted with no displays. Diversify, don't maximize. ;) hehe :) yeap, makes sense! > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index f3b115ce4029..dcb1a98d624d 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, icl_enable_phy_clock_gating(dig_port); - intel_ddi_enable_pipe_clock(crtc_state); + if (!is_mst) + intel_ddi_enable_pipe_clock(crtc_state); } static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder, @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST); - intel_ddi_disable_pipe_clock(old_crtc_state); - - /* - * Power down sink before disabling the port, otherwise we end - * up getting interrupts from the sink on detecting link loss. - */ - if (!is_mst) + if (!is_mst) { + intel_ddi_disable_pipe_clock(old_crtc_state); + /* + * Power down sink before disabling the port, otherwise we end + * up getting interrupts from the sink on detecting link loss. + */ intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); + } intel_disable_ddi_buf(encoder); diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 352e5216cc65..77920f1a3da1 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, struct intel_connector *connector = to_intel_connector(old_conn_state->connector); + intel_ddi_disable_pipe_clock(old_crtc_state); + /* this can fail */ drm_dp_check_act_status(&intel_dp->mst_mgr); /* and this can also fail */ @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder, I915_WRITE(DP_TP_STATUS(port), temp); ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr); + + intel_ddi_enable_pipe_clock(pipe_config); } static void intel_mst_enable_dp(struct intel_encoder *encoder,
commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") inadvertently stopped enabling the pipe clock for any DP-MST stream after the first one. It also rearranged the pipe clock enabling wrt. initial MST payload allocation step (which may or may not be a problem, but it's contrary to the spec.). Fix things by making the above commit truly a non-functional change. Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to encoders") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107365 Reported-by: Lyude Paul <lyude@redhat.com> Reported-by: dmummenschanz@web.de Tested-by: dmummenschanz@web.de Cc: Lyude Paul <lyude@redhat.com> Cc: dmummenschanz@web.de Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 17 +++++++++-------- drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++ 2 files changed, 13 insertions(+), 8 deletions(-)