From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH] drm/i915: simplify possible_clones computation Date: Thu, 12 Jul 2012 16:48:13 -0300 Message-ID: References: <20120712184733.GM5039@phenom.ffwll.local> <1342116498-12955-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-gh0-f177.google.com (mail-gh0-f177.google.com [209.85.160.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 5C0E6A0F84 for ; Thu, 12 Jul 2012 12:48:14 -0700 (PDT) Received: by ghbf11 with SMTP id f11so3088250ghb.36 for ; Thu, 12 Jul 2012 12:48:13 -0700 (PDT) In-Reply-To: <1342116498-12955-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org 2012/7/12 Daniel Vetter : > Intel hw only has one MUX for encoders, so outputs are either not > cloneable or all in the same group of cloneable outputs. This neatly > simplifies the code and allows us to ditch some ugly if cascades in > the dp and hdmi init code (well, we need these if cascades for other > stuff still, but that can be taken care of in follow-up patches). > > Note that this changes two things: > - dvo can now be cloned with sdvo, but dvo is gen2 whereas sdvo is > gen3+, so no problem. Note that the old code had a bug and didn't > allow cloning crt with dvo (but only the other way round). > - sdvo-lvds can now be cloned with sdvo-non-tv. Spec says this won't > work, but the only reason I've found is that you can't use the > panel-fitter (used for lvds upscaling) with anything else. But we > don't use the panel fitter for sdvo-lvds. Imo this part of Bspec is > a) rather confusing b) mostly as a guideline to implementors (i.e. > explicitly stating what is already implicit from the spec, without > always going into the details of why). So I think we can ignore this > - worst case we'll get a bug report from a user with with sdvo-lvds > and sdvo-tmds and have to add that special case back in. > > Because sdvo lvds is a bit special explain in comments why sdvo LVDS > outputs can be cloned, but native LVDS and eDP can't be cloned - we > use the panel fitter for the later, but not for sdvo. > > Note that this also uncoditionally initializes the panel_vdd work used > by eDP. Trying to be clever doesn't buy us anything (but strange bugs) > and this way we can kill the is_edp check. > > v2: Incorporate review from Paulo > - Add in a missing space. > - Pimp comment message to address his concerns. > > Signed-Off-by: Daniel Vetter Reviewed-by: Paulo Zanoni If you try to commit this to dinq you'll get a conflict on intel_drv.h. This is easy to fix, so you can keep the R-B after that :) > --- > drivers/gpu/drm/i915/intel_crt.c | 4 +--- > drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++----- > drivers/gpu/drm/i915/intel_dp.c | 14 +++----------- > drivers/gpu/drm/i915/intel_drv.h | 25 +++++-------------------- > drivers/gpu/drm/i915/intel_dvo.c | 7 ++----- > drivers/gpu/drm/i915/intel_hdmi.c | 10 ++-------- > drivers/gpu/drm/i915/intel_lvds.c | 2 +- > drivers/gpu/drm/i915/intel_sdvo.c | 14 +++++++------- > drivers/gpu/drm/i915/intel_tv.c | 2 +- > 9 files changed, 35 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index 9525822..c3f6680 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -690,9 +690,7 @@ void intel_crt_init(struct drm_device *dev) > intel_connector_attach_encoder(intel_connector, &crt->base); > > crt->base.type = INTEL_OUTPUT_ANALOG; > - crt->base.clone_mask = (1 << INTEL_SDVO_NON_TV_CLONE_BIT | > - 1 << INTEL_ANALOG_CLONE_BIT | > - 1 << INTEL_SDVO_LVDS_CLONE_BIT); > + crt->base.cloneable = true; > if (IS_HASWELL(dev)) > crt->base.crtc_mask = (1 << 0); > else > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1a201b2..38891ff 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6716,15 +6716,23 @@ int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, > return 0; > } > > -static int intel_encoder_clones(struct drm_device *dev, int type_mask) > +static int intel_encoder_clones(struct intel_encoder *encoder) > { > - struct intel_encoder *encoder; > + struct drm_device *dev = encoder->base.dev; > + struct intel_encoder *source_encoder; > int index_mask = 0; > int entry = 0; > > - list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) { > - if (type_mask & encoder->clone_mask) > + list_for_each_entry(source_encoder, > + &dev->mode_config.encoder_list, base.head) { > + > + if (encoder == source_encoder) > index_mask |= (1 << entry); > + > + /* Intel hw has only one MUX where enocoders could be cloned. */ > + if (encoder->cloneable && source_encoder->cloneable) > + index_mask |= (1 << entry); > + > entry++; > } > > @@ -6883,7 +6891,7 @@ static void intel_setup_outputs(struct drm_device *dev) > list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) { > encoder->base.possible_crtcs = encoder->crtc_mask; > encoder->base.possible_clones = > - intel_encoder_clones(dev, encoder->clone_mask); > + intel_encoder_clones(encoder); > } > > /* disable all the possible outputs/crtcs before entering KMS mode */ > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 3190e9d..95b1022 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2481,18 +2481,10 @@ intel_dp_init(struct drm_device *dev, int output_reg) > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > - if (output_reg == DP_B || output_reg == PCH_DP_B) > - intel_encoder->clone_mask = (1 << INTEL_DP_B_CLONE_BIT); > - else if (output_reg == DP_C || output_reg == PCH_DP_C) > - intel_encoder->clone_mask = (1 << INTEL_DP_C_CLONE_BIT); > - else if (output_reg == DP_D || output_reg == PCH_DP_D) > - intel_encoder->clone_mask = (1 << INTEL_DP_D_CLONE_BIT); > + intel_encoder->cloneable = false; > > - if (is_edp(intel_dp)) { > - intel_encoder->clone_mask = (1 << INTEL_EDP_CLONE_BIT); > - INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > - ironlake_panel_vdd_work); > - } > + INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, > + ironlake_panel_vdd_work); > > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 16680e5..f8c1f04 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -89,25 +89,6 @@ > #define INTEL_OUTPUT_DISPLAYPORT 7 > #define INTEL_OUTPUT_EDP 8 > > -/* Intel Pipe Clone Bit */ > -#define INTEL_HDMIB_CLONE_BIT 1 > -#define INTEL_HDMIC_CLONE_BIT 2 > -#define INTEL_HDMID_CLONE_BIT 3 > -#define INTEL_HDMIE_CLONE_BIT 4 > -#define INTEL_HDMIF_CLONE_BIT 5 > -#define INTEL_SDVO_NON_TV_CLONE_BIT 6 > -#define INTEL_SDVO_TV_CLONE_BIT 7 > -#define INTEL_SDVO_LVDS_CLONE_BIT 8 > -#define INTEL_ANALOG_CLONE_BIT 9 > -#define INTEL_TV_CLONE_BIT 10 > -#define INTEL_DP_B_CLONE_BIT 11 > -#define INTEL_DP_C_CLONE_BIT 12 > -#define INTEL_DP_D_CLONE_BIT 13 > -#define INTEL_LVDS_CLONE_BIT 14 > -#define INTEL_DVO_TMDS_CLONE_BIT 15 > -#define INTEL_DVO_LVDS_CLONE_BIT 16 > -#define INTEL_EDP_CLONE_BIT 17 > - > #define INTEL_DVO_CHIP_NONE 0 > #define INTEL_DVO_CHIP_LVDS 1 > #define INTEL_DVO_CHIP_TMDS 2 > @@ -153,11 +134,15 @@ struct intel_encoder { > int type; > bool needs_tv_clock; > bool connectors_active; > + /* > + * Intel hw has only one MUX where encoders could be clone, hence a > + * simple flag is enough to compute the possible_clones mask. > + */ > + bool cloneable; > void (*hot_plug)(struct intel_encoder *); > void (*enable)(struct intel_encoder *); > void (*disable)(struct intel_encoder *); > int crtc_mask; > - int clone_mask; > }; > > struct intel_connector { > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c > index 756e977..86a3d2b 100644 > --- a/drivers/gpu/drm/i915/intel_dvo.c > +++ b/drivers/gpu/drm/i915/intel_dvo.c > @@ -424,17 +424,14 @@ void intel_dvo_init(struct drm_device *dev) > intel_encoder->crtc_mask = (1 << 0) | (1 << 1); > switch (dvo->type) { > case INTEL_DVO_CHIP_TMDS: > - intel_encoder->clone_mask = > - (1 << INTEL_DVO_TMDS_CLONE_BIT) | > - (1 << INTEL_ANALOG_CLONE_BIT); > + intel_encoder->cloneable = true; > drm_connector_init(dev, connector, > &intel_dvo_connector_funcs, > DRM_MODE_CONNECTOR_DVII); > encoder_type = DRM_MODE_ENCODER_TMDS; > break; > case INTEL_DVO_CHIP_LVDS: > - intel_encoder->clone_mask = > - (1 << INTEL_DVO_LVDS_CLONE_BIT); > + intel_encoder->cloneable = false; > drm_connector_init(dev, connector, > &intel_dvo_connector_funcs, > DRM_MODE_CONNECTOR_LVDS); > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 188399f..b01900d 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -959,42 +959,36 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg) > connector->doublescan_allowed = 0; > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > > + intel_encoder->cloneable = false; > + > /* Set up the DDC bus. */ > if (sdvox_reg == SDVOB) { > - intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPB; > dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == SDVOC) { > - intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPC; > dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == HDMIB) { > - intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPB; > dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == HDMIC) { > - intel_encoder->clone_mask = (1 << INTEL_HDMIE_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPC; > dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == HDMID) { > - intel_encoder->clone_mask = (1 << INTEL_HDMIF_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPD; > dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == DDI_BUF_CTL(PORT_B)) { > DRM_DEBUG_DRIVER("LPT: detected output on DDI B\n"); > - intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPB; > intel_hdmi->ddi_port = PORT_B; > dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == DDI_BUF_CTL(PORT_C)) { > DRM_DEBUG_DRIVER("LPT: detected output on DDI C\n"); > - intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPC; > intel_hdmi->ddi_port = PORT_C; > dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS; > } else if (sdvox_reg == DDI_BUF_CTL(PORT_D)) { > DRM_DEBUG_DRIVER("LPT: detected output on DDI D\n"); > - intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT); > intel_hdmi->ddc_bus = GMBUS_PORT_DPD; > intel_hdmi->ddi_port = PORT_D; > dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS; > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index f1d0a05..1e879bf 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -943,7 +943,7 @@ bool intel_lvds_init(struct drm_device *dev) > intel_connector_attach_encoder(intel_connector, intel_encoder); > intel_encoder->type = INTEL_OUTPUT_LVDS; > > - intel_encoder->clone_mask = (1 << INTEL_LVDS_CLONE_BIT); > + intel_encoder->cloneable = false; > if (HAS_PCH_SPLIT(dev)) > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > else if (IS_GEN4(dev)) > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index d630db8..467d92a 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -2129,8 +2129,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) > connector->connector_type = DRM_MODE_CONNECTOR_HDMIA; > intel_sdvo->is_hdmi = true; > } > - intel_sdvo->base.clone_mask = ((1 << INTEL_SDVO_NON_TV_CLONE_BIT) | > - (1 << INTEL_ANALOG_CLONE_BIT)); > + intel_sdvo->base.cloneable = true; > > intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo); > if (intel_sdvo->is_hdmi) > @@ -2161,7 +2160,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) > > intel_sdvo->is_tv = true; > intel_sdvo->base.needs_tv_clock = true; > - intel_sdvo->base.clone_mask = 1 << INTEL_SDVO_TV_CLONE_BIT; > + intel_sdvo->base.cloneable = false; > > intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo); > > @@ -2204,8 +2203,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) > intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1; > } > > - intel_sdvo->base.clone_mask = ((1 << INTEL_SDVO_NON_TV_CLONE_BIT) | > - (1 << INTEL_ANALOG_CLONE_BIT)); > + intel_sdvo->base.cloneable = true; > > intel_sdvo_connector_init(intel_sdvo_connector, > intel_sdvo); > @@ -2237,8 +2235,10 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device) > intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1; > } > > - intel_sdvo->base.clone_mask = ((1 << INTEL_ANALOG_CLONE_BIT) | > - (1 << INTEL_SDVO_LVDS_CLONE_BIT)); > + /* SDVO LVDS is cloneable because the SDVO encoder does the upscaling, > + * as opposed to native LVDS, where we upscale with the panel-fitter > + * (and hence only the native LVDS resolution could be cloned). */ > + intel_sdvo->base.cloneable = true; > > intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo); > if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector)) > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c > index a0ec1eb..ef9e7a5 100644 > --- a/drivers/gpu/drm/i915/intel_tv.c > +++ b/drivers/gpu/drm/i915/intel_tv.c > @@ -1624,7 +1624,7 @@ intel_tv_init(struct drm_device *dev) > intel_connector_attach_encoder(intel_connector, intel_encoder); > intel_encoder->type = INTEL_OUTPUT_TVOUT; > intel_encoder->crtc_mask = (1 << 0) | (1 << 1); > - intel_encoder->clone_mask = (1 << INTEL_TV_CLONE_BIT); > + intel_encoder->cloneable = false; > intel_encoder->base.possible_crtcs = ((1 << 0) | (1 << 1)); > intel_encoder->base.possible_clones = (1 << INTEL_OUTPUT_TVOUT); > intel_tv->type = DRM_MODE_CONNECTOR_Unknown; > -- > 1.7.7.6 > -- Paulo Zanoni