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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 12, 2012 at 03:10:21PM -0300, Paulo Zanoni wrote:
> Hi
> 
> I like your idea.
> 
> 2012/7/11 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.
> 
> 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)

dvo is gen2 only, sdvo is gen3+. For the SDVO-non-tv cloned with SDVO-lvds
I couldn't come up with any reason why cloning SDVO-lvds with SDVO-non-tv
wouldn't work ... After checking the docs the only case I've found as
justification why that would not work is that you can't use the panel
fitter together with anything else. Which explains why lvds can't be
cloned, but not why sdvo-lvds can't be cloned because we simply don't use
the panel fitter there (we use the lvds encoder to do the upscaling ...).

Imo this is a case where the spec is just guidance for implementers and in
the specific case of sdvo-lvds doesn't make much sense - I suspect that
the windows code also uses the panel fitter in some cases for sdvo-lvds
... or they're talking about enabling multiple outputs on the same sdvo
encoder, which again we don't support.

I think we can ignore this special case until someone whith sdvo-tmds +
sdvo-lvds files a bug report about broken clonig ...

All extend the commit message with this discussion.

> 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".

Yeah, will fix.
> 
> 
> 
> > 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?

I've figured 81 patches are enough already, and generally initializing
work items unconditionally is a good thing - we've recently broken resume
due to such an initialization bug.

> > 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 "=".

Will fix.

Thanks, Daniel
> 
> 
> Cheers,
> Paulo
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48