[PATCH v2] CHROMIUM: i915: Added default LVDS options for the no-VBT case

[PATCH v2] CHROMIUM: i915: Added default LVDS options for the no-VBT case

[Simon Que]

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 <sque@chromium.org>

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;
+}
+
 /**
  * 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;
 	}

-- 
1.7.1

Re: [PATCH v2] CHROMIUM: i915: Added default LVDS options for the no-VBT case

[Chris Wilson]

On Mon, 27 Sep 2010 14:06:17 -0700, Simon Que <sque@chromium.org> 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 <sque@chromium.org>
> 
> 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

Re: [PATCH v2] CHROMIUM: i915: Added default LVDS options for the no-VBT case

[Chris Wilson]

On Tue, 28 Sep 2010 11:53:51 -0700, Simon Que <sque@google.com> wrote:
> This function will be called before checking for VBT (ie for all
> cases, as you suggested).  Does that sound reasonable?

Looks good so far. :) Both ways have their drawbacks, and it won't be
until we have many more settings parsed from the VBT that we'll see
which is more unwieldy.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre