Re: [Intel-gfx] [PATCH 04/12] drm/i915: Reject mixing MST and SST/HDMI on the same digital port
On Fri, Jul 29, 2016 at 02:28:26PM +0300, Ville Syrjälä wrote: > On Fri, Jul 29, 2016 at 11:19:18AM +0200, Daniel Vetter wrote: > > On Thu, Jul 28, 2016 at 05:50:40PM +0300, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä > > > > > > We can't mix MST with SST/HDMI on the same physical port, so we'll need > > > to reject such configurations in check_digital_port_conflicts(). Nothing > > > else will prevent this as MST has its fake encoders and its own connectors > > > so the cloning checks won't catch this. > > > > > > The same digital port can be used multiple times, but only if all the > > > encoders involved are MST encoders, so we only want to check MST vs. > > > SST/HDMI, not MST vs. MST. And SST/HDMI vs. SST/HDMI we already check. > > > > > > Cc: Maarten Lankhorst > > > Signed-off-by: Ville Syrjälä > > > > I'd be awesome if we could have some testcases for this. We can e.g. try > > to enable HDMI without it being connected (it'll fall back to dvi mode), > > so could use that to try enabling both hdmi and dp or dp mst out of sheer > > nastiness. I don't even think we need to check that it gets rejected > > correctly, just trying to do such a modeset on a broken kernel should > > result in plenty of fireworks in dmesg (state checker for sure will be > > unhapy) to make it obvious it's broken. Aka would absolutely love if this > > patch gained a Testcase: line. Either way: > > What I've occasionally used for testing is kms_setmode with a patch to > ignore all errors from setcrtc so that it doesn't abort midway. IIRC > last time I did that on eg. HSW there were plenty of state checker > complaints. In its default state kms_setmode has a bit of a problem > because it doesn't know about conflicting connectors, nor does it know > if we're supposed to have one encoder per connector or pair of > connectors (DDI vs. !DDI). And also IIRC encoder/connector stealing > will wreac some extra havoc here. Not sure if there's a good way to > try and fix it so that by default kms_setmode would really test > what's it's pretending to test on all platforms. > > I guess we could at least promote my hack to a proper command line > option, so that we can try to feed as many combinations to the kernel > as possible. We could promote it to a full test using atomic. Do a TEST_ONLY atomic commit, then a real one and compare whether results agree. And if there's any noise on dmesg then also fall over (like we do with all tests). Of course that doens't encode the expectations we have on all platforms, but I'm not sure there's enough value in maintaining such a detailed testcase. The above would be generic, could run everywhere, doesn't need updating and should at least uncover any internal inconsistencies. -Daniel > > > > > Reviewed-by: Daniel Vetter > > > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 10 ++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index c4c1c85366de..a6699c76bef3 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -12305,6 +12305,7 @@ static bool check_digital_port_conflicts(struct > > > drm_atomic_state *state) > > > struct drm_device *dev = state->dev; > > > struct drm_connector *connector; > > > unsigned int used_ports = 0; > > > + unsigned int used_mst_ports = 0; > > > > > > /* > > >* Walk the connector list instead of the encoder > > > @@ -12341,11 +12342,20 @@ static bool check_digital_port_conflicts(struct > > > drm_atomic_state *state) > > > return false; > > > > > > used_ports |= port_mask; > > > + break; > > > + case INTEL_OUTPUT_DP_MST: > > > + used_mst_ports |= > > > + 1 << enc_to_mst(&encoder->base)->primary->port; > > > + break; > > > default: > > > break; > > > } > > > } > > > > > > + /* can't mix MST and SST/HDMI on the same port */ > > > + if (used_ports & used_mst_ports) > > > + return false; > > > + > > > return true; > > > } > > > > > > -- > > > 2.7.4 > > > > > > ___ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Reject mixing MST and SST/HDMI on the same digital port
Op 28-07-16 om 16:50 schreef ville.syrj...@linux.intel.com: > From: Ville Syrjälä > > We can't mix MST with SST/HDMI on the same physical port, so we'll need > to reject such configurations in check_digital_port_conflicts(). Nothing > else will prevent this as MST has its fake encoders and its own connectors > so the cloning checks won't catch this. > > The same digital port can be used multiple times, but only if all the > encoders involved are MST encoders, so we only want to check MST vs. > SST/HDMI, not MST vs. MST. And SST/HDMI vs. SST/HDMI we already check. I guess this is to prevent DP1 and DP1-2 to be both set simultaneously? I'm not sure kms_setmode would handle this case correctly, but the idea is correct. Reviewed-by: Maarten Lankhorst ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Reject mixing MST and SST/HDMI on the same digital port
On Fri, Jul 29, 2016 at 11:19:18AM +0200, Daniel Vetter wrote: > On Thu, Jul 28, 2016 at 05:50:40PM +0300, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > We can't mix MST with SST/HDMI on the same physical port, so we'll need > > to reject such configurations in check_digital_port_conflicts(). Nothing > > else will prevent this as MST has its fake encoders and its own connectors > > so the cloning checks won't catch this. > > > > The same digital port can be used multiple times, but only if all the > > encoders involved are MST encoders, so we only want to check MST vs. > > SST/HDMI, not MST vs. MST. And SST/HDMI vs. SST/HDMI we already check. > > > > Cc: Maarten Lankhorst > > Signed-off-by: Ville Syrjälä > > I'd be awesome if we could have some testcases for this. We can e.g. try > to enable HDMI without it being connected (it'll fall back to dvi mode), > so could use that to try enabling both hdmi and dp or dp mst out of sheer > nastiness. I don't even think we need to check that it gets rejected > correctly, just trying to do such a modeset on a broken kernel should > result in plenty of fireworks in dmesg (state checker for sure will be > unhapy) to make it obvious it's broken. Aka would absolutely love if this > patch gained a Testcase: line. Either way: What I've occasionally used for testing is kms_setmode with a patch to ignore all errors from setcrtc so that it doesn't abort midway. IIRC last time I did that on eg. HSW there were plenty of state checker complaints. In its default state kms_setmode has a bit of a problem because it doesn't know about conflicting connectors, nor does it know if we're supposed to have one encoder per connector or pair of connectors (DDI vs. !DDI). And also IIRC encoder/connector stealing will wreac some extra havoc here. Not sure if there's a good way to try and fix it so that by default kms_setmode would really test what's it's pretending to test on all platforms. I guess we could at least promote my hack to a proper command line option, so that we can try to feed as many combinations to the kernel as possible. > > Reviewed-by: Daniel Vetter > > > --- > > drivers/gpu/drm/i915/intel_display.c | 10 ++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index c4c1c85366de..a6699c76bef3 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -12305,6 +12305,7 @@ static bool check_digital_port_conflicts(struct > > drm_atomic_state *state) > > struct drm_device *dev = state->dev; > > struct drm_connector *connector; > > unsigned int used_ports = 0; > > + unsigned int used_mst_ports = 0; > > > > /* > > * Walk the connector list instead of the encoder > > @@ -12341,11 +12342,20 @@ static bool check_digital_port_conflicts(struct > > drm_atomic_state *state) > > return false; > > > > used_ports |= port_mask; > > + break; > > + case INTEL_OUTPUT_DP_MST: > > + used_mst_ports |= > > + 1 << enc_to_mst(&encoder->base)->primary->port; > > + break; > > default: > > break; > > } > > } > > > > + /* can't mix MST and SST/HDMI on the same port */ > > + if (used_ports & used_mst_ports) > > + return false; > > + > > return true; > > } > > > > -- > > 2.7.4 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Reject mixing MST and SST/HDMI on the same digital port
On Thu, Jul 28, 2016 at 05:50:40PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > We can't mix MST with SST/HDMI on the same physical port, so we'll need > to reject such configurations in check_digital_port_conflicts(). Nothing > else will prevent this as MST has its fake encoders and its own connectors > so the cloning checks won't catch this. > > The same digital port can be used multiple times, but only if all the > encoders involved are MST encoders, so we only want to check MST vs. > SST/HDMI, not MST vs. MST. And SST/HDMI vs. SST/HDMI we already check. > > Cc: Maarten Lankhorst > Signed-off-by: Ville Syrjälä I'd be awesome if we could have some testcases for this. We can e.g. try to enable HDMI without it being connected (it'll fall back to dvi mode), so could use that to try enabling both hdmi and dp or dp mst out of sheer nastiness. I don't even think we need to check that it gets rejected correctly, just trying to do such a modeset on a broken kernel should result in plenty of fireworks in dmesg (state checker for sure will be unhapy) to make it obvious it's broken. Aka would absolutely love if this patch gained a Testcase: line. Either way: Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index c4c1c85366de..a6699c76bef3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12305,6 +12305,7 @@ static bool check_digital_port_conflicts(struct > drm_atomic_state *state) > struct drm_device *dev = state->dev; > struct drm_connector *connector; > unsigned int used_ports = 0; > + unsigned int used_mst_ports = 0; > > /* >* Walk the connector list instead of the encoder > @@ -12341,11 +12342,20 @@ static bool check_digital_port_conflicts(struct > drm_atomic_state *state) > return false; > > used_ports |= port_mask; > + break; > + case INTEL_OUTPUT_DP_MST: > + used_mst_ports |= > + 1 << enc_to_mst(&encoder->base)->primary->port; > + break; > default: > break; > } > } > > + /* can't mix MST and SST/HDMI on the same port */ > + if (used_ports & used_mst_ports) > + return false; > + > return true; > } > > -- > 2.7.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] drm/i915: Reject mixing MST and SST/HDMI on the same digital port
From: Ville Syrjälä We can't mix MST with SST/HDMI on the same physical port, so we'll need to reject such configurations in check_digital_port_conflicts(). Nothing else will prevent this as MST has its fake encoders and its own connectors so the cloning checks won't catch this. The same digital port can be used multiple times, but only if all the encoders involved are MST encoders, so we only want to check MST vs. SST/HDMI, not MST vs. MST. And SST/HDMI vs. SST/HDMI we already check. Cc: Maarten Lankhorst Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c4c1c85366de..a6699c76bef3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12305,6 +12305,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state) struct drm_device *dev = state->dev; struct drm_connector *connector; unsigned int used_ports = 0; + unsigned int used_mst_ports = 0; /* * Walk the connector list instead of the encoder @@ -12341,11 +12342,20 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state) return false; used_ports |= port_mask; + break; + case INTEL_OUTPUT_DP_MST: + used_mst_ports |= + 1 << enc_to_mst(&encoder->base)->primary->port; + break; default: break; } } + /* can't mix MST and SST/HDMI on the same port */ + if (used_ports & used_mst_ports) + return false; + return true; } -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
