diff mbox series

[2/2] drm/i915: Add rotation readout for plane initial config

Message ID 20181120135450.3634-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Force a LUT update in intel_initial_commit() | expand

Commit Message

Ville Syrjälä Nov. 20, 2018, 1:54 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If we need to force a full plane update before userspace/fbdev
have given us a proper plane state we should try to maintain the
current plane state as much as possible (apart from the parts
of the state we're trying to fix up with the plane update).
To that end add basic readout for the plane rotation and
maintain it during the initial fb takeover.

Cc: Hans de Goede <hdegoede@redhat.com>
Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup with external display")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 28 insertions(+)

Comments

Rodrigo Vivi Nov. 20, 2018, 10:20 p.m. UTC | #1 
On Tue, Nov 20, 2018 at 03:54:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If we need to force a full plane update before userspace/fbdev
> have given us a proper plane state we should try to maintain the
> current plane state as much as possible (apart from the parts
> of the state we're trying to fix up with the plane update).
> To that end add basic readout for the plane rotation and
> maintain it during the initial fb takeover.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup with external display")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 60c1e54285c1..4d339f54559c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2831,6 +2831,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	return;
>  
>  valid_fb:
> +	intel_state->base.rotation = plane_config->rotation;
>  	intel_fill_fb_ggtt_view(&intel_state->view, fb,
>  				intel_state->base.rotation);
>  	intel_state->color_plane[0].stride =
> @@ -7787,8 +7788,15 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>  			plane_config->tiling = I915_TILING_X;
>  			fb->modifier = I915_FORMAT_MOD_X_TILED;
>  		}
> +
> +		if (val & DISPPLANE_ROTATE_180)
> +			plane_config->rotation = DRM_MODE_ROTATE_180;
>  	}
>  
> +	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B &&
> +	    val & DISPPLANE_MIRROR)
> +		plane_config->rotation |= DRM_MODE_REFLECT_X;
> +
>  	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
>  	fourcc = i9xx_format_to_fourcc(pixel_format);
>  	fb->format = drm_format_info(fourcc);
> @@ -8898,6 +8906,25 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  		goto error;
>  	}
>  
> +	switch (val & PLANE_CTL_ROTATE_MASK) {
> +	case PLANE_CTL_ROTATE_0:
> +		plane_config->rotation = DRM_MODE_ROTATE_0;
> +		break;
> +	case PLANE_CTL_ROTATE_90:
> +		plane_config->rotation = DRM_MODE_ROTATE_270;

we should add that comment that DRM_MODE_ROTATE_ is counter clockwise
to avoid later confusion.

Also I wonder if we shouldn't move all the cases to a unique
map function.

Anyways,


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



> +		break;
> +	case PLANE_CTL_ROTATE_180:
> +		plane_config->rotation = DRM_MODE_ROTATE_180;
> +		break;
> +	case PLANE_CTL_ROTATE_270:
> +		plane_config->rotation = DRM_MODE_ROTATE_90;
> +		break;
> +	}
> +
> +	if (INTEL_GEN(dev_priv) >= 10 &&
> +	    val & PLANE_CTL_FLIP_HORIZONTAL)
> +		plane_config->rotation |= DRM_MODE_REFLECT_X;
> +
>  	base = I915_READ(PLANE_SURF(pipe, plane_id)) & 0xfffff000;
>  	plane_config->base = base;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f575ba2a59da..a7d9ac912125 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -572,6 +572,7 @@ struct intel_initial_plane_config {
>  	unsigned int tiling;
>  	int size;
>  	u32 base;
> +	u8 rotation;
>  };
>  
>  #define SKL_MIN_SRC_W 8
> -- 
> 2.18.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst Nov. 21, 2018, 11:34 a.m. UTC | #2 
Op 20-11-18 om 23:20 schreef Rodrigo Vivi:
> On Tue, Nov 20, 2018 at 03:54:50PM +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> If we need to force a full plane update before userspace/fbdev
>> have given us a proper plane state we should try to maintain the
>> current plane state as much as possible (apart from the parts
>> of the state we're trying to fix up with the plane update).
>> To that end add basic readout for the plane rotation and
>> maintain it during the initial fb takeover.
>>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup with external display")
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 60c1e54285c1..4d339f54559c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2831,6 +2831,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>>  	return;
>>  
>>  valid_fb:
>> +	intel_state->base.rotation = plane_config->rotation;
>>  	intel_fill_fb_ggtt_view(&intel_state->view, fb,
>>  				intel_state->base.rotation);
>>  	intel_state->color_plane[0].stride =
>> @@ -7787,8 +7788,15 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>>  			plane_config->tiling = I915_TILING_X;
>>  			fb->modifier = I915_FORMAT_MOD_X_TILED;
>>  		}
>> +
>> +		if (val & DISPPLANE_ROTATE_180)
>> +			plane_config->rotation = DRM_MODE_ROTATE_180;
>>  	}
>>  
>> +	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B &&
>> +	    val & DISPPLANE_MIRROR)
>> +		plane_config->rotation |= DRM_MODE_REFLECT_X;
>> +
>>  	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
>>  	fourcc = i9xx_format_to_fourcc(pixel_format);
>>  	fb->format = drm_format_info(fourcc);
>> @@ -8898,6 +8906,25 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>>  		goto error;
>>  	}
>>  
>> +	switch (val & PLANE_CTL_ROTATE_MASK) {
>> +	case PLANE_CTL_ROTATE_0:
>> +		plane_config->rotation = DRM_MODE_ROTATE_0;
>> +		break;
>> +	case PLANE_CTL_ROTATE_90:
>> +		plane_config->rotation = DRM_MODE_ROTATE_270;
> we should add that comment that DRM_MODE_ROTATE_ is counter clockwise
> to avoid later confusion.
>
> Also I wonder if we shouldn't move all the cases to a unique
> map function.
>
> Anyways,
>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>
>
>> +		break;
>> +	case PLANE_CTL_ROTATE_180:
>> +		plane_config->rotation = DRM_MODE_ROTATE_180;
>> +		break;
>> +	case PLANE_CTL_ROTATE_270:
>> +		plane_config->rotation = DRM_MODE_ROTATE_90;
>> +		break;
>> +	}
>> +
>> +	if (INTEL_GEN(dev_priv) >= 10 &&
>> +	    val & PLANE_CTL_FLIP_HORIZONTAL)
>> +		plane_config->rotation |= DRM_MODE_REFLECT_X;
>> +
>>  	base = I915_READ(PLANE_SURF(pipe, plane_id)) & 0xfffff000;
>>  	plane_config->base = base;
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index f575ba2a59da..a7d9ac912125 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -572,6 +572,7 @@ struct intel_initial_plane_config {
>>  	unsigned int tiling;
>>  	int size;
>>  	u32 base;
>> +	u8 rotation;
>>  };
>>  
>>  #define SKL_MIN_SRC_W 8
>> -- 
>> 2.18.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Not sure if pushed yet.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Ville Syrjälä Nov. 21, 2018, 12:13 p.m. UTC | #3 
On Tue, Nov 20, 2018 at 02:20:14PM -0800, Rodrigo Vivi wrote:
> On Tue, Nov 20, 2018 at 03:54:50PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If we need to force a full plane update before userspace/fbdev
> > have given us a proper plane state we should try to maintain the
> > current plane state as much as possible (apart from the parts
> > of the state we're trying to fix up with the plane update).
> > To that end add basic readout for the plane rotation and
> > maintain it during the initial fb takeover.
> > 
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Fixes: 516a49cc1946 ("drm/i915: Fix assert_plane() warning on bootup with external display")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 60c1e54285c1..4d339f54559c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2831,6 +2831,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  	return;
> >  
> >  valid_fb:
> > +	intel_state->base.rotation = plane_config->rotation;
> >  	intel_fill_fb_ggtt_view(&intel_state->view, fb,
> >  				intel_state->base.rotation);
> >  	intel_state->color_plane[0].stride =
> > @@ -7787,8 +7788,15 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> >  			plane_config->tiling = I915_TILING_X;
> >  			fb->modifier = I915_FORMAT_MOD_X_TILED;
> >  		}
> > +
> > +		if (val & DISPPLANE_ROTATE_180)
> > +			plane_config->rotation = DRM_MODE_ROTATE_180;
> >  	}
> >  
> > +	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B &&
> > +	    val & DISPPLANE_MIRROR)
> > +		plane_config->rotation |= DRM_MODE_REFLECT_X;
> > +
> >  	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> >  	fourcc = i9xx_format_to_fourcc(pixel_format);
> >  	fb->format = drm_format_info(fourcc);
> > @@ -8898,6 +8906,25 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
> >  		goto error;
> >  	}
> >  
> > +	switch (val & PLANE_CTL_ROTATE_MASK) {
> > +	case PLANE_CTL_ROTATE_0:
> > +		plane_config->rotation = DRM_MODE_ROTATE_0;
> > +		break;
> > +	case PLANE_CTL_ROTATE_90:
> > +		plane_config->rotation = DRM_MODE_ROTATE_270;
> 
> we should add that comment that DRM_MODE_ROTATE_ is counter clockwise
> to avoid later confusion.

I copied over the comment from the other place, and pushed the series
to dinq. Thanks for the reviews, testing, and tracking down the bug
initially.

> 
> Also I wonder if we shouldn't move all the cases to a unique
> map function.
> 
> Anyways,
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> 
> > +		break;
> > +	case PLANE_CTL_ROTATE_180:
> > +		plane_config->rotation = DRM_MODE_ROTATE_180;
> > +		break;
> > +	case PLANE_CTL_ROTATE_270:
> > +		plane_config->rotation = DRM_MODE_ROTATE_90;
> > +		break;
> > +	}
> > +
> > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > +	    val & PLANE_CTL_FLIP_HORIZONTAL)
> > +		plane_config->rotation |= DRM_MODE_REFLECT_X;
> > +
> >  	base = I915_READ(PLANE_SURF(pipe, plane_id)) & 0xfffff000;
> >  	plane_config->base = base;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index f575ba2a59da..a7d9ac912125 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -572,6 +572,7 @@ struct intel_initial_plane_config {
> >  	unsigned int tiling;
> >  	int size;
> >  	u32 base;
> > +	u8 rotation;
> >  };
> >  
> >  #define SKL_MIN_SRC_W 8
> > -- 
> > 2.18.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60c1e54285c1..4d339f54559c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2831,6 +2831,7 @@  intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	return;
 
 valid_fb:
+	intel_state->base.rotation = plane_config->rotation;
 	intel_fill_fb_ggtt_view(&intel_state->view, fb,
 				intel_state->base.rotation);
 	intel_state->color_plane[0].stride =
@@ -7787,8 +7788,15 @@  i9xx_get_initial_plane_config(struct intel_crtc *crtc,
 			plane_config->tiling = I915_TILING_X;
 			fb->modifier = I915_FORMAT_MOD_X_TILED;
 		}
+
+		if (val & DISPPLANE_ROTATE_180)
+			plane_config->rotation = DRM_MODE_ROTATE_180;
 	}
 
+	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B &&
+	    val & DISPPLANE_MIRROR)
+		plane_config->rotation |= DRM_MODE_REFLECT_X;
+
 	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
 	fourcc = i9xx_format_to_fourcc(pixel_format);
 	fb->format = drm_format_info(fourcc);
@@ -8898,6 +8906,25 @@  skylake_get_initial_plane_config(struct intel_crtc *crtc,
 		goto error;
 	}
 
+	switch (val & PLANE_CTL_ROTATE_MASK) {
+	case PLANE_CTL_ROTATE_0:
+		plane_config->rotation = DRM_MODE_ROTATE_0;
+		break;
+	case PLANE_CTL_ROTATE_90:
+		plane_config->rotation = DRM_MODE_ROTATE_270;
+		break;
+	case PLANE_CTL_ROTATE_180:
+		plane_config->rotation = DRM_MODE_ROTATE_180;
+		break;
+	case PLANE_CTL_ROTATE_270:
+		plane_config->rotation = DRM_MODE_ROTATE_90;
+		break;
+	}
+
+	if (INTEL_GEN(dev_priv) >= 10 &&
+	    val & PLANE_CTL_FLIP_HORIZONTAL)
+		plane_config->rotation |= DRM_MODE_REFLECT_X;
+
 	base = I915_READ(PLANE_SURF(pipe, plane_id)) & 0xfffff000;
 	plane_config->base = base;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f575ba2a59da..a7d9ac912125 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -572,6 +572,7 @@  struct intel_initial_plane_config {
 	unsigned int tiling;
 	int size;
 	u32 base;
+	u8 rotation;
 };
 
 #define SKL_MIN_SRC_W 8