From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Isely 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: References: <1303022613-18414-1-git-send-email-chris@chris-wilson.co.uk> <1303022613-18414-2-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cnc.isely.net (cnc.isely.net [75.149.91.89]) by gabe.freedesktop.org (Postfix) with ESMTP id 254129E744 for ; Wed, 20 Apr 2011 12:48:37 -0700 (PDT) In-Reply-To: <1303022613-18414-2-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: Dave Airlied , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Chris: I've tested this patch and it doesn't help us here. Even if I add lvds_fixed_mode=, 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 > Cc: Oliver Seitz > Cc: Mike Isely > Cc: Dave Airlied > --- > 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