[Intel-gfx] [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort
Mike Isely
isely at isely.net
Wed Apr 20 21:48:36 CEST 2011
Chris:
I've tested this patch and it doesn't help us here. Even if I add
lvds_fixed_mode=<whatever>, the display still comes up with the messed
up configuration from the motherboard's completely ignorant BIOS. It
appears that lvds_fixed_mode is being ignored by the driver.
I think there's still a misunderstanding of the situation. This can't
be treated as a "last resort"; it really should be treated as "oh look
the user is telling us what to do, gee maybe it's because the detected
settings are actually wrong". From the patch comment:
> If we can find no other reliable source of panel configuration data,
^^^^^^^^
> turn to the user and see if they have a passed a mode line (ala video=)
> through the i915.lvds_fixed_mode= string.
>
Note the word "reliable". Is this patch assuming that if there's
configuration data in the BIOS that it is considered to be "reliable"?
See that's the entire point - if the LVDS display device is discrete,
i.e. a separate component from the motherboard, then the BIOS is
unrelated to the display device so how in the world can it possibly be
relied upon to provide good configuration data? In a laptop with an
integral display, sure no problem. But here in the embedded world with
COTS parts, we're combining an SBC with a GMA-4500 from one vendor, with
its canned BIOS, with a special-purpose LVDS-driven display device from
another vendor. The display device has non-standard unusual timing
requirements that the BIOS will never understand. We however understand
exactly what those timing requirements are but there's no way to tell
this to hardware if the driving software insists on preferring the crap
from the BIOS from the SBC vendor who simply has no clue about what
we're connecting to the SBC's LVDS port.
The patch I had posted that implemented the lvds_fixed switch to disable
scaling solves the problem for us because the switch in combination with
the specified mode allows us to force the driver to ignore the BIOS.
This is also what the UMS patch (xorg intel driver) did that I
implemented back in 2008. The lvds_fixed patch I had submitted in this
sense really only restored functionality that had already been
previously available with UMS.
But if this lvds_fixed_mode patch is still allowing the driver to ignore
the user-specified actual lvds_fixed_mode setting in favor of the wrong
BIOS set-up timings, then we're no better off than before.
Another comment from the patch:
+ *
+ * Finally, we allow the user to specify his own mode. We do this
+ * last because we want to prevent the user from damaging their
+ * hardware with a dangerous modeline. Though we may eventually
+ * be forced to add an override for truly broken machines.
As I said, the problem here is that the display device is not integral
to the system, and the BIOS simply cannot have any foreknowledge of what
the display device wants. This is not "broken" behavior, it's a fact of
life when the COTS SBC vendor and the display vendor are different
entities.
Do you really think it's wise to ignore the user when he went out of his
way to specify these timings? Sure, print a warning, make it obscure,
add lots of caveats. But the fact is really if the user went to the
trouble to figure out how to specify video timings to a kernel module,
then there's a pretty good chance that he knows what he's doing.
Trying to be "smarter" than the user here I really think is wrong.
Until this patch can actually override the BIOS, it isn't a functional
replacement for the lvds_fixed patch I had posted earlier. Sorry :-(
Maybe there should also be an "ignore preset LVDS configuration" switch
added with a comment visible from modinfo to the effect of "use at your
own risk since this might damage hardware"?
-Mike
On Sun, 17 Apr 2011, Chris Wilson wrote:
> If we can find no other reliable source of panel configuration data,
> turn to the user and see if they have a passed a mode line (ala video=)
> through the i915.lvds_fixed_mode= string.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Oliver Seitz <info at vtnd.de>
> Cc: Mike Isely <isely at isely.net>
> Cc: Dave Airlied <airlied at linux.ie>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 4 +++
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_lvds.c | 46 ++++++++++++++++++++++++++++---------
> 3 files changed, 41 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 16a2532..4a618f6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -67,6 +67,10 @@ module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
> static bool i915_try_reset = true;
> module_param_named(reset, i915_try_reset, bool, 0600);
>
> +char *i915_lvds_fixed_mode;
> +module_param_named(lvds_fixed_mode, i915_lvds_fixed_mode, charp, 0600);
> +MODULE_PARM_DESC(lvds_fixed_mode, "specify the mode line for the LVDS panel to be used in the absence of any configuration data, using the video= format");
> +
> unsigned int i915_lvds_24bit = 0;
> module_param_named(lvds_24bit, i915_lvds_24bit, int, 0600);
> MODULE_PARM_DESC(lvds_24bit, "LVDS 24 bit pixel format: 0=leave untouched (default), 1=24 bit '2.0' format, 2=24 bit '2.1' format, 3=force older 18 bit format");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2112af3..c6cc4e2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -977,12 +977,13 @@ extern unsigned int i915_fbpercrtc;
> extern int i915_panel_ignore_lid;
> extern unsigned int i915_powersave;
> extern unsigned int i915_semaphores;
> +extern char *i915_lvds_fixed_mode;
> extern unsigned int i915_lvds_channels;
> +extern unsigned int i915_lvds_24bit;
> extern unsigned int i915_lvds_downclock;
> extern unsigned int i915_panel_use_ssc;
> extern int i915_vbt_sdvo_panel_type;
> extern unsigned int i915_enable_rc6;
> -extern unsigned int i915_lvds_24bit;
>
> extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> extern int i915_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index a562bd2..32b86ea 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -924,6 +924,11 @@ bool intel_lvds_init(struct drm_device *dev)
> * if none of the above, no panel
> * 4) make sure lid is open
> * if closed, act like it's not there for now
> + *
> + * Finally, we allow the user to specify his own mode. We do this
> + * last because we want to prevent the user from damaging their
> + * hardware with a dangerous modeline. Though we may eventually
> + * be forced to add an override for truly broken machines.
> */
>
> /*
> @@ -982,19 +987,38 @@ bool intel_lvds_init(struct drm_device *dev)
> */
>
> /* Ironlake: FIXME if still fail, not try pipe mode now */
> - if (HAS_PCH_SPLIT(dev))
> - goto failed;
> + if (!HAS_PCH_SPLIT(dev)) {
> + lvds = I915_READ(LVDS);
> + pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> + crtc = intel_get_crtc_for_pipe(dev, pipe);
> +
> + if (crtc && (lvds & LVDS_PORT_EN)) {
> + intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> + if (intel_lvds->fixed_mode) {
> + intel_lvds->fixed_mode->type |=
> + DRM_MODE_TYPE_PREFERRED;
> + goto out;
> + }
> + }
> + }
>
> - lvds = I915_READ(LVDS);
> - pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> - crtc = intel_get_crtc_for_pipe(dev, pipe);
> + /* No panel cnnfiguration data, and nothing already driving the panel
> + * at its preferred mode. Check to see if the user provided this vital
> + * bit of information...
> + */
> + if (i915_lvds_fixed_mode) {
> + struct drm_cmdline_mode mode;
>
> - if (crtc && (lvds & LVDS_PORT_EN)) {
> - intel_lvds->fixed_mode = intel_crtc_mode_get(dev, crtc);
> - if (intel_lvds->fixed_mode) {
> - intel_lvds->fixed_mode->type |=
> - DRM_MODE_TYPE_PREFERRED;
> - goto out;
> + if (drm_mode_parse_command_line_for_connector(i915_lvds_fixed_mode,
> + connector,
> + &mode)) {
> + intel_lvds->fixed_mode =
> + drm_mode_create_from_cmdline_mode(dev, &mode);
> + if (intel_lvds->fixed_mode) {
> + intel_lvds->fixed_mode->type |=
> + DRM_MODE_TYPE_PREFERRED;
> + goto out;
> + }
> }
> }
>
>
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
More information about the Intel-gfx
mailing list