[Intel-gfx] [PATCH 19/81] drm/i915: simplify possible_clones computation

[Paulo Zanoni]

Hi

I like your idea.

2012/7/11 Daniel Vetter <daniel.vetter at 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.

I don't agree with this sentence... Our documentation contains
sections called "Simultaneous Display Capabilities on a Single Display
Pipe/Transcoder" describing the details and what/how/who/when cloning
is possible.

So in our code, before your patch, these were valid:
- CRT could be cloned with CRT, SDVO-non-tv, SDVO-lvds
- DVO-tmds could be cloned with CRT, DVO-tmds  (notice that even
though DVO-tmds can be cloned with CRT, CRT can't be cloned with
DVO-tmds! bug!)
- SDVO-non-tv could be cloned with CRT and SDVO-non-tv
- SDVO-lvds could be cloned with CRT, SDVO-lvds

After your patch, all these can be cloned with each other:
- CRT, DVO-tmds, SDVO-non-tv, SDVO-lvds

Things that were previously not possible and now are possible:

- SDVO-non-tv with SDVO-lvds
- DVO-tmds with SDVO-non-tv (does hardware with both DVO and SDVO exist?)
- DVO-tmds with SDVO-lvds (same question)

Maybe we should find a way to move all this code to inside
intel_encoder_clones directly? Or keep your patch just like it is, but
add a check inside intel_encoder_clones preventing SDVO-non-tv with
SDVO-lvds?


> 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).
>
> Also explain why sdvo LVDS outputs cannot be cloned: Native LVDS (and

I think you mean "sdvo LVDS can be cloned".



> 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);


You removed the "if" statement... Is this correct? Even if it is
correct, move to a separate patch?


> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index d630db8..5fe044c 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;


Please add a white space between "cloneable" and "=".


Cheers,
Paulo

-- 
Paulo Zanoni