[PATCH 1/4] drm/i915: Dump in-flight plane state while dumping in-flight CRTC state

[PATCH 1/4] drm/i915: Dump in-flight plane state while dumping in-flight CRTC state

[Matt Roper]

The intel_dump_pipe_config() always dumps the currently active plane
state for each plane on a CRTC.  If we're calling this function to dump
CRTC state that is in-flight (not yet active), then this mismatch can be
misleading and confusing.  Let's pay attention to whether the state
we're dumping is part of an in-flight transaction (because
crtc_state->state is non-NULL); if it is, we'll dump the corresponding
in-flight plane state instead of the active state.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493..7bbcb98 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12074,11 +12074,23 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 
 	DRM_DEBUG_KMS("planes on this crtc\n");
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		struct drm_plane_state *ps = NULL;
+
 		intel_plane = to_intel_plane(plane);
 		if (intel_plane->pipe != crtc->pipe)
 			continue;
 
-		state = to_intel_plane_state(plane->state);
+		/* Get in-flight plane state, if any */
+		if (pipe_config->base.state)
+			ps = drm_atomic_get_existing_plane_state(pipe_config->base.state,
+								 plane);
+
+		/* If no in-flight state, use active state instead */
+		if (!ps)
+			ps = plane->state;
+
+		state = to_intel_plane_state(ps);
+
 		fb = state->base.fb;
 		if (!fb) {
 			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
-- 
2.1.4

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

[PATCH 2/4] drm/i915: Clarify plane state during CRTC state dumps

[Matt Roper]

During state dumping, list planes that have an FB but are invisible
(e.g., because they're offscreen or clipped by other planes) as "not
visible" rather than "enabled."  While we're at it, throw the bpp value
into the debugging output.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7bbcb98..d994f52 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12102,13 +12102,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			continue;
 		}
 
-		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
+		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d %s",
 			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
 			plane->base.id, intel_plane->pipe,
 			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
-			drm_plane_index(plane));
-		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
-			fb->base.id, fb->width, fb->height, fb->pixel_format);
+			drm_plane_index(plane),
+			state->visible ? "enabled" : "not visible");
+		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x bpp = %d",
+			fb->base.id, fb->width, fb->height, fb->pixel_format,
+			fb->bits_per_pixel);
 		DRM_DEBUG_KMS("\tscaler:%d src (%u, %u) %ux%u dst (%u, %u) %ux%u\n",
 			state->scaler_id,
 			state->src.x1 >> 16, state->src.y1 >> 16,
-- 
2.1.4

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

Re: [PATCH 2/4] drm/i915: Clarify plane state during CRTC state dumps

[Ville Syrjälä]

On Thu, Nov 12, 2015 at 04:31:57PM -0800, Matt Roper wrote:
> During state dumping, list planes that have an FB but are invisible
> (e.g., because they're offscreen or clipped by other planes) as "not
> visible" rather than "enabled."  While we're at it, throw the bpp value
> into the debugging output.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7bbcb98..d994f52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12102,13 +12102,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  			continue;
>  		}
>  
> -		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
> +		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d %s",
>  			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
>  			plane->base.id, intel_plane->pipe,
>  			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
> -			drm_plane_index(plane));
> -		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
> -			fb->base.id, fb->width, fb->height, fb->pixel_format);
> +			drm_plane_index(plane),
> +			state->visible ? "enabled" : "not visible");
> +		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x bpp = %d",
> +			fb->base.id, fb->width, fb->height, fb->pixel_format,
> +			fb->bits_per_pixel);

Let's try to avoid bits_per_pixel shall we. It's not even populated for
YCbCr formats. We print the format here already so I think it should be
enough. But we should dump in using drm_get_format_name().

>  		DRM_DEBUG_KMS("\tscaler:%d src (%u, %u) %ux%u dst (%u, %u) %ux%u\n",
>  			state->scaler_id,
>  			state->src.x1 >> 16, state->src.y1 >> 16,
> -- 
> 2.1.4
> 
> _______________________________________________
> 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

[PATCH 3/4] drm/i915: Dump pipe config after initial FB is reconstructed

[Matt Roper]

We dump during HW state readout, but that's too early to reflect how the
primary plane is actually configured.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d994f52..9fa041c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15118,6 +15118,9 @@ void intel_modeset_init(struct drm_device *dev)
 		 * just get the first one.
 		 */
 		intel_find_initial_plane_obj(crtc, &plane_config);
+
+		intel_dump_pipe_config(crtc, crtc->config,
+				       "[state after init fb reconstruction]");
 	}
 }
 
-- 
2.1.4

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

Re: [PATCH 3/4] drm/i915: Dump pipe config after initial FB is reconstructed

[Ville Syrjälä]

On Thu, Nov 12, 2015 at 04:31:58PM -0800, Matt Roper wrote:
> We dump during HW state readout, but that's too early to reflect how the
> primary plane is actually configured.

Hmm. Would be nice if we could read out everything at the same place,
and then saniitize later.

And maybe we could introduce some kind of NOP FB for planes where we
haven't yet reconstructed the BIOS FB (or are unable to do so). That
way the plane sanitize could be exactly like a normal modeset?

> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d994f52..9fa041c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15118,6 +15118,9 @@ void intel_modeset_init(struct drm_device *dev)
>  		 * just get the first one.
>  		 */
>  		intel_find_initial_plane_obj(crtc, &plane_config);
> +
> +		intel_dump_pipe_config(crtc, crtc->config,
> +				       "[state after init fb reconstruction]");
>  	}
>  }
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> 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

[PATCH 4/4] drm/i915: Setup clipped src/dest coordinates during FB reconstruction

[Matt Roper]

Plane state objects contain two copies of src/dest coordinates:  the
original (requested by userspace) coordinates in the base
drm_plane_state object, and a second, clipped copy (i.e., what we
actually want to program to the hardware) in intel_plane_state.  We've
only been setting up the former set of values during boot time FB
reconstruction, but we should really be initializing both.

Note that the code here probably still needs some more work.  We seem to
be making two questionable assumptions:
 * BIOS will program the primary plane to the entire display size
   (potentially false on gen9+ platforms where the primary plane can be
   windowed)
 * The BIOS fb size actually matches the plane/display size.  It seems
   that there's nothing stopping the BIOS from overallocating the FB
   (potentially to do an extended mode FB that spans multiple displays),
   so setting our src/dest coordinates to the FB size here may not
   always be right.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9fa041c..3145c9d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2601,6 +2601,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	struct drm_i915_gem_object *obj;
 	struct drm_plane *primary = intel_crtc->base.primary;
 	struct drm_plane_state *plane_state = primary->state;
+	struct intel_plane_state *intel_state =
+		to_intel_plane_state(plane_state);
 	struct drm_framebuffer *fb;
 
 	if (!plane_config->fb)
@@ -2648,6 +2650,15 @@ valid_fb:
 	plane_state->crtc_w = fb->width;
 	plane_state->crtc_h = fb->height;
 
+	intel_state->src.x1 = plane_state->src_x;
+	intel_state->src.y1 = plane_state->src_y;
+	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
+	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
+	intel_state->dst.x1 = plane_state->crtc_x;
+	intel_state->dst.y1 = plane_state->crtc_y;
+	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
+	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;
+
 	obj = intel_fb_obj(fb);
 	if (obj->tiling_mode != I915_TILING_NONE)
 		dev_priv->preserve_bios_swizzle = true;
-- 
2.1.4

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

Re: [PATCH 4/4] drm/i915: Setup clipped src/dest coordinates during FB reconstruction

[Ville Syrjälä]

On Thu, Nov 12, 2015 at 04:31:59PM -0800, Matt Roper wrote:
> Plane state objects contain two copies of src/dest coordinates:  the
> original (requested by userspace) coordinates in the base
> drm_plane_state object, and a second, clipped copy (i.e., what we
> actually want to program to the hardware) in intel_plane_state.  We've
> only been setting up the former set of values during boot time FB
> reconstruction, but we should really be initializing both.
> 
> Note that the code here probably still needs some more work.  We seem to
> be making two questionable assumptions:
>  * BIOS will program the primary plane to the entire display size
>    (potentially false on gen9+ platforms where the primary plane can be
>    windowed)

We don't actually assume that on SKL+. We read out the plane size
register and use that to set the initial fb dimensions. Although if
the BIOS uses a scaled plane, we will get the wrong answer since then
the plane size register will contain the dst size, not the src.

>  * The BIOS fb size actually matches the plane/display size.  It seems
>    that there's nothing stopping the BIOS from overallocating the FB
>    (potentially to do an extended mode FB that spans multiple displays),
>    so setting our src/dest coordinates to the FB size here may not
>    always be right.

I suppose we could have some kind of post-process step after the readout
to see if all the planes actually scan out from different parts of the
same fb. But that seems rather unlikely to me.

The other assumption we do is that the BIOS sets up GTT to 1:1
map the stolen memory. Not sure if some BIOSen would for instance
try to avoid the first page of stolen on gen8 (with EFI it should be
doable I suppose, with VGA probably not), or maybe sets up a 90/270
rotated mapping instead. I have one BYT tablet where the BIOS uses
180 degree rotation, so even 90/270 degree rotation is not totally
out of the question I think.

> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9fa041c..3145c9d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2601,6 +2601,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	struct drm_i915_gem_object *obj;
>  	struct drm_plane *primary = intel_crtc->base.primary;
>  	struct drm_plane_state *plane_state = primary->state;
> +	struct intel_plane_state *intel_state =
> +		to_intel_plane_state(plane_state);
>  	struct drm_framebuffer *fb;
>  
>  	if (!plane_config->fb)
> @@ -2648,6 +2650,15 @@ valid_fb:
>  	plane_state->crtc_w = fb->width;
>  	plane_state->crtc_h = fb->height;
>  
> +	intel_state->src.x1 = plane_state->src_x;
> +	intel_state->src.y1 = plane_state->src_y;
> +	intel_state->src.x2 = plane_state->src_x + plane_state->src_w;
> +	intel_state->src.y2 = plane_state->src_y + plane_state->src_h;
> +	intel_state->dst.x1 = plane_state->crtc_x;
> +	intel_state->dst.y1 = plane_state->crtc_y;
> +	intel_state->dst.x2 = plane_state->crtc_x + plane_state->crtc_w;
> +	intel_state->dst.y2 = plane_state->crtc_y + plane_state->crtc_h;

Patch looks sane.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  	obj = intel_fb_obj(fb);
>  	if (obj->tiling_mode != I915_TILING_NONE)
>  		dev_priv->preserve_bios_swizzle = true;
> -- 
> 2.1.4
> 
> _______________________________________________
> 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