[PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Hans de Goede]

Hi All,

So I recently bought a (second-hand) Bay Trail tablet which has its LCD
mounted upside-down. As such I've ported Ville Syrjala's patches to deal
with this to current mainline and I'm hereby posting them upstream
for merging.

These patches fix the kernel-console as well as the boot-splash
(at leats plymouth does not reset the rotation) being upside down as
soon as a native kms driver such as the i915 driver is loaded.

This fixes the orientation of the displayed image from boot till the
Xserver or a Wayland compositor takes over.

I've a patch for iio-sensor-proxy which fixes the rotation under Xorg /
Wayland when using a desktop environment which honors iio-sensor-proxy's
rotation detection:
https://github.com/hadess/iio-sensor-proxy/pull/162

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Hans de Goede]

From: Ville Syrjala <ville.syrjala@linux.intel.com>

If a connector added through drm_fb_helper_add_one_connector() has
a crtc attached and that crtc has a rotation configured make the
fbdev inherit the crtc's rotation.

This is useful on e.g. some tablets which have their lcd panel mounted
upside down, which before this commit would result in the kernel boot
messages switching from being shown the right way up in efifb to being
shown upside down as soon as a native kms driver loads.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
[hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's
 "drm/fb-helper: Inherit rotation wip" patch]
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 51 ++++++++++++++++++++++++++++++++++++++---
 include/drm/drm_fb_helper.h     |  2 ++
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 324a688..c97e00ab 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
 {
 	struct drm_fb_helper_connector **temp;
 	struct drm_fb_helper_connector *fb_helper_connector;
+	struct drm_crtc *crtc = connector->encoder ?
+				connector->encoder->crtc : NULL;
 
 	if (!drm_fbdev_emulation)
 		return 0;
@@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_
 
 	drm_connector_reference(connector);
 	fb_helper_connector->connector = connector;
+	if (crtc && crtc->primary->state)
+		fb_helper_connector->rotation = crtc->primary->state->rotation;
+	if (!fb_helper_connector->rotation)
+		fb_helper_connector->rotation = DRM_ROTATE_0;
+
 	fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector;
 	return 0;
 }
@@ -333,6 +340,35 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 
+static int fbdev_plane_index(struct drm_fb_helper *fb_helper,
+			     struct drm_plane *plane)
+{
+	int i;
+
+	if (plane->type != DRM_PLANE_TYPE_PRIMARY)
+		return -ENODEV;
+
+	for (i = 0; i < fb_helper->crtc_count; i++) {
+		struct drm_crtc *crtc = fb_helper->crtc_info[i].mode_set.crtc;
+
+		if (crtc && crtc->primary == plane)
+			return i;
+	}
+
+	return -ENODEV;
+}
+
+static unsigned int fbdev_plane_rotation(struct drm_fb_helper *fb_helper,
+					 struct drm_plane *plane)
+{
+	int i = fbdev_plane_index(fb_helper, plane);
+
+	if (i < 0)
+		return DRM_ROTATE_0;
+	else
+		return fb_helper->crtc_info[i].rotation;
+}
+
 static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -357,7 +393,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper)
 			goto fail;
 		}
 
-		plane_state->rotation = DRM_ROTATE_0;
+		plane_state->rotation = fbdev_plane_rotation(fb_helper, plane);
 
 		plane->old_fb = plane->fb;
 		plane_mask |= 1 << drm_plane_index(plane);
@@ -414,8 +450,8 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 
 		if (plane->rotation_property)
 			drm_mode_plane_set_obj_prop(plane,
-						    plane->rotation_property,
-						    DRM_ROTATE_0);
+					plane->rotation_property,
+					fbdev_plane_rotation(fb_helper, plane));
 	}
 
 	for (i = 0; i < fb_helper->crtc_count; i++) {
@@ -760,6 +796,7 @@ int drm_fb_helper_init(struct drm_device *dev,
 		if (!fb_helper->crtc_info[i].mode_set.connectors)
 			goto out_free;
 		fb_helper->crtc_info[i].mode_set.num_connectors = 0;
+		fb_helper->crtc_info[i].rotation = DRM_ROTATE_0;
 	}
 
 	i = 0;
@@ -2090,8 +2127,14 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 
 			if (!drm_mode_equal(modes[o], modes[n]))
 				continue;
+
+			if (crtc->rotation &&
+			    crtc->rotation != fb_helper_conn->rotation)
+				continue;
 		}
 
+		crtc->rotation = fb_helper_conn->rotation;
+
 		crtcs[n] = crtc;
 		memcpy(crtcs, best_crtcs, n * sizeof(struct drm_fb_helper_crtc *));
 		score = my_score + drm_pick_crtcs(fb_helper, crtcs, modes, n + 1,
@@ -2182,6 +2225,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 			fb_crtc->desired_mode = mode;
 			fb_crtc->x = offset->x;
 			fb_crtc->y = offset->y;
+			fb_crtc->rotation =
+				fb_helper->connector_info[i]->rotation;
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
 			drm_connector_reference(connector);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 6f5aceb..19fc313 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -48,6 +48,7 @@ struct drm_fb_helper_crtc {
 	struct drm_mode_set mode_set;
 	struct drm_display_mode *desired_mode;
 	int x, y;
+	uint8_t rotation;
 };
 
 /**
@@ -159,6 +160,7 @@ struct drm_fb_helper_funcs {
 
 struct drm_fb_helper_connector {
 	struct drm_connector *connector;
+	uint8_t rotation;
 };
 
 /**
-- 
2.9.3

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

[PATCH 2/2] drm/i915: Make get_initial_plane_config also get the initial rotation config

[Hans de Goede]

From: Ville Syrjala <ville.syrjala@linux.intel.com>

When retrieving the initial settings / mode from the hardware also
retrieve the initial rotation config.

Together with "drm/fb-helper: Make fbdev inherit the crtc's rotation"
this will make the fbdev inherit the initial rotation.

This is useful on e.g. some tablets which have their lcd panel mounted
upside down, which before this commit would result in the kernel boot
messages switching from being shown the right way up in efifb to being
shown upside down as soon as a native kms driver loads.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
[hdegoede@redhat.com: Split the intel_display bits out of Ville's
 "drm/fb-helper: Inherit rotation wip" patch]
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed1f4f2..5c9f504 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2821,6 +2821,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	plane_state->crtc_w = fb->width;
 	plane_state->crtc_h = fb->height;
 
+	plane_state->rotation = plane_config->rotation;
+
 	intel_state->base.src = drm_plane_state_src(plane_state);
 	intel_state->base.dst = drm_plane_state_dest(plane_state);
 
@@ -8733,6 +8735,9 @@ 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_ROTATE_180;
 	}
 
 	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
@@ -9784,6 +9789,24 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
 		goto error;
 	}
 
+	/*
+	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
+	 * while i915 HW rotation is clockwise, thats why this swapping.
+	 */
+	switch (val & PLANE_CTL_ROTATE_MASK) {
+	case PLANE_CTL_ROTATE_0:
+		break;
+	case PLANE_CTL_ROTATE_90:
+		plane_config->rotation = DRM_ROTATE_270;
+		break;
+	case PLANE_CTL_ROTATE_180:
+		plane_config->rotation = DRM_ROTATE_180;
+		break;
+	case PLANE_CTL_ROTATE_270:
+		plane_config->rotation = DRM_ROTATE_90;
+		break;
+	}
+
 	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
 	plane_config->base = base;
 
@@ -9872,6 +9895,9 @@ ironlake_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_ROTATE_180;
 	}
 
 	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
@@ -16713,7 +16739,9 @@ int intel_modeset_init(struct drm_device *dev)
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
-		struct intel_initial_plane_config plane_config = {};
+		struct intel_initial_plane_config plane_config = {
+			.rotation = DRM_ROTATE_0
+		};
 
 		if (!crtc->active)
 			continue;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 344f238..63623dd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -418,6 +418,7 @@ struct intel_initial_plane_config {
 	unsigned int tiling;
 	int size;
 	u32 base;
+	uint8_t rotation;
 };
 
 #define SKL_MIN_SRC_W 8
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Ville Syrjälä]

On Sun, Apr 23, 2017 at 06:11:04PM +0200, Hans de Goede wrote:
> Hi All,
> 
> So I recently bought a (second-hand) Bay Trail tablet which has its LCD
> mounted upside-down. As such I've ported Ville Syrjala's patches to deal
> with this to current mainline and I'm hereby posting them upstream
> for merging.
> 
> These patches fix the kernel-console as well as the boot-splash
> (at leats plymouth does not reset the rotation) being upside down as
> soon as a native kms driver such as the i915 driver is loaded.
> 
> This fixes the orientation of the displayed image from boot till the
> Xserver or a Wayland compositor takes over.

It doesn't work for X? Using -modesetting perhaps? IIRC it worked just
fine with -intel way back when.

> 
> I've a patch for iio-sensor-proxy which fixes the rotation under Xorg /
> Wayland when using a desktop environment which honors iio-sensor-proxy's
> rotation detection:
> https://github.com/hadess/iio-sensor-proxy/pull/162

Or is it just this thing that clobbers what the DDX inherited from the
kernel as the initial rotation?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Bastien Nocera]

On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote:
> From: Ville Syrjala <ville.syrjala@linux.intel.com>
> 
> If a connector added through drm_fb_helper_add_one_connector() has
> a crtc attached and that crtc has a rotation configured make the
> fbdev inherit the crtc's rotation.
> 
> This is useful on e.g. some tablets which have their lcd panel
> mounted
> upside down, which before this commit would result in the kernel boot
> messages switching from being shown the right way up in efifb to
> being
> shown upside down as soon as a native kms driver loads.
> 
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's
>  "drm/fb-helper: Inherit rotation wip" patch]
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 51
> ++++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_fb_helper.h     |  2 ++
>  2 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> b/drivers/gpu/drm/drm_fb_helper.c
> index 324a688..c97e00ab 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct
> drm_fb_helper *fb_helper, struct drm_
>  {
>  	struct drm_fb_helper_connector **temp;
>  	struct drm_fb_helper_connector *fb_helper_connector;
> +	struct drm_crtc *crtc = connector->encoder ?
> +				connector->encoder->crtc : NULL;
>  
>  	if (!drm_fbdev_emulation)
>  		return 0;
> @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct
> drm_fb_helper *fb_helper, struct drm_
>  
>  	drm_connector_reference(connector);
>  	fb_helper_connector->connector = connector;
> +	if (crtc && crtc->primary->state)
> +		fb_helper_connector->rotation = crtc->primary-
> >state->rotation;
> +	if (!fb_helper_connector->rotation)
> +		fb_helper_connector->rotation = DRM_ROTATE_0;

That's equivalent to:
if (fb_helper_connector->rotation = DRM_ROTATE_0)
   fb_helper_connector->rotation = DRM_ROTATE_0;

Maybe:
if (crtc && crtc->primary->state)
   ...
else
   ...->rotation = DRM_ROTATE_0;
would be clearer? Or simply omit it if the connector is zero'ed.   

<snip>
> diff --git a/include/drm/drm_fb_helper.h
> b/include/drm/drm_fb_helper.h
> index 6f5aceb..19fc313 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -48,6 +48,7 @@ struct drm_fb_helper_crtc {
>  	struct drm_mode_set mode_set;
>  	struct drm_display_mode *desired_mode;
>  	int x, y;
> +	uint8_t rotation;
>  };
>  
>  /**
> @@ -159,6 +160,7 @@ struct drm_fb_helper_funcs {
>  
>  struct drm_fb_helper_connector {
>  	struct drm_connector *connector;
> +	uint8_t rotation;

In both those cases, I'd add a mention of the type of enum/mask etc.
for the rotation, for example:
uint8_t rotation; /* DRM_ROTATE_* */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Make get_initial_plane_config also get the initial rotation config

[Bastien Nocera]

On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote:
> From: Ville Syrjala <ville.syrjala@linux.intel.com>
<snip>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 344f238..63623dd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -418,6 +418,7 @@ struct intel_initial_plane_config {
>  	unsigned int tiling;
>  	int size;
>  	u32 base;
> +	uint8_t rotation;

Mentioning what this is (DRM_ROTATE_* or something else) is even more
important here.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Bastien Nocera]

On Mon, 2017-04-24 at 15:48 +0300, Ville Syrjälä wrote:
> 
<snip>

> > I've a patch for iio-sensor-proxy which fixes the rotation under
> > Xorg /
> > Wayland when using a desktop environment which honors iio-sensor-
> > proxy's
> > rotation detection:
> > https://github.com/hadess/iio-sensor-proxy/pull/162
> 
> Or is it just this thing that clobbers what the DDX inherited from
> the
> kernel as the initial rotation?

I think it's mostly got to do with the compositor (or X) not knowing
what "normal" or "0 degrees rotation" corresponds to.

I would expect the baseline test for this wouldn't involve iio-sensor-
proxy at all, and boot to the desktop with the expected orientation
showing up in the desktop env/XRandR as "not rotated".

Eg. for the GPD Win which does have this problem, this should appear as
being rotated (image 1):
https://c.slashgear.com/wp-content/uploads/2016/11/gpd-win-book-2.jpg
But not this (image 2):
https://c.slashgear.com/wp-content/uploads/2016/11/gpd-win-0-800x420.jpg

After that, the problem is whether the accelerometer is mounted the
same way as the "non-rotated screen" (option 2), or the "non-rotated
screen" (option 1), which would show what quirking is needed.

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

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 02:28:32PM +0200, Bastien Nocera wrote:
> On Mon, 2017-04-24 at 15:48 +0300, Ville Syrjälä wrote:
> > 
> <snip>
> 
> > > I've a patch for iio-sensor-proxy which fixes the rotation under
> > > Xorg /
> > > Wayland when using a desktop environment which honors iio-sensor-
> > > proxy's
> > > rotation detection:
> > > https://github.com/hadess/iio-sensor-proxy/pull/162
> > 
> > Or is it just this thing that clobbers what the DDX inherited from
> > the
> > kernel as the initial rotation?
> 
> I think it's mostly got to do with the compositor (or X) not knowing
> what "normal" or "0 degrees rotation" corresponds to.

Well, there are really two cases to consider:

1. BIOS/whatever configures display hardware rotation in a way
   that matches the orientation of the physical display
2. BIOS didn't do that. Either the hardware can't do what
   would be required, or the BIOS just chose not to do it.

Case 1 should work with these patches as long as the DDX will set up the
initial randr rotation to match what it read out from the kms rotation
property of the primary plane.

Case 2 can't work without some mechanism to query the orientation
of the display from the firmware/etc.

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

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Bastien Nocera]

On Thu, 2017-04-27 at 19:24 +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 02:28:32PM +0200, Bastien Nocera wrote:
> > On Mon, 2017-04-24 at 15:48 +0300, Ville Syrjälä wrote:
> > > 
> > 
> > <snip>
> > 
> > > > I've a patch for iio-sensor-proxy which fixes the rotation
> > > > under
> > > > Xorg /
> > > > Wayland when using a desktop environment which honors iio-
> > > > sensor-
> > > > proxy's
> > > > rotation detection:
> > > > https://github.com/hadess/iio-sensor-proxy/pull/162
> > > 
> > > Or is it just this thing that clobbers what the DDX inherited
> > > from
> > > the
> > > kernel as the initial rotation?
> > 
> > I think it's mostly got to do with the compositor (or X) not
> > knowing
> > what "normal" or "0 degrees rotation" corresponds to.
> 
> Well, there are really two cases to consider:
> 
> 1. BIOS/whatever configures display hardware rotation in a way
>    that matches the orientation of the physical display
> 2. BIOS didn't do that. Either the hardware can't do what
>    would be required, or the BIOS just chose not to do it.
> 
> Case 1 should work with these patches as long as the DDX will set up
> the
> initial randr rotation to match what it read out from the kms
> rotation
> property of the primary plane.

Yes. My problem was that instead of fixing the DDX to behave properly,
reusing the same orientation as already configured, we were using iio-
sensor-proxy to trigger the initial rotation. This doesn't work if
there's no accelerometer, or orientation is locked, which is counter-
intuitive.

> Case 2 can't work without some mechanism to query the orientation
> of the display from the firmware/etc.

Yes. I'm not sure where we'd be exporting this quirk though, as we need
it available early enough so that it can be used by boot splashes. DMI
matches in the graphics driver?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Hans de Goede]

Hi,

On 26-04-17 14:13, Bastien Nocera wrote:
> On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote:
>> From: Ville Syrjala <ville.syrjala@linux.intel.com>
>>
>> If a connector added through drm_fb_helper_add_one_connector() has
>> a crtc attached and that crtc has a rotation configured make the
>> fbdev inherit the crtc's rotation.
>>
>> This is useful on e.g. some tablets which have their lcd panel
>> mounted
>> upside down, which before this commit would result in the kernel boot
>> messages switching from being shown the right way up in efifb to
>> being
>> shown upside down as soon as a native kms driver loads.
>>
>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's
>>   "drm/fb-helper: Inherit rotation wip" patch]
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 51
>> ++++++++++++++++++++++++++++++++++++++---
>>   include/drm/drm_fb_helper.h     |  2 ++
>>   2 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 324a688..c97e00ab 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct
>> drm_fb_helper *fb_helper, struct drm_
>>   {
>>   	struct drm_fb_helper_connector **temp;
>>   	struct drm_fb_helper_connector *fb_helper_connector;
>> +	struct drm_crtc *crtc = connector->encoder ?
>> +				connector->encoder->crtc : NULL;
>>   
>>   	if (!drm_fbdev_emulation)
>>   		return 0;
>> @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct
>> drm_fb_helper *fb_helper, struct drm_
>>   
>>   	drm_connector_reference(connector);
>>   	fb_helper_connector->connector = connector;
>> +	if (crtc && crtc->primary->state)
>> +		fb_helper_connector->rotation = crtc->primary-
>>> state->rotation;
>> +	if (!fb_helper_connector->rotation)
>> +		fb_helper_connector->rotation = DRM_ROTATE_0;
> 
> That's equivalent to:
> if (fb_helper_connector->rotation = DRM_ROTATE_0)
>     fb_helper_connector->rotation = DRM_ROTATE_0;

No it is not equivalent:

include/drm/drm_blend.h
40:#define DRM_ROTATE_0 BIT(0)

and BIT(0) is (1 << 0)

Regards,

Hans

> 
> Maybe:
> if (crtc && crtc->primary->state)
>     ...
> else
>     ...->rotation = DRM_ROTATE_0;
> would be clearer? Or simply omit it if the connector is zero'ed.
> 
> <snip>
>> diff --git a/include/drm/drm_fb_helper.h
>> b/include/drm/drm_fb_helper.h
>> index 6f5aceb..19fc313 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -48,6 +48,7 @@ struct drm_fb_helper_crtc {
>>   	struct drm_mode_set mode_set;
>>   	struct drm_display_mode *desired_mode;
>>   	int x, y;
>> +	uint8_t rotation;
>>   };
>>   
>>   /**
>> @@ -159,6 +160,7 @@ struct drm_fb_helper_funcs {
>>   
>>   struct drm_fb_helper_connector {
>>   	struct drm_connector *connector;
>> +	uint8_t rotation;
> 
> In both those cases, I'd add a mention of the type of enum/mask etc.
> for the rotation, for example:
> uint8_t rotation; /* DRM_ROTATE_* */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Hans de Goede]

Hi,

On 27-04-17 18:39, Bastien Nocera wrote:
> On Thu, 2017-04-27 at 19:24 +0300, Ville Syrjälä wrote:
>> On Wed, Apr 26, 2017 at 02:28:32PM +0200, Bastien Nocera wrote:
>>> On Mon, 2017-04-24 at 15:48 +0300, Ville Syrjälä wrote:
>>>>
>>>
>>> <snip>
>>>
>>>>> I've a patch for iio-sensor-proxy which fixes the rotation
>>>>> under
>>>>> Xorg /
>>>>> Wayland when using a desktop environment which honors iio-
>>>>> sensor-
>>>>> proxy's
>>>>> rotation detection:
>>>>> https://github.com/hadess/iio-sensor-proxy/pull/162
>>>>
>>>> Or is it just this thing that clobbers what the DDX inherited
>>>> from
>>>> the
>>>> kernel as the initial rotation?
>>>
>>> I think it's mostly got to do with the compositor (or X) not
>>> knowing
>>> what "normal" or "0 degrees rotation" corresponds to.
>>
>> Well, there are really two cases to consider:
>>
>> 1. BIOS/whatever configures display hardware rotation in a way
>>     that matches the orientation of the physical display
>> 2. BIOS didn't do that. Either the hardware can't do what
>>     would be required, or the BIOS just chose not to do it.
>>
>> Case 1 should work with these patches as long as the DDX will set up
>> the
>> initial randr rotation to match what it read out from the kms
>> rotation
>> property of the primary plane. >
> Yes. My problem was that instead of fixing the DDX to behave properly,
> reusing the same orientation as already configured, we were using iio-
> sensor-proxy to trigger the initial rotation. This doesn't work if
> there's no accelerometer, or orientation is locked, which is counter-
> intuitive.

Right, so the iio-sensor-proxy approach was a quick fix attemp and
I agree with you that with e.g. orientation locking breaking this
it is not a good fix.

Also note that with wayland we have no DDX and relying on reading
the current primary plane rotation and assuming that that is the
hw lcd panel orientation seems fragile in general, what if the
xserver crashes while the display is rotated ? Then the next instance
will assume that this is hardware panel orientation.

>> Case 2 can't work without some mechanism to query the orientation
>> of the display from the firmware/etc.
> 
> Yes. I'm not sure where we'd be exporting this quirk though, as we need
> it available early enough so that it can be used by boot splashes. DMI
> matches in the graphics driver?

Looking at what gnome currently does on both wayland and X11 AFAICT, I
would like to solve both cases in the same manner in both cases we
need the compositor to be aware that it needs to apply some rotation.

Also taking into account thing like the monitor configuration panel
I think we need to fix this at the compositor level by having some
sort of "lcd-panel-hardware-rotation" property which gets added to
the rotation selected by the user at the compositor level.

As for where to store this for case 1 we have the info in the kernel
from the firmware (once these patches land) so we just need to export
it somehow.

For case 2 we do need a dmi quirk in the kernel to set fbcon.rotate
properly, and once we have it there we can export it in what is
hopefully the same manner.

Note that the GPD-win (which is an example of case 2) seems to be
a rare example and case 1 is the more common case, so lets focus
on fixing that first. It seems that these 2 patches get us quite
far with fixing this, we just need to add some property (sysfs file?)
to the drm_connector where the initial rotation as inherited from
the firmware can be read by the compositor. Once we have that in
place we can add a dmi quirk to set that property for e.g. the
GPD-win and somehow propagate it to fbcon.rotate (case 2 needs to
be done with software rotation in fbcon because the hw cannot handle
all rotation cases).

Regards,

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

Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Bastien Nocera]

On Sun, 2017-04-30 at 21:22 +0200, Hans de Goede wrote:
> Hi,
> 
> On 26-04-17 14:13, Bastien Nocera wrote:
> > On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote:
> > > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > 
> > > If a connector added through drm_fb_helper_add_one_connector()
> > > has
> > > a crtc attached and that crtc has a rotation configured make the
> > > fbdev inherit the crtc's rotation.
> > > 
> > > This is useful on e.g. some tablets which have their lcd panel
> > > mounted
> > > upside down, which before this commit would result in the kernel
> > > boot
> > > messages switching from being shown the right way up in efifb to
> > > being
> > > shown upside down as soon as a native kms driver loads.
> > > 
> > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's
> > >   "drm/fb-helper: Inherit rotation wip" patch]
> > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c | 51
> > > ++++++++++++++++++++++++++++++++++++++---
> > >   include/drm/drm_fb_helper.h     |  2 ++
> > >   2 files changed, 50 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > > b/drivers/gpu/drm/drm_fb_helper.c
> > > index 324a688..c97e00ab 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct
> > > drm_fb_helper *fb_helper, struct drm_
> > >   {
> > >   	struct drm_fb_helper_connector **temp;
> > >   	struct drm_fb_helper_connector *fb_helper_connector;
> > > +	struct drm_crtc *crtc = connector->encoder ?
> > > +				connector->encoder->crtc : NULL;
> > >   
> > >   	if (!drm_fbdev_emulation)
> > >   		return 0;
> > > @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct
> > > drm_fb_helper *fb_helper, struct drm_
> > >   
> > >   	drm_connector_reference(connector);
> > >   	fb_helper_connector->connector = connector;
> > > +	if (crtc && crtc->primary->state)
> > > +		fb_helper_connector->rotation = crtc->primary-
> > > > state->rotation;
> > > 
> > > +	if (!fb_helper_connector->rotation)
> > > +		fb_helper_connector->rotation = DRM_ROTATE_0;
> > 
> > That's equivalent to:
> > if (fb_helper_connector->rotation = DRM_ROTATE_0)
> >     fb_helper_connector->rotation = DRM_ROTATE_0;
> 
> No it is not equivalent:
> 
> include/drm/drm_blend.h
> 40:#define DRM_ROTATE_0 BIT(0)
> 
> and BIT(0) is (1 << 0)

Urgh. I'd have a "DRM_ROTATE_UNSET = 0" added either to the public
header, or to this file and compare to it instead of "!rotation".
Because this really wasn't clear or obvious.

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

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Bastien Nocera]

On Sun, 2017-04-30 at 21:34 +0200, Hans de Goede wrote:
> Hi,
> 
> On 27-04-17 18:39, Bastien Nocera wrote:
> > On Thu, 2017-04-27 at 19:24 +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 26, 2017 at 02:28:32PM +0200, Bastien Nocera wrote:
> > > > On Mon, 2017-04-24 at 15:48 +0300, Ville Syrjälä wrote:
> > > > > 
> > > > 
> > > > <snip>
> > > > 
> > > > > > I've a patch for iio-sensor-proxy which fixes the rotation
> > > > > > under
> > > > > > Xorg /
> > > > > > Wayland when using a desktop environment which honors iio-
> > > > > > sensor-
> > > > > > proxy's
> > > > > > rotation detection:
> > > > > > https://github.com/hadess/iio-sensor-proxy/pull/162
> > > > > 
> > > > > Or is it just this thing that clobbers what the DDX inherited
> > > > > from
> > > > > the
> > > > > kernel as the initial rotation?
> > > > 
> > > > I think it's mostly got to do with the compositor (or X) not
> > > > knowing
> > > > what "normal" or "0 degrees rotation" corresponds to.
> > > 
> > > Well, there are really two cases to consider:
> > > 
> > > 1. BIOS/whatever configures display hardware rotation in a way
> > >     that matches the orientation of the physical display
> > > 2. BIOS didn't do that. Either the hardware can't do what
> > >     would be required, or the BIOS just chose not to do it.
> > > 
> > > Case 1 should work with these patches as long as the DDX will set
> > > up
> > > the
> > > initial randr rotation to match what it read out from the kms
> > > rotation
> > > property of the primary plane. >
> > 
> > Yes. My problem was that instead of fixing the DDX to behave
> > properly,
> > reusing the same orientation as already configured, we were using
> > iio-
> > sensor-proxy to trigger the initial rotation. This doesn't work if
> > there's no accelerometer, or orientation is locked, which is
> > counter-
> > intuitive.
> 
> Right, so the iio-sensor-proxy approach was a quick fix attemp and
> I agree with you that with e.g. orientation locking breaking this
> it is not a good fix.
> 
> Also note that with wayland we have no DDX and relying on reading
> the current primary plane rotation and assuming that that is the
> hw lcd panel orientation seems fragile in general, what if the
> xserver crashes while the display is rotated ? Then the next instance
> will assume that this is hardware panel orientation.
> 
> > > Case 2 can't work without some mechanism to query the orientation
> > > of the display from the firmware/etc.
> > 
> > Yes. I'm not sure where we'd be exporting this quirk though, as we
> > need
> > it available early enough so that it can be used by boot splashes.
> > DMI
> > matches in the graphics driver?
> 
> Looking at what gnome currently does on both wayland and X11 AFAICT,
> I
> would like to solve both cases in the same manner in both cases we
> need the compositor to be aware that it needs to apply some rotation.
> 
> Also taking into account thing like the monitor configuration panel
> I think we need to fix this at the compositor level by having some
> sort of "lcd-panel-hardware-rotation" property which gets added to
> the rotation selected by the user at the compositor level.
> 
> As for where to store this for case 1 we have the info in the kernel
> from the firmware (once these patches land) so we just need to export
> it somehow.
> 
> For case 2 we do need a dmi quirk in the kernel to set fbcon.rotate
> properly, and once we have it there we can export it in what is
> hopefully the same manner.
> 
> Note that the GPD-win (which is an example of case 2) seems to be
> a rare example and case 1 is the more common case, so lets focus
> on fixing that first. It seems that these 2 patches get us quite
> far with fixing this, we just need to add some property (sysfs file?)
> to the drm_connector where the initial rotation as inherited from
> the firmware can be read by the compositor. Once we have that in
> place we can add a dmi quirk to set that property for e.g. the
> GPD-win and somehow propagate it to fbcon.rotate (case 2 needs to
> be done with software rotation in fbcon because the hw cannot handle
> all rotation cases).

Sounds like a good plan!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Make fbdev inherit the crtc's initial rotation

[Hans de Goede]

Hi,

On 30-04-17 21:34, Hans de Goede wrote:
> Hi,
> 
> On 27-04-17 18:39, Bastien Nocera wrote:
>> On Thu, 2017-04-27 at 19:24 +0300, Ville Syrjälä wrote:

<snip>

>>> Well, there are really two cases to consider:
>>>
>>> 1. BIOS/whatever configures display hardware rotation in a way
>>>     that matches the orientation of the physical display
>>> 2. BIOS didn't do that. Either the hardware can't do what
>>>     would be required, or the BIOS just chose not to do it.
>>>
>>> Case 1 should work with these patches as long as the DDX will set up
>>> the
>>> initial randr rotation to match what it read out from the kms
>>> rotation
>>> property of the primary plane. >

So I've been running some tests with the 2 original patches focussing
only on the normal screen is upside down, BIOS has configured hardware
rotation case which is what all devices except on special device
seem to have.

As expected with these 2 patches fbcon and the plymouth splashscreen
work fine. If I then start:

a) Xorg with modesetting driver: I get the screen the right way
up, but the cursor plane is upside down both in coordinates (where
the cursor gets shown) as well as the cursor glyph being upside down

b) Xorg with intel driver, the intel driver re-sets the primary plane's
drm "rotation" property, so everthing is upside down. At least the
primary and cursor planes are consistent with each other now
(the touchscreen coordinates how ever are not).

c) Gnome as Wayland compositor, same as Xorg with intel driver

So think more about this I think that Ville's 2 original cases
really are:

1. All planes support the necessary rotation in hardware, so we
can fix things up fully in hardware, aka the upside down case
which I believe is the majority of all troublesome hardware
out there.

2. The necessary rotation is not supported in hardware. This
is the case with the GPD-win where we've a 90 / 270 degrees
rotation. Here even the EFI configuration menu, grub, efifb,
etc. all are rotated.

My conclusion of all this is that:

1. For 1. we can and should fix this entirely and transparently
in the kernel, we need kernel fixes for fbcon/fbdev anyways, so
we might just as well go all the way, otherwise all of Xorg + modesetting,
Xorg + intel and Gnome as Wayland compositor will need separate
fixes. So this is best dealt with in the kernel, esp. since there
are more Wayland compositors out there.

2. There is nothing the kernel can do here, so this one we really
need to fix in userspace. I've some ideas for this, but those
fall outside the scope of this discussion.

I've already implemented the all kernel fix for 1. It is a single
patch only touching the i915 driver, since if we transparently deal
with the extra rotation in the i915 code we no longer need the
drm_fb_helper changes as things will just work for the fbcon too,
which is really nice to see and to me shows this is the right way
to solve this. The patch adds just 80 lines of code and completely
fixes this issue :)

I'll send out the patch right after this mail.

Regards,

Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel