Re: [Intel-gfx] [PATCH 03/12] drm/i915: Avoid mixing up SST and MST in DDI setup
On Fri, Jul 29, 2016 at 12:55:20PM +0300, Ville Syrjälä wrote: > On Fri, Jul 29, 2016 at 11:16:19AM +0200, Daniel Vetter wrote: > > On Thu, Jul 28, 2016 at 05:50:39PM +0300, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä> > > > > > The MST vs. SST selection should depend purely on the choice of the > > > connector/encoder. So don't try to determine the correct DDI mode > > > based on the intel_dp->is_mst, which simply tells us whether the sink > > > is in MST mode or not. Instead derive the information from the encoder > > > type. Since the link training code deals in non-fake encoders, we'll > > > also need to keep a second copy of that information around, which we'll > > > now designate as 'link_mst'. > > > > > > Cc: Maarten Lankhorst > > > Signed-off-by: Ville Syrjälä > > > > Hm, I think it'd be nice if we could shovel all this stuff into the crtc > > state I think, instead of noodling it out of somewhere else. > > You mean stuffing a pre-computed (partial?) register value in the > crtc state? That could work too. Yeah. But reading ahead I'm not sure where exactly we need to store that, since recovering that with your sync_state rfc might get tricky. -Daniel > > > But since we > > always update it in pre_enable hooks it should be all fine even with > > TEST_ONLY atomic. > > > > I digress, change looks good. > > > > Reviewed-by: Daniel Vetter > > > > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 21 +++-- > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > 3 files changed, 5 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > index dd1d6fe12297..3b3a0a808477 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -,7 +,6 @@ void intel_ddi_enable_transcoder_func(struct > > > drm_crtc *crtc) > > > { > > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); > > > - struct drm_encoder *encoder = _encoder->base; > > > struct drm_device *dev = crtc->dev; > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > enum pipe pipe = intel_crtc->pipe; > > > @@ -1177,29 +1176,15 @@ void intel_ddi_enable_transcoder_func(struct > > > drm_crtc *crtc) > > > temp |= TRANS_DDI_MODE_SELECT_HDMI; > > > else > > > temp |= TRANS_DDI_MODE_SELECT_DVI; > > > - > > > } else if (type == INTEL_OUTPUT_ANALOG) { > > > temp |= TRANS_DDI_MODE_SELECT_FDI; > > > temp |= (intel_crtc->config->fdi_lanes - 1) << 1; > > > - > > > } else if (type == INTEL_OUTPUT_DP || > > > type == INTEL_OUTPUT_EDP) { > > > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > - > > > - if (intel_dp->is_mst) { > > > - temp |= TRANS_DDI_MODE_SELECT_DP_MST; > > > - } else > > > - temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > > - > > > + temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > > temp |= DDI_PORT_WIDTH(intel_crtc->config->lane_count); > > > } else if (type == INTEL_OUTPUT_DP_MST) { > > > - struct intel_dp *intel_dp = _to_mst(encoder)->primary->dp; > > > - > > > - if (intel_dp->is_mst) { > > > - temp |= TRANS_DDI_MODE_SELECT_DP_MST; > > > - } else > > > - temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > > - > > > + temp |= TRANS_DDI_MODE_SELECT_DP_MST; > > > temp |= DDI_PORT_WIDTH(intel_crtc->config->lane_count); > > > } else { > > > WARN(1, "Invalid encoder type %d for pipe %c\n", > > > @@ -2105,7 +2090,7 @@ void intel_ddi_prepare_link_retrain(struct intel_dp > > > *intel_dp) > > > > > > val = DP_TP_CTL_ENABLE | > > > DP_TP_CTL_LINK_TRAIN_PAT1 | DP_TP_CTL_SCRAMBLE_DISABLE; > > > - if (intel_dp->is_mst) > > > + if (intel_dp->link_mst) > > > val |= DP_TP_CTL_MODE_MST; > > > else { > > > val |= DP_TP_CTL_MODE_SST; > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 8c38e3483989..0096c651c21f 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1651,6 +1651,7 @@ void intel_dp_set_link_params(struct intel_dp > > > *intel_dp, > > > { > > > intel_dp->link_rate = pipe_config->port_clock; > > > intel_dp->lane_count = pipe_config->lane_count; > > > + intel_dp->link_mst = intel_crtc_has_type(pipe_config, > > > INTEL_OUTPUT_DP_MST); > > > } > > > > > > static void intel_dp_prepare(struct intel_encoder *encoder) > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Avoid mixing up SST and MST in DDI setup
Op 28-07-16 om 16:50 schreef ville.syrj...@linux.intel.com: > From: Ville Syrjälä> > The MST vs. SST selection should depend purely on the choice of the > connector/encoder. So don't try to determine the correct DDI mode > based on the intel_dp->is_mst, which simply tells us whether the sink > is in MST mode or not. Instead derive the information from the encoder > type. Since the link training code deals in non-fake encoders, we'll > also need to keep a second copy of that information around, which we'll > now designate as 'link_mst'. > > Cc: Maarten Lankhorst > Signed-off-by: Ville Syrjälä 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 03/12] drm/i915: Avoid mixing up SST and MST in DDI setup
On Fri, Jul 29, 2016 at 11:16:19AM +0200, Daniel Vetter wrote: > On Thu, Jul 28, 2016 at 05:50:39PM +0300, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä> > > > The MST vs. SST selection should depend purely on the choice of the > > connector/encoder. So don't try to determine the correct DDI mode > > based on the intel_dp->is_mst, which simply tells us whether the sink > > is in MST mode or not. Instead derive the information from the encoder > > type. Since the link training code deals in non-fake encoders, we'll > > also need to keep a second copy of that information around, which we'll > > now designate as 'link_mst'. > > > > Cc: Maarten Lankhorst > > Signed-off-by: Ville Syrjälä > > Hm, I think it'd be nice if we could shovel all this stuff into the crtc > state I think, instead of noodling it out of somewhere else. You mean stuffing a pre-computed (partial?) register value in the crtc state? That could work too. > But since we > always update it in pre_enable hooks it should be all fine even with > TEST_ONLY atomic. > > I digress, change looks good. > > Reviewed-by: Daniel Vetter > > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 21 +++-- > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 3 files changed, 5 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index dd1d6fe12297..3b3a0a808477 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -,7 +,6 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc > > *crtc) > > { > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); > > - struct drm_encoder *encoder = _encoder->base; > > struct drm_device *dev = crtc->dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > enum pipe pipe = intel_crtc->pipe; > > @@ -1177,29 +1176,15 @@ void intel_ddi_enable_transcoder_func(struct > > drm_crtc *crtc) > > temp |= TRANS_DDI_MODE_SELECT_HDMI; > > else > > temp |= TRANS_DDI_MODE_SELECT_DVI; > > - > > } else if (type == INTEL_OUTPUT_ANALOG) { > > temp |= TRANS_DDI_MODE_SELECT_FDI; > > temp |= (intel_crtc->config->fdi_lanes - 1) << 1; > > - > > } else if (type == INTEL_OUTPUT_DP || > >type == INTEL_OUTPUT_EDP) { > > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > - > > - if (intel_dp->is_mst) { > > - temp |= TRANS_DDI_MODE_SELECT_DP_MST; > > - } else > > - temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > - > > + temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > temp |= DDI_PORT_WIDTH(intel_crtc->config->lane_count); > > } else if (type == INTEL_OUTPUT_DP_MST) { > > - struct intel_dp *intel_dp = _to_mst(encoder)->primary->dp; > > - > > - if (intel_dp->is_mst) { > > - temp |= TRANS_DDI_MODE_SELECT_DP_MST; > > - } else > > - temp |= TRANS_DDI_MODE_SELECT_DP_SST; > > - > > + temp |= TRANS_DDI_MODE_SELECT_DP_MST; > > temp |= DDI_PORT_WIDTH(intel_crtc->config->lane_count); > > } else { > > WARN(1, "Invalid encoder type %d for pipe %c\n", > > @@ -2105,7 +2090,7 @@ void intel_ddi_prepare_link_retrain(struct intel_dp > > *intel_dp) > > > > val = DP_TP_CTL_ENABLE | > > DP_TP_CTL_LINK_TRAIN_PAT1 | DP_TP_CTL_SCRAMBLE_DISABLE; > > - if (intel_dp->is_mst) > > + if (intel_dp->link_mst) > > val |= DP_TP_CTL_MODE_MST; > > else { > > val |= DP_TP_CTL_MODE_SST; > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 8c38e3483989..0096c651c21f 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1651,6 +1651,7 @@ void intel_dp_set_link_params(struct intel_dp > > *intel_dp, > > { > > intel_dp->link_rate = pipe_config->port_clock; > > intel_dp->lane_count = pipe_config->lane_count; > > + intel_dp->link_mst = intel_crtc_has_type(pipe_config, > > INTEL_OUTPUT_DP_MST); > > } > > > > static void intel_dp_prepare(struct intel_encoder *encoder) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index e74d851868c5..70d7f33d6747 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -852,6 +852,7 @@ struct intel_dp { > > int link_rate; > > uint8_t lane_count; > > uint8_t sink_count; > > + bool link_mst; > > bool has_audio; > > bool detect_done; > > enum hdmi_force_audio force_audio; > > -- > > 2.7.4
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Avoid mixing up SST and MST in DDI setup
On Thu, Jul 28, 2016 at 05:50:39PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > The MST vs. SST selection should depend purely on the choice of the > connector/encoder. So don't try to determine the correct DDI mode > based on the intel_dp->is_mst, which simply tells us whether the sink > is in MST mode or not. Instead derive the information from the encoder > type. Since the link training code deals in non-fake encoders, we'll > also need to keep a second copy of that information around, which we'll > now designate as 'link_mst'. > > Cc: Maarten Lankhorst > Signed-off-by: Ville Syrjälä Hm, I think it'd be nice if we could shovel all this stuff into the crtc state I think, instead of noodling it out of somewhere else. But since we always update it in pre_enable hooks it should be all fine even with TEST_ONLY atomic. I digress, change looks good. Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_ddi.c | 21 +++-- > drivers/gpu/drm/i915/intel_dp.c | 1 + > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 5 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index dd1d6fe12297..3b3a0a808477 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -,7 +,6 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc > *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); > - struct drm_encoder *encoder = _encoder->base; > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > enum pipe pipe = intel_crtc->pipe; > @@ -1177,29 +1176,15 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc > *crtc) > temp |= TRANS_DDI_MODE_SELECT_HDMI; > else > temp |= TRANS_DDI_MODE_SELECT_DVI; > - > } else if (type == INTEL_OUTPUT_ANALOG) { > temp |= TRANS_DDI_MODE_SELECT_FDI; > temp |= (intel_crtc->config->fdi_lanes - 1) << 1; > - > } else if (type == INTEL_OUTPUT_DP || > type == INTEL_OUTPUT_EDP) { > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - > - if (intel_dp->is_mst) { > - temp |= TRANS_DDI_MODE_SELECT_DP_MST; > - } else > - temp |= TRANS_DDI_MODE_SELECT_DP_SST; > - > + temp |= TRANS_DDI_MODE_SELECT_DP_SST; > temp |= DDI_PORT_WIDTH(intel_crtc->config->lane_count); > } else if (type == INTEL_OUTPUT_DP_MST) { > - struct intel_dp *intel_dp = _to_mst(encoder)->primary->dp; > - > - if (intel_dp->is_mst) { > - temp |= TRANS_DDI_MODE_SELECT_DP_MST; > - } else > - temp |= TRANS_DDI_MODE_SELECT_DP_SST; > - > + temp |= TRANS_DDI_MODE_SELECT_DP_MST; > temp |= DDI_PORT_WIDTH(intel_crtc->config->lane_count); > } else { > WARN(1, "Invalid encoder type %d for pipe %c\n", > @@ -2105,7 +2090,7 @@ void intel_ddi_prepare_link_retrain(struct intel_dp > *intel_dp) > > val = DP_TP_CTL_ENABLE | > DP_TP_CTL_LINK_TRAIN_PAT1 | DP_TP_CTL_SCRAMBLE_DISABLE; > - if (intel_dp->is_mst) > + if (intel_dp->link_mst) > val |= DP_TP_CTL_MODE_MST; > else { > val |= DP_TP_CTL_MODE_SST; > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 8c38e3483989..0096c651c21f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1651,6 +1651,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp, > { > intel_dp->link_rate = pipe_config->port_clock; > intel_dp->lane_count = pipe_config->lane_count; > + intel_dp->link_mst = intel_crtc_has_type(pipe_config, > INTEL_OUTPUT_DP_MST); > } > > static void intel_dp_prepare(struct intel_encoder *encoder) > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index e74d851868c5..70d7f33d6747 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -852,6 +852,7 @@ struct intel_dp { > int link_rate; > uint8_t lane_count; > uint8_t sink_count; > + bool link_mst; > bool has_audio; > bool detect_done; > enum hdmi_force_audio force_audio; > -- > 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