[PATCH 1/2] drm/i915: s/any_enabled/!fallback/ in fbdev_initial_config

[PATCH 1/2] drm/i915: s/any_enabled/!fallback/ in fbdev_initial_config

[Daniel Vetter]

It started as a simple check whether anything is lit up, but now is't
used to driver the general fallback logic to the default output
configuration selector in the helper library. So rename it for more
clarity.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4e4b461e0a70..df00e6b01f0d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -289,7 +289,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 	struct drm_device *dev = fb_helper->dev;
 	int i, j;
 	bool *save_enabled;
-	bool any_enabled = false;
+	bool fallback = true;
 
 	/*
 	 * If the user specified any force options, just bail here
@@ -347,7 +347,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		 */
 		for (j = 0; j < fb_helper->connector_count; j++) {
 			if (crtcs[j] == new_crtc) {
-				any_enabled = false;
+				fallback = true;
 				goto out;
 			}
 		}
@@ -390,11 +390,11 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 			      encoder->crtc->base.id,
 			      modes[i]->name);
 
-		any_enabled = true;
+		fallback = false;
 	}
 
 out:
-	if (!any_enabled) {
+	if (fallback) {
 		memcpy(enabled, save_enabled, dev->mode_config.num_connector);
 		kfree(save_enabled);
 		return false;
-- 
1.8.5.2

[PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on

[Daniel Vetter]

Both Ville and QA rather immediately complained that with the new
initial_config logic from Jesse not all outputs get enabled. Since the
fbdev emulation pretty much tries to always enable as many outputs as
possible (it even has hotplug handling and all that) fall back if more
outputs could have been enabled.

v2: Fix up my confusion about what enabled means - it's passed from
the fbdev helper, we need to check for a non-zero connector->encoder
link. Spotted by Ville.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75552
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index df00e6b01f0d..c1a20c3babde 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 	int i, j;
 	bool *save_enabled;
 	bool fallback = true;
+	int num_connectors_enabled = 0;
+	int num_connectors_detected = 0;
 
 	/*
 	 * If the user specified any force options, just bail here
@@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 
 		fb_conn = fb_helper->connector_info[i];
 		connector = fb_conn->connector;
+
+		if (connector->status == connector_status_connected)
+			num_connectors_detected++;
+
 		if (!enabled[i]) {
 			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
 				      connector->base.id);
@@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 			continue;
 		}
 
+		num_connectors_enabled++;
+
 		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
 
 		/*
@@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		fallback = false;
 	}
 
+	/*
+	 * If the BIOS didn't enable everything it could, fall back to have the
+	 * same user experiencing of lighting up as much as possible like the
+	 * fbdev helper library.
+	 */
+	if (num_connectors_enabled != num_connectors_detected &&
+	    num_connectors_enabled < INTEL_INFO(dev)->num_pipes)
+		fallback = true;
+
 out:
 	if (fallback) {
 		memcpy(enabled, save_enabled, dev->mode_config.num_connector);
-- 
1.8.5.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: s/any_enabled/!fallback/ in fbdev_initial_config

[Jesse Barnes]

On Tue,  4 Mar 2014 21:08:41 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> It started as a simple check whether anything is lit up, but now is't
> used to driver the general fallback logic to the default output
> configuration selector in the helper library. So rename it for more
> clarity.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 4e4b461e0a70..df00e6b01f0d 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -289,7 +289,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  	struct drm_device *dev = fb_helper->dev;
>  	int i, j;
>  	bool *save_enabled;
> -	bool any_enabled = false;
> +	bool fallback = true;
>  
>  	/*
>  	 * If the user specified any force options, just bail here
> @@ -347,7 +347,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  		 */
>  		for (j = 0; j < fb_helper->connector_count; j++) {
>  			if (crtcs[j] == new_crtc) {
> -				any_enabled = false;
> +				fallback = true;
>  				goto out;
>  			}
>  		}
> @@ -390,11 +390,11 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  			      encoder->crtc->base.id,
>  			      modes[i]->name);
>  
> -		any_enabled = true;
> +		fallback = false;
>  	}
>  
>  out:
> -	if (!any_enabled) {
> +	if (fallback) {
>  		memcpy(enabled, save_enabled, dev->mode_config.num_connector);
>  		kfree(save_enabled);
>  		return false;

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on

[Jesse Barnes]

On Tue,  4 Mar 2014 21:08:42 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Both Ville and QA rather immediately complained that with the new
> initial_config logic from Jesse not all outputs get enabled. Since the
> fbdev emulation pretty much tries to always enable as many outputs as
> possible (it even has hotplug handling and all that) fall back if more
> outputs could have been enabled.
> 
> v2: Fix up my confusion about what enabled means - it's passed from
> the fbdev helper, we need to check for a non-zero connector->encoder
> link. Spotted by Ville.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75552
> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index df00e6b01f0d..c1a20c3babde 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  	int i, j;
>  	bool *save_enabled;
>  	bool fallback = true;
> +	int num_connectors_enabled = 0;
> +	int num_connectors_detected = 0;
>  
>  	/*
>  	 * If the user specified any force options, just bail here
> @@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  
>  		fb_conn = fb_helper->connector_info[i];
>  		connector = fb_conn->connector;
> +
> +		if (connector->status == connector_status_connected)
> +			num_connectors_detected++;
> +
>  		if (!enabled[i]) {
>  			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
>  				      connector->base.id);
> @@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  			continue;
>  		}
>  
> +		num_connectors_enabled++;
> +
>  		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
>  
>  		/*
> @@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
>  		fallback = false;
>  	}
>  
> +	/*
> +	 * If the BIOS didn't enable everything it could, fall back to have the
> +	 * same user experiencing of lighting up as much as possible like the
> +	 * fbdev helper library.
> +	 */
> +	if (num_connectors_enabled != num_connectors_detected &&
> +	    num_connectors_enabled < INTEL_INFO(dev)->num_pipes)
> +		fallback = true;

I think we need a debug message in here so people can figure out why
their fastboot failed with this patch included.  E.g. "some connected
outputs weren't enabled, falling back to old behavior".

Also note that this will probably always happen in certain configs, and
the fallback behavior won't be any better since we may not be able to
light up everything that's attached.

With those caveats:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on

[Daniel Vetter]

On Tue, Mar 04, 2014 at 12:33:01PM -0800, Jesse Barnes wrote:
> On Tue,  4 Mar 2014 21:08:42 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Both Ville and QA rather immediately complained that with the new
> > initial_config logic from Jesse not all outputs get enabled. Since the
> > fbdev emulation pretty much tries to always enable as many outputs as
> > possible (it even has hotplug handling and all that) fall back if more
> > outputs could have been enabled.
> > 
> > v2: Fix up my confusion about what enabled means - it's passed from
> > the fbdev helper, we need to check for a non-zero connector->encoder
> > link. Spotted by Ville.
> > 
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75552
> > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index df00e6b01f0d..c1a20c3babde 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  	int i, j;
> >  	bool *save_enabled;
> >  	bool fallback = true;
> > +	int num_connectors_enabled = 0;
> > +	int num_connectors_detected = 0;
> >  
> >  	/*
> >  	 * If the user specified any force options, just bail here
> > @@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  
> >  		fb_conn = fb_helper->connector_info[i];
> >  		connector = fb_conn->connector;
> > +
> > +		if (connector->status == connector_status_connected)
> > +			num_connectors_detected++;
> > +
> >  		if (!enabled[i]) {
> >  			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> >  				      connector->base.id);
> > @@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  			continue;
> >  		}
> >  
> > +		num_connectors_enabled++;
> > +
> >  		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> >  
> >  		/*
> > @@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> >  		fallback = false;
> >  	}
> >  
> > +	/*
> > +	 * If the BIOS didn't enable everything it could, fall back to have the
> > +	 * same user experiencing of lighting up as much as possible like the
> > +	 * fbdev helper library.
> > +	 */
> > +	if (num_connectors_enabled != num_connectors_detected &&
> > +	    num_connectors_enabled < INTEL_INFO(dev)->num_pipes)
> > +		fallback = true;
> 
> I think we need a debug message in here so people can figure out why
> their fastboot failed with this patch included.  E.g. "some connected
> outputs weren't enabled, falling back to old behavior".
> 
> Also note that this will probably always happen in certain configs, and
> the fallback behavior won't be any better since we may not be able to
> light up everything that's attached.

Excellent suggestion, I've gone ahead and added debug output for all cases
where we fall back.
> 
> With those caveats:
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks for the review, both patches merged to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on

[Jesse Barnes]

On Tue, 4 Mar 2014 22:08:12 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Mar 04, 2014 at 12:33:01PM -0800, Jesse Barnes wrote:
> > On Tue,  4 Mar 2014 21:08:42 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > Both Ville and QA rather immediately complained that with the new
> > > initial_config logic from Jesse not all outputs get enabled. Since the
> > > fbdev emulation pretty much tries to always enable as many outputs as
> > > possible (it even has hotplug handling and all that) fall back if more
> > > outputs could have been enabled.
> > > 
> > > v2: Fix up my confusion about what enabled means - it's passed from
> > > the fbdev helper, we need to check for a non-zero connector->encoder
> > > link. Spotted by Ville.
> > > 
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75552
> > > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index df00e6b01f0d..c1a20c3babde 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > >  	int i, j;
> > >  	bool *save_enabled;
> > >  	bool fallback = true;
> > > +	int num_connectors_enabled = 0;
> > > +	int num_connectors_detected = 0;
> > >  
> > >  	/*
> > >  	 * If the user specified any force options, just bail here
> > > @@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > >  
> > >  		fb_conn = fb_helper->connector_info[i];
> > >  		connector = fb_conn->connector;
> > > +
> > > +		if (connector->status == connector_status_connected)
> > > +			num_connectors_detected++;
> > > +
> > >  		if (!enabled[i]) {
> > >  			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> > >  				      connector->base.id);
> > > @@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > >  			continue;
> > >  		}
> > >  
> > > +		num_connectors_enabled++;
> > > +
> > >  		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> > >  
> > >  		/*
> > > @@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > >  		fallback = false;
> > >  	}
> > >  
> > > +	/*
> > > +	 * If the BIOS didn't enable everything it could, fall back to have the
> > > +	 * same user experiencing of lighting up as much as possible like the
> > > +	 * fbdev helper library.
> > > +	 */
> > > +	if (num_connectors_enabled != num_connectors_detected &&
> > > +	    num_connectors_enabled < INTEL_INFO(dev)->num_pipes)
> > > +		fallback = true;
> > 
> > I think we need a debug message in here so people can figure out why
> > their fastboot failed with this patch included.  E.g. "some connected
> > outputs weren't enabled, falling back to old behavior".
> > 
> > Also note that this will probably always happen in certain configs, and
> > the fallback behavior won't be any better since we may not be able to
> > light up everything that's attached.
> 
> Excellent suggestion, I've gone ahead and added debug output for all cases
> where we fall back.
> > 
> > With those caveats:
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thinking about this some more last night, I think it would be better to
count the pipes and the connectors, and bail out if we have detected
connectors available but not enabled and some free pipes.  That would
prevent unnecessary fastboot breakage I think.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on

[Daniel Vetter]

On Wed, Mar 05, 2014 at 08:27:08AM -0800, Jesse Barnes wrote:
> On Tue, 4 Mar 2014 22:08:12 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Mar 04, 2014 at 12:33:01PM -0800, Jesse Barnes wrote:
> > > On Tue,  4 Mar 2014 21:08:42 +0100
> > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > 
> > > > Both Ville and QA rather immediately complained that with the new
> > > > initial_config logic from Jesse not all outputs get enabled. Since the
> > > > fbdev emulation pretty much tries to always enable as many outputs as
> > > > possible (it even has hotplug handling and all that) fall back if more
> > > > outputs could have been enabled.
> > > > 
> > > > v2: Fix up my confusion about what enabled means - it's passed from
> > > > the fbdev helper, we need to check for a non-zero connector->encoder
> > > > link. Spotted by Ville.
> > > > 
> > > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75552
> > > > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++
> > > >  1 file changed, 17 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > index df00e6b01f0d..c1a20c3babde 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > @@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > >  	int i, j;
> > > >  	bool *save_enabled;
> > > >  	bool fallback = true;
> > > > +	int num_connectors_enabled = 0;
> > > > +	int num_connectors_detected = 0;
> > > >  
> > > >  	/*
> > > >  	 * If the user specified any force options, just bail here
> > > > @@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > >  
> > > >  		fb_conn = fb_helper->connector_info[i];
> > > >  		connector = fb_conn->connector;
> > > > +
> > > > +		if (connector->status == connector_status_connected)
> > > > +			num_connectors_detected++;
> > > > +
> > > >  		if (!enabled[i]) {
> > > >  			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> > > >  				      connector->base.id);
> > > > @@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > >  			continue;
> > > >  		}
> > > >  
> > > > +		num_connectors_enabled++;
> > > > +
> > > >  		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> > > >  
> > > >  		/*
> > > > @@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > >  		fallback = false;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * If the BIOS didn't enable everything it could, fall back to have the
> > > > +	 * same user experiencing of lighting up as much as possible like the
> > > > +	 * fbdev helper library.
> > > > +	 */
> > > > +	if (num_connectors_enabled != num_connectors_detected &&
> > > > +	    num_connectors_enabled < INTEL_INFO(dev)->num_pipes)
> > > > +		fallback = true;
> > > 
> > > I think we need a debug message in here so people can figure out why
> > > their fastboot failed with this patch included.  E.g. "some connected
> > > outputs weren't enabled, falling back to old behavior".
> > > 
> > > Also note that this will probably always happen in certain configs, and
> > > the fallback behavior won't be any better since we may not be able to
> > > light up everything that's attached.
> > 
> > Excellent suggestion, I've gone ahead and added debug output for all cases
> > where we fall back.
> > > 
> > > With those caveats:
> > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Thinking about this some more last night, I think it would be better to
> count the pipes and the connectors, and bail out if we have detected
> connectors available but not enabled and some free pipes.  That would
> prevent unnecessary fastboot breakage I think.

I do take pipes into account and only bail out if we'd have a free one.
I don't see what more we could do (beside trying to keep the crtcs for the
already enabled connectors)?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on

[Jesse Barnes]

On Wed, 5 Mar 2014 19:34:45 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Mar 05, 2014 at 08:27:08AM -0800, Jesse Barnes wrote:
> > On Tue, 4 Mar 2014 22:08:12 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Tue, Mar 04, 2014 at 12:33:01PM -0800, Jesse Barnes wrote:
> > > > On Tue,  4 Mar 2014 21:08:42 +0100
> > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > 
> > > > > Both Ville and QA rather immediately complained that with the new
> > > > > initial_config logic from Jesse not all outputs get enabled. Since the
> > > > > fbdev emulation pretty much tries to always enable as many outputs as
> > > > > possible (it even has hotplug handling and all that) fall back if more
> > > > > outputs could have been enabled.
> > > > > 
> > > > > v2: Fix up my confusion about what enabled means - it's passed from
> > > > > the fbdev helper, we need to check for a non-zero connector->encoder
> > > > > link. Spotted by Ville.
> > > > > 
> > > > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75552
> > > > > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++
> > > > >  1 file changed, 17 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > > index df00e6b01f0d..c1a20c3babde 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > > @@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > > >  	int i, j;
> > > > >  	bool *save_enabled;
> > > > >  	bool fallback = true;
> > > > > +	int num_connectors_enabled = 0;
> > > > > +	int num_connectors_detected = 0;
> > > > >  
> > > > >  	/*
> > > > >  	 * If the user specified any force options, just bail here
> > > > > @@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > > >  
> > > > >  		fb_conn = fb_helper->connector_info[i];
> > > > >  		connector = fb_conn->connector;
> > > > > +
> > > > > +		if (connector->status == connector_status_connected)
> > > > > +			num_connectors_detected++;
> > > > > +
> > > > >  		if (!enabled[i]) {
> > > > >  			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> > > > >  				      connector->base.id);
> > > > > @@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > > >  			continue;
> > > > >  		}
> > > > >  
> > > > > +		num_connectors_enabled++;
> > > > > +
> > > > >  		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> > > > >  
> > > > >  		/*
> > > > > @@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> > > > >  		fallback = false;
> > > > >  	}
> > > > >  
> > > > > +	/*
> > > > > +	 * If the BIOS didn't enable everything it could, fall back to have the
> > > > > +	 * same user experiencing of lighting up as much as possible like the
> > > > > +	 * fbdev helper library.
> > > > > +	 */
> > > > > +	if (num_connectors_enabled != num_connectors_detected &&
> > > > > +	    num_connectors_enabled < INTEL_INFO(dev)->num_pipes)
> > > > > +		fallback = true;
> > > > 
> > > > I think we need a debug message in here so people can figure out why
> > > > their fastboot failed with this patch included.  E.g. "some connected
> > > > outputs weren't enabled, falling back to old behavior".
> > > > 
> > > > Also note that this will probably always happen in certain configs, and
> > > > the fallback behavior won't be any better since we may not be able to
> > > > light up everything that's attached.
> > > 
> > > Excellent suggestion, I've gone ahead and added debug output for all cases
> > > where we fall back.
> > > > 
> > > > With those caveats:
> > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > Thinking about this some more last night, I think it would be better to
> > count the pipes and the connectors, and bail out if we have detected
> > connectors available but not enabled and some free pipes.  That would
> > prevent unnecessary fastboot breakage I think.
> 
> I do take pipes into account and only bail out if we'd have a free one.
> I don't see what more we could do (beside trying to keep the crtcs for the
> already enabled connectors)?

Ah ok I missed the check against num_pipes... yeah looks like it should
be fine.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx