Intel-GFX Archive on lore.kernel.org
 help / color / Atom feed
From: Mike Isely <isely@isely.net>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlied <airlied@linux.ie>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort
Date: Wed, 20 Apr 2011 14:48:36 -0500 (CDT)
Message-ID: <alpine.DEB.1.10.1104201419170.31843@cnc.isely.net> (raw)
In-Reply-To: <1303022613-18414-2-git-send-email-chris@chris-wilson.co.uk>


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@chris-wilson.co.uk>
> Cc: Oliver Seitz <info@vtnd.de>
> Cc: Mike Isely <isely@isely.net>
> Cc: Dave Airlied <airlied@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

  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-17  6:43 [PATCH 1/2] drm: Export the command-line mode parser Chris Wilson
2011-04-17  6:43 ` [PATCH 2/2] drm/i915/lvds: Use i915.lvds_fixed_mode= as a last resort Chris Wilson
2011-04-18 16:46   ` Mike Isely
2011-04-18 17:46     ` Mike Isely
2011-04-20 19:48   ` Mike Isely [this message]
2011-04-20 19:56     ` Chris Wilson
2011-04-20 20:09       ` Mike Isely
2011-04-21 22:30         ` Mike Isely
2011-04-21 22:37           ` Chris Wilson
2011-04-21 22:46             ` Mike Isely
2011-05-04 22:14               ` Mike Isely
2011-05-04 22:18                 ` [PATCH] drm/i915: Fix unset margins flag in drm_mode_parse_command_line_for_connector Mike Isely
2011-05-05  2:40                   ` Robert Lowery

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.1.10.1104201419170.31843@cnc.isely.net \
    --to=isely@isely.net \
    --cc=airlied@linux.ie \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Intel-GFX Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/intel-gfx/0 intel-gfx/git/0.git
	git clone --mirror https://lore.kernel.org/intel-gfx/1 intel-gfx/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 intel-gfx intel-gfx/ https://lore.kernel.org/intel-gfx \
		intel-gfx@lists.freedesktop.org
	public-inbox-index intel-gfx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.intel-gfx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git