From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: Re: [PATCH 19/81] drm/i915: simplify possible_clones computation Date: Thu, 12 Jul 2012 15:10:21 -0300 Message-ID: References: <1342016944-23395-1-git-send-email-daniel.vetter@ffwll.ch> <1342016944-23395-20-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 7581E9E916 for ; Thu, 12 Jul 2012 11:10:22 -0700 (PDT) Received: by ghbf11 with SMTP id f11so2964831ghb.36 for ; Thu, 12 Jul 2012 11:10:22 -0700 (PDT) In-Reply-To: <1342016944-23395-20-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 Hi I like your idea. 2012/7/11 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. 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