From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH v2] CHROMIUM: i915: Added default LVDS options for the no-VBT case Date: Mon, 27 Sep 2010 22:37:39 +0100 Message-ID: <89kc63$i6ldq7@fmsmga002.fm.intel.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 76C9D9E7A9 for ; Mon, 27 Sep 2010 14:37:52 -0700 (PDT) In-Reply-To: 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: Simon Que , Jesse Barnes , Eric Anholt , intel-gfx@lists.freedesktop.org Cc: Olof Johansson , Mandeep Baines , Bryan Freed List-Id: intel-gfx@lists.freedesktop.org On Mon, 27 Sep 2010 14:06:17 -0700, Simon Que wrote: > Hello, > > I've updated my patch for default LVDS options, based on feedback from > Chris Wilson. Updates that I've made: > - Removed Kconfig option that enables dither bit being set -- because > we can assume that we want dither enabled for all architectures > anyway. No need to have an option to turn it off. > - In the parse function, the dither bit is now set to 1 by default. > That will be used by the case where VBT exists but the LVDS section > does not. > > Thanks, > Simon > > =============================================================== > Subject: [PATCH] CHROMIUM: i915: Added default LVDS options for the no-VBT case > > Added a function that sets the LVDS values to default settings (currently only > dither bit) when there is no VBT (video BIOS table) found. Also updated dither > bit in parse function to be set by default. > > Signed-off-by: Simon Que > > BUG=none > TEST=Splash screen looks dithered upon boot. > > Change-Id: If19c763824ee938ad107f655d8d94c65e39cfa56 > --- > drivers/gpu/drm/i915/intel_bios.c | 10 +++++++++- > 1 files changed, 9 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_bios.c > b/drivers/gpu/drm/i915/intel_bios.c > index 70c9d4b..6cb872c 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -120,7 +120,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv, > struct drm_display_mode *temp_mode; > > /* Defaults if we can't find VBT info */ > - dev_priv->lvds_dither = 0; > + dev_priv->lvds_dither = 1; > dev_priv->lvds_vbt = 0; > > lvds_options = find_section(bdb, BDB_LVDS_OPTIONS); > @@ -501,6 +501,13 @@ parse_device_mapping(struct drm_i915_private *dev_priv, > } > return; > } > + > +static void > +get_no_vbt_default_settings(struct drm_i915_private *dev_priv) > +{ > + dev_priv->lvds_dither = 1; > +} So we still end up doing this initialisation twice and confuse the issue with less-than-clear naming. init_vbt_defaults(dev_priv) ? And then we could move *all* the initialisation from the separate routines there. The counter-argument is that we then set the values in two different places and so it becomes easy to loose track of when we need to update them. Perhaps: static void init_lfp_panel_data(dev_priv) { dev_priv->lvds_dither = 1; ... } static void parse_lfp_panel_data(dev_priv, bios) { ... } static void init_vbt_defaults(dev_priv) { init_lfp_panel_data(dev_priv); ... } > + > /** > * intel_init_bios - initialize VBIOS settings & find VBT > * @dev: DRM device > @@ -541,6 +548,7 @@ intel_init_bios(struct drm_device *dev) > if (!vbt) { > DRM_ERROR("VBT signature missing\n"); > pci_unmap_rom(pdev, bios); > + get_no_vbt_default_settings(dev_priv); > return -1; > } I'd prefer: init_vbt_defaults(dev_priv) if (!vbt) { ... return -1; } That way we can do all the initialisation upfront and know that the values are good whether or not the VBT is complete. -Chris -- Chris Wilson, Intel Open Source Technology Centre