| Message ID | 1512468939-3262-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote: > crtc_mask is defined explicitly defined for a certain number of pipes per > platform. Let's generalize this in a way that crtc_mask dependens only on > the number of pipes defined in device info. > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/intel_crt.c | 9 ++++++--- > drivers/gpu/drm/i915/intel_ddi.c | 5 ++++- > drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++---- > drivers/gpu/drm/i915/intel_hdmi.c | 4 +++- > drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++----- > drivers/gpu/drm/i915/intel_sdvo.c | 5 ++++- > drivers/gpu/drm/i915/intel_tv.c | 6 +++++- > 7 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index 9f31aea..34f65b5 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private *dev_priv) > struct intel_connector *intel_connector; > i915_reg_t adpa_reg; > u32 adpa; > + enum pipe pipe; > > if (HAS_PCH_SPLIT(dev_priv)) > adpa_reg = PCH_ADPA; > @@ -950,10 +951,12 @@ void intel_crt_init(struct drm_i915_private *dev_priv) > > crt->base.type = INTEL_OUTPUT_ANALOG; > crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 << INTEL_OUTPUT_HDMI); > - if (IS_I830(dev_priv)) > + if (IS_I830(dev_priv)) { > crt->base.crtc_mask = (1 << 0); > - else > - crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > + } else { > + for_each_pipe(dev_priv, pipe) > + crt->base.crtc_mask |= (1 << pipe); > + } The only places you have to touch are DDI and MST. None of the other encoder types are relevant for new platforms at all. Looks like you actually missed MST in this patch, and it looks like the code there is just wrong even now. It should really just set 'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe specific. In fact I think what we should do is have a small function that filters out the non-existent pipes from the crtc_mask when populating encoder->possible_crtcs. And I wouldn't be opposed to s/1<<0/BIT(PIPE_A)/ etc. everywhere we populate crtc_mask (maybe even s/crtc_mask/pipe_mask/ to make it clear what we're talking about). And maybe actually get rid of crtc_mask entirely and just populate possible_crtcs directly (with the help of aforementioned filtering function). Possibly some igt would be nice to confirm that possibly_crtcs etc. don't advertize invalid crtc indices.
On Tue, 2017-12-05 at 15:59 +0200, Ville Syrjälä wrote: > On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote: > > > > crtc_mask is defined explicitly defined for a certain number of > > pipes per > > platform. Let's generalize this in a way that crtc_mask dependens > > only on > > the number of pipes defined in device info. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/intel_crt.c | 9 ++++++--- > > drivers/gpu/drm/i915/intel_ddi.c | 5 ++++- > > drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++---- > > drivers/gpu/drm/i915/intel_hdmi.c | 4 +++- > > drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++----- > > drivers/gpu/drm/i915/intel_sdvo.c | 5 ++++- > > drivers/gpu/drm/i915/intel_tv.c | 6 +++++- > > 7 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_crt.c > > b/drivers/gpu/drm/i915/intel_crt.c > > index 9f31aea..34f65b5 100644 > > --- a/drivers/gpu/drm/i915/intel_crt.c > > +++ b/drivers/gpu/drm/i915/intel_crt.c > > @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private > > *dev_priv) > > struct intel_connector *intel_connector; > > i915_reg_t adpa_reg; > > u32 adpa; > > + enum pipe pipe; > > > > if (HAS_PCH_SPLIT(dev_priv)) > > adpa_reg = PCH_ADPA; > > @@ -950,10 +951,12 @@ void intel_crt_init(struct drm_i915_private > > *dev_priv) > > > > crt->base.type = INTEL_OUTPUT_ANALOG; > > crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 << > > INTEL_OUTPUT_HDMI); > > - if (IS_I830(dev_priv)) > > + if (IS_I830(dev_priv)) { > > crt->base.crtc_mask = (1 << 0); > > - else > > - crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << > > 2); > > + } else { > > + for_each_pipe(dev_priv, pipe) > > + crt->base.crtc_mask |= (1 << pipe); > > + } > The only places you have to touch are DDI and MST. None of the other > encoder types are relevant for new platforms at all. That's true. For these newer platforms I wouldn't have needed to touch other encoder types but I decided to go that road due to consistency. I may drop these for the next iteration of the patch. > > Looks like you actually missed MST in this patch, and it looks like > the > code there is just wrong even now. It should really just set > 'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe > specific. Ouch, I completely missed the MST part. > > In fact I think what we should do is have a small function that > filters > out the non-existent pipes from the crtc_mask when populating > encoder->possible_crtcs. And I wouldn't be opposed to > s/1<<0/BIT(PIPE_A)/ > etc. everywhere we populate crtc_mask (maybe even > s/crtc_mask/pipe_mask/ > to make it clear what we're talking about). I'll test this small helper function and see what the outcome looks like. Thanks for the review! > > And maybe actually get rid of crtc_mask entirely and just populate > possible_crtcs directly (with the help of aforementioned filtering > function). > > Possibly some igt would be nice to confirm that possibly_crtcs etc. > don't advertize invalid crtc indices. >
On Tue, Dec 05, 2017 at 03:59:35PM +0200, Ville Syrjälä wrote: > On Tue, Dec 05, 2017 at 12:15:39PM +0200, Mika Kahola wrote: > > crtc_mask is defined explicitly defined for a certain number of pipes per > > platform. Let's generalize this in a way that crtc_mask dependens only on > > the number of pipes defined in device info. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > drivers/gpu/drm/i915/intel_crt.c | 9 ++++++--- > > drivers/gpu/drm/i915/intel_ddi.c | 5 ++++- > > drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++---- > > drivers/gpu/drm/i915/intel_hdmi.c | 4 +++- > > drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++----- > > drivers/gpu/drm/i915/intel_sdvo.c | 5 ++++- > > drivers/gpu/drm/i915/intel_tv.c | 6 +++++- > > 7 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > > index 9f31aea..34f65b5 100644 > > --- a/drivers/gpu/drm/i915/intel_crt.c > > +++ b/drivers/gpu/drm/i915/intel_crt.c > > @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private *dev_priv) > > struct intel_connector *intel_connector; > > i915_reg_t adpa_reg; > > u32 adpa; > > + enum pipe pipe; > > > > if (HAS_PCH_SPLIT(dev_priv)) > > adpa_reg = PCH_ADPA; > > @@ -950,10 +951,12 @@ void intel_crt_init(struct drm_i915_private *dev_priv) > > > > crt->base.type = INTEL_OUTPUT_ANALOG; > > crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 << INTEL_OUTPUT_HDMI); > > - if (IS_I830(dev_priv)) > > + if (IS_I830(dev_priv)) { > > crt->base.crtc_mask = (1 << 0); > > - else > > - crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > > + } else { > > + for_each_pipe(dev_priv, pipe) > > + crt->base.crtc_mask |= (1 << pipe); > > + } > > The only places you have to touch are DDI and MST. None of the other > encoder types are relevant for new platforms at all. > > Looks like you actually missed MST in this patch, and it looks like the > code there is just wrong even now. It should really just set > 'crtc_mask = BIT(pipe)' since the fake mst encoders are pipe specific. > > In fact I think what we should do is have a small function that filters > out the non-existent pipes from the crtc_mask when populating > encoder->possible_crtcs. And I wouldn't be opposed to s/1<<0/BIT(PIPE_A)/ > etc. everywhere we populate crtc_mask (maybe even s/crtc_mask/pipe_mask/ > to make it clear what we're talking about). > > And maybe actually get rid of crtc_mask entirely and just populate > possible_crtcs directly (with the help of aforementioned filtering > function). > > Possibly some igt would be nice to confirm that possibly_crtcs etc. > don't advertize invalid crtc indices. +1 on a generic (i.e. DRIVER_ANY) igt that validates the various possible_* masks. Lots of drivers e.g. don't set anything, hooray. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 9f31aea..34f65b5 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -903,6 +903,7 @@ void intel_crt_init(struct drm_i915_private *dev_priv) struct intel_connector *intel_connector; i915_reg_t adpa_reg; u32 adpa; + enum pipe pipe; if (HAS_PCH_SPLIT(dev_priv)) adpa_reg = PCH_ADPA; @@ -950,10 +951,12 @@ void intel_crt_init(struct drm_i915_private *dev_priv) crt->base.type = INTEL_OUTPUT_ANALOG; crt->base.cloneable = (1 << INTEL_OUTPUT_DVO) | (1 << INTEL_OUTPUT_HDMI); - if (IS_I830(dev_priv)) + if (IS_I830(dev_priv)) { crt->base.crtc_mask = (1 << 0); - else - crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); + } else { + for_each_pipe(dev_priv, pipe) + crt->base.crtc_mask |= (1 << pipe); + } if (IS_GEN2(dev_priv)) connector->interlace_allowed = 0; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index eff3b51..9320542 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2766,6 +2766,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) struct drm_encoder *encoder; bool init_hdmi, init_dp, init_lspcon = false; int max_lanes; + enum pipe pipe; if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) { switch (port) { @@ -2884,9 +2885,11 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) intel_encoder->type = INTEL_OUTPUT_DDI; intel_encoder->power_domain = intel_port_to_power_domain(port); intel_encoder->port = port; - intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); intel_encoder->cloneable = 0; + for_each_pipe(dev_priv, pipe) + intel_encoder->crtc_mask |= (1 << pipe); + intel_infoframe_init(intel_dig_port); if (init_dp) { diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 957735c..37ba90d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -6139,6 +6139,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv, struct intel_encoder *intel_encoder; struct drm_encoder *encoder; struct intel_connector *intel_connector; + enum pipe pipe; intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL); if (!intel_dig_port) @@ -6190,12 +6191,15 @@ bool intel_dp_init(struct drm_i915_private *dev_priv, intel_encoder->type = INTEL_OUTPUT_DP; intel_encoder->power_domain = intel_port_to_power_domain(port); if (IS_CHERRYVIEW(dev_priv)) { - if (port == PORT_D) + if (port == PORT_D) { intel_encoder->crtc_mask = 1 << 2; - else - intel_encoder->crtc_mask = (1 << 0) | (1 << 1); + } else { + for_each_pipe(dev_priv, pipe) + intel_encoder->crtc_mask |= (1 << pipe); + } } else { - intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); + for_each_pipe(dev_priv, pipe) + intel_encoder->crtc_mask |= (1 << pipe); } intel_encoder->cloneable = 0; intel_encoder->port = port; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index a40f35a..43584d9 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2072,6 +2072,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; struct intel_connector *intel_connector; + enum pipe pipe; intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL); if (!intel_dig_port) @@ -2128,7 +2129,8 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, else intel_encoder->crtc_mask = (1 << 0) | (1 << 1); } else { - intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); + for_each_pipe(dev_priv, pipe) + intel_encoder->crtc_mask |= (1 << pipe); } intel_encoder->cloneable = 1 << INTEL_OUTPUT_ANALOG; /* diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index ef80499..36eab86 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -945,6 +945,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) u32 lvds; u8 pin; u32 allowed_scalers; + enum pipe pipe; if (!intel_lvds_supported(dev_priv)) return; @@ -1025,13 +1026,14 @@ void intel_lvds_init(struct drm_i915_private *dev_priv) intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER; intel_encoder->port = PORT_NONE; intel_encoder->cloneable = 0; - if (HAS_PCH_SPLIT(dev_priv)) - intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); - else if (IS_GEN4(dev_priv)) + if (HAS_PCH_SPLIT(dev_priv)) { + for_each_pipe(dev_priv, pipe) + intel_encoder->crtc_mask |= (1 << pipe); + } else if (IS_GEN4(dev_priv)) { intel_encoder->crtc_mask = (1 << 0) | (1 << 1); - else + } else { intel_encoder->crtc_mask = (1 << 1); - + } drm_connector_helper_add(connector, &intel_lvds_connector_helper_funcs); connector->display_info.subpixel_order = SubPixelHorizontalRGB; connector->interlace_allowed = false; diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 2b87648..ac27fb9 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2697,7 +2697,6 @@ intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags) bytes[0], bytes[1]); return false; } - intel_sdvo->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); return true; } @@ -3018,6 +3017,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv, struct intel_encoder *intel_encoder; struct intel_sdvo *intel_sdvo; int i; + enum pipe pipe; assert_sdvo_port_valid(dev_priv, port); @@ -3065,6 +3065,9 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv, intel_encoder->get_hw_state = intel_sdvo_get_hw_state; intel_encoder->get_config = intel_sdvo_get_config; + for_each_pipe(dev_priv, pipe) + intel_sdvo->base.crtc_mask = (1 << pipe); + /* In default case sdvo lvds is false */ if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps)) goto err; diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index b3dabc2..2eb7d67 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1465,6 +1465,7 @@ intel_tv_init(struct drm_i915_private *dev_priv) const char *tv_format_names[ARRAY_SIZE(tv_modes)]; int i, initial_mode = 0; struct drm_connector_state *state; + enum pipe pipe; if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED) return; @@ -1542,7 +1543,10 @@ intel_tv_init(struct drm_i915_private *dev_priv) intel_encoder->type = INTEL_OUTPUT_TVOUT; intel_encoder->power_domain = POWER_DOMAIN_PORT_OTHER; intel_encoder->port = PORT_NONE; - intel_encoder->crtc_mask = (1 << 0) | (1 << 1); + + for_each_pipe(dev_priv, pipe) + intel_encoder->crtc_mask |= (1 << pipe); + intel_encoder->cloneable = 0; intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1)); intel_tv->type = DRM_MODE_CONNECTOR_Unknown;
crtc_mask is defined explicitly defined for a certain number of pipes per platform. Let's generalize this in a way that crtc_mask dependens only on the number of pipes defined in device info. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/intel_crt.c | 9 ++++++--- drivers/gpu/drm/i915/intel_ddi.c | 5 ++++- drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++---- drivers/gpu/drm/i915/intel_hdmi.c | 4 +++- drivers/gpu/drm/i915/intel_lvds.c | 12 +++++++----- drivers/gpu/drm/i915/intel_sdvo.c | 5 ++++- drivers/gpu/drm/i915/intel_tv.c | 6 +++++- 7 files changed, 37 insertions(+), 16 deletions(-)