More fastboot bits

More fastboot bits

[Jesse Barnes]

This one adds some extra checks on top of Chris's last set:
  - check for panel fit modes when inheriting from the BIOS
  - update pfit state at pipe_set_base time

It also changes the mode set vs flip checking to include the non-fb case
(e.g. if the BIOS fb was too small for the native mode), since we might
still be able to flip in that case.

Finally, it includes a clock_get routine for ilk+.  I'd appreciate if
someone could test this out on a machine where VBIOS supports the native
panel mode, so the kernel can boot from the boot loader in the native
mode.  In that case, it should actually fastboot and avoid the whole
mode set/panel power sequence.

Thanks,
Jesse

[PATCH 01/13] drm/i915: Skip modifying PCH DREF if not changing clock sources

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

Modifying the clock sources (via the DREF control on the PCH) is a slow
multi-stage process as we need to let the clocks stabilise between each
stage. If we are not actually changing the clock sources, then we can
return early.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   83 +++++++++++++++++++++++++---------
 1 file changed, 61 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e6dadf..f20555e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4758,7 +4758,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_encoder *encoder;
-	u32 temp;
+	u32 val, final;
 	bool has_lvds = false;
 	bool has_cpu_edp = false;
 	bool has_pch_edp = false;
@@ -4801,70 +4801,109 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
 	 * PCH B stepping, previous chipset stepping should be
 	 * ignoring this setting.
 	 */
-	temp = I915_READ(PCH_DREF_CONTROL);
+	val = I915_READ(PCH_DREF_CONTROL);
+
+	/* As we must carefully and slowly disable/enable each source in turn,
+	 * compute the final state we want first and check if we need to
+	 * make any changes at all.
+	 */
+	final = val;
+	final &= ~DREF_NONSPREAD_SOURCE_MASK;
+	if (has_ck505)
+		final |= DREF_NONSPREAD_CK505_ENABLE;
+	else
+		final |= DREF_NONSPREAD_SOURCE_ENABLE;
+
+	final &= ~DREF_SSC_SOURCE_MASK;
+	final &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
+	final &= ~DREF_SSC1_ENABLE;
+
+	if (has_panel) {
+		final |= DREF_SSC_SOURCE_ENABLE;
+
+		if (intel_panel_use_ssc(dev_priv) && can_ssc)
+			final |= DREF_SSC1_ENABLE;
+
+		if (has_cpu_edp) {
+			if (intel_panel_use_ssc(dev_priv) && can_ssc)
+				final |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
+			else
+				final |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
+		} else
+			final |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+	} else {
+		final |= DREF_SSC_SOURCE_DISABLE;
+		final |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+	}
+
+	if (final == val)
+		return;
+
 	/* Always enable nonspread source */
-	temp &= ~DREF_NONSPREAD_SOURCE_MASK;
+	val &= ~DREF_NONSPREAD_SOURCE_MASK;
 
 	if (has_ck505)
-		temp |= DREF_NONSPREAD_CK505_ENABLE;
+		val |= DREF_NONSPREAD_CK505_ENABLE;
 	else
-		temp |= DREF_NONSPREAD_SOURCE_ENABLE;
+		val |= DREF_NONSPREAD_SOURCE_ENABLE;
 
 	if (has_panel) {
-		temp &= ~DREF_SSC_SOURCE_MASK;
-		temp |= DREF_SSC_SOURCE_ENABLE;
+		val &= ~DREF_SSC_SOURCE_MASK;
+		val |= DREF_SSC_SOURCE_ENABLE;
 
 		/* SSC must be turned on before enabling the CPU output  */
 		if (intel_panel_use_ssc(dev_priv) && can_ssc) {
 			DRM_DEBUG_KMS("Using SSC on panel\n");
-			temp |= DREF_SSC1_ENABLE;
+			val |= DREF_SSC1_ENABLE;
 		} else
-			temp &= ~DREF_SSC1_ENABLE;
+			val &= ~DREF_SSC1_ENABLE;
 
 		/* Get SSC going before enabling the outputs */
-		I915_WRITE(PCH_DREF_CONTROL, temp);
+		I915_WRITE(PCH_DREF_CONTROL, val);
 		POSTING_READ(PCH_DREF_CONTROL);
 		udelay(200);
 
-		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
+		val &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
 
 		/* Enable CPU source on CPU attached eDP */
 		if (has_cpu_edp) {
 			if (intel_panel_use_ssc(dev_priv) && can_ssc) {
 				DRM_DEBUG_KMS("Using SSC on eDP\n");
-				temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
+				val |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
 			}
 			else
-				temp |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
+				val |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
 		} else
-			temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+			val |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
 
-		I915_WRITE(PCH_DREF_CONTROL, temp);
+		I915_WRITE(PCH_DREF_CONTROL, val);
 		POSTING_READ(PCH_DREF_CONTROL);
 		udelay(200);
 	} else {
 		DRM_DEBUG_KMS("Disabling SSC entirely\n");
 
-		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
+		val &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
 
 		/* Turn off CPU output */
-		temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+		val |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
 
-		I915_WRITE(PCH_DREF_CONTROL, temp);
+		I915_WRITE(PCH_DREF_CONTROL, val);
 		POSTING_READ(PCH_DREF_CONTROL);
 		udelay(200);
 
 		/* Turn off the SSC source */
-		temp &= ~DREF_SSC_SOURCE_MASK;
-		temp |= DREF_SSC_SOURCE_DISABLE;
+		val &= ~DREF_SSC_SOURCE_MASK;
+		val |= DREF_SSC_SOURCE_DISABLE;
 
 		/* Turn off SSC1 */
-		temp &= ~ DREF_SSC1_ENABLE;
+		val &= ~ DREF_SSC1_ENABLE;
 
-		I915_WRITE(PCH_DREF_CONTROL, temp);
+		I915_WRITE(PCH_DREF_CONTROL, val);
 		POSTING_READ(PCH_DREF_CONTROL);
 		udelay(200);
 	}
+
+	BUG_ON(val != final);
 }
 
 /* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
-- 
1.7.9.5

Re: [PATCH 01/13] drm/i915: Skip modifying PCH DREF if not changing clock sources

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Modifying the clock sources (via the DREF control on the PCH) is a slow
> multi-stage process as we need to let the clocks stabilise between each
> stage. If we are not actually changing the clock sources, then we can
> return early.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   83 +++++++++++++++++++++++++---------
>  1 file changed, 61 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e6dadf..f20555e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4758,7 +4758,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_encoder *encoder;
> -	u32 temp;
> +	u32 val, final;
>  	bool has_lvds = false;
>  	bool has_cpu_edp = false;
>  	bool has_pch_edp = false;
> @@ -4801,70 +4801,109 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
>  	 * PCH B stepping, previous chipset stepping should be
>  	 * ignoring this setting.
>  	 */
> -	temp = I915_READ(PCH_DREF_CONTROL);
> +	val = I915_READ(PCH_DREF_CONTROL);
> +
> +	/* As we must carefully and slowly disable/enable each source in turn,
> +	 * compute the final state we want first and check if we need to
> +	 * make any changes at all.
> +	 */
> +	final = val;
> +	final &= ~DREF_NONSPREAD_SOURCE_MASK;
> +	if (has_ck505)
> +		final |= DREF_NONSPREAD_CK505_ENABLE;
> +	else
> +		final |= DREF_NONSPREAD_SOURCE_ENABLE;
> +
> +	final &= ~DREF_SSC_SOURCE_MASK;
> +	final &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
> +	final &= ~DREF_SSC1_ENABLE;
> +
> +	if (has_panel) {
> +		final |= DREF_SSC_SOURCE_ENABLE;
> +
> +		if (intel_panel_use_ssc(dev_priv) && can_ssc)
> +			final |= DREF_SSC1_ENABLE;
> +
> +		if (has_cpu_edp) {
> +			if (intel_panel_use_ssc(dev_priv) && can_ssc)
> +				final |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
> +			else
> +				final |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
> +		} else
> +			final |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
> +	} else {
> +		final |= DREF_SSC_SOURCE_DISABLE;
> +		final |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
> +	}

This would be clearer in a separate function with a 'check_only' flag,
then we could do away with the code below.

--Imre

> +
> +	if (final == val)
> +		return;
> +
>  	/* Always enable nonspread source */
> -	temp &= ~DREF_NONSPREAD_SOURCE_MASK;
> +	val &= ~DREF_NONSPREAD_SOURCE_MASK;
>  
>  	if (has_ck505)
> -		temp |= DREF_NONSPREAD_CK505_ENABLE;
> +		val |= DREF_NONSPREAD_CK505_ENABLE;
>  	else
> -		temp |= DREF_NONSPREAD_SOURCE_ENABLE;
> +		val |= DREF_NONSPREAD_SOURCE_ENABLE;
>  
>  	if (has_panel) {
> -		temp &= ~DREF_SSC_SOURCE_MASK;
> -		temp |= DREF_SSC_SOURCE_ENABLE;
> +		val &= ~DREF_SSC_SOURCE_MASK;
> +		val |= DREF_SSC_SOURCE_ENABLE;
>  
>  		/* SSC must be turned on before enabling the CPU output  */
>  		if (intel_panel_use_ssc(dev_priv) && can_ssc) {
>  			DRM_DEBUG_KMS("Using SSC on panel\n");
> -			temp |= DREF_SSC1_ENABLE;
> +			val |= DREF_SSC1_ENABLE;
>  		} else
> -			temp &= ~DREF_SSC1_ENABLE;
> +			val &= ~DREF_SSC1_ENABLE;
>  
>  		/* Get SSC going before enabling the outputs */
> -		I915_WRITE(PCH_DREF_CONTROL, temp);
> +		I915_WRITE(PCH_DREF_CONTROL, val);
>  		POSTING_READ(PCH_DREF_CONTROL);
>  		udelay(200);
>  
> -		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
> +		val &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
>  
>  		/* Enable CPU source on CPU attached eDP */
>  		if (has_cpu_edp) {
>  			if (intel_panel_use_ssc(dev_priv) && can_ssc) {
>  				DRM_DEBUG_KMS("Using SSC on eDP\n");
> -				temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
> +				val |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
>  			}
>  			else
> -				temp |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
> +				val |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
>  		} else
> -			temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
> +			val |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
>  
> -		I915_WRITE(PCH_DREF_CONTROL, temp);
> +		I915_WRITE(PCH_DREF_CONTROL, val);
>  		POSTING_READ(PCH_DREF_CONTROL);
>  		udelay(200);
>  	} else {
>  		DRM_DEBUG_KMS("Disabling SSC entirely\n");
>  
> -		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
> +		val &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
>  
>  		/* Turn off CPU output */
> -		temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
> +		val |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
>  
> -		I915_WRITE(PCH_DREF_CONTROL, temp);
> +		I915_WRITE(PCH_DREF_CONTROL, val);
>  		POSTING_READ(PCH_DREF_CONTROL);
>  		udelay(200);
>  
>  		/* Turn off the SSC source */
> -		temp &= ~DREF_SSC_SOURCE_MASK;
> -		temp |= DREF_SSC_SOURCE_DISABLE;
> +		val &= ~DREF_SSC_SOURCE_MASK;
> +		val |= DREF_SSC_SOURCE_DISABLE;
>  
>  		/* Turn off SSC1 */
> -		temp &= ~ DREF_SSC1_ENABLE;
> +		val &= ~ DREF_SSC1_ENABLE;
>  
> -		I915_WRITE(PCH_DREF_CONTROL, temp);
> +		I915_WRITE(PCH_DREF_CONTROL, val);
>  		POSTING_READ(PCH_DREF_CONTROL);
>  		udelay(200);
>  	}
> +
> +	BUG_ON(val != final);
>  }
>  
>  /* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */

Re: [PATCH 01/13] drm/i915: Skip modifying PCH DREF if not changing clock sources

[Jesse Barnes]

On Wed, 20 Mar 2013 14:17:00 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Modifying the clock sources (via the DREF control on the PCH) is a slow
> > multi-stage process as we need to let the clocks stabilise between each
> > stage. If we are not actually changing the clock sources, then we can
> > return early.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   83 +++++++++++++++++++++++++---------
> >  1 file changed, 61 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3e6dadf..f20555e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4758,7 +4758,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_mode_config *mode_config = &dev->mode_config;
> >  	struct intel_encoder *encoder;
> > -	u32 temp;
> > +	u32 val, final;
> >  	bool has_lvds = false;
> >  	bool has_cpu_edp = false;
> >  	bool has_pch_edp = false;
> > @@ -4801,70 +4801,109 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
> >  	 * PCH B stepping, previous chipset stepping should be
> >  	 * ignoring this setting.
> >  	 */
> > -	temp = I915_READ(PCH_DREF_CONTROL);
> > +	val = I915_READ(PCH_DREF_CONTROL);
> > +
> > +	/* As we must carefully and slowly disable/enable each source in turn,
> > +	 * compute the final state we want first and check if we need to
> > +	 * make any changes at all.
> > +	 */
> > +	final = val;
> > +	final &= ~DREF_NONSPREAD_SOURCE_MASK;
> > +	if (has_ck505)
> > +		final |= DREF_NONSPREAD_CK505_ENABLE;
> > +	else
> > +		final |= DREF_NONSPREAD_SOURCE_ENABLE;
> > +
> > +	final &= ~DREF_SSC_SOURCE_MASK;
> > +	final &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
> > +	final &= ~DREF_SSC1_ENABLE;
> > +
> > +	if (has_panel) {
> > +		final |= DREF_SSC_SOURCE_ENABLE;
> > +
> > +		if (intel_panel_use_ssc(dev_priv) && can_ssc)
> > +			final |= DREF_SSC1_ENABLE;
> > +
> > +		if (has_cpu_edp) {
> > +			if (intel_panel_use_ssc(dev_priv) && can_ssc)
> > +				final |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
> > +			else
> > +				final |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
> > +		} else
> > +			final |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
> > +	} else {
> > +		final |= DREF_SSC_SOURCE_DISABLE;
> > +		final |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
> > +	}
> 
> This would be clearer in a separate function with a 'check_only' flag,
> then we could do away with the code below.

Yeah, it would shorten things a bit, but since we only have one caller
right now I'll leave that cleanup alone.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 02/13] drm/i915: Introduce i915_gem_object_create_stolen_for_preallocated

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

Wrap a preallocated region of stolen memory within an ordinary GEM
object, for example the BIOS framebuffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h        |    5 +++
 drivers/gpu/drm/i915/i915_gem_stolen.c |   65 ++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e95337c..9b5478f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1717,6 +1717,11 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev);
 void i915_gem_cleanup_stolen(struct drm_device *dev);
 struct drm_i915_gem_object *
 i915_gem_object_create_stolen(struct drm_device *dev, u32 size);
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
+					       u32 stolen_offset,
+					       u32 gtt_offset,
+					       u32 size);
 void i915_gem_object_release_stolen(struct drm_i915_gem_object *obj);
 
 /* i915_gem_tiling.c */
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 9f01332..7f1735c 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -309,6 +309,71 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	return NULL;
 }
 
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
+					       u32 stolen_offset,
+					       u32 gtt_offset,
+					       u32 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+
+	if (dev_priv->mm.stolen_base == 0)
+		return NULL;
+
+	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
+			stolen_offset, gtt_offset, size);
+
+	/* KISS and expect everything to be page-aligned */
+	BUG_ON(stolen_offset & 4095);
+	BUG_ON(gtt_offset & 4095);
+	BUG_ON(size & 4095);
+
+	if (WARN_ON(size == 0))
+		return NULL;
+
+	stolen = drm_mm_create_block(&dev_priv->mm.stolen,
+				     stolen_offset, size,
+				     false);
+	if (stolen == NULL) {
+		DRM_DEBUG_KMS("failed to allocate stolen space\n");
+		return NULL;
+	}
+
+	obj = _i915_gem_object_create_stolen(dev, stolen);
+	if (obj == NULL) {
+		DRM_DEBUG_KMS("failed to allocate stolen object\n");
+		drm_mm_put_block(stolen);
+		return NULL;
+	}
+
+	/* To simplify the initialisation sequence between KMS and GTT,
+	 * we allow construction of the stolen object prior to
+	 * setting up the GTT space. The actual reservation will occur
+	 * later.
+	 */
+	if (drm_mm_initialized(&dev_priv->mm.gtt_space)) {
+		obj->gtt_space = drm_mm_create_block(&dev_priv->mm.gtt_space,
+						     gtt_offset, size,
+						     false);
+		if (obj->gtt_space == NULL) {
+			DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
+			drm_gem_object_unreference(&obj->base);
+			return NULL;
+		}
+	} else
+		obj->gtt_space = I915_GTT_RESERVED;
+
+	obj->gtt_offset = gtt_offset;
+	obj->has_global_gtt_mapping = 1;
+
+	list_add_tail(&obj->gtt_list, &dev_priv->mm.bound_list);
+	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
+
+	return obj;
+}
+
 void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
-- 
1.7.9.5

Re: [PATCH 02/13] drm/i915: Introduce i915_gem_object_create_stolen_for_preallocated

[Daniel Vetter]

On Tue, Feb 19, 2013 at 01:31:37PM -0800, Jesse Barnes wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Wrap a preallocated region of stolen memory within an ordinary GEM
> object, for example the BIOS framebuffer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch. My merge plan here still stands
like it was a few weeks ago: I'd like to slurp in patches 2-5 asap (i.e.
up to the framebuffer wrapping), but I think the inital mode readout
should be rebased on top of the pipe_config infrastructure.

For patch 1 I still think that would look better when we just move the pch
refclock init into the global modeset resources stuff - we need do to such
things with our refclock handling anyway for some cute hsw features.

But since the other patches in the fb wrapping part seem to have pending
review request from Imre, I'll punt on the others for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 02/13] drm/i915: Introduce i915_gem_object_create_stolen_for_preallocated

[Jesse Barnes]

On Tue, 26 Mar 2013 20:46:24 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Feb 19, 2013 at 01:31:37PM -0800, Jesse Barnes wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Wrap a preallocated region of stolen memory within an ordinary GEM
> > object, for example the BIOS framebuffer.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Queued for -next, thanks for the patch. My merge plan here still stands
> like it was a few weeks ago: I'd like to slurp in patches 2-5 asap (i.e.
> up to the framebuffer wrapping), but I think the inital mode readout
> should be rebased on top of the pipe_config infrastructure.
> 
> For patch 1 I still think that would look better when we just move the pch
> refclock init into the global modeset resources stuff - we need do to such
> things with our refclock handling anyway for some cute hsw features.
> 
> But since the other patches in the fb wrapping part seem to have pending
> review request from Imre, I'll punt on the others for now.

Ok thanks, I'll update with Imre's comments and see about the PCH
refclk bits and repost the series.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 03/13] drm/i915: Split the framebuffer_info creation into a separate routine

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

This will be shared with wrapping the BIOS framebuffer into the fbdev
later. In the meantime, we can tidy the code slightly and improve the
error path handling.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    7 --
 drivers/gpu/drm/i915/intel_drv.h     |    7 ++
 drivers/gpu/drm/i915/intel_fb.c      |  154 ++++++++++++++++++----------------
 3 files changed, 91 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f20555e..dc58b01 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6422,13 +6422,6 @@ intel_framebuffer_create(struct drm_device *dev,
 }
 
 static u32
-intel_framebuffer_pitch_for_width(int width, int bpp)
-{
-	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
-	return ALIGN(pitch, 64);
-}
-
-static u32
 intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
 {
 	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 005a91f..f93653d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -134,6 +134,13 @@ struct intel_framebuffer {
 	struct drm_i915_gem_object *obj;
 };
 
+inline static u32
+intel_framebuffer_pitch_for_width(int width, int bpp)
+{
+	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
+	return ALIGN(pitch, 64);
+}
+
 struct intel_fbdev {
 	struct drm_fb_helper helper;
 	struct intel_framebuffer ifb;
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 1c510da..5afc31b 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -57,29 +57,96 @@ static struct fb_ops intelfb_ops = {
 	.fb_debug_leave = drm_fb_helper_debug_leave,
 };
 
+static struct fb_info *intelfb_create_info(struct intel_fbdev *ifbdev)
+{
+	struct drm_framebuffer *fb = &ifbdev->ifb.base;
+	struct drm_device *dev = fb->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct fb_info *info;
+	u32 gtt_offset, size;
+	int ret;
+
+	info = framebuffer_alloc(0, &dev->pdev->dev);
+	if (!info)
+		return NULL;
+
+	info->par = ifbdev;
+	ifbdev->helper.fb = fb;
+	ifbdev->helper.fbdev = info;
+
+	strcpy(info->fix.id, "inteldrmfb");
+
+	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
+	info->fbops = &intelfb_ops;
+
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret)
+		goto err_info;
+
+	/* setup aperture base/size for vesafb takeover */
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures)
+		goto err_cmap;
+
+	info->apertures->ranges[0].base = dev->mode_config.fb_base;
+	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
+
+	gtt_offset = ifbdev->ifb.obj->gtt_offset;
+	size = ifbdev->ifb.obj->base.size;
+
+	info->fix.smem_start = dev->mode_config.fb_base + gtt_offset;
+	info->fix.smem_len = size;
+
+	info->screen_size = size;
+	info->screen_base = ioremap_wc(dev_priv->gtt.mappable_base + gtt_offset,
+				       size);
+	if (!info->screen_base)
+		goto err_cmap;
+
+	/* If the object is shmemfs backed, it will have given us zeroed pages.
+	 * If the object is stolen however, it will be full of whatever
+	 * garbage was left in there.
+	 */
+	if (ifbdev->ifb.obj->stolen)
+		memset_io(info->screen_base, 0, info->screen_size);
+
+	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
+
+	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
+	drm_fb_helper_fill_var(info, &ifbdev->helper, fb->width, fb->height);
+
+	return info;
+
+err_cmap:
+	if (info->cmap.len)
+		fb_dealloc_cmap(&info->cmap);
+err_info:
+	framebuffer_release(info);
+	return NULL;
+}
+
 static int intelfb_create(struct intel_fbdev *ifbdev,
 			  struct drm_fb_helper_surface_size *sizes)
 {
 	struct drm_device *dev = ifbdev->helper.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct fb_info *info;
-	struct drm_framebuffer *fb;
-	struct drm_mode_fb_cmd2 mode_cmd = {};
+	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 	struct drm_i915_gem_object *obj;
-	struct device *device = &dev->pdev->dev;
+	struct fb_info *info;
 	int size, ret;
 
 	/* we don't do packed 24bpp */
 	if (sizes->surface_bpp == 24)
 		sizes->surface_bpp = 32;
 
-	mode_cmd.width = sizes->surface_width;
+	mode_cmd.width  = sizes->surface_width;
 	mode_cmd.height = sizes->surface_height;
 
-	mode_cmd.pitches[0] = ALIGN(mode_cmd.width * ((sizes->surface_bpp + 7) /
-						      8), 64);
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-							  sizes->surface_depth);
+	mode_cmd.pitches[0] =
+		intel_framebuffer_pitch_for_width(mode_cmd.width,
+						  sizes->surface_bpp);
+	mode_cmd.pixel_format =
+		drm_mode_legacy_fb_format(sizes->surface_bpp,
+					  sizes->surface_depth);
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = ALIGN(size, PAGE_SIZE);
@@ -101,72 +168,19 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 		goto out_unref;
 	}
 
-	info = framebuffer_alloc(0, device);
-	if (!info) {
-		ret = -ENOMEM;
-		goto out_unpin;
-	}
-
-	info->par = ifbdev;
-
 	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
 	if (ret)
 		goto out_unpin;
 
-	fb = &ifbdev->ifb.base;
-
-	ifbdev->helper.fb = fb;
-	ifbdev->helper.fbdev = info;
-
-	strcpy(info->fix.id, "inteldrmfb");
-
-	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
-	info->fbops = &intelfb_ops;
+	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
+		      mode_cmd.width, mode_cmd.height,
+		      obj->gtt_offset, obj);
 
-	ret = fb_alloc_cmap(&info->cmap, 256, 0);
-	if (ret) {
-		ret = -ENOMEM;
-		goto out_unpin;
-	}
-	/* setup aperture base/size for vesafb takeover */
-	info->apertures = alloc_apertures(1);
-	if (!info->apertures) {
+	info = intelfb_create_info(ifbdev);
+	if (info == NULL) {
 		ret = -ENOMEM;
 		goto out_unpin;
 	}
-	info->apertures->ranges[0].base = dev->mode_config.fb_base;
-	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
-
-	info->fix.smem_start = dev->mode_config.fb_base + obj->gtt_offset;
-	info->fix.smem_len = size;
-
-	info->screen_base =
-		ioremap_wc(dev_priv->gtt.mappable_base + obj->gtt_offset,
-			   size);
-	if (!info->screen_base) {
-		ret = -ENOSPC;
-		goto out_unpin;
-	}
-	info->screen_size = size;
-
-//	memset(info->screen_base, 0, size);
-
-	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
-	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
-
-	/* If the object is shmemfs backed, it will have given us zeroed pages.
-	 * If the object is stolen however, it will be full of whatever
-	 * garbage was left in there.
-	 */
-	if (ifbdev->ifb.obj->stolen)
-		memset_io(info->screen_base, 0, info->screen_size);
-
-	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
-
-	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
-		      fb->width, fb->height,
-		      obj->gtt_offset, obj);
-
 
 	mutex_unlock(&dev->struct_mutex);
 	vga_switcheroo_client_fb_set(dev->pdev, info);
@@ -206,11 +220,11 @@ static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 static void intel_fbdev_destroy(struct drm_device *dev,
 				struct intel_fbdev *ifbdev)
 {
-	struct fb_info *info;
 	struct intel_framebuffer *ifb = &ifbdev->ifb;
 
 	if (ifbdev->helper.fbdev) {
-		info = ifbdev->helper.fbdev;
+		struct fb_info *info = ifbdev->helper.fbdev;
+
 		unregister_framebuffer(info);
 		iounmap(info->screen_base);
 		if (info->cmap.len)
-- 
1.7.9.5

Re: [PATCH 03/13] drm/i915: Split the framebuffer_info creation into a separate routine

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This will be shared with wrapping the BIOS framebuffer into the fbdev
> later. In the meantime, we can tidy the code slightly and improve the
> error path handling.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 --
>  drivers/gpu/drm/i915/intel_drv.h     |    7 ++
>  drivers/gpu/drm/i915/intel_fb.c      |  154 ++++++++++++++++++----------------
>  3 files changed, 91 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f20555e..dc58b01 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6422,13 +6422,6 @@ intel_framebuffer_create(struct drm_device *dev,
>  }
>  
>  static u32
> -intel_framebuffer_pitch_for_width(int width, int bpp)
> -{
> -	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> -	return ALIGN(pitch, 64);
> -}
> -
> -static u32
>  intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
>  {
>  	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 005a91f..f93653d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -134,6 +134,13 @@ struct intel_framebuffer {
>  	struct drm_i915_gem_object *obj;
>  };
>  
> +inline static u32
> +intel_framebuffer_pitch_for_width(int width, int bpp)
> +{
> +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> +	return ALIGN(pitch, 64);
> +}
> +
>  struct intel_fbdev {
>  	struct drm_fb_helper helper;
>  	struct intel_framebuffer ifb;
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 1c510da..5afc31b 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -57,29 +57,96 @@ static struct fb_ops intelfb_ops = {
>  	.fb_debug_leave = drm_fb_helper_debug_leave,
>  };
>  
> +static struct fb_info *intelfb_create_info(struct intel_fbdev *ifbdev)
> +{
> +	struct drm_framebuffer *fb = &ifbdev->ifb.base;
> +	struct drm_device *dev = fb->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct fb_info *info;
> +	u32 gtt_offset, size;
> +	int ret;
> +
> +	info = framebuffer_alloc(0, &dev->pdev->dev);
> +	if (!info)
> +		return NULL;
> +
> +	info->par = ifbdev;
> +	ifbdev->helper.fb = fb;
> +	ifbdev->helper.fbdev = info;
> +
> +	strcpy(info->fix.id, "inteldrmfb");
> +
> +	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
> +	info->fbops = &intelfb_ops;
> +
> +	ret = fb_alloc_cmap(&info->cmap, 256, 0);
> +	if (ret)
> +		goto err_info;
> +
> +	/* setup aperture base/size for vesafb takeover */
> +	info->apertures = alloc_apertures(1);
> +	if (!info->apertures)
> +		goto err_cmap;
> +
> +	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> +	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
> +
> +	gtt_offset = ifbdev->ifb.obj->gtt_offset;
> +	size = ifbdev->ifb.obj->base.size;
> +
> +	info->fix.smem_start = dev->mode_config.fb_base + gtt_offset;
> +	info->fix.smem_len = size;
> +
> +	info->screen_size = size;
> +	info->screen_base = ioremap_wc(dev_priv->gtt.mappable_base + gtt_offset,
> +				       size);
> +	if (!info->screen_base)

kfree(info->apertures) is missing. The same goes for
intel_fbdev_destroy().

> +		goto err_cmap;
> +
> +	/* If the object is shmemfs backed, it will have given us zeroed pages.
> +	 * If the object is stolen however, it will be full of whatever
> +	 * garbage was left in there.
> +	 */
> +	if (ifbdev->ifb.obj->stolen)
> +		memset_io(info->screen_base, 0, info->screen_size);
> +
> +	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> +
> +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> +	drm_fb_helper_fill_var(info, &ifbdev->helper, fb->width, fb->height);
> +
> +	return info;
> +
> +err_cmap:
> +	if (info->cmap.len)
> +		fb_dealloc_cmap(&info->cmap);

Should be fine to call w/o checking cmap.len.

> +err_info:
> +	framebuffer_release(info);
> +	return NULL;
> +}
> +
>  static int intelfb_create(struct intel_fbdev *ifbdev,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
>  	struct drm_device *dev = ifbdev->helper.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct fb_info *info;
> -	struct drm_framebuffer *fb;
> -	struct drm_mode_fb_cmd2 mode_cmd = {};
> +	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>  	struct drm_i915_gem_object *obj;
> -	struct device *device = &dev->pdev->dev;
> +	struct fb_info *info;
>  	int size, ret;
>  
>  	/* we don't do packed 24bpp */
>  	if (sizes->surface_bpp == 24)
>  		sizes->surface_bpp = 32;
>  
> -	mode_cmd.width = sizes->surface_width;
> +	mode_cmd.width  = sizes->surface_width;
>  	mode_cmd.height = sizes->surface_height;
>  
> -	mode_cmd.pitches[0] = ALIGN(mode_cmd.width * ((sizes->surface_bpp + 7) /
> -						      8), 64);
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -							  sizes->surface_depth);
> +	mode_cmd.pitches[0] =
> +		intel_framebuffer_pitch_for_width(mode_cmd.width,
> +						  sizes->surface_bpp);

This changes the way pitches[0] is calculated for surface_bpp % 8 != 0,
but there's no mention of it in the commit message.

> +	mode_cmd.pixel_format =
> +		drm_mode_legacy_fb_format(sizes->surface_bpp,
> +					  sizes->surface_depth);
>  
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = ALIGN(size, PAGE_SIZE);
> @@ -101,72 +168,19 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
>  		goto out_unref;
>  	}
>  
> -	info = framebuffer_alloc(0, device);
> -	if (!info) {
> -		ret = -ENOMEM;
> -		goto out_unpin;
> -	}
> -
> -	info->par = ifbdev;
> -
>  	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
>  	if (ret)
>  		goto out_unpin;
>  
> -	fb = &ifbdev->ifb.base;
> -
> -	ifbdev->helper.fb = fb;
> -	ifbdev->helper.fbdev = info;
> -
> -	strcpy(info->fix.id, "inteldrmfb");
> -
> -	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
> -	info->fbops = &intelfb_ops;
> +	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
> +		      mode_cmd.width, mode_cmd.height,
> +		      obj->gtt_offset, obj);
>  
> -	ret = fb_alloc_cmap(&info->cmap, 256, 0);
> -	if (ret) {
> -		ret = -ENOMEM;
> -		goto out_unpin;
> -	}
> -	/* setup aperture base/size for vesafb takeover */
> -	info->apertures = alloc_apertures(1);
> -	if (!info->apertures) {
> +	info = intelfb_create_info(ifbdev);
> +	if (info == NULL) {
>  		ret = -ENOMEM;
>  		goto out_unpin;
>  	}
> -	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> -	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
> -
> -	info->fix.smem_start = dev->mode_config.fb_base + obj->gtt_offset;
> -	info->fix.smem_len = size;
> -
> -	info->screen_base =
> -		ioremap_wc(dev_priv->gtt.mappable_base + obj->gtt_offset,
> -			   size);
> -	if (!info->screen_base) {
> -		ret = -ENOSPC;
> -		goto out_unpin;
> -	}
> -	info->screen_size = size;
> -
> -//	memset(info->screen_base, 0, size);
> -
> -	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> -	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
> -
> -	/* If the object is shmemfs backed, it will have given us zeroed pages.
> -	 * If the object is stolen however, it will be full of whatever
> -	 * garbage was left in there.
> -	 */
> -	if (ifbdev->ifb.obj->stolen)
> -		memset_io(info->screen_base, 0, info->screen_size);
> -
> -	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> -
> -	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n",
> -		      fb->width, fb->height,
> -		      obj->gtt_offset, obj);
> -
>  
>  	mutex_unlock(&dev->struct_mutex);
>  	vga_switcheroo_client_fb_set(dev->pdev, info);
> @@ -206,11 +220,11 @@ static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  static void intel_fbdev_destroy(struct drm_device *dev,
>  				struct intel_fbdev *ifbdev)
>  {
> -	struct fb_info *info;
>  	struct intel_framebuffer *ifb = &ifbdev->ifb;
>  
>  	if (ifbdev->helper.fbdev) {
> -		info = ifbdev->helper.fbdev;
> +		struct fb_info *info = ifbdev->helper.fbdev;
> +
>  		unregister_framebuffer(info);
>  		iounmap(info->screen_base);
>  		if (info->cmap.len)

Re: [PATCH 03/13] drm/i915: Split the framebuffer_info creation into a separate routine

[Jesse Barnes]

On Wed, 20 Mar 2013 14:23:48 +0200
Imre Deak <imre.deak@intel.com> wrote:

> > +	if (!info->screen_base)
> 
> kfree(info->apertures) is missing. The same goes for
> intel_fbdev_destroy().

Fixed in both places.

> 
> > +		goto err_cmap;
> > +
> > +	/* If the object is shmemfs backed, it will have given us zeroed pages.
> > +	 * If the object is stolen however, it will be full of whatever
> > +	 * garbage was left in there.
> > +	 */
> > +	if (ifbdev->ifb.obj->stolen)
> > +		memset_io(info->screen_base, 0, info->screen_size);
> > +
> > +	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> > +
> > +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > +	drm_fb_helper_fill_var(info, &ifbdev->helper, fb->width, fb->height);
> > +
> > +	return info;
> > +
> > +err_cmap:
> > +	if (info->cmap.len)
> > +		fb_dealloc_cmap(&info->cmap);
> 
> Should be fine to call w/o checking cmap.len.

Fixed in both places.

> 
> > +err_info:
> > +	framebuffer_release(info);
> > +	return NULL;
> > +}
> > +
> >  static int intelfb_create(struct intel_fbdev *ifbdev,
> >  			  struct drm_fb_helper_surface_size *sizes)
> >  {
> >  	struct drm_device *dev = ifbdev->helper.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct fb_info *info;
> > -	struct drm_framebuffer *fb;
> > -	struct drm_mode_fb_cmd2 mode_cmd = {};
> > +	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> >  	struct drm_i915_gem_object *obj;
> > -	struct device *device = &dev->pdev->dev;
> > +	struct fb_info *info;
> >  	int size, ret;
> >  
> >  	/* we don't do packed 24bpp */
> >  	if (sizes->surface_bpp == 24)
> >  		sizes->surface_bpp = 32;
> >  
> > -	mode_cmd.width = sizes->surface_width;
> > +	mode_cmd.width  = sizes->surface_width;
> >  	mode_cmd.height = sizes->surface_height;
> >  
> > -	mode_cmd.pitches[0] = ALIGN(mode_cmd.width * ((sizes->surface_bpp + 7) /
> > -						      8), 64);
> > -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > -							  sizes->surface_depth);
> > +	mode_cmd.pitches[0] =
> > +		intel_framebuffer_pitch_for_width(mode_cmd.width,
> > +						  sizes->surface_bpp);
> 
> This changes the way pitches[0] is calculated for surface_bpp % 8 != 0,
> but there's no mention of it in the commit message.

It just removes the open coding; we still do the rounding and alignment
to 64 bytes.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 03/13] drm/i915: Split the framebuffer_info creation into a separate routine

[Imre Deak]

On Tue, 2013-03-26 at 16:07 -0700, Jesse Barnes wrote:
> On Wed, 20 Mar 2013 14:23:48 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > > +	if (!info->screen_base)
> > 
> > kfree(info->apertures) is missing. The same goes for
> > intel_fbdev_destroy().
> 
> Fixed in both places.
> 
> > 
> > > +		goto err_cmap;
> > > +
> > > +	/* If the object is shmemfs backed, it will have given us zeroed pages.
> > > +	 * If the object is stolen however, it will be full of whatever
> > > +	 * garbage was left in there.
> > > +	 */
> > > +	if (ifbdev->ifb.obj->stolen)
> > > +		memset_io(info->screen_base, 0, info->screen_size);
> > > +
> > > +	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> > > +
> > > +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > > +	drm_fb_helper_fill_var(info, &ifbdev->helper, fb->width, fb->height);
> > > +
> > > +	return info;
> > > +
> > > +err_cmap:
> > > +	if (info->cmap.len)
> > > +		fb_dealloc_cmap(&info->cmap);
> > 
> > Should be fine to call w/o checking cmap.len.
> 
> Fixed in both places.
> 
> > 
> > > +err_info:
> > > +	framebuffer_release(info);
> > > +	return NULL;
> > > +}
> > > +
> > >  static int intelfb_create(struct intel_fbdev *ifbdev,
> > >  			  struct drm_fb_helper_surface_size *sizes)
> > >  {
> > >  	struct drm_device *dev = ifbdev->helper.dev;
> > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	struct fb_info *info;
> > > -	struct drm_framebuffer *fb;
> > > -	struct drm_mode_fb_cmd2 mode_cmd = {};
> > > +	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> > >  	struct drm_i915_gem_object *obj;
> > > -	struct device *device = &dev->pdev->dev;
> > > +	struct fb_info *info;
> > >  	int size, ret;
> > >  
> > >  	/* we don't do packed 24bpp */
> > >  	if (sizes->surface_bpp == 24)
> > >  		sizes->surface_bpp = 32;
> > >  
> > > -	mode_cmd.width = sizes->surface_width;
> > > +	mode_cmd.width  = sizes->surface_width;
> > >  	mode_cmd.height = sizes->surface_height;
> > >  
> > > -	mode_cmd.pitches[0] = ALIGN(mode_cmd.width * ((sizes->surface_bpp + 7) /
> > > -						      8), 64);
> > > -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > > -							  sizes->surface_depth);
> > > +	mode_cmd.pitches[0] =
> > > +		intel_framebuffer_pitch_for_width(mode_cmd.width,
> > > +						  sizes->surface_bpp);
> > 
> > This changes the way pitches[0] is calculated for surface_bpp % 8 != 0,
> > but there's no mention of it in the commit message.
> 
> It just removes the open coding; we still do the rounding and alignment
> to 64 bytes.

Yea, but you get different results due to the different way of rounding
for certain bpps. For example:

sizes->surface_bpp = 4
mode_cmd.width = 1000

You get pitches[0]=1024 with the old way and 512 with the new way.

--Imre

Re: [PATCH 03/13] drm/i915: Split the framebuffer_info creation into a separate routine

[Chris Wilson]

On Wed, Mar 27, 2013 at 01:49:06PM +0200, Imre Deak wrote:
> On Tue, 2013-03-26 at 16:07 -0700, Jesse Barnes wrote:
> > On Wed, 20 Mar 2013 14:23:48 +0200
> > Imre Deak <imre.deak@intel.com> wrote:
> > 
> > > > +	if (!info->screen_base)
> > > 
> > > kfree(info->apertures) is missing. The same goes for
> > > intel_fbdev_destroy().
> > 
> > Fixed in both places.
> > 
> > > 
> > > > +		goto err_cmap;
> > > > +
> > > > +	/* If the object is shmemfs backed, it will have given us zeroed pages.
> > > > +	 * If the object is stolen however, it will be full of whatever
> > > > +	 * garbage was left in there.
> > > > +	 */
> > > > +	if (ifbdev->ifb.obj->stolen)
> > > > +		memset_io(info->screen_base, 0, info->screen_size);
> > > > +
> > > > +	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> > > > +
> > > > +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > > > +	drm_fb_helper_fill_var(info, &ifbdev->helper, fb->width, fb->height);
> > > > +
> > > > +	return info;
> > > > +
> > > > +err_cmap:
> > > > +	if (info->cmap.len)
> > > > +		fb_dealloc_cmap(&info->cmap);
> > > 
> > > Should be fine to call w/o checking cmap.len.
> > 
> > Fixed in both places.
> > 
> > > 
> > > > +err_info:
> > > > +	framebuffer_release(info);
> > > > +	return NULL;
> > > > +}
> > > > +
> > > >  static int intelfb_create(struct intel_fbdev *ifbdev,
> > > >  			  struct drm_fb_helper_surface_size *sizes)
> > > >  {
> > > >  	struct drm_device *dev = ifbdev->helper.dev;
> > > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > -	struct fb_info *info;
> > > > -	struct drm_framebuffer *fb;
> > > > -	struct drm_mode_fb_cmd2 mode_cmd = {};
> > > > +	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> > > >  	struct drm_i915_gem_object *obj;
> > > > -	struct device *device = &dev->pdev->dev;
> > > > +	struct fb_info *info;
> > > >  	int size, ret;
> > > >  
> > > >  	/* we don't do packed 24bpp */
> > > >  	if (sizes->surface_bpp == 24)
> > > >  		sizes->surface_bpp = 32;
> > > >  
> > > > -	mode_cmd.width = sizes->surface_width;
> > > > +	mode_cmd.width  = sizes->surface_width;
> > > >  	mode_cmd.height = sizes->surface_height;
> > > >  
> > > > -	mode_cmd.pitches[0] = ALIGN(mode_cmd.width * ((sizes->surface_bpp + 7) /
> > > > -						      8), 64);
> > > > -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > > > -							  sizes->surface_depth);
> > > > +	mode_cmd.pitches[0] =
> > > > +		intel_framebuffer_pitch_for_width(mode_cmd.width,
> > > > +						  sizes->surface_bpp);
> > > 
> > > This changes the way pitches[0] is calculated for surface_bpp % 8 != 0,
> > > but there's no mention of it in the commit message.
> > 
> > It just removes the open coding; we still do the rounding and alignment
> > to 64 bytes.
> 
> Yea, but you get different results due to the different way of rounding
> for certain bpps. For example:
> 
> sizes->surface_bpp = 4
> mode_cmd.width = 1000
> 
> You get pitches[0]=1024 with the old way and 512 with the new way.

Not a bug in Jesse's patch though, but an earlier one of mine trying to
use the kernel macros and failing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 04/13] drm: add initial_config function to fb helper

[Jesse Barnes]

Rather than building a config which may or may not work, let the driver
build an initial fb config.  This allows the driver to use the BIOS boot
configuration for example, displaying kernel messages and the initial fb
console on the same outputs the BIOS lit up at boot time.  If that
fails, the driver can still fall back the same way as the core.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_fb_helper.c |   23 +++++++++++++++--------
 include/drm/drm_fb_helper.h     |    4 ++++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0c6e25e..0d96471 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1275,7 +1275,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 	struct drm_mode_set *modeset;
 	bool *enabled;
 	int width, height;
-	int i, ret;
+	int i;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -1296,16 +1296,23 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
 
 	drm_enable_connectors(fb_helper, enabled);
 
-	ret = drm_target_cloned(fb_helper, modes, enabled, width, height);
-	if (!ret) {
-		ret = drm_target_preferred(fb_helper, modes, enabled, width, height);
-		if (!ret)
+	if (!(fb_helper->funcs->initial_config &&
+	      fb_helper->funcs->initial_config(fb_helper, crtcs, modes,
+					       enabled, width, height))) {
+		memset(modes, 0, dev->mode_config.num_connector*sizeof(modes[0]));
+		memset(crtcs, 0, dev->mode_config.num_connector*sizeof(crtcs[0]));
+
+		if (!drm_target_cloned(fb_helper,
+				       modes, enabled, width, height) &&
+		    !drm_target_preferred(fb_helper,
+					  modes, enabled, width, height))
 			DRM_ERROR("Unable to find initial modes\n");
-	}
 
-	DRM_DEBUG_KMS("picking CRTCs for %dx%d config\n", width, height);
+		DRM_DEBUG_KMS("picking CRTCs for %dx%d config\n",
+			      width, height);
 
-	drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
+		drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
+	}
 
 	/* need to set the modesets up here for use later */
 	/* fill out the connector<->crtc mappings into the modesets */
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 5120b01..40b0c5c 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -56,6 +56,10 @@ struct drm_fb_helper_funcs {
 
 	int (*fb_probe)(struct drm_fb_helper *helper,
 			struct drm_fb_helper_surface_size *sizes);
+	bool (*initial_config)(struct drm_fb_helper *fb_helper,
+			       struct drm_fb_helper_crtc **crtcs,
+			       struct drm_display_mode **modes,
+			       bool *enabled, int width, int height);
 };
 
 struct drm_fb_helper_connector {
-- 
1.7.9.5

Re: [PATCH 04/13] drm: add initial_config function to fb helper

[Daniel Vetter]

On Tue, Feb 19, 2013 at 01:31:39PM -0800, Jesse Barnes wrote:
> Rather than building a config which may or may not work, let the driver
> build an initial fb config.  This allows the driver to use the BIOS boot
> configuration for example, displaying kernel messages and the initial fb
> console on the same outputs the BIOS lit up at boot time.  If that
> fails, the driver can still fall back the same way as the core.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: dri-devel@lists.freedesktop.org
Queued for -next with Dave's irc ack, thanks for the patch. Note that
you've frobbed the authorship stuff a bit, probably through an amend,
without adding a patch reflog. Tsk ;-)
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c |   23 +++++++++++++++--------
>  include/drm/drm_fb_helper.h     |    4 ++++
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0c6e25e..0d96471 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1275,7 +1275,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  	struct drm_mode_set *modeset;
>  	bool *enabled;
>  	int width, height;
> -	int i, ret;
> +	int i;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> @@ -1296,16 +1296,23 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
>  
>  	drm_enable_connectors(fb_helper, enabled);
>  
> -	ret = drm_target_cloned(fb_helper, modes, enabled, width, height);
> -	if (!ret) {
> -		ret = drm_target_preferred(fb_helper, modes, enabled, width, height);
> -		if (!ret)
> +	if (!(fb_helper->funcs->initial_config &&
> +	      fb_helper->funcs->initial_config(fb_helper, crtcs, modes,
> +					       enabled, width, height))) {
> +		memset(modes, 0, dev->mode_config.num_connector*sizeof(modes[0]));
> +		memset(crtcs, 0, dev->mode_config.num_connector*sizeof(crtcs[0]));
> +
> +		if (!drm_target_cloned(fb_helper,
> +				       modes, enabled, width, height) &&
> +		    !drm_target_preferred(fb_helper,
> +					  modes, enabled, width, height))
>  			DRM_ERROR("Unable to find initial modes\n");
> -	}
>  
> -	DRM_DEBUG_KMS("picking CRTCs for %dx%d config\n", width, height);
> +		DRM_DEBUG_KMS("picking CRTCs for %dx%d config\n",
> +			      width, height);
>  
> -	drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
> +		drm_pick_crtcs(fb_helper, crtcs, modes, 0, width, height);
> +	}
>  
>  	/* need to set the modesets up here for use later */
>  	/* fill out the connector<->crtc mappings into the modesets */
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 5120b01..40b0c5c 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -56,6 +56,10 @@ struct drm_fb_helper_funcs {
>  
>  	int (*fb_probe)(struct drm_fb_helper *helper,
>  			struct drm_fb_helper_surface_size *sizes);
> +	bool (*initial_config)(struct drm_fb_helper *fb_helper,
> +			       struct drm_fb_helper_crtc **crtcs,
> +			       struct drm_display_mode **modes,
> +			       bool *enabled, int width, int height);
>  };
>  
>  struct drm_fb_helper_connector {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/13] drm: add initial_config function to fb helper

[Chris Wilson]

On Tue, Mar 26, 2013 at 09:34:29PM +0100, Daniel Vetter wrote:
> On Tue, Feb 19, 2013 at 01:31:39PM -0800, Jesse Barnes wrote:
> > Rather than building a config which may or may not work, let the driver
> > build an initial fb config.  This allows the driver to use the BIOS boot
> > configuration for example, displaying kernel messages and the initial fb
> > console on the same outputs the BIOS lit up at boot time.  If that
> > fails, the driver can still fall back the same way as the core.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: dri-devel@lists.freedesktop.org
> Queued for -next with Dave's irc ack, thanks for the patch. Note that
> you've frobbed the authorship stuff a bit, probably through an amend,
> without adding a patch reflog. Tsk ;-)

It's one of Jesse's patches that I substantially altered, the core idea
and implementation is still Jesse's. If I remember the history
correctly, that is.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/13] drm: add initial_config function to fb helper

[Jesse Barnes]

On Tue, 26 Mar 2013 20:52:53 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, Mar 26, 2013 at 09:34:29PM +0100, Daniel Vetter wrote:
> > On Tue, Feb 19, 2013 at 01:31:39PM -0800, Jesse Barnes wrote:
> > > Rather than building a config which may or may not work, let the driver
> > > build an initial fb config.  This allows the driver to use the BIOS boot
> > > configuration for example, displaying kernel messages and the initial fb
> > > console on the same outputs the BIOS lit up at boot time.  If that
> > > fails, the driver can still fall back the same way as the core.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: dri-devel@lists.freedesktop.org
> > Queued for -next with Dave's irc ack, thanks for the patch. Note that
> > you've frobbed the authorship stuff a bit, probably through an amend,
> > without adding a patch reflog. Tsk ;-)
> 
> It's one of Jesse's patches that I substantially altered, the core idea
> and implementation is still Jesse's. If I remember the history
> correctly, that is.

Yeah either way is fine with me, thanks for applying.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 05/13] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c      |    8 +-
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   14 +-
 drivers/gpu/drm/i915/intel_drv.h     |    4 +
 drivers/gpu/drm/i915/intel_fb.c      |  305 +++++++++++++++++++++++++++++++---
 5 files changed, 306 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4fa6beb..f2b7db7 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1273,6 +1273,7 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
 static int i915_load_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool was_vga_enabled;
 	int ret;
 
 	ret = intel_parse_bios(dev);
@@ -1309,7 +1310,11 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
-	intel_modeset_init(dev);
+	intel_modeset_init(dev, &was_vga_enabled);
+
+	/* Wrap existing BIOS mode configuration prior to GEM takeover */
+	if (!was_vga_enabled)
+		intel_fbdev_init_bios(dev);
 
 	ret = i915_gem_init(dev);
 	if (ret)
@@ -1323,6 +1328,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
 
+	/* Install a default KMS/GEM fbcon if we failed to wrap the BIOS fb */
 	ret = intel_fbdev_init(dev);
 	if (ret)
 		goto cleanup_gem;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b5478f..30cf7e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1811,7 +1811,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
 
 /* modesetting */
 extern void intel_modeset_init_hw(struct drm_device *dev);
-extern void intel_modeset_init(struct drm_device *dev);
+extern void intel_modeset_init(struct drm_device *dev, bool *was_vga_enabled);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dc58b01..9793e66 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8735,12 +8735,17 @@ static void intel_init_quirks(struct drm_device *dev)
 }
 
 /* Disable the VGA plane that we never use */
-static void i915_disable_vga(struct drm_device *dev)
+static bool i915_disable_vga(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool was_enabled;
 	u8 sr1;
 	u32 vga_reg = i915_vgacntrl_reg(dev);
 
+	was_enabled = !(I915_READ(vga_reg) & VGA_DISP_DISABLE);
+	DRM_DEBUG_KMS("VGA output is currently %s\n",
+		      was_enabled ? "enabled" : "disabled");
+
 	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
 	outb(SR01, VGA_SR_INDEX);
 	sr1 = inb(VGA_SR_DATA);
@@ -8750,6 +8755,8 @@ static void i915_disable_vga(struct drm_device *dev)
 
 	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
 	POSTING_READ(vga_reg);
+
+	return was_enabled;
 }
 
 void intel_modeset_init_hw(struct drm_device *dev)
@@ -8765,7 +8772,8 @@ void intel_modeset_init_hw(struct drm_device *dev)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-void intel_modeset_init(struct drm_device *dev)
+void intel_modeset_init(struct drm_device *dev,
+			bool *was_vga_enabled)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int i, ret;
@@ -8812,7 +8820,7 @@ void intel_modeset_init(struct drm_device *dev)
 	intel_pch_pll_init(dev);
 
 	/* Just disable it once at startup */
-	i915_disable_vga(dev);
+	*was_vga_enabled = i915_disable_vga(dev);
 	intel_setup_outputs(dev);
 
 	/* Just in case the BIOS is doing something questionable. */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f93653d..9cf794f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -146,6 +146,8 @@ struct intel_fbdev {
 	struct intel_framebuffer ifb;
 	struct list_head fbdev_list;
 	struct drm_display_mode *our_mode;
+	bool stolen;
+	int preferred_bpp;
 };
 
 struct intel_encoder {
@@ -212,6 +214,7 @@ struct intel_crtc {
 	enum plane plane;
 	enum transcoder cpu_transcoder;
 	u8 lut_r[256], lut_g[256], lut_b[256];
+	bool mode_valid;
 	/*
 	 * Whether the crtc and the connected output pipeline is active. Implies
 	 * that crtc->enabled is set, i.e. the current mode configuration has
@@ -616,6 +619,7 @@ extern int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
 				  struct drm_i915_gem_object *obj);
+extern void intel_fbdev_init_bios(struct drm_device *dev);
 extern int intel_fbdev_init(struct drm_device *dev);
 extern void intel_fbdev_initial_config(struct drm_device *dev);
 extern void intel_fbdev_fini(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 5afc31b..4ca2ee4 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -131,7 +131,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 	struct drm_device *dev = ifbdev->helper.dev;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
 	struct drm_i915_gem_object *obj;
-	struct fb_info *info;
 	int size, ret;
 
 	/* we don't do packed 24bpp */
@@ -176,14 +175,7 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 		      mode_cmd.width, mode_cmd.height,
 		      obj->gtt_offset, obj);
 
-	info = intelfb_create_info(ifbdev);
-	if (info == NULL) {
-		ret = -ENOMEM;
-		goto out_unpin;
-	}
-
 	mutex_unlock(&dev->struct_mutex);
-	vga_switcheroo_client_fb_set(dev->pdev, info);
 	return 0;
 
 out_unpin:
@@ -200,17 +192,92 @@ static int intel_fb_find_or_create_single(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev = (struct intel_fbdev *)helper;
 	int new_fb = 0;
-	int ret;
 
 	if (!helper->fb) {
-		ret = intelfb_create(ifbdev, sizes);
-		if (ret)
-			return ret;
+		struct fb_info *info;
+
+		if (!ifbdev->stolen) {
+			int ret = intelfb_create(ifbdev, sizes);
+			if (ret)
+				return ret;
+		}
+
+		info = intelfb_create_info(ifbdev);
+		if (info == NULL) {
+			DRM_DEBUG_KMS("fb creation failed\n");
+			return -ENOMEM;
+		}
+		vga_switcheroo_client_fb_set(helper->dev->pdev, info);
+
 		new_fb = 1;
 	}
+
 	return new_fb;
 }
 
+static struct drm_fb_helper_crtc *
+intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
+{
+	int i;
+
+	for (i = 0; i < fb_helper->crtc_count; i++)
+		if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
+			return &fb_helper->crtc_info[i];
+
+	return NULL;
+}
+
+static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
+				    struct drm_fb_helper_crtc **crtcs,
+				    struct drm_display_mode **modes,
+				    bool *enabled, int width, int height)
+{
+	int i;
+
+	for (i = 0; i < fb_helper->connector_count; i++) {
+		struct drm_connector *connector;
+		struct drm_encoder *encoder;
+
+		connector = fb_helper->connector_info[i]->connector;
+		if (!enabled[i]) {
+			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
+				      connector->base.id);
+			continue;
+		}
+
+		encoder = connector->encoder;
+		if (!encoder || !encoder->crtc) {
+			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
+				      connector->base.id);
+			continue;
+		}
+
+		if (WARN_ON(!encoder->crtc->enabled)) {
+			DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
+				      drm_get_connector_name(connector),
+				      encoder->crtc->base.id);
+			return false;
+		}
+
+		if (!to_intel_crtc(encoder->crtc)->mode_valid) {
+			DRM_DEBUG_KMS("connector %s on crtc %d has an invalid mode, aborting\n",
+				      drm_get_connector_name(connector),
+				      encoder->crtc->base.id);
+			return false;
+		}
+
+		modes[i] = &encoder->crtc->mode;
+		crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
+
+		DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
+			      drm_get_connector_name(connector),
+			      encoder->crtc->base.id,
+			      modes[i]->name);
+	}
+
+	return true;
+}
+
 static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 	.gamma_set = intel_crtc_fb_gamma_set,
 	.gamma_get = intel_crtc_fb_gamma_get,
@@ -242,23 +309,215 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	}
 }
 
-int intel_fbdev_init(struct drm_device *dev)
+static bool pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
+	enum transcoder cpu_transcoder =
+		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+	return !!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE);
+}
+
+/*
+ * Try to read the BIOS display configuration and use it for the initial
+ * fb configuration.
+ *
+ * The BIOS or boot loader will generally create an initial display
+ * configuration for us that includes some set of active pipes and displays.
+ * This routine tries to figure out which pipes are active, what resolutions
+ * are being displayed, and then allocates a framebuffer and initial fb
+ * config based on that data.
+ *
+ * If the BIOS or boot loader leaves the display in VGA mode, there's not
+ * much we can do; switching out of that mode involves allocating a new,
+ * high res buffer, and also recalculating bandwidth requirements for the
+ * new bpp configuration.
+ *
+ * However, if we're loaded into an existing, high res mode, we should
+ * be able to allocate a buffer big enough to handle the largest active
+ * mode, create a mode_set for it, and pass it to the fb helper to create
+ * the configuration.
+ */
+void intel_fbdev_init_bios(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_fbdev *ifbdev;
-	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret;
+	struct drm_crtc *crtc;
+	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+	struct drm_i915_gem_object *obj;
+	u32 obj_offset = 0;
+	int mode_bpp = 0;
+	u32 active = 0;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		int pipe = intel_crtc->pipe, plane = intel_crtc->plane;
+		u32 val, bpp, offset, format;
+		int pitch, width, height;
+
+		if (!pipe_enabled(dev_priv, pipe)) {
+			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
+				      pipe_name(pipe));
+			continue;
+		}
+
+		val = I915_READ(DSPCNTR(plane));
+
+		if (INTEL_INFO(dev)->gen >= 4) {
+			if (val & DISPPLANE_TILED) {
+				DRM_DEBUG_KMS("tiled BIOS fb?\n");
+				continue; /* unexpected! */
+			}
+		}
+
+		switch (val & DISPPLANE_PIXFORMAT_MASK) {
+		case DISPPLANE_YUV422:
+		default:
+			DRM_DEBUG_KMS("pipe %c unsupported pixel format %x, skipping\n",
+				      pipe_name(pipe), (val & DISPPLANE_PIXFORMAT_MASK) >> 26);
+			continue;
+		case DISPPLANE_8BPP:
+			format = DRM_FORMAT_C8;
+			bpp = 8;
+			break;
+		case DISPPLANE_BGRX555:
+			format = DRM_FORMAT_XRGB1555;
+			bpp = 16;
+			break;
+		case DISPPLANE_BGRX565:
+			format = DRM_FORMAT_RGB565;
+			bpp = 16;
+			break;
+		case DISPPLANE_BGRX888:
+			format = DRM_FORMAT_XRGB8888;
+			bpp = 32;
+			break;
+		}
+
+		if (mode_cmd.pixel_format == 0) {
+			mode_bpp = bpp;
+			mode_cmd.pixel_format = format;
+		}
+
+		if (mode_cmd.pixel_format != format) {
+			DRM_DEBUG_KMS("pipe %c has format/bpp (%d, %d) mismatch: skipping\n",
+				      pipe_name(pipe), format, bpp);
+			continue;
+		}
+
+		if (INTEL_INFO(dev)->gen >= 4) {
+			if (I915_READ(DSPTILEOFF(plane))) {
+				DRM_DEBUG_KMS("pipe %c is offset: skipping\n",
+					      pipe_name(pipe));
+				continue;
+			}
+
+			offset = I915_READ(DSPSURF(plane));
+		} else
+			offset = I915_READ(DSPADDR(plane));
+		if (!obj_offset)
+			obj_offset = offset;
+
+		pitch = I915_READ(DSPSTRIDE(plane));
+		if (mode_cmd.pitches[0] == 0)
+			mode_cmd.pitches[0] = pitch;
+
+		if (offset != obj_offset || pitch != mode_cmd.pitches[0]) {
+			DRM_DEBUG_KMS("multiple pipe setup not in clone mode, sjipping\n");
+			continue;
+		}
+
+		val = I915_READ(PIPESRC(pipe));
+		width = ((val >> 16) & 0xfff) + 1;
+		height = ((val >> 0) & 0xfff) + 1;
+
+		DRM_DEBUG_KMS("Found active pipe [%d/%d]: size=%dx%d@%d, offset=%x\n",
+			      pipe, plane, width, height, bpp, offset);
+
+		if (width > mode_cmd.width)
+			mode_cmd.width = width;
+
+		if (height > mode_cmd.height)
+			mode_cmd.height = height;
+
+		active |= 1 << pipe;
+	}
+
+	if (active == 0) {
+		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
+		return;
+	}
 
 	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
-	if (!ifbdev)
-		return -ENOMEM;
+	if (ifbdev == NULL) {
+		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
+		return;
+	}
 
-	dev_priv->fbdev = ifbdev;
+	ifbdev->stolen = true;
+	ifbdev->preferred_bpp = mode_bpp;
 	ifbdev->helper.funcs = &intel_fb_helper_funcs;
+	ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
+
+	/* assume a 1:1 linear mapping between stolen and GTT */
+	obj = i915_gem_object_create_stolen_for_preallocated(dev,
+							     obj_offset,
+							     obj_offset,
+							     ALIGN(mode_cmd.pitches[0] * mode_cmd.height, PAGE_SIZE));
+	if (obj == NULL) {
+		DRM_DEBUG_KMS("failed to create stolen fb\n");
+		goto out_free_ifbdev;
+	}
+
+	if (intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj)) {
+		DRM_DEBUG_KMS("intel fb init failed\n");
+		goto out_unref_obj;
+	}
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		int ret;
+
+		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
+			continue;
+
+		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+		if (ret)
+			goto out_unref_obj;
+
+		crtc->fb = &ifbdev->ifb.base;
+	}
+
+	dev_priv->fbdev = ifbdev;
+
+	DRM_DEBUG_KMS("using BIOS fb for initial console\n");
+	return;
+
+out_unref_obj:
+	drm_gem_object_unreference(&obj->base);
+out_free_ifbdev:
+	kfree(ifbdev);
+}
+
+int intel_fbdev_init(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_fbdev *ifbdev;
+	int ret;
+
+	if ((ifbdev = dev_priv->fbdev) == NULL) {
+		ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+		if (ifbdev == NULL)
+			return -ENOMEM;
+
+		ifbdev->helper.funcs = &intel_fb_helper_funcs;
+		ifbdev->preferred_bpp = 32;
+
+		dev_priv->fbdev = ifbdev;
+	}
 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
-				 dev_priv->num_pipe,
-				 INTELFB_CONN_LIMIT);
+			dev_priv->num_pipe,
+			INTELFB_CONN_LIMIT);
 	if (ret) {
+		dev_priv->fbdev = NULL;
 		kfree(ifbdev);
 		return ret;
 	}
@@ -271,9 +530,10 @@ int intel_fbdev_init(struct drm_device *dev)
 void intel_fbdev_initial_config(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
+	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
 }
 
 void intel_fbdev_fini(struct drm_device *dev)
@@ -301,7 +561,8 @@ MODULE_LICENSE("GPL and additional rights");
 void intel_fb_output_poll_changed(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+	if (dev_priv->fbdev)
+		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
 }
 
 void intel_fb_restore_mode(struct drm_device *dev)
-- 
1.7.9.5

Re: [PATCH 05/13] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |    8 +-
>  drivers/gpu/drm/i915/i915_drv.h      |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |   14 +-
>  drivers/gpu/drm/i915/intel_drv.h     |    4 +
>  drivers/gpu/drm/i915/intel_fb.c      |  305 +++++++++++++++++++++++++++++++---
>  5 files changed, 306 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 4fa6beb..f2b7db7 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1273,6 +1273,7 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
>  static int i915_load_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool was_vga_enabled;
>  	int ret;
>  
>  	ret = intel_parse_bios(dev);
> @@ -1309,7 +1310,11 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	/* Important: The output setup functions called by modeset_init need
>  	 * working irqs for e.g. gmbus and dp aux transfers. */
> -	intel_modeset_init(dev);
> +	intel_modeset_init(dev, &was_vga_enabled);
> +
> +	/* Wrap existing BIOS mode configuration prior to GEM takeover */
> +	if (!was_vga_enabled)
> +		intel_fbdev_init_bios(dev);
>  
>  	ret = i915_gem_init(dev);
>  	if (ret)
> @@ -1323,6 +1328,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	/* FIXME: do pre/post-mode set stuff in core KMS code */
>  	dev->vblank_disable_allowed = 1;
>  
> +	/* Install a default KMS/GEM fbcon if we failed to wrap the BIOS fb */
>  	ret = intel_fbdev_init(dev);
>  	if (ret)
>  		goto cleanup_gem;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9b5478f..30cf7e6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1811,7 +1811,7 @@ static inline void intel_unregister_dsm_handler(void) { return; }
>  
>  /* modesetting */
>  extern void intel_modeset_init_hw(struct drm_device *dev);
> -extern void intel_modeset_init(struct drm_device *dev);
> +extern void intel_modeset_init(struct drm_device *dev, bool *was_vga_enabled);
>  extern void intel_modeset_gem_init(struct drm_device *dev);
>  extern void intel_modeset_cleanup(struct drm_device *dev);
>  extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dc58b01..9793e66 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8735,12 +8735,17 @@ static void intel_init_quirks(struct drm_device *dev)
>  }
>  
>  /* Disable the VGA plane that we never use */
> -static void i915_disable_vga(struct drm_device *dev)
> +static bool i915_disable_vga(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool was_enabled;
>  	u8 sr1;
>  	u32 vga_reg = i915_vgacntrl_reg(dev);
>  
> +	was_enabled = !(I915_READ(vga_reg) & VGA_DISP_DISABLE);
> +	DRM_DEBUG_KMS("VGA output is currently %s\n",
> +		      was_enabled ? "enabled" : "disabled");
> +
>  	vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
>  	outb(SR01, VGA_SR_INDEX);
>  	sr1 = inb(VGA_SR_DATA);
> @@ -8750,6 +8755,8 @@ static void i915_disable_vga(struct drm_device *dev)
>  
>  	I915_WRITE(vga_reg, VGA_DISP_DISABLE);
>  	POSTING_READ(vga_reg);
> +
> +	return was_enabled;
>  }
>  
>  void intel_modeset_init_hw(struct drm_device *dev)
> @@ -8765,7 +8772,8 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -void intel_modeset_init(struct drm_device *dev)
> +void intel_modeset_init(struct drm_device *dev,
> +			bool *was_vga_enabled)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int i, ret;
> @@ -8812,7 +8820,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	intel_pch_pll_init(dev);
>  
>  	/* Just disable it once at startup */
> -	i915_disable_vga(dev);
> +	*was_vga_enabled = i915_disable_vga(dev);
>  	intel_setup_outputs(dev);
>  
>  	/* Just in case the BIOS is doing something questionable. */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f93653d..9cf794f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -146,6 +146,8 @@ struct intel_fbdev {
>  	struct intel_framebuffer ifb;
>  	struct list_head fbdev_list;
>  	struct drm_display_mode *our_mode;
> +	bool stolen;
> +	int preferred_bpp;
>  };
>  
>  struct intel_encoder {
> @@ -212,6 +214,7 @@ struct intel_crtc {
>  	enum plane plane;
>  	enum transcoder cpu_transcoder;
>  	u8 lut_r[256], lut_g[256], lut_b[256];
> +	bool mode_valid;
>  	/*
>  	 * Whether the crtc and the connected output pipeline is active. Implies
>  	 * that crtc->enabled is set, i.e. the current mode configuration has
> @@ -616,6 +619,7 @@ extern int intel_framebuffer_init(struct drm_device *dev,
>  				  struct intel_framebuffer *ifb,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
>  				  struct drm_i915_gem_object *obj);
> +extern void intel_fbdev_init_bios(struct drm_device *dev);
>  extern int intel_fbdev_init(struct drm_device *dev);
>  extern void intel_fbdev_initial_config(struct drm_device *dev);
>  extern void intel_fbdev_fini(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 5afc31b..4ca2ee4 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -131,7 +131,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
>  	struct drm_device *dev = ifbdev->helper.dev;
>  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>  	struct drm_i915_gem_object *obj;
> -	struct fb_info *info;
>  	int size, ret;
>  
>  	/* we don't do packed 24bpp */
> @@ -176,14 +175,7 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
>  		      mode_cmd.width, mode_cmd.height,
>  		      obj->gtt_offset, obj);
>  
> -	info = intelfb_create_info(ifbdev);
> -	if (info == NULL) {
> -		ret = -ENOMEM;
> -		goto out_unpin;
> -	}
> -
>  	mutex_unlock(&dev->struct_mutex);
> -	vga_switcheroo_client_fb_set(dev->pdev, info);
>  	return 0;
>  
>  out_unpin:
> @@ -200,17 +192,92 @@ static int intel_fb_find_or_create_single(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev = (struct intel_fbdev *)helper;
>  	int new_fb = 0;
> -	int ret;
>  
>  	if (!helper->fb) {
> -		ret = intelfb_create(ifbdev, sizes);
> -		if (ret)
> -			return ret;
> +		struct fb_info *info;
> +
> +		if (!ifbdev->stolen) {
> +			int ret = intelfb_create(ifbdev, sizes);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		info = intelfb_create_info(ifbdev);
> +		if (info == NULL) {
> +			DRM_DEBUG_KMS("fb creation failed\n");
> +			return -ENOMEM;
> +		}
> +		vga_switcheroo_client_fb_set(helper->dev->pdev, info);
> +
>  		new_fb = 1;
>  	}
> +
>  	return new_fb;
>  }
>  
> +static struct drm_fb_helper_crtc *
> +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> +{
> +	int i;
> +
> +	for (i = 0; i < fb_helper->crtc_count; i++)
> +		if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> +			return &fb_helper->crtc_info[i];
> +
> +	return NULL;
> +}
> +
> +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> +				    struct drm_fb_helper_crtc **crtcs,
> +				    struct drm_display_mode **modes,
> +				    bool *enabled, int width, int height)
> +{
> +	int i;
> +
> +	for (i = 0; i < fb_helper->connector_count; i++) {
> +		struct drm_connector *connector;
> +		struct drm_encoder *encoder;
> +
> +		connector = fb_helper->connector_info[i]->connector;
> +		if (!enabled[i]) {
> +			DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> +				      connector->base.id);
> +			continue;
> +		}
> +
> +		encoder = connector->encoder;
> +		if (!encoder || !encoder->crtc) {
> +			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
> +				      connector->base.id);
> +			continue;
> +		}
> +
> +		if (WARN_ON(!encoder->crtc->enabled)) {
> +			DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
> +				      drm_get_connector_name(connector),
> +				      encoder->crtc->base.id);
> +			return false;
> +		}
> +
> +		if (!to_intel_crtc(encoder->crtc)->mode_valid) {
> +			DRM_DEBUG_KMS("connector %s on crtc %d has an invalid mode, aborting\n",
> +				      drm_get_connector_name(connector),
> +				      encoder->crtc->base.id);
> +			return false;
> +		}
> +
> +		modes[i] = &encoder->crtc->mode;
> +		crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> +
> +		DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
> +			      drm_get_connector_name(connector),
> +			      encoder->crtc->base.id,
> +			      modes[i]->name);
> +	}
> +
> +	return true;
> +}
> +
>  static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  	.gamma_set = intel_crtc_fb_gamma_set,
>  	.gamma_get = intel_crtc_fb_gamma_get,
> @@ -242,23 +309,215 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>  	}
>  }
>  
> -int intel_fbdev_init(struct drm_device *dev)
> +static bool pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe)
>  {
> +	enum transcoder cpu_transcoder =
> +		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +	return !!(I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE);
> +}
> +
> +/*
> + * Try to read the BIOS display configuration and use it for the initial
> + * fb configuration.
> + *
> + * The BIOS or boot loader will generally create an initial display
> + * configuration for us that includes some set of active pipes and displays.
> + * This routine tries to figure out which pipes are active, what resolutions
> + * are being displayed, and then allocates a framebuffer and initial fb
> + * config based on that data.
> + *
> + * If the BIOS or boot loader leaves the display in VGA mode, there's not
> + * much we can do; switching out of that mode involves allocating a new,
> + * high res buffer, and also recalculating bandwidth requirements for the
> + * new bpp configuration.
> + *
> + * However, if we're loaded into an existing, high res mode, we should
> + * be able to allocate a buffer big enough to handle the largest active
> + * mode, create a mode_set for it, and pass it to the fb helper to create
> + * the configuration.
> + */
> +void intel_fbdev_init_bios(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_fbdev *ifbdev;
> -	drm_i915_private_t *dev_priv = dev->dev_private;
> -	int ret;
> +	struct drm_crtc *crtc;
> +	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> +	struct drm_i915_gem_object *obj;
> +	u32 obj_offset = 0;
> +	int mode_bpp = 0;
> +	u32 active = 0;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		int pipe = intel_crtc->pipe, plane = intel_crtc->plane;
> +		u32 val, bpp, offset, format;
> +		int pitch, width, height;
> +
> +		if (!pipe_enabled(dev_priv, pipe)) {
> +			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
> +				      pipe_name(pipe));
> +			continue;
> +		}
> +
> +		val = I915_READ(DSPCNTR(plane));
> +
> +		if (INTEL_INFO(dev)->gen >= 4) {
> +			if (val & DISPPLANE_TILED) {
> +				DRM_DEBUG_KMS("tiled BIOS fb?\n");
> +				continue; /* unexpected! */
> +			}
> +		}
> +
> +		switch (val & DISPPLANE_PIXFORMAT_MASK) {
> +		case DISPPLANE_YUV422:
> +		default:
> +			DRM_DEBUG_KMS("pipe %c unsupported pixel format %x, skipping\n",
> +				      pipe_name(pipe), (val & DISPPLANE_PIXFORMAT_MASK) >> 26);
> +			continue;
> +		case DISPPLANE_8BPP:
> +			format = DRM_FORMAT_C8;
> +			bpp = 8;
> +			break;
> +		case DISPPLANE_BGRX555:
> +			format = DRM_FORMAT_XRGB1555;
> +			bpp = 16;
> +			break;
> +		case DISPPLANE_BGRX565:
> +			format = DRM_FORMAT_RGB565;
> +			bpp = 16;
> +			break;
> +		case DISPPLANE_BGRX888:
> +			format = DRM_FORMAT_XRGB8888;
> +			bpp = 32;
> +			break;
> +		}
> +
> +		if (mode_cmd.pixel_format == 0) {
> +			mode_bpp = bpp;
> +			mode_cmd.pixel_format = format;
> +		}
> +
> +		if (mode_cmd.pixel_format != format) {
> +			DRM_DEBUG_KMS("pipe %c has format/bpp (%d, %d) mismatch: skipping\n",
> +				      pipe_name(pipe), format, bpp);
> +			continue;
> +		}
> +
> +		if (INTEL_INFO(dev)->gen >= 4) {
> +			if (I915_READ(DSPTILEOFF(plane))) {
> +				DRM_DEBUG_KMS("pipe %c is offset: skipping\n",
> +					      pipe_name(pipe));
> +				continue;
> +			}
> +
> +			offset = I915_READ(DSPSURF(plane));
> +		} else
> +			offset = I915_READ(DSPADDR(plane));

Nitpick: the second branch should be inside { } too.

> +		if (!obj_offset)
> +			obj_offset = offset;
> +
> +		pitch = I915_READ(DSPSTRIDE(plane));
> +		if (mode_cmd.pitches[0] == 0)
> +			mode_cmd.pitches[0] = pitch;
> +
> +		if (offset != obj_offset || pitch != mode_cmd.pitches[0]) {
> +			DRM_DEBUG_KMS("multiple pipe setup not in clone mode, sjipping\n");

s/sjipping/skipping/

> +			continue;
> +		}
> +
> +		val = I915_READ(PIPESRC(pipe));
> +		width = ((val >> 16) & 0xfff) + 1;
> +		height = ((val >> 0) & 0xfff) + 1;
> +
> +		DRM_DEBUG_KMS("Found active pipe [%d/%d]: size=%dx%d@%d, offset=%x\n",
> +			      pipe, plane, width, height, bpp, offset);
> +
> +		if (width > mode_cmd.width)
> +			mode_cmd.width = width;
> +
> +		if (height > mode_cmd.height)
> +			mode_cmd.height = height;
> +
> +		active |= 1 << pipe;
> +	}
> +
> +	if (active == 0) {
> +		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> +		return;
> +	}
>  
>  	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> -	if (!ifbdev)
> -		return -ENOMEM;
> +	if (ifbdev == NULL) {
> +		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> +		return;
> +	}
>  
> -	dev_priv->fbdev = ifbdev;
> +	ifbdev->stolen = true;
> +	ifbdev->preferred_bpp = mode_bpp;
>  	ifbdev->helper.funcs = &intel_fb_helper_funcs;
> +	ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> +
> +	/* assume a 1:1 linear mapping between stolen and GTT */
> +	obj = i915_gem_object_create_stolen_for_preallocated(dev,
> +							     obj_offset,
> +							     obj_offset,
> +							     ALIGN(mode_cmd.pitches[0] * mode_cmd.height, PAGE_SIZE));
> +	if (obj == NULL) {
> +		DRM_DEBUG_KMS("failed to create stolen fb\n");
> +		goto out_free_ifbdev;
> +	}
> +
> +	if (intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj)) {
> +		DRM_DEBUG_KMS("intel fb init failed\n");
> +		goto out_unref_obj;
> +	}
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		int ret;
> +
> +		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
> +			continue;
> +
> +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +		if (ret)
> +			goto out_unref_obj;

Since fb will be destroyed, is it ok to leave references to it in
crtc->fb set in previous iterations?

> +
> +		crtc->fb = &ifbdev->ifb.base;
> +	}
> +
> +	dev_priv->fbdev = ifbdev;
> +
> +	DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> +	return;
> +
> +out_unref_obj:
> +	drm_gem_object_unreference(&obj->base);
> +out_free_ifbdev:
> +	kfree(ifbdev);
> +}
> +
> +int intel_fbdev_init(struct drm_device *dev)
> +{
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct intel_fbdev *ifbdev;
> +	int ret;
> +
> +	if ((ifbdev = dev_priv->fbdev) == NULL) {
> +		ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> +		if (ifbdev == NULL)
> +			return -ENOMEM;
> +
> +		ifbdev->helper.funcs = &intel_fb_helper_funcs;
> +		ifbdev->preferred_bpp = 32;
> +
> +		dev_priv->fbdev = ifbdev;
> +	}
>  
>  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
> -				 dev_priv->num_pipe,
> -				 INTELFB_CONN_LIMIT);
> +			dev_priv->num_pipe,
> +			INTELFB_CONN_LIMIT);

Unnecessary w/s change.

>  	if (ret) {
> +		dev_priv->fbdev = NULL;
>  		kfree(ifbdev);
>  		return ret;
>  	}
> @@ -271,9 +530,10 @@ int intel_fbdev_init(struct drm_device *dev)
>  void intel_fbdev_initial_config(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct intel_fbdev *ifbdev = dev_priv->fbdev;
>  
>  	/* Due to peculiar init order wrt to hpd handling this is separate. */
> -	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
> +	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
>  }
>  
>  void intel_fbdev_fini(struct drm_device *dev)
> @@ -301,7 +561,8 @@ MODULE_LICENSE("GPL and additional rights");
>  void intel_fb_output_poll_changed(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> +	if (dev_priv->fbdev)
> +		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>  }
>  
>  void intel_fb_restore_mode(struct drm_device *dev)

Re: [PATCH 05/13] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon

[Jesse Barnes]

On Wed, 20 Mar 2013 14:31:38 +0200
Imre Deak <imre.deak@intel.com> wrote:

> > +			offset = I915_READ(DSPSURF(plane));
> > +		} else
> > +			offset = I915_READ(DSPADDR(plane));
> 
> Nitpick: the second branch should be inside { } too.

Fixed.

> 
> > +		if (!obj_offset)
> > +			obj_offset = offset;
> > +
> > +		pitch = I915_READ(DSPSTRIDE(plane));
> > +		if (mode_cmd.pitches[0] == 0)
> > +			mode_cmd.pitches[0] = pitch;
> > +
> > +		if (offset != obj_offset || pitch != mode_cmd.pitches[0]) {
> > +			DRM_DEBUG_KMS("multiple pipe setup not in clone mode, sjipping\n");
> 
> s/sjipping/skipping/

Fixed.

> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		int ret;
> > +
> > +		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
> > +			continue;
> > +
> > +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> > +		if (ret)
> > +			goto out_unref_obj;
> 
> Since fb will be destroyed, is it ok to leave references to it in
> crtc->fb set in previous iterations?

It should only fail the first time (if ever).  I've commented it.  I
think we need to keep it in the loop so that each crtc has a ref on the
fb right?

> >  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
> > -				 dev_priv->num_pipe,
> > -				 INTELFB_CONN_LIMIT);
> > +			dev_priv->num_pipe,
> > +			INTELFB_CONN_LIMIT);
> 
> Unnecessary w/s change.

Things look correct here, maybe I've fixed it or made the tabs sensible.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 06/13] drm/i915: Retrieve the current mode upon KMS takeover

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

Read the current hardware state to retrieve the active mode and populate
our CRTC config if that mode matches our presumptions.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +
 drivers/gpu/drm/i915/intel_crt.c     |   27 +++++++-
 drivers/gpu/drm/i915/intel_display.c |  119 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_dp.c      |   22 +++++++
 drivers/gpu/drm/i915/intel_drv.h     |    7 +-
 drivers/gpu/drm/i915/intel_dvo.c     |   36 ++++++----
 drivers/gpu/drm/i915/intel_hdmi.c    |   22 +++++++
 drivers/gpu/drm/i915/intel_lvds.c    |   27 +++++++-
 drivers/gpu/drm/i915/intel_sdvo.c    |   23 +++++++
 9 files changed, 242 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 30cf7e6..8473db4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -283,6 +283,8 @@ struct drm_i915_display_funcs {
 	void (*update_linetime_wm)(struct drm_device *dev, int pipe,
 				 struct drm_display_mode *mode);
 	void (*modeset_global_resources)(struct drm_device *dev);
+	bool (*crtc_get_mode)(struct drm_crtc *crtc,
+			     struct drm_display_mode *mode);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
 			     struct drm_display_mode *adjusted_mode,
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index cfc9687..f1d68e8 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -81,6 +81,27 @@ static bool intel_crt_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+static unsigned intel_crt_get_mode_flags(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct intel_crt *crt = intel_encoder_to_crt(encoder);
+	u32 tmp, flags = 0;
+
+	tmp = I915_READ(crt->adpa_reg);
+
+	if (tmp & ADPA_HSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NHSYNC;
+
+	if (tmp & ADPA_VSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NVSYNC;
+
+	return flags;
+}
+
 static void intel_disable_crt(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
@@ -777,10 +798,12 @@ void intel_crt_init(struct drm_device *dev)
 
 	crt->base.disable = intel_disable_crt;
 	crt->base.enable = intel_enable_crt;
-	if (HAS_DDI(dev))
+	if (HAS_DDI(dev)) {
 		crt->base.get_hw_state = intel_ddi_get_hw_state;
-	else
+	} else {
 		crt->base.get_hw_state = intel_crt_get_hw_state;
+		crt->base.get_mode_flags = intel_crt_get_mode_flags;
+	}
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
 
 	drm_encoder_helper_add(&crt->base.base, &crt_encoder_funcs);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9793e66..e19b637 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6618,11 +6618,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 }
 
 /* Returns the clock of the currently programmed mode of the given pipe. */
-static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
+static int i9xx_crtc_clock_get(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
+	enum pipe pipe = intel_crtc->pipe;
 	u32 dpll = I915_READ(DPLL(pipe));
 	u32 fp;
 	intel_clock_t clock;
@@ -6705,35 +6706,84 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
 }
 
 /** Returns the currently programmed mode of the given pipe. */
-struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
-					     struct drm_crtc *crtc)
+static bool i9xx_crtc_get_mode(struct drm_crtc *crtc,
+			       struct drm_display_mode *mode)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
-	struct drm_display_mode *mode;
-	int htot = I915_READ(HTOTAL(cpu_transcoder));
-	int hsync = I915_READ(HSYNC(cpu_transcoder));
-	int vtot = I915_READ(VTOTAL(cpu_transcoder));
-	int vsync = I915_READ(VSYNC(cpu_transcoder));
+	u32 tmp;
 
-	mode = kzalloc(sizeof(*mode), GFP_KERNEL);
-	if (!mode)
-		return NULL;
+	memset(mode, 0, sizeof(*mode));
+
+	tmp = I915_READ(HTOTAL(cpu_transcoder));
+	mode->hdisplay = (tmp & 0xffff) + 1;
+	mode->htotal = ((tmp & 0xffff0000) >> 16) + 1;
+
+	tmp = I915_READ(HSYNC(cpu_transcoder));
+	mode->hsync_start = (tmp & 0xffff) + 1;
+	mode->hsync_end = ((tmp & 0xffff0000) >> 16) + 1;
+
+	tmp = I915_READ(VTOTAL(cpu_transcoder));
+	mode->vdisplay = (tmp & 0xffff) + 1;
+	mode->vtotal = ((tmp & 0xffff0000) >> 16) + 1;
+
+	tmp = I915_READ(VSYNC(cpu_transcoder));
+	mode->vsync_start = (tmp & 0xffff) + 1;
+	mode->vsync_end = ((tmp & 0xffff0000) >> 16) + 1;
+
+	mode->clock = i9xx_crtc_clock_get(crtc);
+
+	drm_mode_set_name(mode);
+
+	return true;
+}
+
+static bool ironlake_crtc_get_mode(struct drm_crtc *crtc,
+				   struct drm_display_mode *mode)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
+	u32 tmp;
+
+	memset(mode, 0, sizeof(*mode));
+
+	tmp = I915_READ(HTOTAL(cpu_transcoder));
+	mode->hdisplay = (tmp & 0xffff) + 1;
+	mode->htotal = ((tmp & 0xffff0000) >> 16) + 1;
+
+	tmp = I915_READ(HSYNC(cpu_transcoder));
+	mode->hsync_start = (tmp & 0xffff) + 1;
+	mode->hsync_end = ((tmp & 0xffff0000) >> 16) + 1;
+
+	tmp = I915_READ(VTOTAL(cpu_transcoder));
+	mode->vdisplay = (tmp & 0xffff) + 1;
+	mode->vtotal = ((tmp & 0xffff0000) >> 16) + 1;
 
-	mode->clock = intel_crtc_clock_get(dev, crtc);
-	mode->hdisplay = (htot & 0xffff) + 1;
-	mode->htotal = ((htot & 0xffff0000) >> 16) + 1;
-	mode->hsync_start = (hsync & 0xffff) + 1;
-	mode->hsync_end = ((hsync & 0xffff0000) >> 16) + 1;
-	mode->vdisplay = (vtot & 0xffff) + 1;
-	mode->vtotal = ((vtot & 0xffff0000) >> 16) + 1;
-	mode->vsync_start = (vsync & 0xffff) + 1;
-	mode->vsync_end = ((vsync & 0xffff0000) >> 16) + 1;
+	tmp = I915_READ(VSYNC(cpu_transcoder));
+	mode->vsync_start = (tmp & 0xffff) + 1;
+	mode->vsync_end = ((tmp & 0xffff0000) >> 16) + 1;
+
+	//mode->clock = i9xx_crtc_clock_get(crtc);
+	//mode->clock = 69300;
 
 	drm_mode_set_name(mode);
 
-	return mode;
+	return false; /* XXX mode->clock unset */
+}
+
+static __maybe_unused bool no_crtc_get_mode(struct drm_crtc *crtc,
+					    struct drm_display_mode *mode)
+{
+	return false;
+}
+
+bool intel_crtc_get_mode(struct drm_crtc *crtc,
+			 struct drm_display_mode *mode)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	return dev_priv->display.crtc_get_mode(crtc, mode);
 }
 
 static void intel_increase_pllclock(struct drm_crtc *crtc)
@@ -8515,20 +8565,25 @@ static void intel_init_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	dev_priv->display.crtc_get_mode = no_crtc_get_mode;
+
 	/* We always want a DPMS function */
 	if (HAS_DDI(dev)) {
+		dev_priv->display.crtc_get_mode = ironlake_crtc_get_mode;
 		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = haswell_crtc_off;
 		dev_priv->display.update_plane = ironlake_update_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
+		dev_priv->display.crtc_get_mode = ironlake_crtc_get_mode;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
 		dev_priv->display.update_plane = ironlake_update_plane;
 	} else {
+		dev_priv->display.crtc_get_mode = i9xx_crtc_get_mode;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -9068,6 +9123,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,
 			      crtc->active ? "enabled" : "disabled");
+
+
+		if (crtc->base.enabled)
+			crtc->mode_valid = intel_crtc_get_mode(&crtc->base, &crtc->base.mode);
+		if (crtc->mode_valid) {
+			DRM_DEBUG_KMS("found active mode: ");
+			drm_mode_debug_printmodeline(&crtc->base.mode);
+		}
 	}
 
 	if (HAS_DDI(dev))
@@ -9075,12 +9138,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
 			    base.head) {
-		pipe = 0;
+		pipe = -1;
 
 		if (encoder->get_hw_state(encoder, &pipe)) {
-			encoder->base.crtc =
-				dev_priv->pipe_to_crtc_mapping[pipe];
-		} else {
+			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+			if (crtc->mode_valid && encoder->get_mode_flags)
+				crtc->base.mode.flags |= encoder->get_mode_flags(encoder);
+			encoder->base.crtc = &crtc->base;
+		} else
 			encoder->base.crtc = NULL;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7b8bfe8..e84d4dd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1411,6 +1411,27 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+static unsigned intel_dp_get_mode_flags(struct intel_encoder *encoder)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	u32 tmp, flags = 0;
+
+	tmp = I915_READ(intel_dp->output_reg);
+
+	if (tmp & DP_SYNC_HS_HIGH)
+		flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NHSYNC;
+
+	if (tmp & DP_SYNC_VS_HIGH)
+		flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NVSYNC;
+
+	return flags;
+}
+
 static void intel_disable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
@@ -2966,6 +2987,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	intel_encoder->disable = intel_disable_dp;
 	intel_encoder->post_disable = intel_post_disable_dp;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
+	intel_encoder->get_mode_flags = intel_dp_get_mode_flags;
 
 	intel_dig_port->port = port;
 	intel_dig_port->dp.output_reg = output_reg;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9cf794f..de8928b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -176,6 +176,9 @@ struct intel_encoder {
 	 * the encoder is active. If the encoder is enabled it also set the pipe
 	 * it is connected to in the pipe parameter. */
 	bool (*get_hw_state)(struct intel_encoder *, enum pipe *pipe);
+	/* Reconstructs the equivalent mode flags for the current hardware
+	 * state. */
+	unsigned (*get_mode_flags)(struct intel_encoder *);
 	int crtc_mask;
 };
 
@@ -581,8 +584,8 @@ extern void intel_connector_attach_encoder(struct intel_connector *connector,
 					   struct intel_encoder *encoder);
 extern struct drm_encoder *intel_best_encoder(struct drm_connector *connector);
 
-extern struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
-						    struct drm_crtc *crtc);
+extern bool intel_crtc_get_mode(struct drm_crtc *crtc,
+				struct drm_display_mode *mode);
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
 extern enum transcoder
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 15da995..46b549e 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -129,6 +129,22 @@ static bool intel_dvo_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+static unsigned
+intel_dvo_get_mode_flags(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct intel_dvo *intel_dvo = enc_to_intel_dvo(&encoder->base);
+	u32 tmp, flags = 0;
+
+	tmp = I915_READ(intel_dvo->dev.dvo_reg);
+	if (tmp & DVO_HSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PHSYNC;
+	if (tmp & DVO_VSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PVSYNC;
+
+	return flags;
+}
+
 static void intel_disable_dvo(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
@@ -390,29 +406,26 @@ intel_dvo_get_current_mode(struct drm_connector *connector)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
 	uint32_t dvo_val = I915_READ(intel_dvo->dev.dvo_reg);
-	struct drm_display_mode *mode = NULL;
+	struct drm_display_mode *fixed_mode = NULL;
 
 	/* If the DVO port is active, that'll be the LVDS, so we can pull out
 	 * its timings to get how the BIOS set up the panel.
 	 */
 	if (dvo_val & DVO_ENABLE) {
+		struct drm_display_mode mode;
 		struct drm_crtc *crtc;
 		int pipe = (dvo_val & DVO_PIPE_B_SELECT) ? 1 : 0;
 
 		crtc = intel_get_crtc_for_pipe(dev, pipe);
-		if (crtc) {
-			mode = intel_crtc_mode_get(dev, crtc);
-			if (mode) {
-				mode->type |= DRM_MODE_TYPE_PREFERRED;
-				if (dvo_val & DVO_HSYNC_ACTIVE_HIGH)
-					mode->flags |= DRM_MODE_FLAG_PHSYNC;
-				if (dvo_val & DVO_VSYNC_ACTIVE_HIGH)
-					mode->flags |= DRM_MODE_FLAG_PVSYNC;
-			}
+		if (intel_crtc_get_mode(crtc, &mode))
+			fixed_mode = drm_mode_duplicate(dev, &mode);
+		if (fixed_mode) {
+			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
+			fixed_mode->flags = intel_dvo_get_mode_flags(&intel_dvo->base);
 		}
 	}
 
-	return mode;
+	return fixed_mode;
 }
 
 void intel_dvo_init(struct drm_device *dev)
@@ -441,6 +454,7 @@ void intel_dvo_init(struct drm_device *dev)
 	intel_encoder->disable = intel_disable_dvo;
 	intel_encoder->enable = intel_enable_dvo;
 	intel_encoder->get_hw_state = intel_dvo_get_hw_state;
+	intel_encoder->get_mode_flags = intel_dvo_get_mode_flags;
 	intel_connector->get_hw_state = intel_dvo_connector_get_hw_state;
 
 	/* Now, try to find a controller */
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 5a6138c..005b9f9 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -656,6 +656,27 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+static unsigned intel_hdmi_get_mode_flags(struct intel_encoder *encoder)
+{
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	u32 tmp, flags = 0;
+
+	tmp = I915_READ(intel_hdmi->sdvox_reg);
+
+	if (tmp & SDVO_HSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NHSYNC;
+
+	if (tmp & SDVO_VSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NVSYNC;
+
+	return flags;
+}
+
 static void intel_enable_hdmi(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -1102,6 +1123,7 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg, enum port port)
 	intel_encoder->enable = intel_enable_hdmi;
 	intel_encoder->disable = intel_disable_hdmi;
 	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
+	intel_encoder->get_mode_flags = intel_hdmi_get_mode_flags;
 
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index c7154bf..400afa4 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -88,6 +88,26 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+static unsigned intel_lvds_get_mode_flags(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 lvds_reg, tmp, flags = 0;
+
+	if (HAS_PCH_SPLIT(dev))
+		lvds_reg = PCH_LVDS;
+	else
+		lvds_reg = LVDS;
+
+	tmp = I915_READ(lvds_reg);
+	if (tmp & LVDS_HSYNC_POLARITY)
+		flags |= DRM_MODE_FLAG_NHSYNC;
+	if (tmp & LVDS_VSYNC_POLARITY)
+		flags |= DRM_MODE_FLAG_NVSYNC;
+
+	return flags;
+}
+
 /* The LVDS pin pair needs to be on before the DPLLs are enabled.
  * This is an exception to the general rule that mode_set doesn't turn
  * things on.
@@ -1105,6 +1125,7 @@ bool intel_lvds_init(struct drm_device *dev)
 	intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
 	intel_encoder->disable = intel_disable_lvds;
 	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
+	intel_encoder->get_mode_flags = intel_lvds_get_mode_flags;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
@@ -1216,11 +1237,15 @@ bool intel_lvds_init(struct drm_device *dev)
 	crtc = intel_get_crtc_for_pipe(dev, pipe);
 
 	if (crtc && (lvds & LVDS_PORT_EN)) {
-		fixed_mode = intel_crtc_mode_get(dev, crtc);
+		struct drm_display_mode mode;
+
+		if (intel_crtc_get_mode(crtc, &mode))
+			fixed_mode = drm_mode_duplicate(dev, &mode);
 		if (fixed_mode) {
 			DRM_DEBUG_KMS("using current (BIOS) mode: ");
 			drm_mode_debug_printmodeline(fixed_mode);
 			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
+			fixed_mode->flags = intel_lvds_get_mode_flags(intel_encoder);
 			goto out;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index f01063a..918c269 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1250,6 +1250,27 @@ static bool intel_sdvo_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+static unsigned intel_sdvo_get_mode_flags(struct intel_encoder *encoder)
+{
+	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	u32 tmp, flags = 0;
+
+	tmp = I915_READ(intel_sdvo->sdvo_reg);
+
+	if (tmp & SDVO_HSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NHSYNC;
+
+	if (tmp & SDVO_VSYNC_ACTIVE_HIGH)
+		flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		flags |= DRM_MODE_FLAG_NVSYNC;
+
+	return flags;
+}
+
 static void intel_disable_sdvo(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
@@ -2785,6 +2806,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 	intel_encoder->disable = intel_disable_sdvo;
 	intel_encoder->enable = intel_enable_sdvo;
 	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
+	if (INTEL_INFO(dev)->gen >= 4)
+		intel_encoder->get_mode_flags = intel_sdvo_get_mode_flags;
 
 	/* In default case sdvo lvds is false */
 	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))
-- 
1.7.9.5

Re: [PATCH 06/13] drm/i915: Retrieve the current mode upon KMS takeover

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Read the current hardware state to retrieve the active mode and populate
> our CRTC config if that mode matches our presumptions.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    2 +
>  drivers/gpu/drm/i915/intel_crt.c     |   27 +++++++-
>  drivers/gpu/drm/i915/intel_display.c |  119 ++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_dp.c      |   22 +++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    7 +-
>  drivers/gpu/drm/i915/intel_dvo.c     |   36 ++++++----
>  drivers/gpu/drm/i915/intel_hdmi.c    |   22 +++++++
>  drivers/gpu/drm/i915/intel_lvds.c    |   27 +++++++-
>  drivers/gpu/drm/i915/intel_sdvo.c    |   23 +++++++
>  9 files changed, 242 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 30cf7e6..8473db4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -283,6 +283,8 @@ struct drm_i915_display_funcs {
>  	void (*update_linetime_wm)(struct drm_device *dev, int pipe,
>  				 struct drm_display_mode *mode);
>  	void (*modeset_global_resources)(struct drm_device *dev);
> +	bool (*crtc_get_mode)(struct drm_crtc *crtc,
> +			     struct drm_display_mode *mode);
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     struct drm_display_mode *mode,
>  			     struct drm_display_mode *adjusted_mode,
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index cfc9687..f1d68e8 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -81,6 +81,27 @@ static bool intel_crt_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static unsigned intel_crt_get_mode_flags(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crt *crt = intel_encoder_to_crt(encoder);
> +	u32 tmp, flags = 0;
> +
> +	tmp = I915_READ(crt->adpa_reg);
> +
> +	if (tmp & ADPA_HSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (tmp & ADPA_VSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	return flags;
> +}
> +
>  static void intel_disable_crt(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> @@ -777,10 +798,12 @@ void intel_crt_init(struct drm_device *dev)
>  
>  	crt->base.disable = intel_disable_crt;
>  	crt->base.enable = intel_enable_crt;
> -	if (HAS_DDI(dev))
> +	if (HAS_DDI(dev)) {
>  		crt->base.get_hw_state = intel_ddi_get_hw_state;
> -	else
> +	} else {
>  		crt->base.get_hw_state = intel_crt_get_hw_state;
> +		crt->base.get_mode_flags = intel_crt_get_mode_flags;
> +	}
>  	intel_connector->get_hw_state = intel_connector_get_hw_state;
>  
>  	drm_encoder_helper_add(&crt->base.base, &crt_encoder_funcs);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9793e66..e19b637 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6618,11 +6618,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  }
>  
>  /* Returns the clock of the currently programmed mode of the given pipe. */
> -static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
> +static int i9xx_crtc_clock_get(struct drm_crtc *crtc)
>  {
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int pipe = intel_crtc->pipe;
> +	enum pipe pipe = intel_crtc->pipe;
>  	u32 dpll = I915_READ(DPLL(pipe));
>  	u32 fp;
>  	intel_clock_t clock;
> @@ -6705,35 +6706,84 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
>  }
>  
>  /** Returns the currently programmed mode of the given pipe. */
> -struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
> -					     struct drm_crtc *crtc)
> +static bool i9xx_crtc_get_mode(struct drm_crtc *crtc,
> +			       struct drm_display_mode *mode)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> -	struct drm_display_mode *mode;
> -	int htot = I915_READ(HTOTAL(cpu_transcoder));
> -	int hsync = I915_READ(HSYNC(cpu_transcoder));
> -	int vtot = I915_READ(VTOTAL(cpu_transcoder));
> -	int vsync = I915_READ(VSYNC(cpu_transcoder));
> +	u32 tmp;
>  
> -	mode = kzalloc(sizeof(*mode), GFP_KERNEL);
> -	if (!mode)
> -		return NULL;
> +	memset(mode, 0, sizeof(*mode));
> +
> +	tmp = I915_READ(HTOTAL(cpu_transcoder));
> +	mode->hdisplay = (tmp & 0xffff) + 1;
> +	mode->htotal = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	tmp = I915_READ(HSYNC(cpu_transcoder));
> +	mode->hsync_start = (tmp & 0xffff) + 1;
> +	mode->hsync_end = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	tmp = I915_READ(VTOTAL(cpu_transcoder));
> +	mode->vdisplay = (tmp & 0xffff) + 1;
> +	mode->vtotal = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	tmp = I915_READ(VSYNC(cpu_transcoder));
> +	mode->vsync_start = (tmp & 0xffff) + 1;
> +	mode->vsync_end = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	mode->clock = i9xx_crtc_clock_get(crtc);
> +
> +	drm_mode_set_name(mode);
> +
> +	return true;
> +}
> +
> +static bool ironlake_crtc_get_mode(struct drm_crtc *crtc,
> +				   struct drm_display_mode *mode)
> +{
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> +	u32 tmp;
> +
> +	memset(mode, 0, sizeof(*mode));
> +
> +	tmp = I915_READ(HTOTAL(cpu_transcoder));
> +	mode->hdisplay = (tmp & 0xffff) + 1;
> +	mode->htotal = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	tmp = I915_READ(HSYNC(cpu_transcoder));
> +	mode->hsync_start = (tmp & 0xffff) + 1;
> +	mode->hsync_end = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	tmp = I915_READ(VTOTAL(cpu_transcoder));
> +	mode->vdisplay = (tmp & 0xffff) + 1;
> +	mode->vtotal = ((tmp & 0xffff0000) >> 16) + 1;
>  
> -	mode->clock = intel_crtc_clock_get(dev, crtc);
> -	mode->hdisplay = (htot & 0xffff) + 1;
> -	mode->htotal = ((htot & 0xffff0000) >> 16) + 1;
> -	mode->hsync_start = (hsync & 0xffff) + 1;
> -	mode->hsync_end = ((hsync & 0xffff0000) >> 16) + 1;
> -	mode->vdisplay = (vtot & 0xffff) + 1;
> -	mode->vtotal = ((vtot & 0xffff0000) >> 16) + 1;
> -	mode->vsync_start = (vsync & 0xffff) + 1;
> -	mode->vsync_end = ((vsync & 0xffff0000) >> 16) + 1;
> +	tmp = I915_READ(VSYNC(cpu_transcoder));
> +	mode->vsync_start = (tmp & 0xffff) + 1;
> +	mode->vsync_end = ((tmp & 0xffff0000) >> 16) + 1;
> +
> +	//mode->clock = i9xx_crtc_clock_get(crtc);
> +	//mode->clock = 69300;
>  
>  	drm_mode_set_name(mode);
>  
> -	return mode;
> +	return false; /* XXX mode->clock unset */
> +}
> +
> +static __maybe_unused bool no_crtc_get_mode(struct drm_crtc *crtc,
> +					    struct drm_display_mode *mode)
> +{
> +	return false;
> +}
> +
> +bool intel_crtc_get_mode(struct drm_crtc *crtc,
> +			 struct drm_display_mode *mode)
> +{
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	return dev_priv->display.crtc_get_mode(crtc, mode);
>  }
>  
>  static void intel_increase_pllclock(struct drm_crtc *crtc)
> @@ -8515,20 +8565,25 @@ static void intel_init_display(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	dev_priv->display.crtc_get_mode = no_crtc_get_mode;
> +
>  	/* We always want a DPMS function */
>  	if (HAS_DDI(dev)) {
> +		dev_priv->display.crtc_get_mode = ironlake_crtc_get_mode;
>  		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
>  		dev_priv->display.crtc_enable = haswell_crtc_enable;
>  		dev_priv->display.crtc_disable = haswell_crtc_disable;
>  		dev_priv->display.off = haswell_crtc_off;
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> +		dev_priv->display.crtc_get_mode = ironlake_crtc_get_mode;
>  		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>  		dev_priv->display.crtc_enable = ironlake_crtc_enable;
>  		dev_priv->display.crtc_disable = ironlake_crtc_disable;
>  		dev_priv->display.off = ironlake_crtc_off;
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else {
> +		dev_priv->display.crtc_get_mode = i9xx_crtc_get_mode;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -9068,6 +9123,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  			      crtc->base.base.id,
>  			      crtc->active ? "enabled" : "disabled");
> +
> +
> +		if (crtc->base.enabled)
> +			crtc->mode_valid = intel_crtc_get_mode(&crtc->base, &crtc->base.mode);
> +		if (crtc->mode_valid) {
> +			DRM_DEBUG_KMS("found active mode: ");
> +			drm_mode_debug_printmodeline(&crtc->base.mode);
> +		}
>  	}
>  
>  	if (HAS_DDI(dev))
> @@ -9075,12 +9138,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
>  			    base.head) {
> -		pipe = 0;
> +		pipe = -1;
>  
>  		if (encoder->get_hw_state(encoder, &pipe)) {
> -			encoder->base.crtc =
> -				dev_priv->pipe_to_crtc_mapping[pipe];
> -		} else {
> +			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);

get_hw_state() can return true and still leave pipe at -1, causing an
invalid access here.

> +			if (crtc->mode_valid && encoder->get_mode_flags)
> +				crtc->base.mode.flags |= encoder->get_mode_flags(encoder);
> +			encoder->base.crtc = &crtc->base;
> +		} else
>  			encoder->base.crtc = NULL;
>  		}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7b8bfe8..e84d4dd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1411,6 +1411,27 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static unsigned intel_dp_get_mode_flags(struct intel_encoder *encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	u32 tmp, flags = 0;
> +
> +	tmp = I915_READ(intel_dp->output_reg);
> +
> +	if (tmp & DP_SYNC_HS_HIGH)
> +		flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (tmp & DP_SYNC_VS_HIGH)
> +		flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	return flags;
> +}
> +
>  static void intel_disable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -2966,6 +2987,7 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>  	intel_encoder->disable = intel_disable_dp;
>  	intel_encoder->post_disable = intel_post_disable_dp;
>  	intel_encoder->get_hw_state = intel_dp_get_hw_state;
> +	intel_encoder->get_mode_flags = intel_dp_get_mode_flags;
>  
>  	intel_dig_port->port = port;
>  	intel_dig_port->dp.output_reg = output_reg;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9cf794f..de8928b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -176,6 +176,9 @@ struct intel_encoder {
>  	 * the encoder is active. If the encoder is enabled it also set the pipe
>  	 * it is connected to in the pipe parameter. */
>  	bool (*get_hw_state)(struct intel_encoder *, enum pipe *pipe);
> +	/* Reconstructs the equivalent mode flags for the current hardware
> +	 * state. */
> +	unsigned (*get_mode_flags)(struct intel_encoder *);
>  	int crtc_mask;
>  };
>  
> @@ -581,8 +584,8 @@ extern void intel_connector_attach_encoder(struct intel_connector *connector,
>  					   struct intel_encoder *encoder);
>  extern struct drm_encoder *intel_best_encoder(struct drm_connector *connector);
>  
> -extern struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
> -						    struct drm_crtc *crtc);
> +extern bool intel_crtc_get_mode(struct drm_crtc *crtc,
> +				struct drm_display_mode *mode);
>  int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
>  				struct drm_file *file_priv);
>  extern enum transcoder
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 15da995..46b549e 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -129,6 +129,22 @@ static bool intel_dvo_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static unsigned
> +intel_dvo_get_mode_flags(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_dvo *intel_dvo = enc_to_intel_dvo(&encoder->base);
> +	u32 tmp, flags = 0;
> +
> +	tmp = I915_READ(intel_dvo->dev.dvo_reg);
> +	if (tmp & DVO_HSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PHSYNC;
> +	if (tmp & DVO_VSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PVSYNC;
> +
> +	return flags;
> +}
> +
>  static void intel_disable_dvo(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> @@ -390,29 +406,26 @@ intel_dvo_get_current_mode(struct drm_connector *connector)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
>  	uint32_t dvo_val = I915_READ(intel_dvo->dev.dvo_reg);
> -	struct drm_display_mode *mode = NULL;
> +	struct drm_display_mode *fixed_mode = NULL;
>  
>  	/* If the DVO port is active, that'll be the LVDS, so we can pull out
>  	 * its timings to get how the BIOS set up the panel.
>  	 */
>  	if (dvo_val & DVO_ENABLE) {
> +		struct drm_display_mode mode;
>  		struct drm_crtc *crtc;
>  		int pipe = (dvo_val & DVO_PIPE_B_SELECT) ? 1 : 0;
>  
>  		crtc = intel_get_crtc_for_pipe(dev, pipe);
> -		if (crtc) {
> -			mode = intel_crtc_mode_get(dev, crtc);
> -			if (mode) {
> -				mode->type |= DRM_MODE_TYPE_PREFERRED;
> -				if (dvo_val & DVO_HSYNC_ACTIVE_HIGH)
> -					mode->flags |= DRM_MODE_FLAG_PHSYNC;
> -				if (dvo_val & DVO_VSYNC_ACTIVE_HIGH)
> -					mode->flags |= DRM_MODE_FLAG_PVSYNC;
> -			}
> +		if (intel_crtc_get_mode(crtc, &mode))
> +			fixed_mode = drm_mode_duplicate(dev, &mode);
> +		if (fixed_mode) {
> +			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> +			fixed_mode->flags = intel_dvo_get_mode_flags(&intel_dvo->base);
>  		}
>  	}
>  
> -	return mode;
> +	return fixed_mode;
>  }
>  
>  void intel_dvo_init(struct drm_device *dev)
> @@ -441,6 +454,7 @@ void intel_dvo_init(struct drm_device *dev)
>  	intel_encoder->disable = intel_disable_dvo;
>  	intel_encoder->enable = intel_enable_dvo;
>  	intel_encoder->get_hw_state = intel_dvo_get_hw_state;
> +	intel_encoder->get_mode_flags = intel_dvo_get_mode_flags;
>  	intel_connector->get_hw_state = intel_dvo_connector_get_hw_state;
>  
>  	/* Now, try to find a controller */
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 5a6138c..005b9f9 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -656,6 +656,27 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static unsigned intel_hdmi_get_mode_flags(struct intel_encoder *encoder)
> +{
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	u32 tmp, flags = 0;
> +
> +	tmp = I915_READ(intel_hdmi->sdvox_reg);
> +
> +	if (tmp & SDVO_HSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (tmp & SDVO_VSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	return flags;
> +}
> +
>  static void intel_enable_hdmi(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
> @@ -1102,6 +1123,7 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg, enum port port)
>  	intel_encoder->enable = intel_enable_hdmi;
>  	intel_encoder->disable = intel_disable_hdmi;
>  	intel_encoder->get_hw_state = intel_hdmi_get_hw_state;
> +	intel_encoder->get_mode_flags = intel_hdmi_get_mode_flags;
>  
>  	intel_encoder->type = INTEL_OUTPUT_HDMI;
>  	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index c7154bf..400afa4 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -88,6 +88,26 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static unsigned intel_lvds_get_mode_flags(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 lvds_reg, tmp, flags = 0;
> +
> +	if (HAS_PCH_SPLIT(dev))
> +		lvds_reg = PCH_LVDS;
> +	else
> +		lvds_reg = LVDS;
> +
> +	tmp = I915_READ(lvds_reg);
> +	if (tmp & LVDS_HSYNC_POLARITY)
> +		flags |= DRM_MODE_FLAG_NHSYNC;
> +	if (tmp & LVDS_VSYNC_POLARITY)
> +		flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	return flags;
> +}
> +
>  /* The LVDS pin pair needs to be on before the DPLLs are enabled.
>   * This is an exception to the general rule that mode_set doesn't turn
>   * things on.
> @@ -1105,6 +1125,7 @@ bool intel_lvds_init(struct drm_device *dev)
>  	intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
>  	intel_encoder->disable = intel_disable_lvds;
>  	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
> +	intel_encoder->get_mode_flags = intel_lvds_get_mode_flags;
>  	intel_connector->get_hw_state = intel_connector_get_hw_state;
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> @@ -1216,11 +1237,15 @@ bool intel_lvds_init(struct drm_device *dev)
>  	crtc = intel_get_crtc_for_pipe(dev, pipe);
>  
>  	if (crtc && (lvds & LVDS_PORT_EN)) {
> -		fixed_mode = intel_crtc_mode_get(dev, crtc);
> +		struct drm_display_mode mode;
> +
> +		if (intel_crtc_get_mode(crtc, &mode))
> +			fixed_mode = drm_mode_duplicate(dev, &mode);
>  		if (fixed_mode) {
>  			DRM_DEBUG_KMS("using current (BIOS) mode: ");
>  			drm_mode_debug_printmodeline(fixed_mode);
>  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> +			fixed_mode->flags = intel_lvds_get_mode_flags(intel_encoder);
>  			goto out;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index f01063a..918c269 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1250,6 +1250,27 @@ static bool intel_sdvo_get_hw_state(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +static unsigned intel_sdvo_get_mode_flags(struct intel_encoder *encoder)
> +{
> +	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	u32 tmp, flags = 0;
> +
> +	tmp = I915_READ(intel_sdvo->sdvo_reg);
> +
> +	if (tmp & SDVO_HSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (tmp & SDVO_VSYNC_ACTIVE_HIGH)
> +		flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	return flags;
> +}
> +
>  static void intel_disable_sdvo(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> @@ -2785,6 +2806,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
>  	intel_encoder->disable = intel_disable_sdvo;
>  	intel_encoder->enable = intel_enable_sdvo;
>  	intel_encoder->get_hw_state = intel_sdvo_get_hw_state;
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		intel_encoder->get_mode_flags = intel_sdvo_get_mode_flags;
>  
>  	/* In default case sdvo lvds is false */
>  	if (!intel_sdvo_get_capabilities(intel_sdvo, &intel_sdvo->caps))

Re: [PATCH 06/13] drm/i915: Retrieve the current mode upon KMS takeover

[Jesse Barnes]

On Wed, 20 Mar 2013 14:36:25 +0200
Imre Deak <imre.deak@intel.com> wrote:

> > +		pipe = -1;
> >  
> >  		if (encoder->get_hw_state(encoder, &pipe)) {
> > -			encoder->base.crtc =
> > -				dev_priv->pipe_to_crtc_mapping[pipe];
> > -		} else {
> > +			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);  
> 
> get_hw_state() can return true and still leave pipe at -1, causing an
> invalid access here.

Fixed, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 06/13] drm/i915: Retrieve the current mode upon KMS takeover

[Daniel Vetter]

On Tue, Mar 26, 2013 at 04:24:48PM -0700, Jesse Barnes wrote:
> On Wed, 20 Mar 2013 14:36:25 +0200
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > > +		pipe = -1;
> > >  
> > >  		if (encoder->get_hw_state(encoder, &pipe)) {
> > > -			encoder->base.crtc =
> > > -				dev_priv->pipe_to_crtc_mapping[pipe];
> > > -		} else {
> > > +			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);  
> > 
> > get_hw_state() can return true and still leave pipe at -1, causing an
> > invalid access here.
> 
> Fixed, thanks.

That shouldn't actually be possible - if encoder->get_hw_state returns
true but doesn't set the pipe, that's a bug in the callback. So maybe yell
with a WARN?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 06/13] drm/i915: Retrieve the current mode upon KMS takeover

[Jesse Barnes]

On Wed, 27 Mar 2013 00:52:47 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Mar 26, 2013 at 04:24:48PM -0700, Jesse Barnes wrote:
> > On Wed, 20 Mar 2013 14:36:25 +0200
> > Imre Deak <imre.deak@intel.com> wrote:
> > 
> > > > +		pipe = -1;
> > > >  
> > > >  		if (encoder->get_hw_state(encoder, &pipe)) {
> > > > -			encoder->base.crtc =
> > > > -				dev_priv->pipe_to_crtc_mapping[pipe];
> > > > -		} else {
> > > > +			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);  
> > > 
> > > get_hw_state() can return true and still leave pipe at -1, causing an
> > > invalid access here.
> > 
> > Fixed, thanks.
> 
> That shouldn't actually be possible - if encoder->get_hw_state returns
> true but doesn't set the pipe, that's a bug in the callback. So maybe yell
> with a WARN?

I'll just check all the callbacks; I thought I remembered one case
where it seemed to make sense, but yeah a WARN at least would be nice
if not.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 07/13] drm/i915: Only preserve the BIOS modes if they are the preferred ones

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    9 +++++++++
 drivers/gpu/drm/i915/intel_dp.c      |    1 +
 drivers/gpu/drm/i915/intel_drv.h     |    8 ++++++++
 drivers/gpu/drm/i915/intel_fb.c      |    9 +++++++++
 drivers/gpu/drm/i915/intel_lvds.c    |    1 +
 drivers/gpu/drm/i915/intel_panel.c   |   10 ++++++++++
 6 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e19b637..8ca47e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9270,6 +9270,15 @@ void intel_connector_attach_encoder(struct intel_connector *connector,
 					  &encoder->base);
 }
 
+bool intel_connector_get_preferred_mode(struct intel_connector *connector,
+					struct drm_display_mode *mode)
+{
+	if (!connector->get_preferred_mode)
+		return false;
+
+	return connector->get_preferred_mode(connector, mode);
+}
+
 /*
  * set vga decode state - true == enable VGA decode
  */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e84d4dd..edd29d9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2941,6 +2941,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	}
 
 	if (is_edp(intel_dp)) {
+		intel_connector->get_preferred_mode = intel_connector_get_panel_fixed_mode;
 		intel_panel_init(&intel_connector->panel, fixed_mode);
 		intel_panel_setup_backlight(connector);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index de8928b..5487285 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -204,6 +204,9 @@ struct intel_connector {
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
 
+	bool (*get_preferred_mode)(struct intel_connector *,
+				   struct drm_display_mode *);
+
 	/* Panel info for eDP and LVDS */
 	struct intel_panel panel;
 
@@ -509,6 +512,9 @@ extern int intel_panel_init(struct intel_panel *panel,
 			    struct drm_display_mode *fixed_mode);
 extern void intel_panel_fini(struct intel_panel *panel);
 
+extern bool intel_connector_get_panel_fixed_mode(struct intel_connector *connector,
+						 struct drm_display_mode *mode);
+
 extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 				   struct drm_display_mode *adjusted_mode);
 extern void intel_pch_panel_fitting(struct drm_device *dev,
@@ -580,6 +586,8 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
 
+extern bool intel_connector_get_preferred_mode(struct intel_connector *connector,
+					       struct drm_display_mode *mode);
 extern void intel_connector_attach_encoder(struct intel_connector *connector,
 					   struct intel_encoder *encoder);
 extern struct drm_encoder *intel_best_encoder(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 4ca2ee4..b60f277 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -237,6 +237,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 	for (i = 0; i < fb_helper->connector_count; i++) {
 		struct drm_connector *connector;
 		struct drm_encoder *encoder;
+		struct drm_display_mode mode;
 
 		connector = fb_helper->connector_info[i]->connector;
 		if (!enabled[i]) {
@@ -266,6 +267,14 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 			return false;
 		}
 
+		if (intel_connector_get_preferred_mode(to_intel_connector(connector), &mode) &&
+		    !drm_mode_equal(&mode, &encoder->crtc->mode)) {
+			DRM_DEBUG_KMS("connector %s on crtc %d has an non-native mode, aborting\n",
+				      drm_get_connector_name(connector),
+				      encoder->crtc->base.id);
+			return false;
+		}
+
 		modes[i] = &encoder->crtc->mode;
 		crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 400afa4..fa3a543 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1127,6 +1127,7 @@ bool intel_lvds_init(struct drm_device *dev)
 	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
 	intel_encoder->get_mode_flags = intel_lvds_get_mode_flags;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
+	intel_connector->get_preferred_mode = intel_connector_get_panel_fixed_mode;
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	intel_encoder->type = INTEL_OUTPUT_LVDS;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a3730e0..704bf7a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -35,6 +35,16 @@
 
 #define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
 
+bool intel_connector_get_panel_fixed_mode(struct intel_connector *connector,
+					  struct drm_display_mode *mode)
+{
+	if (!connector->panel.fixed_mode)
+		return false;
+
+	*mode = *connector->panel.fixed_mode;
+	return true;
+}
+
 void
 intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 		       struct drm_display_mode *adjusted_mode)
-- 
1.7.9.5

[PATCH 08/13] drm/i915: Validate that the framebuffer accommodates the current mode

[Jesse Barnes]

From: Chris Wilson <chris@chris-wilson.co.uk>

As we retrieve the mode from the BIOS it may be constructed using
different assumptions for its configuration, such as utilizing the panel
fitter in a conflicting manner. As such the associated framebuffer may be
insufficient for our setup, and so we need to reject the current mode
and install our own.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   38 +++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ca47e5..0a2279e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6450,27 +6450,40 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 	return intel_framebuffer_create(dev, &mode_cmd, obj);
 }
 
+static bool
+mode_fits_in_fb(struct drm_display_mode *mode,
+		struct drm_framebuffer *fb)
+{
+	struct drm_i915_gem_object *obj;
+	int min_pitch;
+
+	min_pitch = intel_framebuffer_pitch_for_width(mode->hdisplay,
+						      fb->bits_per_pixel);
+	if (fb->pitches[0] < min_pitch)
+		return false;
+
+	obj = to_intel_framebuffer(fb)->obj;
+	if (obj == NULL)
+		return false;
+
+	if (obj->base.size < mode->vdisplay * fb->pitches[0])
+		return false;
+
+	return true;
+}
+
 static struct drm_framebuffer *
 mode_fits_in_fbdev(struct drm_device *dev,
 		   struct drm_display_mode *mode)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
 	struct drm_framebuffer *fb;
 
 	if (dev_priv->fbdev == NULL)
 		return NULL;
 
-	obj = dev_priv->fbdev->ifb.obj;
-	if (obj == NULL)
-		return NULL;
-
 	fb = &dev_priv->fbdev->ifb.base;
-	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
-							       fb->bits_per_pixel))
-		return NULL;
-
-	if (obj->base.size < mode->vdisplay * fb->pitches[0])
+	if (!mode_fits_in_fb(mode, fb))
 		return NULL;
 
 	return fb;
@@ -9127,6 +9140,11 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 		if (crtc->base.enabled)
 			crtc->mode_valid = intel_crtc_get_mode(&crtc->base, &crtc->base.mode);
+
+		if (crtc->base.fb &&
+		    !mode_fits_in_fb(&crtc->base.mode, crtc->base.fb))
+			crtc->mode_valid = false;
+
 		if (crtc->mode_valid) {
 			DRM_DEBUG_KMS("found active mode: ");
 			drm_mode_debug_printmodeline(&crtc->base.mode);
-- 
1.7.9.5

[PATCH 09/13] drm/i915: fix build in intel_display.c

[Jesse Barnes]

Missing a curly brace.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0a2279e..595590c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9163,7 +9163,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 			if (crtc->mode_valid && encoder->get_mode_flags)
 				crtc->base.mode.flags |= encoder->get_mode_flags(encoder);
 			encoder->base.crtc = &crtc->base;
-		} else
+		} else {
 			encoder->base.crtc = NULL;
 		}
 
-- 
1.7.9.5

Re: [PATCH 09/13] drm/i915: fix build in intel_display.c

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> Missing a curly brace.

Should be merged into 6/13.

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0a2279e..595590c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9163,7 +9163,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  			if (crtc->mode_valid && encoder->get_mode_flags)
>  				crtc->base.mode.flags |= encoder->get_mode_flags(encoder);
>  			encoder->base.crtc = &crtc->base;
> -		} else
> +		} else {
>  			encoder->base.crtc = NULL;
>  		}
>  

Re: [PATCH 09/13] drm/i915: fix build in intel_display.c

[Jesse Barnes]

On Wed, 20 Mar 2013 14:41:32 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> > Missing a curly brace.
> 
> Should be merged into 6/13.

Fixed.  Thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 10/13] drm/i915: check panel fit status at update_plane time

[Jesse Barnes]

We may need to disable the panel when flipping to a new buffer, so check
the state here and zero it out if needed, otherwise leave it alone.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 595590c..91660b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2304,6 +2304,25 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	if (crtc->fb)
 		intel_finish_fb(crtc->fb);
 
+	I915_WRITE(PIPESRC(intel_crtc->pipe),
+		   ((crtc->mode.hdisplay - 1) << 16) |
+		   (crtc->mode.vdisplay - 1));
+	if (!dev_priv->pch_pf_size &&
+	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
+	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
+		/* Force use of hard-coded filter coefficients
+		 * as some pre-programmed values are broken,
+		 * e.g. x201.
+		 */
+		if (IS_IVYBRIDGE(dev))
+			I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
+		else
+			I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
+		I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);
+		I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);
+	}
+
+
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
 		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
-- 
1.7.9.5

Re: [PATCH 10/13] drm/i915: check panel fit status at update_plane time

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> We may need to disable the panel when flipping to a new buffer, so check
> the state here and zero it out if needed, otherwise leave it alone.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 595590c..91660b1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2304,6 +2304,25 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  	if (crtc->fb)
>  		intel_finish_fb(crtc->fb);
>  
> +	I915_WRITE(PIPESRC(intel_crtc->pipe),
> +		   ((crtc->mode.hdisplay - 1) << 16) |
> +		   (crtc->mode.vdisplay - 1));
> +	if (!dev_priv->pch_pf_size &&
> +	    (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
> +	     intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
> +		/* Force use of hard-coded filter coefficients
> +		 * as some pre-programmed values are broken,
> +		 * e.g. x201.
> +		 */

Nitpick: multiline comments should start with an empty /* line.

> +		if (IS_IVYBRIDGE(dev))
> +			I915_WRITE(PF_CTL(intel_crtc->pipe), 0);
> +		else
> +			I915_WRITE(PF_CTL(intel_crtc->pipe), 0);

No difference in the branches?

> +		I915_WRITE(PF_WIN_POS(intel_crtc->pipe), 0);
> +		I915_WRITE(PF_WIN_SZ(intel_crtc->pipe), 0);
> +	}
> +
> +
>  	ret = dev_priv->display.update_plane(crtc, fb, x, y);
>  	if (ret) {
>  		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);

[PATCH 11/13] drm/i915: add clock_get for ironlake+

[Jesse Barnes]

Turns out it's easy to get the clock, though it may correspond to a
potential pfit mode.  In that case, we may still be able to flip if
we can get the native mode params somehow.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 91660b1..861af1a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6771,6 +6771,27 @@ static bool i9xx_crtc_get_mode(struct drm_crtc *crtc,
 	return true;
 }
 
+static int ironlake_crtc_clock_get(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
+	int clock;
+	u32 link_m;
+
+	/*
+	 * PCH platforms make this easy: we can just use the LINK_M1 reg.
+	 * Note: this may be the pixel clock for a fitted mode, in which
+	 * case it won't match the native mode clock.  That means we won't be
+	 * able to do a simple flip in the fastboot case.
+	 */
+	link_m = I915_READ(PIPE_LINK_M1(cpu_transcoder));
+
+	clock = link_m;
+
+	return clock;
+}
+
 static bool ironlake_crtc_get_mode(struct drm_crtc *crtc,
 				   struct drm_display_mode *mode)
 {
@@ -6797,12 +6818,11 @@ static bool ironlake_crtc_get_mode(struct drm_crtc *crtc,
 	mode->vsync_start = (tmp & 0xffff) + 1;
 	mode->vsync_end = ((tmp & 0xffff0000) >> 16) + 1;
 
-	//mode->clock = i9xx_crtc_clock_get(crtc);
-	//mode->clock = 69300;
+	mode->clock = ironlake_crtc_clock_get(crtc);
 
 	drm_mode_set_name(mode);
 
-	return false; /* XXX mode->clock unset */
+	return true;
 }
 
 static __maybe_unused bool no_crtc_get_mode(struct drm_crtc *crtc,
-- 
1.7.9.5

Re: [PATCH 11/13] drm/i915: add clock_get for ironlake+

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> Turns out it's easy to get the clock, though it may correspond to a
> potential pfit mode.  In that case, we may still be able to flip if
> we can get the native mode params somehow.

This should be merged to 6/13.

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 91660b1..861af1a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6771,6 +6771,27 @@ static bool i9xx_crtc_get_mode(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +static int ironlake_crtc_clock_get(struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> +	int clock;
> +	u32 link_m;
> +
> +	/*
> +	 * PCH platforms make this easy: we can just use the LINK_M1 reg.
> +	 * Note: this may be the pixel clock for a fitted mode, in which
> +	 * case it won't match the native mode clock.  That means we won't be
> +	 * able to do a simple flip in the fastboot case.
> +	 */
> +	link_m = I915_READ(PIPE_LINK_M1(cpu_transcoder));
> +
> +	clock = link_m;
> +
> +	return clock;
> +}

Could be simply return I915_READ(PIPE_LINK_M1(cpu_transcoder));

> +
>  static bool ironlake_crtc_get_mode(struct drm_crtc *crtc,
>  				   struct drm_display_mode *mode)
>  {
> @@ -6797,12 +6818,11 @@ static bool ironlake_crtc_get_mode(struct drm_crtc *crtc,
>  	mode->vsync_start = (tmp & 0xffff) + 1;
>  	mode->vsync_end = ((tmp & 0xffff0000) >> 16) + 1;
>  
> -	//mode->clock = i9xx_crtc_clock_get(crtc);
> -	//mode->clock = 69300;
> +	mode->clock = ironlake_crtc_clock_get(crtc);
>  
>  	drm_mode_set_name(mode);
>  
> -	return false; /* XXX mode->clock unset */
> +	return true;
>  }
>  
>  static __maybe_unused bool no_crtc_get_mode(struct drm_crtc *crtc,

Re: [PATCH 11/13] drm/i915: add clock_get for ironlake+

[Jesse Barnes]

On Wed, 20 Mar 2013 14:48:22 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> > Turns out it's easy to get the clock, though it may correspond to a
> > potential pfit mode.  In that case, we may still be able to flip if
> > we can get the native mode params somehow.
> 
> This should be merged to 6/13.

Done, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 12/13] drm/i915: treat no fb -> fb as simple flip instead of full mode set

[Jesse Barnes]

In case we don't get an fb from the BIOS, we may still be able to re-use
existing state and flip a new buffer.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 861af1a..b04d8b5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7993,10 +7993,8 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 	/* We should be able to check here if the fb has the same properties
 	 * and then just flip_or_move it */
 	if (set->crtc->fb != set->fb) {
-		/* If we have no fb then treat it as a full mode set */
 		if (set->crtc->fb == NULL) {
-			DRM_DEBUG_KMS("crtc has no fb, full mode set\n");
-			config->mode_changed = true;
+			config->fb_changed = true;
 		} else if (set->fb == NULL) {
 			config->mode_changed = true;
 		} else if (set->fb->depth != set->crtc->fb->depth) {
-- 
1.7.9.5

[PATCH 13/13] drm/i915: check for non-native modes when inheriting a BIOS fb

[Jesse Barnes]

If the mode is non-native using the panel fitter, don't try to re-use
the fb the BIOS allocated for it.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_fb.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index b60f277..9ff12aa 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -438,6 +438,18 @@ void intel_fbdev_init_bios(struct drm_device *dev)
 		width = ((val >> 16) & 0xfff) + 1;
 		height = ((val >> 0) & 0xfff) + 1;
 
+		/* Don't bother inheriting panel fitted modes */
+		val = I915_READ(HTOTAL(pipe));
+		if (((val & 0xffff) + 1) != width) {
+			DRM_ERROR("BIOS fb not native width (%d vs %d), skipping\n", width, (val & 0xffff) + 1);
+			continue;
+		}
+		val = I915_READ(VTOTAL(pipe));
+		if (((val & 0xffff) + 1) != height) {
+			DRM_ERROR("BIOS fb not native width (%d vs %d), skipping\n", height, (val & 0xffff) + 1);
+			continue;
+		}
+
 		DRM_DEBUG_KMS("Found active pipe [%d/%d]: size=%dx%d@%d, offset=%x\n",
 			      pipe, plane, width, height, bpp, offset);
 
-- 
1.7.9.5

Re: [PATCH 13/13] drm/i915: check for non-native modes when inheriting a BIOS fb

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> If the mode is non-native using the panel fitter, don't try to re-use
> the fb the BIOS allocated for it.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_fb.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index b60f277..9ff12aa 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -438,6 +438,18 @@ void intel_fbdev_init_bios(struct drm_device *dev)
>  		width = ((val >> 16) & 0xfff) + 1;
>  		height = ((val >> 0) & 0xfff) + 1;
>  
> +		/* Don't bother inheriting panel fitted modes */
> +		val = I915_READ(HTOTAL(pipe));
> +		if (((val & 0xffff) + 1) != width) {
> +			DRM_ERROR("BIOS fb not native width (%d vs %d), skipping\n", width, (val & 0xffff) + 1);
> +			continue;
> +		}
> +		val = I915_READ(VTOTAL(pipe));
> +		if (((val & 0xffff) + 1) != height) {
> +			DRM_ERROR("BIOS fb not native width (%d vs %d), skipping\n", height, (val & 0xffff) + 1);

s/width/height/

> +			continue;
> +		}
> +
>  		DRM_DEBUG_KMS("Found active pipe [%d/%d]: size=%dx%d@%d, offset=%x\n",
>  			      pipe, plane, width, height, bpp, offset);
>  

On the series with my comments and the note that I couldn't yet test it:
Reviewed-by: Imre Deak <imre.deak@intel.com>

Re: [PATCH 13/13] drm/i915: check for non-native modes when inheriting a BIOS fb

[Jesse Barnes]

On Wed, 20 Mar 2013 14:51:12 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> > If the mode is non-native using the panel fitter, don't try to re-use
> > the fb the BIOS allocated for it.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_fb.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> > index b60f277..9ff12aa 100644
> > --- a/drivers/gpu/drm/i915/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/intel_fb.c
> > @@ -438,6 +438,18 @@ void intel_fbdev_init_bios(struct drm_device *dev)
> >  		width = ((val >> 16) & 0xfff) + 1;
> >  		height = ((val >> 0) & 0xfff) + 1;
> >  
> > +		/* Don't bother inheriting panel fitted modes */
> > +		val = I915_READ(HTOTAL(pipe));
> > +		if (((val & 0xffff) + 1) != width) {
> > +			DRM_ERROR("BIOS fb not native width (%d vs %d), skipping\n", width, (val & 0xffff) + 1);
> > +			continue;
> > +		}
> > +		val = I915_READ(VTOTAL(pipe));
> > +		if (((val & 0xffff) + 1) != height) {
> > +			DRM_ERROR("BIOS fb not native width (%d vs %d), skipping\n", height, (val & 0xffff) + 1);
> 
> s/width/height/
> 
> > +			continue;
> > +		}
> > +
> >  		DRM_DEBUG_KMS("Found active pipe [%d/%d]: size=%dx%d@%d, offset=%x\n",
> >  			      pipe, plane, width, height, bpp, offset);
> >  
> 
> On the series with my comments and the note that I couldn't yet test it:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Fixed, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: More fastboot bits

[Imre Deak]

On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> This one adds some extra checks on top of Chris's last set:
>   - check for panel fit modes when inheriting from the BIOS
>   - update pfit state at pipe_set_base time

I missed this version of the patchset and reviewed the previous one :/
Sending it with a v2 subject prefix with a proper In-reply-to header
would've been nice, consider the high traffic on this list.

Some of the comments from that review are still valid, so I'll copy them
over.

> It also changes the mode set vs flip checking to include the non-fb case
> (e.g. if the BIOS fb was too small for the native mode), since we might
> still be able to flip in that case.
> 
> Finally, it includes a clock_get routine for ilk+.  I'd appreciate if
> someone could test this out on a machine where VBIOS supports the native
> panel mode, so the kernel can boot from the boot loader in the native
> mode.  In that case, it should actually fastboot and avoid the whole
> mode set/panel power sequence.

My ilk doesn't seem to start with a native mode, so can't test it there.
Would it be possible to test this with reloading the module?

--Imre

Re: More fastboot bits

[Jesse Barnes]

On Wed, 20 Mar 2013 14:15:32 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote:
> > This one adds some extra checks on top of Chris's last set:
> >   - check for panel fit modes when inheriting from the BIOS
> >   - update pfit state at pipe_set_base time
> 
> I missed this version of the patchset and reviewed the previous one :/
> Sending it with a v2 subject prefix with a proper In-reply-to header
> would've been nice, consider the high traffic on this list.
> 
> Some of the comments from that review are still valid, so I'll copy them
> over.

Great, thanks.  Sorry about the confusion, reposts like this are a bad
habit from a lower volume mailing list. :)  I'll endeavor to improve
myself.

> 
> > It also changes the mode set vs flip checking to include the non-fb case
> > (e.g. if the BIOS fb was too small for the native mode), since we might
> > still be able to flip in that case.
> > 
> > Finally, it includes a clock_get routine for ilk+.  I'd appreciate if
> > someone could test this out on a machine where VBIOS supports the native
> > panel mode, so the kernel can boot from the boot loader in the native
> > mode.  In that case, it should actually fastboot and avoid the whole
> > mode set/panel power sequence.
> 
> My ilk doesn't seem to start with a native mode, so can't test it there.
> Would it be possible to test this with reloading the module?

Yeah, that should also work, but I definitely haven't tested it as much.

-- 
Jesse Barnes, Intel Open Source Technology Center