| Message ID | 1303022613-18414-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris: Can you specify the commit this patch was built on top of? I'm still not totally fluent in git. I did get it to apply cleanly but my build attempt failed due to errors which suggest stuff is needed from other commits missing from my build branch :-( For example, "drm_mode_parse_command_line_for_connector()" is missing. -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; > + } > } > } > >
Never mind. As usual the answer is right in front of my nose - the other patch in this series. I was only focusing on the second patch not the first one. D'Oh! -Mike On Mon, 18 Apr 2011, Mike Isely wrote: > > Chris: > > Can you specify the commit this patch was built on top of? I'm still > not totally fluent in git. I did get it to apply cleanly but my build > attempt failed due to errors which suggest stuff is needed from other > commits missing from my build branch :-( For example, > "drm_mode_parse_command_line_for_connector()" is missing. > > -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; > > + } > > } > > } > > > > > >
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; > + } > } > } > >
On Wed, 20 Apr 2011 14:48:36 -0500 (CDT), Mike Isely <isely@isely.net> wrote: > > 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. You can test the functionality of the patch by parsing i915.lvds_fixed_mode first rather than last. Then I just have to weigh up the wishes of Dave who has hordes of users desperate to fry their hardware, versus the tiny number who truly need an override and know what they are doing... -Chris
On Wed, 20 Apr 2011, Chris Wilson wrote: > On Wed, 20 Apr 2011 14:48:36 -0500 (CDT), Mike Isely <isely@isely.net> wrote: > > > > 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. > > You can test the functionality of the patch by parsing > i915.lvds_fixed_mode first rather than last. I will test for that - it was the next thing I was going to dig into. > > Then I just have to weigh up the wishes of Dave who has hordes of users > desperate to fry their hardware, versus the tiny number who truly need > an override and know what they are doing... I understand your sentiment here. But please also consider that this same feature existed on the UMS side for 3 years and I don't remember hearing about any flood of fried hardware because of it... And really, this should be all about making legitimate capabilities available, not deliberately blocking them. A good compromise, if this is really viewed as a legitimate problem (which I don't think it should be), would be to add an "I know what I'm doing, darnit" switch to the driver which would enable potentially hazardous tweaks. Then one can hang all the "here there be dragons", "do not enter", "don't blame me for frying your hardware", etc caveats and warnings onto that switch... -Mike
On Wed, 20 Apr 2011, Mike Isely wrote: > On Wed, 20 Apr 2011, Chris Wilson wrote: > > > On Wed, 20 Apr 2011 14:48:36 -0500 (CDT), Mike Isely <isely@isely.net> wrote: > > > > > > 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. > > > > You can test the functionality of the patch by parsing > > i915.lvds_fixed_mode first rather than last. > > I will test for that - it was the next thing I was going to dig into. Just tested this. However the results were not conclusive. It looks like the driver did notice the specified mode and used it - because the display's refresh rate did get adjusted and the overall resolution is correct. However the display timings are slightly "off", the image is stretched a few pixels off the right edge and a few lines off the bottom edge. Yet the same modeline given to "video=" in combination with my controversial disable-fixed-mode patch however produces a perfectly aligned image. There might some other affect here. I'm not testing in the same git repo in the two cases and there are other differences. So this problem might be unrelated to the patch. I will sort that out. -Mike
On Thu, 21 Apr 2011 17:30:21 -0500 (CDT), Mike Isely <isely@isely.net> wrote: > It looks like the driver did notice the specified mode and used it - > because the display's refresh rate did get adjusted and the overall > resolution is correct. However the display timings are slightly "off", > the image is stretched a few pixels off the right edge and a few lines > off the bottom edge. Yet the same modeline given to "video=" in > combination with my controversial disable-fixed-mode patch however > produces a perfectly aligned image. Adding a few printk will help. And perhaps comparing the resultant register settings with intel_reg_dumper. -Chris
On Thu, 21 Apr 2011, Chris Wilson wrote: > On Thu, 21 Apr 2011 17:30:21 -0500 (CDT), Mike Isely <isely@isely.net> wrote: > > It looks like the driver did notice the specified mode and used it - > > because the display's refresh rate did get adjusted and the overall > > resolution is correct. However the display timings are slightly "off", > > the image is stretched a few pixels off the right edge and a few lines > > off the bottom edge. Yet the same modeline given to "video=" in > > combination with my controversial disable-fixed-mode patch however > > produces a perfectly aligned image. > > Adding a few printk will help. And perhaps comparing the resultant register > settings with intel_reg_dumper. Oh absolutely. But right now I'm not doing a very fair apples-to-apples comparison. I need to address that first and make sure nothing else is influencing this. -Mike
On Thu, 21 Apr 2011, Mike Isely wrote: > On Thu, 21 Apr 2011, Chris Wilson wrote: > > > On Thu, 21 Apr 2011 17:30:21 -0500 (CDT), Mike Isely <isely@isely.net> wrote: > > > It looks like the driver did notice the specified mode and used it - > > > because the display's refresh rate did get adjusted and the overall > > > resolution is correct. However the display timings are slightly "off", > > > the image is stretched a few pixels off the right edge and a few lines > > > off the bottom edge. Yet the same modeline given to "video=" in > > > combination with my controversial disable-fixed-mode patch however > > > produces a perfectly aligned image. > > > > Adding a few printk will help. And perhaps comparing the resultant register > > settings with intel_reg_dumper. > > Oh absolutely. But right now I'm not doing a very fair apples-to-apples > comparison. I need to address that first and make sure nothing else is > influencing this. > > -Mike > Chris: Sorry this simple thing has taken 2 weeks, but I'm having to time-slice a lot of different tasks at the moment. People keep pulling me away from this :-( However I've finally dug down and found the problem. There's a bug in the function drm_mode_parse_command_line_for_connector (the subject of this thread) that is leaving the margins field uninitialized. This has been the cause all along for the differing results I was getting. This bug didn't start with the underlying patch ("Use i915.lvds_fixed_mode= as a last resort"). I also looked at drm_fb_helper_connector_parse_command_line from before this patch and it looks like the same bug exists there. Amazing that nobody has noticed this. Patch to follow. -Mike
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; + } } }
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(-)