Re: [PATCH] drm/i915: simplify possible_clones computation

[Paulo Zanoni <przanoni@gmail.com>]

2012/7/12 Daniel Vetter <daniel.vetter@ffwll.ch>:
> 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 <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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