CHROMIUM: i915: Added default LVDS options for the no-VBT case (issue3415020)

[sq...@chromium.org]

Reviewers: Olof Johansson, bfreed, Mandeep Singh Baines,

Description:
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.

Signed-off-by: Simon Que <sq...@chromium.org>

BUG=none
TEST=Splash screen looks dithered upon boot.

Change-Id: If19c763824ee938ad107f655d8d94c65e39cfa56

Merge remote branch 'cros/master' into dither_branch

Check in Kconfig

Change-Id: Ib32a17f897fbf84c0955eab3c53dba46e4fd0c37

Please review this at http://codereview.chromium.org/3415020/show

SVN Base: http://git.chromium.org/git/kernel.git

Affected files:
M drivers/gpu/drm/Kconfig
M drivers/gpu/drm/i915/intel_bios.c

Index: drivers/gpu/drm/Kconfig
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index
14e21c37c01118907cfde0279dc0b3b296341c5a..94979a52f1c2420a5bf7788ab82638e3dc05b014
100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -136,6 +136,15 @@ config DRM_I915_DIRECT_BACKLIGHT
Direct backlight control gives finer granularity (0-256) than ACPI
and does not require BIOS support.

+config DRM_I915_ENABLE_NO_VBT_DEFAULTS
+ bool "Set default BIOS settings when VBT is not found"
+ depends on DRM_I915
+ help
+ Choose this option if you want the i915 driver to set the device
+ settings to a set of defaults in the event that VBT is not found.
+ This will happen on systems with firmware that does not initialize
+ video.
+
endchoice

config DRM_MGA
Index: drivers/gpu/drm/i915/intel_bios.c
diff --git a/drivers/gpu/drm/i915/intel_bios.c
b/drivers/gpu/drm/i915/intel_bios.c
index
70c9d4ba7042b57dc677a9e2fcbcf03a6abe5e4e..ad76f30041ea1f0886904bbf88f779bbdeec8c78
100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -501,6 +501,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
}
return;
}
+
+static void
+get_no_vbt_default_settings(struct drm_i915_private *dev_priv)
+{
+#if CONFIG_DRM_I915_ENABLE_NO_VBT_DEFAULTS
+ dev_priv->lvds_dither = 1;
+#endif
+}
+
/**
* intel_init_bios - initialize VBIOS settings & find VBT
* @dev: DRM device
@@ -541,6 +550,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;
}

[ol...@chromium.org]

Nice! This has bugged a lot of people, excellent to see a proposed fix.

I'm not sure a kernel config option is necessary in this case though, it's
a bit
of an overkill.

If you move the setting of defaults from down in parse_lfp_panel_data() to
up
before the VBT check in intel_bios_init(), there's only one place to
check/change.

Upstream maintainers might prefer the change to be something different, for
example a sysctl/module parameter. So please post the patch for upstream
inclusion for them to review/comment on.

-Olof

http://codereview.chromium.org/3415020/show

[sq...@chromium.org]

We want to have two separate levels of default settings for the video
driver:
1. If the entire VBT were missing, we would want to put in default values
from
intel_bios_init() after the VBT check. This is what my changes is doing.
2. If the VBT is present but a particular section of it is missing (what the
parse functions scan for), then we want each parse function to fill in the
default values for that section. This is what the existing code does.

We would like to keep these two separate. If we follow your proposal, we
would
override the latter, and the code would be less safe.

Simon

[ol...@chromium.org]

On 2010/09/22 23:22:00, Simon Que wrote:
> We want to have two separate levels of default settings for the video
> driver:
> 1. If the entire VBT were missing, we would want to put in default values
> from
> intel_bios_init() after the VBT check. This is what my changes is doing.
> 2. If the VBT is present but a particular section of it is missing (what
> the
> parse functions scan for), then we want each parse function to fill in the
> default values for that section. This is what the existing code does.

> We would like to keep these two separate. If we follow your proposal, we
would
> override the latter, and the code would be less safe.

Just post it for upstream then, and let the upstream maintainer decide if it
warrants a kconfig option. There's little point in arguing here just to
redo it
for upstream.

-Olof

http://codereview.chromium.org/3415020/show

[sq...@chromium.org]

http://codereview.chromium.org/3415020/show