[PATCH 0/5] Fix a couple of warning introduced recently

[PATCH 0/5] Fix a couple of warning introduced recently

[Damien Lespiau]

At least here, the following commit introduced 2 warnings when loading the driver:

      commit 706dc7b549175e47f23e913b7f1e52874a7d0f56
      Author: Matt Roper <matthew.d.roper@intel.com>
      Date:   Tue Feb 3 13:10:04 2015 -0800
    
          drm/i915: Ensure plane->state->fb stays in sync with plane->fb

The series tries to address those.

-- 
Damien

Damien Lespiau (5):
  drm/i915: Put update_state_fb() next to the fb update
  drm/i915: Use an intermediate variable to avoid repeating ourselves
  drm/i915: Don't try to reference the fb in get_initial_plane_config()
  drm/i915: Store the initial framebuffer in initial_plane_config
  drm/i915: Fix atomic state when reusing the firmware fb

 drivers/gpu/drm/i915/intel_display.c | 67 +++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 37 insertions(+), 31 deletions(-)

-- 
1.8.3.1

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

[PATCH 1/5] drm/i915: Put update_state_fb() next to the fb update

[Damien Lespiau]

update_state_fb() at the end of intel_find_plane_obj() is misleading as
it leads us to believe the update is done for all code path.

A successful call to intel_alloc_plane_obj() will return and
update_state_fb() is then only needed when we share a fb from another
CRTC. Put the update() function there then.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 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 e5c0579..8900ce0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2466,12 +2466,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
+			update_state_fb(intel_crtc->base.primary);
 			obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
 		}
 	}
 
-	update_state_fb(intel_crtc->base.primary);
 }
 
 static void i9xx_update_primary_plane(struct drm_crtc *crtc,
-- 
1.8.3.1

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

[PATCH 2/5] drm/i915: Use an intermediate variable to avoid repeating ourselves

[Damien Lespiau]

The code look slightly better this way and will ease the next commit,
changing where we take the fb pointer from.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8900ce0..3301b61 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2371,6 +2371,7 @@ intel_alloc_plane_obj(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_gem_object *obj = NULL;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
 	u32 base = plane_config->base;
 
 	if (plane_config->size == 0)
@@ -2383,16 +2384,16 @@ intel_alloc_plane_obj(struct intel_crtc *crtc,
 
 	obj->tiling_mode = plane_config->tiling;
 	if (obj->tiling_mode == I915_TILING_X)
-		obj->stride = crtc->base.primary->fb->pitches[0];
+		obj->stride = fb->pitches[0];
 
-	mode_cmd.pixel_format = crtc->base.primary->fb->pixel_format;
-	mode_cmd.width = crtc->base.primary->fb->width;
-	mode_cmd.height = crtc->base.primary->fb->height;
-	mode_cmd.pitches[0] = crtc->base.primary->fb->pitches[0];
+	mode_cmd.pixel_format = fb->pixel_format;
+	mode_cmd.width = fb->width;
+	mode_cmd.height = fb->height;
+	mode_cmd.pitches[0] = fb->pitches[0];
 
 	mutex_lock(&dev->struct_mutex);
 
-	if (intel_framebuffer_init(dev, to_intel_framebuffer(crtc->base.primary->fb),
+	if (intel_framebuffer_init(dev, to_intel_framebuffer(fb),
 				   &mode_cmd, obj)) {
 		DRM_DEBUG_KMS("intel fb init failed\n");
 		goto out_unref_obj;
-- 
1.8.3.1

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

[PATCH 3/5] drm/i915: Don't try to reference the fb in get_initial_plane_config()

[Damien Lespiau]

Tvrtko noticed a new warning on boot:

  WARNING: CPU: 1 PID: 353 at include/linux/kref.h:47 drm_framebuffer_reference+0x6c/0x80 [drm]()
  Call Trace:
  [<ffffffff8161f10c>] dump_stack+0x4f/0x7b
  [<ffffffff81052caa>] warn_slowpath_common+0xaa/0xd0
  [<ffffffff81052d8a>] warn_slowpath_null+0x1a/0x20
  [<ffffffffa00d035c>] drm_framebuffer_reference+0x6c/0x80 [drm]
  [<ffffffffa01c0df7>] update_state_fb.isra.54+0x47/0x50 [i915]
  [<ffffffffa01ccd5c>] skylake_get_initial_plane_config+0x93c/0x950 [i915]
  [<ffffffffa01e8721>] intel_modeset_init+0x1551/0x17c0 [i915]
  [<ffffffffa02476e0>] i915_driver_load+0xed0/0x11e0 [i915]
  [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
  [<ffffffffa00ca8b7>] drm_dev_register+0x77/0x110 [drm]
  [<ffffffffa00cda3b>] drm_get_pci_dev+0x11b/0x1f0 [drm]
  [<ffffffff81098e3d>] ? trace_hardirqs_on+0xd/0x10
  [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
  [<ffffffffa0145276>] i915_pci_probe+0x56/0x60 [i915]
  [<ffffffff813ad59c>] pci_device_probe+0x7c/0x100
  [<ffffffff81466aad>] driver_probe_device+0x16d/0x380

We cannot take a reference at this point, not before
intel_framebuffer_init() and the underlying drm_framebuffer_init().

Introduced in:

  commit 706dc7b549175e47f23e913b7f1e52874a7d0f56
  Author: Matt Roper <matthew.d.roper@intel.com>
  Date:   Tue Feb 3 13:10:04 2015 -0800

      drm/i915: Ensure plane->state->fb stays in sync with plane->fb

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3301b61..e1e89d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2364,6 +2364,20 @@ static int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)
 	}
 }
 
+/* Update plane->state->fb to match plane->fb after driver-internal updates */
+static void
+update_state_fb(struct drm_plane *plane)
+{
+	if (plane->fb == plane->state->fb)
+		return;
+
+	if (plane->state->fb)
+		drm_framebuffer_unreference(plane->state->fb);
+	plane->state->fb = plane->fb;
+	if (plane->state->fb)
+		drm_framebuffer_reference(plane->state->fb);
+}
+
 static bool
 intel_alloc_plane_obj(struct intel_crtc *crtc,
 		      struct intel_initial_plane_config *plane_config)
@@ -2411,20 +2425,6 @@ out_unref_obj:
 	return false;
 }
 
-/* Update plane->state->fb to match plane->fb after driver-internal updates */
-static void
-update_state_fb(struct drm_plane *plane)
-{
-	if (plane->fb == plane->state->fb)
-		return;
-
-	if (plane->state->fb)
-		drm_framebuffer_unreference(plane->state->fb);
-	plane->state->fb = plane->fb;
-	if (plane->state->fb)
-		drm_framebuffer_reference(plane->state->fb);
-}
-
 static void
 intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		     struct intel_initial_plane_config *plane_config)
@@ -2438,8 +2438,10 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 	if (!intel_crtc->base.primary->fb)
 		return;
 
-	if (intel_alloc_plane_obj(intel_crtc, plane_config))
+	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
+		update_state_fb(intel_crtc->base.primary);
 		return;
+	}
 
 	kfree(intel_crtc->base.primary->fb);
 	intel_crtc->base.primary->fb = NULL;
@@ -6652,7 +6654,6 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 		      plane_config->size);
 
 	crtc->base.primary->fb = fb;
-	update_state_fb(crtc->base.primary);
 }
 
 static void chv_crtc_clock_get(struct intel_crtc *crtc,
@@ -7690,7 +7691,6 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 		      plane_config->size);
 
 	crtc->base.primary->fb = fb;
-	update_state_fb(crtc->base.primary);
 	return;
 
 error:
@@ -7782,7 +7782,6 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 		      plane_config->size);
 
 	crtc->base.primary->fb = fb;
-	update_state_fb(crtc->base.primary);
 }
 
 static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
-- 
1.8.3.1

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

[PATCH 4/5] drm/i915: Store the initial framebuffer in initial_plane_config

[Damien Lespiau]

At the moment we use crtc->base.primary->fb to hold the initial
framebuffer allocation, disregarding if it's valid or not.

This lead to believe we were actually updating the fb at this point, but
it's not true and we haven't even called drm_framebuffer_init() on this
fb.

Instead, let's store the state in struct intel_initial_plane_config
until we know we can reuse that framebuffer.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1e89d0..0d07535 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2385,7 +2385,7 @@ intel_alloc_plane_obj(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_gem_object *obj = NULL;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
-	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	struct drm_framebuffer *fb = &plane_config->fb->base;
 	u32 base = plane_config->base;
 
 	if (plane_config->size == 0)
@@ -2435,16 +2435,16 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 	struct intel_crtc *i;
 	struct drm_i915_gem_object *obj;
 
-	if (!intel_crtc->base.primary->fb)
+	if (!plane_config->fb)
 		return;
 
 	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
+		intel_crtc->base.primary->fb = &plane_config->fb->base;
 		update_state_fb(intel_crtc->base.primary);
 		return;
 	}
 
-	kfree(intel_crtc->base.primary->fb);
-	intel_crtc->base.primary->fb = NULL;
+	kfree(plane_config->fb);
 
 	/*
 	 * Failed to alloc the obj, check to see if we should share
@@ -6653,7 +6653,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 		      fb->bits_per_pixel, base, fb->pitches[0],
 		      plane_config->size);
 
-	crtc->base.primary->fb = fb;
+	plane_config->fb = intel_fb;
 }
 
 static void chv_crtc_clock_get(struct intel_crtc *crtc,
@@ -7690,7 +7690,7 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 		      fb->bits_per_pixel, base, fb->pitches[0],
 		      plane_config->size);
 
-	crtc->base.primary->fb = fb;
+	plane_config->fb = intel_fb;
 	return;
 
 error:
@@ -7781,7 +7781,7 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 		      fb->bits_per_pixel, base, fb->pitches[0],
 		      plane_config->size);
 
-	crtc->base.primary->fb = fb;
+	plane_config->fb = intel_fb;
 }
 
 static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f048f8b..76b3c20 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -258,6 +258,7 @@ struct intel_plane_state {
 };
 
 struct intel_initial_plane_config {
+	struct intel_framebuffer *fb;
 	unsigned int tiling;
 	int size;
 	u32 base;
-- 
1.8.3.1

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

[PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Damien Lespiau]

Right now, we get a warning when taking over the firmware fb:

  [drm:drm_atomic_plane_check] FB set but no CRTC

with the following backtrace:

  [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
  [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
  [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
  [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
  [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
  [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
  [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
  [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
  [<ffffffff813ad444>] fbcon_init+0x4f4/0x580

That's because we update the plane state with the fb from the firmware, but we
never associate the plane to that CRTC.

We don't quite have the full DRM take over from HW state just yet, so
fake enough of the plane atomic state to pass the checks.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0d07535..eed1c0c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		return;
 
 	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
-		intel_crtc->base.primary->fb = &plane_config->fb->base;
-		update_state_fb(intel_crtc->base.primary);
+		struct drm_plane *primary = intel_crtc->base.primary;
+
+		primary->fb = &plane_config->fb->base;
+		primary->state->crtc = &intel_crtc->base;
+		update_state_fb(primary);
+
 		return;
 	}
 
@@ -2469,6 +2473,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 
 			drm_framebuffer_reference(c->primary->fb);
 			intel_crtc->base.primary->fb = c->primary->fb;
+			c->state->crtc = &intel_crtc->base;
 			update_state_fb(intel_crtc->base.primary);
 			obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
-- 
1.8.3.1

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

Re: [PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Matt Roper]

On Thu, Feb 05, 2015 at 05:22:19PM +0000, Damien Lespiau wrote:
> Right now, we get a warning when taking over the firmware fb:
> 
>   [drm:drm_atomic_plane_check] FB set but no CRTC
> 
> with the following backtrace:
> 
>   [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
>   [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
>   [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
>   [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
>   [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
>   [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
>   [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
>   [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
>   [<ffffffff813ad444>] fbcon_init+0x4f4/0x580
> 
> That's because we update the plane state with the fb from the firmware, but we
> never associate the plane to that CRTC.
> 
> We don't quite have the full DRM take over from HW state just yet, so
> fake enough of the plane atomic state to pass the checks.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0d07535..eed1c0c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  		return;
>  
>  	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> -		intel_crtc->base.primary->fb = &plane_config->fb->base;
> -		update_state_fb(intel_crtc->base.primary);
> +		struct drm_plane *primary = intel_crtc->base.primary;
> +
> +		primary->fb = &plane_config->fb->base;
> +		primary->state->crtc = &intel_crtc->base;

Would

        drm_atomic_set_crtc_for_plane(primary->state, &intel_crtc->base)

be better here so that the various bitmasks get updated?

> +		update_state_fb(primary);
> +
>  		return;
>  	}
>  
> @@ -2469,6 +2473,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  
>  			drm_framebuffer_reference(c->primary->fb);
>  			intel_crtc->base.primary->fb = c->primary->fb;
> +			c->state->crtc = &intel_crtc->base;

I'm not sure I understand this line.  c->state is the crtc state for the
other CRTC we're going to share with with (and has a backpointer of
c->state->crtc = c).  Was this supposed to be something like

        drm_atomic_set_crtc_for_plane(intel_crtc->base.primary->state,
                                      &intel_crtc->base)

instead?  I'm not sure if updating the other crtc (that we're sharing
with) was a mistake here.


Matt

>  			update_state_fb(intel_crtc->base.primary);
>  			obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>  			break;
> -- 
> 1.8.3.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Don't try to reference the fb in get_initial_plane_config()

[Matt Roper]

On Thu, Feb 05, 2015 at 05:22:17PM +0000, Damien Lespiau wrote:
> Tvrtko noticed a new warning on boot:
> 
>   WARNING: CPU: 1 PID: 353 at include/linux/kref.h:47 drm_framebuffer_reference+0x6c/0x80 [drm]()
>   Call Trace:
>   [<ffffffff8161f10c>] dump_stack+0x4f/0x7b
>   [<ffffffff81052caa>] warn_slowpath_common+0xaa/0xd0
>   [<ffffffff81052d8a>] warn_slowpath_null+0x1a/0x20
>   [<ffffffffa00d035c>] drm_framebuffer_reference+0x6c/0x80 [drm]
>   [<ffffffffa01c0df7>] update_state_fb.isra.54+0x47/0x50 [i915]
>   [<ffffffffa01ccd5c>] skylake_get_initial_plane_config+0x93c/0x950 [i915]
>   [<ffffffffa01e8721>] intel_modeset_init+0x1551/0x17c0 [i915]
>   [<ffffffffa02476e0>] i915_driver_load+0xed0/0x11e0 [i915]
>   [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
>   [<ffffffffa00ca8b7>] drm_dev_register+0x77/0x110 [drm]
>   [<ffffffffa00cda3b>] drm_get_pci_dev+0x11b/0x1f0 [drm]
>   [<ffffffff81098e3d>] ? trace_hardirqs_on+0xd/0x10
>   [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
>   [<ffffffffa0145276>] i915_pci_probe+0x56/0x60 [i915]
>   [<ffffffff813ad59c>] pci_device_probe+0x7c/0x100
>   [<ffffffff81466aad>] driver_probe_device+0x16d/0x380
> 
> We cannot take a reference at this point, not before
> intel_framebuffer_init() and the underlying drm_framebuffer_init().
> 
> Introduced in:
> 
>   commit 706dc7b549175e47f23e913b7f1e52874a7d0f56
>   Author: Matt Roper <matthew.d.roper@intel.com>
>   Date:   Tue Feb 3 13:10:04 2015 -0800
> 
>       drm/i915: Ensure plane->state->fb stays in sync with plane->fb
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3301b61..e1e89d0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2364,6 +2364,20 @@ static int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)
>  	}
>  }
>  
> +/* Update plane->state->fb to match plane->fb after driver-internal updates */
> +static void
> +update_state_fb(struct drm_plane *plane)
> +{
> +	if (plane->fb == plane->state->fb)
> +		return;
> +
> +	if (plane->state->fb)
> +		drm_framebuffer_unreference(plane->state->fb);
> +	plane->state->fb = plane->fb;
> +	if (plane->state->fb)
> +		drm_framebuffer_reference(plane->state->fb);
> +}
> +

Was moving this function without changes intentional?  It seems to be
unrelated to the other changes here.

Anyway, #1-4 are

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I left a couple comments on #5.


Matt

>  static bool
>  intel_alloc_plane_obj(struct intel_crtc *crtc,
>  		      struct intel_initial_plane_config *plane_config)
> @@ -2411,20 +2425,6 @@ out_unref_obj:
>  	return false;
>  }
>  
> -/* Update plane->state->fb to match plane->fb after driver-internal updates */
> -static void
> -update_state_fb(struct drm_plane *plane)
> -{
> -	if (plane->fb == plane->state->fb)
> -		return;
> -
> -	if (plane->state->fb)
> -		drm_framebuffer_unreference(plane->state->fb);
> -	plane->state->fb = plane->fb;
> -	if (plane->state->fb)
> -		drm_framebuffer_reference(plane->state->fb);
> -}
> -
>  static void
>  intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  		     struct intel_initial_plane_config *plane_config)
> @@ -2438,8 +2438,10 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  	if (!intel_crtc->base.primary->fb)
>  		return;
>  
> -	if (intel_alloc_plane_obj(intel_crtc, plane_config))
> +	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> +		update_state_fb(intel_crtc->base.primary);
>  		return;
> +	}
>  
>  	kfree(intel_crtc->base.primary->fb);
>  	intel_crtc->base.primary->fb = NULL;
> @@ -6652,7 +6654,6 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>  		      plane_config->size);
>  
>  	crtc->base.primary->fb = fb;
> -	update_state_fb(crtc->base.primary);
>  }
>  
>  static void chv_crtc_clock_get(struct intel_crtc *crtc,
> @@ -7690,7 +7691,6 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  		      plane_config->size);
>  
>  	crtc->base.primary->fb = fb;
> -	update_state_fb(crtc->base.primary);
>  	return;
>  
>  error:
> @@ -7782,7 +7782,6 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>  		      plane_config->size);
>  
>  	crtc->base.primary->fb = fb;
> -	update_state_fb(crtc->base.primary);
>  }
>  
>  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> -- 
> 1.8.3.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5 v2] drm/i915: Don't try to reference the fb in get_initial_plane_config()

[Damien Lespiau]

Tvrtko noticed a new warning on boot:

  WARNING: CPU: 1 PID: 353 at include/linux/kref.h:47 drm_framebuffer_reference+0x6c/0x80 [drm]()
  Call Trace:
  [<ffffffff8161f10c>] dump_stack+0x4f/0x7b
  [<ffffffff81052caa>] warn_slowpath_common+0xaa/0xd0
  [<ffffffff81052d8a>] warn_slowpath_null+0x1a/0x20
  [<ffffffffa00d035c>] drm_framebuffer_reference+0x6c/0x80 [drm]
  [<ffffffffa01c0df7>] update_state_fb.isra.54+0x47/0x50 [i915]
  [<ffffffffa01ccd5c>] skylake_get_initial_plane_config+0x93c/0x950 [i915]
  [<ffffffffa01e8721>] intel_modeset_init+0x1551/0x17c0 [i915]
  [<ffffffffa02476e0>] i915_driver_load+0xed0/0x11e0 [i915]
  [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
  [<ffffffffa00ca8b7>] drm_dev_register+0x77/0x110 [drm]
  [<ffffffffa00cda3b>] drm_get_pci_dev+0x11b/0x1f0 [drm]
  [<ffffffff81098e3d>] ? trace_hardirqs_on+0xd/0x10
  [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
  [<ffffffffa0145276>] i915_pci_probe+0x56/0x60 [i915]
  [<ffffffff813ad59c>] pci_device_probe+0x7c/0x100
  [<ffffffff81466aad>] driver_probe_device+0x16d/0x380

We cannot take a reference at this point, not before
intel_framebuffer_init() and the underlying drm_framebuffer_init().

Introduced in:

  commit 706dc7b549175e47f23e913b7f1e52874a7d0f56
  Author: Matt Roper <matthew.d.roper@intel.com>
  Date:   Tue Feb 3 13:10:04 2015 -0800

      drm/i915: Ensure plane->state->fb stays in sync with plane->fb

v2: Don't move update_state_fb(). It was moved around because I
    originally put update_state_fb() in intel_alloc_plane_obj() before
    finding a better place. (Matt)

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3301b61..a25ff00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2438,8 +2438,10 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 	if (!intel_crtc->base.primary->fb)
 		return;
 
-	if (intel_alloc_plane_obj(intel_crtc, plane_config))
+	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
+		update_state_fb(intel_crtc->base.primary);
 		return;
+	}
 
 	kfree(intel_crtc->base.primary->fb);
 	intel_crtc->base.primary->fb = NULL;
@@ -6652,7 +6654,6 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 		      plane_config->size);
 
 	crtc->base.primary->fb = fb;
-	update_state_fb(crtc->base.primary);
 }
 
 static void chv_crtc_clock_get(struct intel_crtc *crtc,
@@ -7690,7 +7691,6 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 		      plane_config->size);
 
 	crtc->base.primary->fb = fb;
-	update_state_fb(crtc->base.primary);
 	return;
 
 error:
@@ -7782,7 +7782,6 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 		      plane_config->size);
 
 	crtc->base.primary->fb = fb;
-	update_state_fb(crtc->base.primary);
 }
 
 static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
-- 
1.8.3.1

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

Re: [PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Ville Syrjälä]

On Thu, Feb 05, 2015 at 10:10:26AM -0800, Matt Roper wrote:
> On Thu, Feb 05, 2015 at 05:22:19PM +0000, Damien Lespiau wrote:
> > Right now, we get a warning when taking over the firmware fb:
> > 
> >   [drm:drm_atomic_plane_check] FB set but no CRTC
> > 
> > with the following backtrace:
> > 
> >   [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
> >   [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
> >   [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
> >   [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
> >   [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
> >   [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
> >   [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
> >   [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
> >   [<ffffffff813ad444>] fbcon_init+0x4f4/0x580
> > 
> > That's because we update the plane state with the fb from the firmware, but we
> > never associate the plane to that CRTC.
> > 
> > We don't quite have the full DRM take over from HW state just yet, so
> > fake enough of the plane atomic state to pass the checks.
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0d07535..eed1c0c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >  		return;
> >  
> >  	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> > -		intel_crtc->base.primary->fb = &plane_config->fb->base;
> > -		update_state_fb(intel_crtc->base.primary);
> > +		struct drm_plane *primary = intel_crtc->base.primary;
> > +
> > +		primary->fb = &plane_config->fb->base;
> > +		primary->state->crtc = &intel_crtc->base;
> 
> Would
> 
>         drm_atomic_set_crtc_for_plane(primary->state, &intel_crtc->base)
> 
> be better here so that the various bitmasks get updated?
> 
> > +		update_state_fb(primary);
> > +
> >  		return;
> >  	}
> >  
> > @@ -2469,6 +2473,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >  
> >  			drm_framebuffer_reference(c->primary->fb);
> >  			intel_crtc->base.primary->fb = c->primary->fb;
> > +			c->state->crtc = &intel_crtc->base;
> 
> I'm not sure I understand this line.  c->state is the crtc state for the
> other CRTC we're going to share with with (and has a backpointer of
> c->state->crtc = c).  Was this supposed to be something like
> 
>         drm_atomic_set_crtc_for_plane(intel_crtc->base.primary->state,
>                                       &intel_crtc->base)
> 
> instead?  I'm not sure if updating the other crtc (that we're sharing
> with) was a mistake here.

plane->crtc != NULL indicates that the plane is actually enabled. But
that might not be true here actually. We know the pipe is enabled, but
we haven't actually checked the primary plane state. If some silly BIOS
boots with the plane disabled on some pipe we might get confused here.
Or did we add some check to sanitize to turn off the entire pipe if
the primary plane isn't enabled? There was some problem in this area as
I recall, but can't remember if anything came of it.

> 
> 
> Matt
> 
> >  			update_state_fb(intel_crtc->base.primary);
> >  			obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> >  			break;
> > -- 
> > 1.8.3.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Damien Lespiau]

On Thu, Feb 05, 2015 at 10:10:26AM -0800, Matt Roper wrote:
> On Thu, Feb 05, 2015 at 05:22:19PM +0000, Damien Lespiau wrote:
> > Right now, we get a warning when taking over the firmware fb:
> > 
> >   [drm:drm_atomic_plane_check] FB set but no CRTC
> > 
> > with the following backtrace:
> > 
> >   [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
> >   [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
> >   [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
> >   [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
> >   [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
> >   [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
> >   [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
> >   [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
> >   [<ffffffff813ad444>] fbcon_init+0x4f4/0x580
> > 
> > That's because we update the plane state with the fb from the firmware, but we
> > never associate the plane to that CRTC.
> > 
> > We don't quite have the full DRM take over from HW state just yet, so
> > fake enough of the plane atomic state to pass the checks.
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0d07535..eed1c0c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >  		return;
> >  
> >  	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> > -		intel_crtc->base.primary->fb = &plane_config->fb->base;
> > -		update_state_fb(intel_crtc->base.primary);
> > +		struct drm_plane *primary = intel_crtc->base.primary;
> > +
> > +		primary->fb = &plane_config->fb->base;
> > +		primary->state->crtc = &intel_crtc->base;
> 
> Would
> 
>         drm_atomic_set_crtc_for_plane(primary->state, &intel_crtc->base)
> 
> be better here so that the various bitmasks get updated?

So, this is what I tried first, but it was oopsing in:

0x1fd31 is in drm_atomic_get_crtc_state (drivers/gpu/drm/drm_atomic.c:205).
200		int ret, index;
201		struct drm_crtc_state *crtc_state;
202	
203		index = drm_crtc_index(crtc);
204	
205		if (state->crtc_states[index])
206			return state->crtc_states[index];
207	
208		ret = drm_modeset_lock(&crtc->mutex, state->acquire_ctx);
209		if (ret)

Presumably because we don't have a atomic modeset context setup in
->state when initially reading out the hw state.

I went for the shortest way to make the warnings stop.

> 
> > +		update_state_fb(primary);
> > +
> >  		return;
> >  	}
> >  
> > @@ -2469,6 +2473,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >  
> >  			drm_framebuffer_reference(c->primary->fb);
> >  			intel_crtc->base.primary->fb = c->primary->fb;
> > +			c->state->crtc = &intel_crtc->base;
> 
> I'm not sure I understand this line.  c->state is the crtc state for the
> other CRTC we're going to share with with (and has a backpointer of
> c->state->crtc = c).  Was this supposed to be something like
> 
>         drm_atomic_set_crtc_for_plane(intel_crtc->base.primary->state,
>                                       &intel_crtc->base)
> 
> instead?  I'm not sure if updating the other crtc (that we're sharing
> with) was a mistake here.

I did mean to write:
	intel_crtc->base.primary->state->crtc = &intel_crtc->base;

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

Re: [PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Matt Roper]

On Thu, Feb 05, 2015 at 06:58:06PM +0000, Damien Lespiau wrote:
> On Thu, Feb 05, 2015 at 10:10:26AM -0800, Matt Roper wrote:
> > On Thu, Feb 05, 2015 at 05:22:19PM +0000, Damien Lespiau wrote:
> > > Right now, we get a warning when taking over the firmware fb:
> > > 
> > >   [drm:drm_atomic_plane_check] FB set but no CRTC
> > > 
> > > with the following backtrace:
> > > 
> > >   [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
> > >   [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
> > >   [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
> > >   [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
> > >   [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
> > >   [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
> > >   [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
> > >   [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
> > >   [<ffffffff813ad444>] fbcon_init+0x4f4/0x580
> > > 
> > > That's because we update the plane state with the fb from the firmware, but we
> > > never associate the plane to that CRTC.
> > > 
> > > We don't quite have the full DRM take over from HW state just yet, so
> > > fake enough of the plane atomic state to pass the checks.
> > > 
> > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 0d07535..eed1c0c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> > >  		return;
> > >  
> > >  	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> > > -		intel_crtc->base.primary->fb = &plane_config->fb->base;
> > > -		update_state_fb(intel_crtc->base.primary);
> > > +		struct drm_plane *primary = intel_crtc->base.primary;
> > > +
> > > +		primary->fb = &plane_config->fb->base;
> > > +		primary->state->crtc = &intel_crtc->base;
> > 
> > Would
> > 
> >         drm_atomic_set_crtc_for_plane(primary->state, &intel_crtc->base)
> > 
> > be better here so that the various bitmasks get updated?
> 
> So, this is what I tried first, but it was oopsing in:
> 
> 0x1fd31 is in drm_atomic_get_crtc_state (drivers/gpu/drm/drm_atomic.c:205).
> 200		int ret, index;
> 201		struct drm_crtc_state *crtc_state;
> 202	
> 203		index = drm_crtc_index(crtc);
> 204	
> 205		if (state->crtc_states[index])
> 206			return state->crtc_states[index];
> 207	
> 208		ret = drm_modeset_lock(&crtc->mutex, state->acquire_ctx);
> 209		if (ret)
> 
> Presumably because we don't have a atomic modeset context setup in
> ->state when initially reading out the hw state.
> 
> I went for the shortest way to make the warnings stop.

Ah, you're right.  We're not in the middle of an atomic operation, so we
only have a plane_state and not a top-level drm_atomic_state to work
with.

I think what you have here should be fine.


> > 
> > > +		update_state_fb(primary);
> > > +
> > >  		return;
> > >  	}
> > >  
> > > @@ -2469,6 +2473,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> > >  
> > >  			drm_framebuffer_reference(c->primary->fb);
> > >  			intel_crtc->base.primary->fb = c->primary->fb;
> > > +			c->state->crtc = &intel_crtc->base;
> > 
> > I'm not sure I understand this line.  c->state is the crtc state for the
> > other CRTC we're going to share with with (and has a backpointer of
> > c->state->crtc = c).  Was this supposed to be something like
> > 
> >         drm_atomic_set_crtc_for_plane(intel_crtc->base.primary->state,
> >                                       &intel_crtc->base)
> > 
> > instead?  I'm not sure if updating the other crtc (that we're sharing
> > with) was a mistake here.
> 
> I did mean to write:
> 	intel_crtc->base.primary->state->crtc = &intel_crtc->base;

Okay, that looks good to me.

I think the only thing we need to close on is Ville's concern about the
plane winding up with non-NULL fb and crtc pointers even though a crazy
firmware or bootloader somehow left the plane turned off.  I'm not super
familiar with the fastboot stuff, but maybe our
get_initial_plane_config() calls need a test to ensure the plane is
actually turned on before they start trying to read out the FB base and
such?


Matt

> 
> -- 
> Damien

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Damien Lespiau]

On Thu, Feb 05, 2015 at 11:12:20AM -0800, Matt Roper wrote:
> I think the only thing we need to close on is Ville's concern about the
> plane winding up with non-NULL fb and crtc pointers even though a crazy
> firmware or bootloader somehow left the plane turned off.  I'm not super
> familiar with the fastboot stuff, but maybe our
> get_initial_plane_config() calls need a test to ensure the plane is
> actually turned on before they start trying to read out the FB base and
> such?
 
I was also looking at get_initial_plane_config() and was thinking the
same, will add a patch on top.

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

[PATCH 5/5 v2] drm/i915: Fix atomic state when reusing the firmware fb

[Damien Lespiau]

Right now, we get a warning when taking over the firmware fb:

  [drm:drm_atomic_plane_check] FB set but no CRTC

with the following backtrace:

  [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
  [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
  [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
  [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
  [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
  [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
  [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
  [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
  [<ffffffff813ad444>] fbcon_init+0x4f4/0x580

That's because we update the plane state with the fb from the firmware, but we
never associate the plane to that CRTC.

We don't quite have the full DRM take over from HW state just yet, so
fake enough of the plane atomic state to pass the checks.

v2: Fix the state on which we set the CRTC in the case we're sharing the
    initial fb with another pipe. (Matt)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 86cbc77..074f204 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		return;
 
 	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
-		intel_crtc->base.primary->fb = &plane_config->fb->base;
-		update_state_fb(intel_crtc->base.primary);
+		struct drm_plane *primary = intel_crtc->base.primary;
+
+		primary->fb = &plane_config->fb->base;
+		primary->state->crtc = &intel_crtc->base;
+		update_state_fb(primary);
+
 		return;
 	}
 
@@ -2464,11 +2468,14 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 			continue;
 
 		if (i915_gem_obj_ggtt_offset(obj) == plane_config->base) {
+			struct drm_plane *primary = intel_crtc->base.primary;
+
 			if (obj->tiling_mode != I915_TILING_NONE)
 				dev_priv->preserve_bios_swizzle = true;
 
 			drm_framebuffer_reference(c->primary->fb);
-			intel_crtc->base.primary->fb = c->primary->fb;
+			primary->fb = c->primary->fb;
+			primary->state->crtc = &intel_crtc->base;
 			update_state_fb(intel_crtc->base.primary);
 			obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 			break;
-- 
1.8.3.1

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

[PATCH 6/5] drm/i915: Make sure the primary plane is enabled before reading out the fb state

[Damien Lespiau]

We don't want to end up in a state where we track that the pipe has its
primary plane enabled when primary plane registers are programmed with
values that look possible but the plane actually disabled.

Refuse to read out the fb state when the primary plane isn't enabled.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Suggested-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 074f204..3fe9598 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6613,6 +6613,10 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 	struct drm_framebuffer *fb;
 	struct intel_framebuffer *intel_fb;
 
+	val = I915_READ(DSPCNTR(plane));
+	if (!(val & DISPLAY_PLANE_ENABLE))
+		return;
+
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
 	if (!intel_fb) {
 		DRM_DEBUG_KMS("failed to alloc fb\n");
@@ -6621,8 +6625,6 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 
 	fb = &intel_fb->base;
 
-	val = I915_READ(DSPCNTR(plane));
-
 	if (INTEL_INFO(dev)->gen >= 4)
 		if (val & DISPPLANE_TILED)
 			plane_config->tiling = I915_TILING_X;
@@ -7654,6 +7656,9 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 	fb = &intel_fb->base;
 
 	val = I915_READ(PLANE_CTL(pipe, 0));
+	if (!(val & PLANE_CTL_ENABLE))
+		goto error;
+
 	if (val & PLANE_CTL_TILED_MASK)
 		plane_config->tiling = I915_TILING_X;
 
@@ -7741,6 +7746,10 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 	struct drm_framebuffer *fb;
 	struct intel_framebuffer *intel_fb;
 
+	val = I915_READ(DSPCNTR(pipe));
+	if (!(val & DISPLAY_PLANE_ENABLE))
+		return;
+
 	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
 	if (!intel_fb) {
 		DRM_DEBUG_KMS("failed to alloc fb\n");
@@ -7749,8 +7758,6 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
 
 	fb = &intel_fb->base;
 
-	val = I915_READ(DSPCNTR(pipe));
-
 	if (INTEL_INFO(dev)->gen >= 4)
 		if (val & DISPPLANE_TILED)
 			plane_config->tiling = I915_TILING_X;
-- 
1.8.3.1

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

Re: [PATCH 6/5] drm/i915: Make sure the primary plane is enabled before reading out the fb state

[Matt Roper]

On Thu, Feb 05, 2015 at 07:35:13PM +0000, Damien Lespiau wrote:
> We don't want to end up in a state where we track that the pipe has its
> primary plane enabled when primary plane registers are programmed with
> values that look possible but the plane actually disabled.
> 
> Refuse to read out the fb state when the primary plane isn't enabled.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

For the series:

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 074f204..3fe9598 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6613,6 +6613,10 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>  	struct drm_framebuffer *fb;
>  	struct intel_framebuffer *intel_fb;
>  
> +	val = I915_READ(DSPCNTR(plane));
> +	if (!(val & DISPLAY_PLANE_ENABLE))
> +		return;
> +
>  	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
>  	if (!intel_fb) {
>  		DRM_DEBUG_KMS("failed to alloc fb\n");
> @@ -6621,8 +6625,6 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>  
>  	fb = &intel_fb->base;
>  
> -	val = I915_READ(DSPCNTR(plane));
> -
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		if (val & DISPPLANE_TILED)
>  			plane_config->tiling = I915_TILING_X;
> @@ -7654,6 +7656,9 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  	fb = &intel_fb->base;
>  
>  	val = I915_READ(PLANE_CTL(pipe, 0));
> +	if (!(val & PLANE_CTL_ENABLE))
> +		goto error;
> +
>  	if (val & PLANE_CTL_TILED_MASK)
>  		plane_config->tiling = I915_TILING_X;
>  
> @@ -7741,6 +7746,10 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>  	struct drm_framebuffer *fb;
>  	struct intel_framebuffer *intel_fb;
>  
> +	val = I915_READ(DSPCNTR(pipe));
> +	if (!(val & DISPLAY_PLANE_ENABLE))
> +		return;
> +
>  	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
>  	if (!intel_fb) {
>  		DRM_DEBUG_KMS("failed to alloc fb\n");
> @@ -7749,8 +7758,6 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>  
>  	fb = &intel_fb->base;
>  
> -	val = I915_READ(DSPCNTR(pipe));
> -
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		if (val & DISPPLANE_TILED)
>  			plane_config->tiling = I915_TILING_X;
> -- 
> 1.8.3.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/5] drm/i915: Make sure the primary plane is enabled before reading out the fb state

[Daniel Vetter]

On Thu, Feb 05, 2015 at 01:00:53PM -0800, Matt Roper wrote:
> On Thu, Feb 05, 2015 at 07:35:13PM +0000, Damien Lespiau wrote:
> > We don't want to end up in a state where we track that the pipe has its
> > primary plane enabled when primary plane registers are programmed with
> > values that look possible but the plane actually disabled.
> > 
> > Refuse to read out the fb state when the primary plane isn't enabled.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> For the series:
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

All merged, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx