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

[Chris Wilson <chris@chris-wilson.co.uk>]

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