From: Arjan v. de V. <ar...@in...> - 2009-03-23 20:47:43
|
From dfe256fc9d28f240f1a84fb4c3dfed4a4e18d5d4 Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <ar...@li...> Date: Mon, 23 Mar 2009 13:32:47 -0700 Subject: [PATCH] KMS: Do the actual mode setting from asynchronous context The mode setting (and the expensive part of mode detection) can take quite a bit of time, and will delay the kernel boot quite a bit. This patch puts these two parts into async context, so that the normal execution of the system continues while the slow hardware probing and poking happens. Signed-off-by: Arjan van de Ven <ar...@li...> --- drivers/gpu/drm/drm_crtc_helper.c | 50 ++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/drm_drv.c | 4 +++ include/drm/drmP.h | 1 + 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 1c3a8c5..7aa3107 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -29,6 +29,8 @@ * Jesse Barnes <jes...@in...> */ +#include <linux/async.h> + #include "drmP.h" #include "drm_crtc.h" #include "drm_crtc_helper.h" @@ -42,6 +44,8 @@ static struct drm_display_mode std_modes[] = { DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC) }, }; +LIST_HEAD(drm_async_list); + /** * drm_helper_probe_connector_modes - get complete set of display modes * @dev: DRM device @@ -137,6 +141,26 @@ int drm_helper_probe_connector_modes(struct drm_device *dev, uint32_t maxX, } EXPORT_SYMBOL(drm_helper_probe_connector_modes); +int drm_helper_probe_connector_modes_fast(struct drm_device *dev, uint32_t maxX, + uint32_t maxY) +{ + struct drm_connector *connector; + int count = 0; + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + count += drm_helper_probe_single_connector_modes(connector, + maxX, maxY); + /* + * If we found a 'good' connector, we stop probing futher. + */ + if (count > 0) + break; + } + + return count; +} +EXPORT_SYMBOL(drm_helper_probe_connector_modes_fast); + static void drm_helper_add_std_modes(struct drm_device *dev, struct drm_connector *connector) { @@ -882,6 +906,24 @@ bool drm_helper_plugged_event(struct drm_device *dev) /* FIXME: send hotplug event */ return true; } + +static void async_notify_fb_changed(void *data, async_cookie_t cookie) +{ + struct drm_device *dev = data; + /* Need to wait for async_probe_hard be done */ + async_synchronize_cookie_domain(cookie, &drm_async_list); + dev->mode_config.funcs->fb_changed(dev); +} + +static void async_probe_hard(void *data, async_cookie_t cookie) +{ + struct drm_device *dev = data; + drm_helper_probe_connector_modes(dev, + dev->mode_config.max_width, + dev->mode_config.max_height); +} + + /** * drm_initial_config - setup a sane initial connector configuration * @dev: DRM device @@ -902,7 +944,7 @@ bool drm_helper_initial_config(struct drm_device *dev, bool can_grow) struct drm_connector *connector; int count = 0; - count = drm_helper_probe_connector_modes(dev, + count = drm_helper_probe_connector_modes_fast(dev, dev->mode_config.max_width, dev->mode_config.max_height); @@ -920,8 +962,10 @@ bool drm_helper_initial_config(struct drm_device *dev, bool can_grow) drm_setup_crtcs(dev); - /* alert the driver fb layer */ - dev->mode_config.funcs->fb_changed(dev); + /* probe further outputs asynchronously */ + async_schedule_domain(async_probe_hard, dev, &drm_async_list); + /* alert the driver fb layer to actually change the mode */ + async_schedule_domain(async_notify_fb_changed, dev, &drm_async_list); return 0; } diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 14c7a23..ef52021 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -48,6 +48,7 @@ #include "drmP.h" #include "drm_core.h" +#include <linux/async.h> static int drm_version(struct drm_device *dev, void *data, struct drm_file *file_priv); @@ -345,6 +346,9 @@ void drm_exit(struct drm_driver *driver) struct drm_device *dev, *tmp; DRM_DEBUG("\n"); + /* make sure all async DRM operations are finished */ + async_synchronize_full_domain(&drm_async_list); + list_for_each_entry_safe(dev, tmp, &driver->device_list, driver_item) drm_cleanup(dev); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index e5f4ae9..69ce4f4 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -304,6 +304,7 @@ struct drm_vma_entry { pid_t pid; }; +extern struct list_head drm_async_list; /** * DMA buffer. */ -- 1.6.0.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Arjan v. de V. <ar...@in...> - 2009-03-23 20:47:43
|
From eb512d6d953c8acc8abe1048862a92cec58d9791 Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <ar...@li...> Date: Mon, 23 Mar 2009 13:38:49 -0700 Subject: [PATCH] KMS: cache the EDID information of the LVDS you're not going to replace your LVDS at runtime..... so this patch caches the EDID information of the LVDS. An LVDS probe can easily take 200 milliseconds, and we do this multiple times during a system startup. Signed-off-by: Arjan van de Ven <ar...@li...> --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_lvds.c | 2 ++ drivers/gpu/drm/i915/intel_modes.c | 8 +++++++- 3 files changed, 10 insertions(+), 1 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 957daef..22a74bd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -81,6 +81,7 @@ struct intel_output { int type; struct intel_i2c_chan *i2c_bus; /* for control functions */ struct intel_i2c_chan *ddc_bus; /* for DDC only stuff */ + struct edid *edid; bool load_detect_temp; bool needs_tv_clock; void *dev_priv; diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 0d211af..dc4fecc 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -336,6 +336,7 @@ static void intel_lvds_destroy(struct drm_connector *connector) intel_i2c_destroy(intel_output->ddc_bus); drm_sysfs_connector_remove(connector); drm_connector_cleanup(connector); + kfree(intel_output->edid); kfree(connector); } @@ -516,5 +517,6 @@ failed: if (intel_output->ddc_bus) intel_i2c_destroy(intel_output->ddc_bus); drm_connector_cleanup(connector); + kfree(intel_output->edid); kfree(connector); } diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index e42019e..83816a4 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -70,13 +70,19 @@ int intel_ddc_get_modes(struct intel_output *intel_output) struct edid *edid; int ret = 0; + if (intel_output->edid) + return ret; + edid = drm_get_edid(&intel_output->base, &intel_output->ddc_bus->adapter); if (edid) { drm_mode_connector_update_edid_property(&intel_output->base, edid); ret = drm_add_edid_modes(&intel_output->base, edid); - kfree(edid); + if (intel_output->type == INTEL_OUTPUT_LVDS) + intel_output->edid = edid; + else + kfree(edid); } return ret; -- 1.6.0.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Arjan v. de V. <ar...@in...> - 2009-03-23 20:47:46
|
From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <ar...@li...> Date: Mon, 23 Mar 2009 13:34:46 -0700 Subject: [PATCH] KMS: register the LVDS before the CRT for the cases where the user only really cares about lighting up one output, or only one output at first, lighting up the LVDS (which only gets detected with lid-up) rather than the CRT is the right answer. This patch flips their order of registration so that this becomes possible. Signed-off-by: Arjan van de Ven <ar...@li...> --- drivers/gpu/drm/i915/intel_display.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a283427..c0ab079 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1466,12 +1466,12 @@ static void intel_setup_outputs(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_connector *connector; - intel_crt_init(dev); - - /* Set up integrated LVDS */ + /* Set up integrated LVDS -- will skip if the lid is closed */ if (IS_MOBILE(dev) && !IS_I830(dev)) intel_lvds_init(dev); + intel_crt_init(dev); + if (IS_I9XX(dev)) { int found; -- 1.6.0.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Arjan v. de V. <ar...@in...> - 2009-03-23 20:47:53
|
From a121507f31e37a809dfa21c9756aa83f36d7acbf Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <ar...@li...> Date: Mon, 23 Mar 2009 13:37:20 -0700 Subject: [PATCH] KMS: do a faster EDID for some outputs (like LVDS), the current delays in the EDID reader are way overkill. This patch makes the EDID reader try twice; first with a 5x faster probe, and if that fails, it probes at the current speed. This reduces probe time on my LVDS panel significantly. Signed-off-by: Arjan van de Ven <ar...@li...> --- drivers/gpu/drm/drm_edid.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a839a28..069b189 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -588,20 +588,22 @@ static unsigned char *drm_ddc_read(struct i2c_adapter *adapter) { struct i2c_algo_bit_data *algo_data = adapter->algo_data; unsigned char *edid = NULL; + int divider = 5; int i, j; algo_data->setscl(algo_data->data, 1); - for (i = 0; i < 1; i++) { + for (i = 0; i < 2; i++) { /* For some old monitors we need the * following process to initialize/stop DDC */ + algo_data->setsda(algo_data->data, 1); - msleep(13); + msleep(13 / divider); algo_data->setscl(algo_data->data, 1); for (j = 0; j < 5; j++) { - msleep(10); + msleep(10 / divider); if (algo_data->getscl(algo_data->data)) break; } @@ -609,31 +611,33 @@ static unsigned char *drm_ddc_read(struct i2c_adapter *adapter) continue; algo_data->setsda(algo_data->data, 0); - msleep(15); + msleep(15 / divider); algo_data->setscl(algo_data->data, 0); - msleep(15); + msleep(15 / divider); algo_data->setsda(algo_data->data, 1); - msleep(15); + msleep(15 / divider); /* Do the real work */ edid = drm_do_probe_ddc_edid(adapter); algo_data->setsda(algo_data->data, 0); algo_data->setscl(algo_data->data, 0); - msleep(15); + msleep(15 / divider); algo_data->setscl(algo_data->data, 1); for (j = 0; j < 10; j++) { - msleep(10); + msleep(10 / divider); if (algo_data->getscl(algo_data->data)) break; } algo_data->setsda(algo_data->data, 1); - msleep(15); + msleep(15 / divider); algo_data->setscl(algo_data->data, 0); algo_data->setsda(algo_data->data, 0); + if (edid) break; + divider = 1; } /* Release the DDC lines when done or the Apple Cinema HD display * will switch off -- 1.6.0.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Dave A. <ai...@gm...> - 2009-04-30 23:42:02
|
On Tue, Mar 24, 2009 at 6:48 AM, Arjan van de Ven <ar...@in...> wrote: > >From a121507f31e37a809dfa21c9756aa83f36d7acbf Mon Sep 17 00:00:00 2001 > From: Arjan van de Ven <ar...@li...> > Date: Mon, 23 Mar 2009 13:37:20 -0700 > Subject: [PATCH] KMS: do a faster EDID > > for some outputs (like LVDS), the current delays in the EDID reader are way > overkill. > > This patch makes the EDID reader try twice; first with a 5x faster probe, and > if that fails, it probes at the current speed. This reduces probe time on my LVDS > panel significantly. TBH I'd do DDC the first time with none of the setup crap in it. Just call the DDC function. If that fails then I'd go through all the sleeping path, which is mainly for some bonghit monitors from the 90s. So like the idea but I'd take it further. Dave. > > Signed-off-by: Arjan van de Ven <ar...@li...> > --- > drivers/gpu/drm/drm_edid.c | 22 +++++++++++++--------- > 1 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a839a28..069b189 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -588,20 +588,22 @@ static unsigned char *drm_ddc_read(struct i2c_adapter *adapter) > { > struct i2c_algo_bit_data *algo_data = adapter->algo_data; > unsigned char *edid = NULL; > + int divider = 5; > int i, j; > > algo_data->setscl(algo_data->data, 1); > > - for (i = 0; i < 1; i++) { > + for (i = 0; i < 2; i++) { > /* For some old monitors we need the > * following process to initialize/stop DDC > */ > + > algo_data->setsda(algo_data->data, 1); > - msleep(13); > + msleep(13 / divider); > > algo_data->setscl(algo_data->data, 1); > for (j = 0; j < 5; j++) { > - msleep(10); > + msleep(10 / divider); > if (algo_data->getscl(algo_data->data)) > break; > } > @@ -609,31 +611,33 @@ static unsigned char *drm_ddc_read(struct i2c_adapter *adapter) > continue; > > algo_data->setsda(algo_data->data, 0); > - msleep(15); > + msleep(15 / divider); > algo_data->setscl(algo_data->data, 0); > - msleep(15); > + msleep(15 / divider); > algo_data->setsda(algo_data->data, 1); > - msleep(15); > + msleep(15 / divider); > > /* Do the real work */ > edid = drm_do_probe_ddc_edid(adapter); > algo_data->setsda(algo_data->data, 0); > algo_data->setscl(algo_data->data, 0); > - msleep(15); > + msleep(15 / divider); > > algo_data->setscl(algo_data->data, 1); > for (j = 0; j < 10; j++) { > - msleep(10); > + msleep(10 / divider); > if (algo_data->getscl(algo_data->data)) > break; > } > > algo_data->setsda(algo_data->data, 1); > - msleep(15); > + msleep(15 / divider); > algo_data->setscl(algo_data->data, 0); > algo_data->setsda(algo_data->data, 0); > + > if (edid) > break; > + divider = 1; > } > /* Release the DDC lines when done or the Apple Cinema HD display > * will switch off > -- > 1.6.0.6 > > > > > -- > Arjan van de Ven Intel Open Source Technology Centre > For development, discussion and tips for power savings, > visit http://www.lesswatts.org > > ------------------------------------------------------------------------------ > Apps built with the Adobe(R) Flex(R) framework and Flex Builder(TM) are > powering Web 2.0 with engaging, cross-platform capabilities. Quickly and > easily build your RIAs with Flex Builder, the Eclipse(TM)based development > software that enables intelligent coding and step-through debugging. > Download the free 60 day trial. http://p.sf.net/sfu/www-adobe-com > -- > _______________________________________________ > Dri-devel mailing list > Dri...@li... > https://lists.sourceforge.net/lists/listinfo/dri-devel > |
From: Arjan v. de V. <ar...@in...> - 2009-03-23 20:47:55
|
From 04795556b9ef5cd036a182535d04c4c853b61c96 Mon Sep 17 00:00:00 2001 From: Arjan van de Ven <ar...@li...> Date: Mon, 23 Mar 2009 13:36:25 -0700 Subject: [PATCH] KMS: clean up udelay usage udelay() of 20 milliseconds really ought to just use mdelay(), that avoids the various wrap scenarios and also is more readable Signed-off-by: Arjan van de Ven <ar...@li...> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c0ab079..6f2eced 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -319,7 +319,7 @@ void intel_wait_for_vblank(struct drm_device *dev) { /* Wait for 20ms, i.e. one cycle at 50hz. */ - udelay(20000); + mdelay(20); } static int -- 1.6.0.6 -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Jesse B. <jb...@vi...> - 2009-03-27 23:08:35
|
On Mon, 23 Mar 2009 13:47:33 -0700 Arjan van de Ven <ar...@in...> wrote: > >From 04795556b9ef5cd036a182535d04c4c853b61c96 Mon Sep 17 00:00:00 > >2001 > From: Arjan van de Ven <ar...@li...> > Date: Mon, 23 Mar 2009 13:36:25 -0700 > Subject: [PATCH] KMS: clean up udelay usage > > udelay() of 20 milliseconds really ought to just use mdelay(), that > avoids the various wrap scenarios and also is more readable > > Signed-off-by: Arjan van de Ven <ar...@li...> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c index c0ab079..6f2eced 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -319,7 +319,7 @@ void > intel_wait_for_vblank(struct drm_device *dev) > { > /* Wait for 20ms, i.e. one cycle at 50hz. */ > - udelay(20000); > + mdelay(20); > } > > static int Yeah, looks fine. Hopefully Eric can pick it up soon. Acked-by: Jesse Barnes <jb...@vi...> -- Jesse Barnes, Intel Open Source Technology Center |
From: Jakob B. <wal...@gm...> - 2009-03-26 10:59:25
|
On Mon, Mar 23, 2009 at 9:46 PM, Arjan van de Ven <ar...@in...> wrote: > >From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 00:00:00 2001 > From: Arjan van de Ven <ar...@li...> > Date: Mon, 23 Mar 2009 13:34:46 -0700 > Subject: [PATCH] KMS: register the LVDS before the CRT > > for the cases where the user only really cares about lighting up one output, > or only one output at first, lighting up the LVDS (which only gets detected > with lid-up) rather than the CRT is the right answer. > > This patch flips their order of registration so that this becomes possible. > > Signed-off-by: Arjan van de Ven <ar...@li...> Not-acked-by: Jakob Bornecrantz <wal...@gm...> I'm going to nack this patch out of principle, getting the correct behavior from userspace depending on the order of connectors is bad. The interface gives you enough information to find the LVDS and turn that on only that on. Is there some other reason that the getting of output name is slow? If so I would be happy to see patches that fixes that. You also have a comment change in there that isn't related to this change. Cheers Jakob. > --- > drivers/gpu/drm/i915/intel_display.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a283427..c0ab079 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1466,12 +1466,12 @@ static void intel_setup_outputs(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_connector *connector; > > - intel_crt_init(dev); > - > - /* Set up integrated LVDS */ > + /* Set up integrated LVDS -- will skip if the lid is closed */ > if (IS_MOBILE(dev) && !IS_I830(dev)) > intel_lvds_init(dev); > > + intel_crt_init(dev); > + > if (IS_I9XX(dev)) { > int found; > > -- > 1.6.0.6 > > > > > -- > Arjan van de Ven Intel Open Source Technology Centre > For development, discussion and tips for power savings, > visit http://www.lesswatts.org > > ------------------------------------------------------------------------------ > Apps built with the Adobe(R) Flex(R) framework and Flex Builder(TM) are > powering Web 2.0 with engaging, cross-platform capabilities. Quickly and > easily build your RIAs with Flex Builder, the Eclipse(TM)based development > software that enables intelligent coding and step-through debugging. > Download the free 60 day trial. http://p.sf.net/sfu/www-adobe-com > -- > _______________________________________________ > Dri-devel mailing list > Dri...@li... > https://lists.sourceforge.net/lists/listinfo/dri-devel > |
From: Arjan v. de V. <ar...@in...> - 2009-03-26 14:25:04
|
On Thu, 26 Mar 2009 11:59:13 +0100 Jakob Bornecrantz <wal...@gm...> wrote: > On Mon, Mar 23, 2009 at 9:46 PM, Arjan van de Ven > <ar...@in...> wrote: > > >From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 00:00:00 > > >2001 > > From: Arjan van de Ven <ar...@li...> > > Date: Mon, 23 Mar 2009 13:34:46 -0700 > > Subject: [PATCH] KMS: register the LVDS before the CRT > > > > for the cases where the user only really cares about lighting up > > one output, or only one output at first, lighting up the LVDS > > (which only gets detected with lid-up) rather than the CRT is the > > right answer. > > > > This patch flips their order of registration so that this becomes > > possible. > > > > Signed-off-by: Arjan van de Ven <ar...@li...> > > Not-acked-by: Jakob Bornecrantz <wal...@gm...> > > I'm going to nack this patch out of principle, getting the correct > behavior from userspace depending on the order of connectors is bad. this has nothing to do with userspace though, but all about user choice. > The interface gives you enough information to find the LVDS and turn > that on only that on. > Is there some other reason that the getting of > output name is slow? If so I would be happy to see patches that fixes > that. This patch has nothing to do with speed; it is only about registration order. LVDS-before-CRT makes total sense (as long as the lid of the laptop is not closed, which is why I added the comment); while CRT-before-LVDS does not. -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Jakob B. <wal...@gm...> - 2009-03-26 14:59:16
|
On Thu, Mar 26, 2009 at 3:25 PM, Arjan van de Ven <ar...@in...> wrote: > On Thu, 26 Mar 2009 11:59:13 +0100 > Jakob Bornecrantz <wal...@gm...> wrote: > >> On Mon, Mar 23, 2009 at 9:46 PM, Arjan van de Ven >> <ar...@in...> wrote: >> > >From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 00:00:00 >> > >2001 >> > From: Arjan van de Ven <ar...@li...> >> > Date: Mon, 23 Mar 2009 13:34:46 -0700 >> > Subject: [PATCH] KMS: register the LVDS before the CRT >> > >> > for the cases where the user only really cares about lighting up >> > one output, or only one output at first, lighting up the LVDS >> > (which only gets detected with lid-up) rather than the CRT is the >> > right answer. >> > >> > This patch flips their order of registration so that this becomes >> > possible. >> > >> > Signed-off-by: Arjan van de Ven <ar...@li...> >> >> Not-acked-by: Jakob Bornecrantz <wal...@gm...> >> >> I'm going to nack this patch out of principle, getting the correct >> behavior from userspace depending on the order of connectors is bad. > > this has nothing to do with userspace though, but all about user choice. Hard coding a specific order is not giving the user choice. Please tell me how doing crt-before-lvds fails with something that isn't userspace, currently I don't see how the order is important? Last time I checked there where only one kernel side consumer of the kms, the legacy fbdev stuff. Which if I remember correctly tried to clone one fb to all connectors. If you want to do some sort of no clone setup with fbdev you should let the user define the binding on the command line. Something like "i915 fb0=lvds0 fb1=crt0". There might be some issues with this naming and hotplugable connectors. And all the other users that I know of is userspace. > > > >> The interface gives you enough information to find the LVDS and turn >> that on only that on. > > >> Is there some other reason that the getting of >> output name is slow? If so I would be happy to see patches that fixes >> that. > > This patch has nothing to do with speed; it is only about registration > order. LVDS-before-CRT makes total sense (as long as the lid of the > laptop is not closed, which is why I added the comment); while > CRT-before-LVDS does not. It makes totally sense for you, for somebody else not, which is why policy in the kernel is a bad idea. What happens if the lid is closed? Is no lvds connector created? The comment you added make it sounds so, at least that was my first impression. Cheers Jakob. |
From: Arjan v. de V. <ar...@in...> - 2009-03-26 15:24:41
|
On Thu, 26 Mar 2009 15:59:12 +0100 Jakob Bornecrantz <wal...@gm...> wrote: > On Thu, Mar 26, 2009 at 3:25 PM, Arjan van de Ven > <ar...@in...> wrote: > > On Thu, 26 Mar 2009 11:59:13 +0100 > > Jakob Bornecrantz <wal...@gm...> wrote: > > > >> On Mon, Mar 23, 2009 at 9:46 PM, Arjan van de Ven > >> <ar...@in...> wrote: > >> > >From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 > >> > >00:00:00 2001 > >> > From: Arjan van de Ven <ar...@li...> > >> > Date: Mon, 23 Mar 2009 13:34:46 -0700 > >> > Subject: [PATCH] KMS: register the LVDS before the CRT > >> > > >> > for the cases where the user only really cares about lighting up > >> > one output, or only one output at first, lighting up the LVDS > >> > (which only gets detected with lid-up) rather than the CRT is the > >> > right answer. > >> > > >> > This patch flips their order of registration so that this becomes > >> > possible. > >> > > >> > Signed-off-by: Arjan van de Ven <ar...@li...> > >> > >> Not-acked-by: Jakob Bornecrantz <wal...@gm...> > >> > >> I'm going to nack this patch out of principle, getting the correct > >> behavior from userspace depending on the order of connectors is > >> bad. > > > > this has nothing to do with userspace though, but all about user > > choice. > > Hard coding a specific order is not giving the user choice. Please > tell me how doing crt-before-lvds fails with something that isn't > userspace, currently I don't see how the order is important? > > Last time I checked there where only one kernel side consumer of the > kms, the legacy fbdev stuff. Which if I remember correctly tried to > clone one fb to all connectors. If you want to do some sort of no > clone setup with fbdev you should let the user define the binding on > the command line. Something like "i915 fb0=lvds0 fb1=crt0". There > might be some issues with this naming and hotplugable connectors. so the other patches in this patch series added a consumer that basically lights up the first one and then continues booting the kernel, while the "all but first" get detected asynchronously. The concern from various people was that if there was an oops, around that time, it should be on the "primary" display, where the driver decided what was primary. > > This patch has nothing to do with speed; it is only about > > registration order. LVDS-before-CRT makes total sense (as long as > > the lid of the laptop is not closed, which is why I added the > > comment); while CRT-before-LVDS does not. > > It makes totally sense for you, for somebody else not, which is why > policy in the kernel is a bad idea. complaining about a change in policy by saying "there should be no policy" is not very useful; the policy is there today. > > What happens if the lid is closed? Is no lvds connector created? The > comment you added make it sounds so, at least that was my first > impression. that is what the rest of the code says yes. I just wanted to add the comment to show that. -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Jakob B. <wal...@gm...> - 2009-03-26 15:39:35
|
On Thu, Mar 26, 2009 at 4:25 PM, Arjan van de Ven <ar...@in...> wrote: > On Thu, 26 Mar 2009 15:59:12 +0100 > Jakob Bornecrantz <wal...@gm...> wrote: > >> On Thu, Mar 26, 2009 at 3:25 PM, Arjan van de Ven >> <ar...@in...> wrote: >> > On Thu, 26 Mar 2009 11:59:13 +0100 >> > Jakob Bornecrantz <wal...@gm...> wrote: >> > >> >> On Mon, Mar 23, 2009 at 9:46 PM, Arjan van de Ven >> >> <ar...@in...> wrote: >> >> > >From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 >> >> > >00:00:00 2001 >> >> > From: Arjan van de Ven <ar...@li...> >> >> > Date: Mon, 23 Mar 2009 13:34:46 -0700 >> >> > Subject: [PATCH] KMS: register the LVDS before the CRT >> >> > >> >> > for the cases where the user only really cares about lighting up >> >> > one output, or only one output at first, lighting up the LVDS >> >> > (which only gets detected with lid-up) rather than the CRT is the >> >> > right answer. >> >> > >> >> > This patch flips their order of registration so that this becomes >> >> > possible. >> >> > >> >> > Signed-off-by: Arjan van de Ven <ar...@li...> >> >> >> >> Not-acked-by: Jakob Bornecrantz <wal...@gm...> >> >> >> >> I'm going to nack this patch out of principle, getting the correct >> >> behavior from userspace depending on the order of connectors is >> >> bad. >> > >> > this has nothing to do with userspace though, but all about user >> > choice. >> >> Hard coding a specific order is not giving the user choice. Please >> tell me how doing crt-before-lvds fails with something that isn't >> userspace, currently I don't see how the order is important? >> >> Last time I checked there where only one kernel side consumer of the >> kms, the legacy fbdev stuff. Which if I remember correctly tried to >> clone one fb to all connectors. If you want to do some sort of no >> clone setup with fbdev you should let the user define the binding on >> the command line. Something like "i915 fb0=lvds0 fb1=crt0". There >> might be some issues with this naming and hotplugable connectors. > > so the other patches in this patch series added a consumer that > basically lights up the first one and then continues booting the kernel, > while the "all but first" get detected asynchronously. > The concern from various people was that if there was an oops, around > that time, it should be on the "primary" display, where the driver > decided what was primary. Now we are talking. Having a primary screen does make sense, but I do not think that the order in which they where discovered on the device should decide that. Couldn't there be some sort of primary field on the dev struct that get turned on? And that could be selected by a command line option like "i915.primary=hdmi0" and the default being lvds, if it is present. Giving choice to the user and the default being something which I have to agree does make sense. Does this sounds okay with you? Also a primary option for drm would be smart of multi gpu systems like "drm.primary=i915" maybe? > >> > This patch has nothing to do with speed; it is only about >> > registration order. LVDS-before-CRT makes total sense (as long as >> > the lid of the laptop is not closed, which is why I added the >> > comment); while CRT-before-LVDS does not. >> >> It makes totally sense for you, for somebody else not, which is why >> policy in the kernel is a bad idea. > > complaining about a change in policy by saying "there should be no > policy" is not very useful; the policy is there today. > >> >> What happens if the lid is closed? Is no lvds connector created? The >> comment you added make it sounds so, at least that was my first >> impression. > > that is what the rest of the code says yes. I just wanted to add the > comment to show that. That sounds like a bug, I don't want to have to reload my i915 driver because I had the laptop lid closed when I booted it. Or is there some code in there that adds the connector later on? Cheers Jakob. |
From: Jesse B. <jb...@vi...> - 2009-03-26 16:32:48
|
On Thu, 26 Mar 2009 16:39:31 +0100 Jakob Bornecrantz <wal...@gm...> wrote: > On Thu, Mar 26, 2009 at 4:25 PM, Arjan van de Ven > <ar...@in...> wrote: > > On Thu, 26 Mar 2009 15:59:12 +0100 > > Jakob Bornecrantz <wal...@gm...> wrote: > > > >> On Thu, Mar 26, 2009 at 3:25 PM, Arjan van de Ven > >> <ar...@in...> wrote: > >> > On Thu, 26 Mar 2009 11:59:13 +0100 > >> > Jakob Bornecrantz <wal...@gm...> wrote: > >> > > >> >> On Mon, Mar 23, 2009 at 9:46 PM, Arjan van de Ven > >> >> <ar...@in...> wrote: > >> >> > >From 785bb9f968ead288395ead4f921d7c1fb794ee71 Mon Sep 17 > >> >> > >00:00:00 2001 > >> >> > From: Arjan van de Ven <ar...@li...> > >> >> > Date: Mon, 23 Mar 2009 13:34:46 -0700 > >> >> > Subject: [PATCH] KMS: register the LVDS before the CRT > >> >> > > >> >> > for the cases where the user only really cares about lighting > >> >> > up one output, or only one output at first, lighting up the > >> >> > LVDS (which only gets detected with lid-up) rather than the > >> >> > CRT is the right answer. > >> >> > > >> >> > This patch flips their order of registration so that this > >> >> > becomes possible. > >> >> > > >> >> > Signed-off-by: Arjan van de Ven <ar...@li...> > >> >> > >> >> Not-acked-by: Jakob Bornecrantz <wal...@gm...> > >> >> > >> >> I'm going to nack this patch out of principle, getting the > >> >> correct behavior from userspace depending on the order of > >> >> connectors is bad. > >> > > >> > this has nothing to do with userspace though, but all about user > >> > choice. > >> > >> Hard coding a specific order is not giving the user choice. Please > >> tell me how doing crt-before-lvds fails with something that isn't > >> userspace, currently I don't see how the order is important? > >> > >> Last time I checked there where only one kernel side consumer of > >> the kms, the legacy fbdev stuff. Which if I remember correctly > >> tried to clone one fb to all connectors. If you want to do some > >> sort of no clone setup with fbdev you should let the user define > >> the binding on the command line. Something like "i915 fb0=lvds0 > >> fb1=crt0". There might be some issues with this naming and > >> hotplugable connectors. > > > > so the other patches in this patch series added a consumer that > > basically lights up the first one and then continues booting the > > kernel, while the "all but first" get detected asynchronously. > > The concern from various people was that if there was an oops, > > around that time, it should be on the "primary" display, where the > > driver decided what was primary. > > Now we are talking. Having a primary screen does make sense, but I do > not think that the order in which they where discovered on the device > should decide that. > > Couldn't there be some sort of primary field on the dev struct that > get turned on? And that could be selected by a command line option > like "i915.primary=hdmi0" and the default being lvds, if it is > present. Giving choice to the user and the default being something > which I have to agree does make sense. Does this sounds okay with you? > > Also a primary option for drm would be smart of multi gpu systems like > "drm.primary=i915" maybe? Yeah that would be great. The driver should be able to indicate to the core (or just init the fb driver directly on) which output is the "primary" head, i.e. which is most likely to be available and visible at driver init time, and which head should be used for panic & kernel messages. The module params you propose here sound great; we'd also need some structure internals to handle it (and default to the "clone everything" in the absence of a primary head). -- Jesse Barnes, Intel Open Source Technology Center |
From: Jesse B. <jb...@vi...> - 2009-03-27 22:58:01
|
On Mon, 23 Mar 2009 13:46:23 -0700 Arjan van de Ven <ar...@in...> wrote: > >From dfe256fc9d28f240f1a84fb4c3dfed4a4e18d5d4 Mon Sep 17 00:00:00 > >2001 > From: Arjan van de Ven <ar...@li...> > Date: Mon, 23 Mar 2009 13:32:47 -0700 > Subject: [PATCH] KMS: Do the actual mode setting from asynchronous > context > > The mode setting (and the expensive part of mode detection) can take > quite a bit of time, and will delay the kernel boot quite a bit. > > This patch puts these two parts into async context, so that the > normal execution of the system continues while the slow hardware > probing and poking happens. I'm curious if the patch I recently posted to add the idea of a primary head to the driver have a similar effect? It avoids probing secondary connectors if the primary one is found, so it should provide the same speedup as this one. -- Jesse Barnes, Intel Open Source Technology Center |
From: Arjan v. de V. <ar...@in...> - 2009-03-28 04:59:16
|
On Fri, 27 Mar 2009 15:57:54 -0700 Jesse Barnes <jb...@vi...> wrote: > On Mon, 23 Mar 2009 13:46:23 -0700 > Arjan van de Ven <ar...@in...> wrote: > > > >From dfe256fc9d28f240f1a84fb4c3dfed4a4e18d5d4 Mon Sep 17 00:00:00 > > >2001 > > From: Arjan van de Ven <ar...@li...> > > Date: Mon, 23 Mar 2009 13:32:47 -0700 > > Subject: [PATCH] KMS: Do the actual mode setting from asynchronous > > context > > > > The mode setting (and the expensive part of mode detection) can take > > quite a bit of time, and will delay the kernel boot quite a bit. > > > > This patch puts these two parts into async context, so that the > > normal execution of the system continues while the slow hardware > > probing and poking happens. > > I'm curious if the patch I recently posted to add the idea of a > primary head to the driver have a similar effect? It avoids probing > secondary connectors if the primary one is found, so it should > provide the same speedup as this one. part of (actually the majority of) the improvement is from doing the actual modeset async.... that is where on my systems most of the time goes. The probing only applies if you actually connect a vga screen... -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Jesse B. <jb...@vi...> - 2009-03-27 23:08:25
|
On Mon, 23 Mar 2009 13:48:33 -0700 Arjan van de Ven <ar...@in...> wrote: > >From eb512d6d953c8acc8abe1048862a92cec58d9791 Mon Sep 17 00:00:00 > >2001 > From: Arjan van de Ven <ar...@li...> > Date: Mon, 23 Mar 2009 13:38:49 -0700 > Subject: [PATCH] KMS: cache the EDID information of the LVDS > > you're not going to replace your LVDS at runtime..... so this patch > caches the EDID information of the LVDS. An LVDS probe can easily take > 200 milliseconds, and we do this multiple times during a system > startup. > > Signed-off-by: Arjan van de Ven <ar...@li...> > --- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_lvds.c | 2 ++ > drivers/gpu/drm/i915/intel_modes.c | 8 +++++++- > 3 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h index 957daef..22a74bd 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -81,6 +81,7 @@ struct intel_output { > int type; > struct intel_i2c_chan *i2c_bus; /* for control functions */ > struct intel_i2c_chan *ddc_bus; /* for DDC only stuff */ > + struct edid *edid; > bool load_detect_temp; > bool needs_tv_clock; > void *dev_priv; > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > b/drivers/gpu/drm/i915/intel_lvds.c index 0d211af..dc4fecc 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -336,6 +336,7 @@ static void intel_lvds_destroy(struct > drm_connector *connector) intel_i2c_destroy(intel_output->ddc_bus); > drm_sysfs_connector_remove(connector); > drm_connector_cleanup(connector); > + kfree(intel_output->edid); > kfree(connector); > } > > @@ -516,5 +517,6 @@ failed: > if (intel_output->ddc_bus) > intel_i2c_destroy(intel_output->ddc_bus); > drm_connector_cleanup(connector); > + kfree(intel_output->edid); > kfree(connector); > } > diff --git a/drivers/gpu/drm/i915/intel_modes.c > b/drivers/gpu/drm/i915/intel_modes.c index e42019e..83816a4 100644 > --- a/drivers/gpu/drm/i915/intel_modes.c > +++ b/drivers/gpu/drm/i915/intel_modes.c > @@ -70,13 +70,19 @@ int intel_ddc_get_modes(struct intel_output > *intel_output) struct edid *edid; > int ret = 0; > > + if (intel_output->edid) > + return ret; > + > edid = drm_get_edid(&intel_output->base, > &intel_output->ddc_bus->adapter); > if (edid) { > drm_mode_connector_update_edid_property(&intel_output->base, > edid); > ret = drm_add_edid_modes(&intel_output->base, edid); > - kfree(edid); > + if (intel_output->type == INTEL_OUTPUT_LVDS) > + intel_output->edid = edid; > + else > + kfree(edid); > } > > return ret; I definitely like the idea of caching the LVDS EDID, but rather than adding it to the intel_output struct I'd rather see it stored with the rest of the LVDS stuff in i915_private (though that stuff should be pulled out into an output private at some point). We could also just open code the intel_ddc_get_modes stuff in intel_lvds_init, getting the EDID just once, storing the property once, and adding the mode list once. intel_lvds_get_modes would need to be fixed too... -- Jesse Barnes, Intel Open Source Technology Center |
From: Jesse B. <jb...@vi...> - 2009-03-27 23:08:30
|
On Mon, 23 Mar 2009 13:48:01 -0700 Arjan van de Ven <ar...@in...> wrote: > >From a121507f31e37a809dfa21c9756aa83f36d7acbf Mon Sep 17 00:00:00 > >2001 > From: Arjan van de Ven <ar...@li...> > Date: Mon, 23 Mar 2009 13:37:20 -0700 > Subject: [PATCH] KMS: do a faster EDID > > for some outputs (like LVDS), the current delays in the EDID reader > are way overkill. > > This patch makes the EDID reader try twice; first with a 5x faster > probe, and if that fails, it probes at the current speed. This > reduces probe time on my LVDS panel significantly. > > Signed-off-by: Arjan van de Ven <ar...@li...> Theoretically this should work fine, but there may be more savings to be had in the underlying EDID fetching code & timing. I know our timings aren't as good as X yet, but we may be able to do something similar in the actual DDC probing as well (use fast timings first, then fall back to robust ones if that fails) and get a double benefit. Have you looked at that at all? Either way, this one looks ok, so I'm ok with it as long as Dave is. Acked-by: Jesse Barnes <jb...@vi...> -- Jesse Barnes, Intel Open Source Technology Center |
From: Eric A. <er...@an...> - 2009-04-30 22:50:54
|
On Fri, 2009-03-27 at 16:08 -0700, Jesse Barnes wrote: > On Mon, 23 Mar 2009 13:48:01 -0700 > Arjan van de Ven <ar...@in...> wrote: > > > >From a121507f31e37a809dfa21c9756aa83f36d7acbf Mon Sep 17 00:00:00 > > >2001 > > From: Arjan van de Ven <ar...@li...> > > Date: Mon, 23 Mar 2009 13:37:20 -0700 > > Subject: [PATCH] KMS: do a faster EDID > > > > for some outputs (like LVDS), the current delays in the EDID reader > > are way overkill. > > > > This patch makes the EDID reader try twice; first with a 5x faster > > probe, and if that fails, it probes at the current speed. This > > reduces probe time on my LVDS panel significantly. > > > > Signed-off-by: Arjan van de Ven <ar...@li...> > > Theoretically this should work fine, but there may be more savings to > be had in the underlying EDID fetching code & timing. I know our > timings aren't as good as X yet, but we may be able to do something > similar in the actual DDC probing as well (use fast timings first, then > fall back to robust ones if that fails) and get a double benefit. Have > you looked at that at all? > > Either way, this one looks ok, so I'm ok with it as long as Dave is. > > Acked-by: Jesse Barnes <jb...@vi...> We need to just figure out how to use the GMBUS stuff successfully. It's what everyone else uses as far as I know. -- Eric Anholt er...@an... eri...@in... |
From: Arjan v. de V. <ar...@in...> - 2009-03-28 05:00:05
|
On Fri, 27 Mar 2009 16:08:18 -0700 Jesse Barnes <jb...@vi...> wrote: > > > > return ret; > > I definitely like the idea of caching the LVDS EDID, but rather than > adding it to the intel_output struct I'd rather see it stored with the > rest of the LVDS stuff in i915_private (though that stuff should be > pulled out into an output private at some point). We could also just > open code the intel_ddc_get_modes stuff in intel_lvds_init, getting > the EDID just once, storing the property once, and adding the mode > list once. intel_lvds_get_modes would need to be fixed too... > that gets a bit messy though, esp if other output types decide to also want to cache. Remember that some of this code gets called from DRM or even userland, so the edid will end up being stored in a generic structure.... -- Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org |
From: Eric A. <er...@an...> - 2009-04-01 19:29:23
|
On Fri, 2009-03-27 at 22:01 -0700, Arjan van de Ven wrote: > On Fri, 27 Mar 2009 16:08:18 -0700 > Jesse Barnes <jb...@vi...> wrote: > > > > > > return ret; > > > > I definitely like the idea of caching the LVDS EDID, but rather than > > adding it to the intel_output struct I'd rather see it stored with the > > rest of the LVDS stuff in i915_private (though that stuff should be > > pulled out into an output private at some point). We could also just > > open code the intel_ddc_get_modes stuff in intel_lvds_init, getting > > the EDID just once, storing the property once, and adding the mode > > list once. intel_lvds_get_modes would need to be fixed too... > > > > that gets a bit messy though, esp if other output types decide to also > want to cache. Remember that some of this code gets called from DRM or > even userland, so the edid will end up being stored in a generic > structure.... Ack. SDVO LVDS and eDP would also want to do this, and i915_private then seems like the wrong place. -- Eric Anholt er...@an... eri...@in... |
From: Jesse B. <jb...@vi...> - 2009-04-30 22:45:10
|
On Wed, 01 Apr 2009 12:29:04 -0700 Eric Anholt <er...@an...> wrote: > On Fri, 2009-03-27 at 22:01 -0700, Arjan van de Ven wrote: > > On Fri, 27 Mar 2009 16:08:18 -0700 > > Jesse Barnes <jb...@vi...> wrote: > > > > > > > > return ret; > > > > > > I definitely like the idea of caching the LVDS EDID, but rather > > > than adding it to the intel_output struct I'd rather see it > > > stored with the rest of the LVDS stuff in i915_private (though > > > that stuff should be pulled out into an output private at some > > > point). We could also just open code the intel_ddc_get_modes > > > stuff in intel_lvds_init, getting the EDID just once, storing the > > > property once, and adding the mode list once. > > > intel_lvds_get_modes would need to be fixed too... > > > > > > > that gets a bit messy though, esp if other output types decide to > > also want to cache. Remember that some of this code gets called > > from DRM or even userland, so the edid will end up being stored in > > a generic structure.... > > Ack. SDVO LVDS and eDP would also want to do this, and i915_private > then seems like the wrong place. Yeah after looking again (and fixing this issue myself having forgotten about this), I ack it. Wanna pull it in Eric? Acked-by: Jesse Barnes <jb...@vi...> -- Jesse Barnes, Intel Open Source Technology Center |