[PATCH v3 0/7] drm/fbdev: Panel orientation connector property support

[PATCH v3 0/7] drm/fbdev: Panel orientation connector property support

[Hans de Goede]

Hi All,

Here is v3 of my series to add a "panel orientation" property to
the drm-connector for the LCD panel to let userspace know about LCD
panels which are not mounted upright, as well as detecting upside-down
panels without needing quirks (like we do for 90 degree rotated screens).

As requested by Daniel this version moves the quirks over from the fbdev
subsys to the drm subsys. I've done this by simpy starting with a copy of
the quirk table and eventually removing the fbdev version.

The 1st patch in this series is a small fbdev/fbcon patch, patches 2-5
are all drm patches since patches 2-5 depend on patch 1 I believe it
would be best to merge patches 1-5 through the drm tree.

For merging patches 6-7 I see 3 options:

1) Wait a kernel cycle, things will work fine without them, they are really
just there to remove the fbdev copy of the quirks

2) Merge all 7 patches through the drm tree

3) Use a stable tag in the drm tree which the fbdev tree can merge and then
merge patches 6-7 through the drm tree.

Regards,

Hans

[PATCH v3 1/7] fbcon: Add fbcon_rotate_hint to struct fb_info

[Hans de Goede]

On some hardware the LCD panel is not mounted upright in the casing,
but upside-down or rotated 90 degrees. In this case we want the console
to automatically be rotated to compensate.

The fbdev-driver may know about the need to rotate. Add a new
fbcon_rotate_hint field to struct fb_info, which gets initialized to -1.
If the fbdev-driver knows that some sort of rotation is necessary then
it can set this field to a FB_ROTATE_* value to tell the fbcon console
driver to rotate the console.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/core/fbcon.c   | 18 ++++++++++++------
 drivers/video/fbdev/core/fbsysfs.c |  1 +
 include/linux/fb.h                 |  5 +++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 04612f938bab..fb317ed76b45 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -963,10 +963,13 @@ static const char *fbcon_startup(void)
 	ops->cur_rotate = -1;
 	ops->cur_blink_jiffies = HZ / 5;
 	info->fbcon_par = ops;
-	if (initial_rotation != -1)
-		p->con_rotate = initial_rotation;
-	else
+
+	p->con_rotate = initial_rotation;
+	if (p->con_rotate = -1)
+		p->con_rotate = info->fbcon_rotate_hint;
+	if (p->con_rotate = -1)
 		p->con_rotate = fbcon_platform_get_rotate(info);
+
 	set_blitting_type(vc, info);
 
 	if (info->fix.type != FB_TYPE_TEXT) {
@@ -1103,10 +1106,13 @@ static void fbcon_init(struct vc_data *vc, int init)
 
 	ops = info->fbcon_par;
 	ops->cur_blink_jiffies = msecs_to_jiffies(vc->vc_cur_blink_ms);
-	if (initial_rotation != -1)
-		p->con_rotate = initial_rotation;
-	else
+
+	p->con_rotate = initial_rotation;
+	if (p->con_rotate = -1)
+		p->con_rotate = info->fbcon_rotate_hint;
+	if (p->con_rotate = -1)
 		p->con_rotate = fbcon_platform_get_rotate(info);
+
 	set_blitting_type(vc, info);
 
 	cols = vc->vc_cols;
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index 15755ce1d26c..e31a182b42bf 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -58,6 +58,7 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
 		info->par = p + fb_info_size;
 
 	info->device = dev;
+	info->fbcon_rotate_hint = -1;
 
 #ifdef CONFIG_FB_BACKLIGHT
 	mutex_init(&info->bl_curve_mutex);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f4386b0ccf40..10cf71cc5d13 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -464,6 +464,11 @@ struct fb_info {
 	atomic_t count;
 	int node;
 	int flags;
+	/*
+	 * -1 by default, set to a FB_ROTATE_* value by the driver, if it knows
+	 * a lcd is not mounted upright and fbcon should rotate to compensate.
+	 */
+	int fbcon_rotate_hint;
 	struct mutex lock;		/* Lock for open/release/ioctl funcs */
 	struct mutex mm_lock;		/* Lock for fb_mmap and smem_* fields */
 	struct fb_var_screeninfo var;	/* Current var */
-- 
2.14.2

Re: [PATCH v3 1/7] fbcon: Add fbcon_rotate_hint to struct fb_info

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

Hi Hans,

On Mon, Oct 23, 2017 at 09:14:19AM +0200, Hans de Goede wrote:
> On some hardware the LCD panel is not mounted upright in the casing,
> but upside-down or rotated 90 degrees. In this case we want the console
> to automatically be rotated to compensate.
> 
> The fbdev-driver may know about the need to rotate. Add a new
> fbcon_rotate_hint field to struct fb_info, which gets initialized to -1.
> If the fbdev-driver knows that some sort of rotation is necessary then
> it can set this field to a FB_ROTATE_* value to tell the fbcon console
> driver to rotate the console.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Thanks for your work. I will give it a try with Droid 4 and N950
once I find some time :)

[...]

> +	p->con_rotate = initial_rotation;
> +	if (p->con_rotate == -1)
> +		p->con_rotate = info->fbcon_rotate_hint;
> +	if (p->con_rotate == -1)
>  		p->con_rotate = fbcon_platform_get_rotate(info);

[...]

> +	p->con_rotate = initial_rotation;
> +	if (p->con_rotate == -1)
> +		p->con_rotate = info->fbcon_rotate_hint;
> +	if (p->con_rotate == -1)
>  		p->con_rotate = fbcon_platform_get_rotate(info);
> +

maybe add a little helper function to reduce code duplication?

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/7] fbcon: Add fbcon_rotate_hint to struct fb_info

[Hans de Goede]

Hi,

On 23-10-17 14:43, Sebastian Reichel wrote:
> Hi Hans,
> 
> On Mon, Oct 23, 2017 at 09:14:19AM +0200, Hans de Goede wrote:
>> On some hardware the LCD panel is not mounted upright in the casing,
>> but upside-down or rotated 90 degrees. In this case we want the console
>> to automatically be rotated to compensate.
>>
>> The fbdev-driver may know about the need to rotate. Add a new
>> fbcon_rotate_hint field to struct fb_info, which gets initialized to -1.
>> If the fbdev-driver knows that some sort of rotation is necessary then
>> it can set this field to a FB_ROTATE_* value to tell the fbcon console
>> driver to rotate the console.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> 
> Thanks for your work. I will give it a try with Droid 4 and N950
> once I find some time :)

Ah, I did not even realize that this work would be useful for those
too, but yes that makes sense.


> [...]
> 
>> +	p->con_rotate = initial_rotation;
>> +	if (p->con_rotate = -1)
>> +		p->con_rotate = info->fbcon_rotate_hint;
>> +	if (p->con_rotate = -1)
>>   		p->con_rotate = fbcon_platform_get_rotate(info);
> 
> [...]
> 
>> +	p->con_rotate = initial_rotation;
>> +	if (p->con_rotate = -1)
>> +		p->con_rotate = info->fbcon_rotate_hint;
>> +	if (p->con_rotate = -1)
>>   		p->con_rotate = fbcon_platform_get_rotate(info);
>> +
> 
> maybe add a little helper function to reduce code duplication?

Maybe, I took a look and there already is a fbcon_set_rotation()
helper which does something completely different, so it might
be best to just keep this as is to avoid confusion between
2 similar named functions.

Regards,

Hans

[PATCH v3 2/7] drm: Add panel orientation quirks

[Hans de Goede]

Some x86 clamshell design devices use portrait tablet screens and a display
engine which cannot rotate in hardware, so the firmware just leaves things
as is and we cannot figure out that the display is oriented non upright
from the hardware.

So at least on x86, we need a quirk table for this. This commit adds a DMI
based quirk table which is initially populated with 5 such devices: Asus
T100HA, GPD Pocket, GPD win, I.T.Works TW891 and the VIOS LTH17.

This quirk table will be used by the drm code to let userspace know that
the display is not mounted upright inside the device's case through a new
panel orientation drm-connector property, as well as to tell fbcon to
rotate the console so that it shows the right way up.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/Kconfig                        |   3 +
 drivers/gpu/drm/Makefile                       |   1 +
 drivers/gpu/drm/drm_panel_orientation_quirks.c | 157 +++++++++++++++++++++++++
 include/drm/drm_utils.h                        |  18 +++
 4 files changed, 179 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_panel_orientation_quirks.c
 create mode 100644 include/drm/drm_utils.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 4d9f21831741..9d005ac98c2b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -26,6 +26,9 @@ config DRM_MIPI_DSI
 	bool
 	depends on DRM
 
+config DRM_PANEL_ORIENTATION_QUIRKS
+	tristate
+
 config DRM_DP_AUX_CHARDEV
 	bool "DRM DP AUX Interface"
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index a3fdc5a68dff..ffb621819bbf 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
 
 obj-$(CONFIG_DRM)	+= drm.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
+obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
 obj-$(CONFIG_DRM_ARM)	+= arm/
 obj-$(CONFIG_DRM_TTM)	+= ttm/
 obj-$(CONFIG_DRM_TDFX)	+= tdfx/
diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c b/drivers/gpu/drm/drm_panel_orientation_quirks.c
new file mode 100644
index 000000000000..cea4d71f4978
--- /dev/null
+++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
@@ -0,0 +1,157 @@
+/*
+ *  drm_panel_orientation_quirks.c -- Quirks for non-normal panel orientation
+ *
+ *	Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License.  See the file COPYING in the main directory of this archive for
+ *  more details.
+ */
+
+#include <linux/dmi.h>
+#include <drm/drm_connector.h>
+
+#ifdef CONFIG_DMI
+
+/*
+ * Some x86 clamshell design devices use portrait tablet screens and a display
+ * engine which cannot rotate in hardware, so we need to rotate the fbcon to
+ * compensate. Unfortunately these (cheap) devices also typically have quite
+ * generic DMI data, so we match on a combination of DMI data, screen resolution
+ * and a list of known BIOS dates to avoid false positives.
+ */
+
+struct drm_dmi_panel_orientation_data {
+	int width;
+	int height;
+	const char * const *bios_dates;
+	int orientation;
+};
+
+static const struct drm_dmi_panel_orientation_data asus_t100ha = {
+	.width = 800,
+	.height = 1280,
+	.orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
+};
+
+static const struct drm_dmi_panel_orientation_data gpd_pocket = {
+	.width = 1200,
+	.height = 1920,
+	.bios_dates = (const char * const []){ "05/26/2017", "06/28/2017",
+		"07/05/2017", "08/07/2017", NULL },
+	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
+};
+
+static const struct drm_dmi_panel_orientation_data gpd_win = {
+	.width = 720,
+	.height = 1280,
+	.bios_dates = (const char * const []){
+		"10/25/2016", "11/18/2016", "12/23/2016", "12/26/2016",
+		"02/21/2017", "03/20/2017", "05/25/2017", NULL },
+	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
+};
+
+static const struct drm_dmi_panel_orientation_data itworks_tw891 = {
+	.width = 800,
+	.height = 1280,
+	.bios_dates = (const char * const []){ "10/16/2015", NULL },
+	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
+};
+
+static const struct drm_dmi_panel_orientation_data vios_lth17 = {
+	.width = 800,
+	.height = 1280,
+	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
+};
+
+static const struct dmi_system_id orientation_data[] = {
+	{	/* Asus T100HA */
+		.matches = {
+		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100HAN"),
+		},
+		.driver_data = (void *)&asus_t100ha,
+	}, {	/*
+		 * GPD Pocket, note that the the DMI data is less generic then
+		 * it seems, devices with a board-vendor of "AMI Corporation"
+		 * are quite rare, as are devices which have both board- *and*
+		 * product-id set to "Default String"
+		 */
+		.matches = {
+		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
+		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
+		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
+		},
+		.driver_data = (void *)&gpd_pocket,
+	}, {	/* GPD Win (same note on DMI match as GPD Pocket) */
+		.matches = {
+		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
+		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
+		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
+		},
+		.driver_data = (void *)&gpd_win,
+	}, {	/* I.T.Works TW891 */
+		.matches = {
+		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "To be filled by O.E.M."),
+		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "TW891"),
+		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "To be filled by O.E.M."),
+		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "TW891"),
+		},
+		.driver_data = (void *)&itworks_tw891,
+	}, {	/* VIOS LTH17 */
+		.matches = {
+		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "VIOS"),
+		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "LTH17"),
+		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "VIOS"),
+		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "LTH17"),
+		},
+		.driver_data = (void *)&vios_lth17,
+	},
+	{}
+};
+
+int drm_get_panel_orientation_quirk(int width, int height)
+{
+	const struct dmi_system_id *match;
+	const struct drm_dmi_panel_orientation_data *data;
+	const char *bios_date;
+	int i;
+
+	for (match = dmi_first_match(orientation_data);
+	     match;
+	     match = dmi_first_match(match + 1)) {
+		data = match->driver_data;
+
+		if (data->width != width ||
+		    data->height != height)
+			continue;
+
+		if (!data->bios_dates)
+			return data->orientation;
+
+		bios_date = dmi_get_system_info(DMI_BIOS_DATE);
+		if (!bios_date)
+			continue;
+
+		for (i = 0; data->bios_dates[i]; i++) {
+			if (!strcmp(data->bios_dates[i], bios_date))
+				return data->orientation;
+		}
+	}
+
+	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+}
+EXPORT_SYMBOL(drm_get_panel_orientation_quirk);
+
+#else
+
+/* There are no quirks for non x86 devices yet */
+int drm_get_panel_orientation_quirk(int width, int height)
+{
+	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+}
+EXPORT_SYMBOL(drm_get_panel_orientation_quirk);
+
+#endif
diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
new file mode 100644
index 000000000000..cea362aeaffe
--- /dev/null
+++ b/include/drm/drm_utils.h
@@ -0,0 +1,18 @@
+/*
+ * Function prototypes for misc. drm utility functions.
+ * Specifically this file is for function prototypes for functions which
+ * may also be used outside of drm code (e.g. in fbdev drivers).
+ *
+ * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __DRM_UTILS_H__
+#define __DRM_UTILS_H__
+
+int drm_get_panel_orientation_quirk(int width, int height);
+
+#endif
-- 
2.14.2

Re: [Intel-gfx] [PATCH v3 2/7] drm: Add panel orientation quirks

[Daniel Vetter]

On Mon, Oct 23, 2017 at 09:14:20AM +0200, Hans de Goede wrote:
> Some x86 clamshell design devices use portrait tablet screens and a display
> engine which cannot rotate in hardware, so the firmware just leaves things
> as is and we cannot figure out that the display is oriented non upright
> from the hardware.
> 
> So at least on x86, we need a quirk table for this. This commit adds a DMI
> based quirk table which is initially populated with 5 such devices: Asus
> T100HA, GPD Pocket, GPD win, I.T.Works TW891 and the VIOS LTH17.
> 
> This quirk table will be used by the drm code to let userspace know that
> the display is not mounted upright inside the device's case through a new
> panel orientation drm-connector property, as well as to tell fbcon to
> rotate the console so that it shows the right way up.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Found a few organizational bikesheds below, but makes sense I think.

> ---
>  drivers/gpu/drm/Kconfig                        |   3 +
>  drivers/gpu/drm/Makefile                       |   1 +
>  drivers/gpu/drm/drm_panel_orientation_quirks.c | 157 +++++++++++++++++++++++++
>  include/drm/drm_utils.h                        |  18 +++
>  4 files changed, 179 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_panel_orientation_quirks.c
>  create mode 100644 include/drm/drm_utils.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 4d9f21831741..9d005ac98c2b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -26,6 +26,9 @@ config DRM_MIPI_DSI
>  	bool
>  	depends on DRM
>  
> +config DRM_PANEL_ORIENTATION_QUIRKS
> +	tristate

Why a config option? We don't make the edid quirks optional either ...

> +
>  config DRM_DP_AUX_CHARDEV
>  	bool "DRM DP AUX Interface"
>  	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index a3fdc5a68dff..ffb621819bbf 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
>  
>  obj-$(CONFIG_DRM)	+= drm.o
>  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> +obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o

Since it's a kms thing, probably should be added to the drm_kms_helper.ko
module. There's panles/bridges which also should be there but aren't, but
those aren't the best examples.

>  obj-$(CONFIG_DRM_ARM)	+= arm/
>  obj-$(CONFIG_DRM_TTM)	+= ttm/
>  obj-$(CONFIG_DRM_TDFX)	+= tdfx/
> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> new file mode 100644
> index 000000000000..cea4d71f4978
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
> @@ -0,0 +1,157 @@
> +/*
> + *  drm_panel_orientation_quirks.c -- Quirks for non-normal panel orientation
> + *
> + *	Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
> + *
> + *  This file is subject to the terms and conditions of the GNU General Public
> + *  License.  See the file COPYING in the main directory of this archive for
> + *  more details.
> + */
> +
> +#include <linux/dmi.h>
> +#include <drm/drm_connector.h>
> +
> +#ifdef CONFIG_DMI
> +
> +/*
> + * Some x86 clamshell design devices use portrait tablet screens and a display
> + * engine which cannot rotate in hardware, so we need to rotate the fbcon to
> + * compensate. Unfortunately these (cheap) devices also typically have quite
> + * generic DMI data, so we match on a combination of DMI data, screen resolution
> + * and a list of known BIOS dates to avoid false positives.
> + */
> +
> +struct drm_dmi_panel_orientation_data {
> +	int width;
> +	int height;
> +	const char * const *bios_dates;
> +	int orientation;
> +};
> +
> +static const struct drm_dmi_panel_orientation_data asus_t100ha = {
> +	.width = 800,
> +	.height = 1280,
> +	.orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
> +};
> +
> +static const struct drm_dmi_panel_orientation_data gpd_pocket = {
> +	.width = 1200,
> +	.height = 1920,
> +	.bios_dates = (const char * const []){ "05/26/2017", "06/28/2017",
> +		"07/05/2017", "08/07/2017", NULL },
> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> +};
> +
> +static const struct drm_dmi_panel_orientation_data gpd_win = {
> +	.width = 720,
> +	.height = 1280,
> +	.bios_dates = (const char * const []){
> +		"10/25/2016", "11/18/2016", "12/23/2016", "12/26/2016",
> +		"02/21/2017", "03/20/2017", "05/25/2017", NULL },
> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> +};
> +
> +static const struct drm_dmi_panel_orientation_data itworks_tw891 = {
> +	.width = 800,
> +	.height = 1280,
> +	.bios_dates = (const char * const []){ "10/16/2015", NULL },
> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> +};
> +
> +static const struct drm_dmi_panel_orientation_data vios_lth17 = {
> +	.width = 800,
> +	.height = 1280,
> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> +};
> +
> +static const struct dmi_system_id orientation_data[] = {
> +	{	/* Asus T100HA */
> +		.matches = {
> +		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100HAN"),
> +		},
> +		.driver_data = (void *)&asus_t100ha,
> +	}, {	/*
> +		 * GPD Pocket, note that the the DMI data is less generic then
> +		 * it seems, devices with a board-vendor of "AMI Corporation"
> +		 * are quite rare, as are devices which have both board- *and*
> +		 * product-id set to "Default String"
> +		 */
> +		.matches = {
> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
> +		},
> +		.driver_data = (void *)&gpd_pocket,
> +	}, {	/* GPD Win (same note on DMI match as GPD Pocket) */
> +		.matches = {
> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
> +		},
> +		.driver_data = (void *)&gpd_win,
> +	}, {	/* I.T.Works TW891 */
> +		.matches = {
> +		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "To be filled by O.E.M."),
> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "TW891"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "To be filled by O.E.M."),
> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "TW891"),
> +		},
> +		.driver_data = (void *)&itworks_tw891,
> +	}, {	/* VIOS LTH17 */
> +		.matches = {
> +		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "VIOS"),
> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "LTH17"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "VIOS"),
> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "LTH17"),
> +		},
> +		.driver_data = (void *)&vios_lth17,
> +	},
> +	{}
> +};
> +
> +int drm_get_panel_orientation_quirk(int width, int height)
> +{
> +	const struct dmi_system_id *match;
> +	const struct drm_dmi_panel_orientation_data *data;
> +	const char *bios_date;
> +	int i;
> +
> +	for (match = dmi_first_match(orientation_data);
> +	     match;
> +	     match = dmi_first_match(match + 1)) {
> +		data = match->driver_data;
> +
> +		if (data->width != width ||
> +		    data->height != height)
> +			continue;
> +
> +		if (!data->bios_dates)
> +			return data->orientation;
> +
> +		bios_date = dmi_get_system_info(DMI_BIOS_DATE);
> +		if (!bios_date)
> +			continue;
> +
> +		for (i = 0; data->bios_dates[i]; i++) {
> +			if (!strcmp(data->bios_dates[i], bios_date))
> +				return data->orientation;
> +		}
> +	}
> +
> +	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +}
> +EXPORT_SYMBOL(drm_get_panel_orientation_quirk);

Can't we integrate this into drm_add_display_info so that it just gets
auto-added wherever we need it? Maybe there's going to be OF and EDID ways
os specifying this in the future ...
-Daniel

> +
> +#else
> +
> +/* There are no quirks for non x86 devices yet */
> +int drm_get_panel_orientation_quirk(int width, int height)
> +{
> +	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +}
> +EXPORT_SYMBOL(drm_get_panel_orientation_quirk);
> +
> +#endif
> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> new file mode 100644
> index 000000000000..cea362aeaffe
> --- /dev/null
> +++ b/include/drm/drm_utils.h
> @@ -0,0 +1,18 @@
> +/*
> + * Function prototypes for misc. drm utility functions.
> + * Specifically this file is for function prototypes for functions which
> + * may also be used outside of drm code (e.g. in fbdev drivers).
> + *
> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __DRM_UTILS_H__
> +#define __DRM_UTILS_H__
> +
> +int drm_get_panel_orientation_quirk(int width, int height);
> +
> +#endif
> -- 
> 2.14.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v3 2/7] drm: Add panel orientation quirks

[Hans de Goede]

Hi,

On 30-10-17 10:39, Daniel Vetter wrote:
> On Mon, Oct 23, 2017 at 09:14:20AM +0200, Hans de Goede wrote:
>> Some x86 clamshell design devices use portrait tablet screens and a display
>> engine which cannot rotate in hardware, so the firmware just leaves things
>> as is and we cannot figure out that the display is oriented non upright
>> from the hardware.
>>
>> So at least on x86, we need a quirk table for this. This commit adds a DMI
>> based quirk table which is initially populated with 5 such devices: Asus
>> T100HA, GPD Pocket, GPD win, I.T.Works TW891 and the VIOS LTH17.
>>
>> This quirk table will be used by the drm code to let userspace know that
>> the display is not mounted upright inside the device's case through a new
>> panel orientation drm-connector property, as well as to tell fbcon to
>> rotate the console so that it shows the right way up.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Found a few organizational bikesheds below, but makes sense I think.
> 
>> ---
>>   drivers/gpu/drm/Kconfig                        |   3 +
>>   drivers/gpu/drm/Makefile                       |   1 +
>>   drivers/gpu/drm/drm_panel_orientation_quirks.c | 157 +++++++++++++++++++++++++
>>   include/drm/drm_utils.h                        |  18 +++
>>   4 files changed, 179 insertions(+)
>>   create mode 100644 drivers/gpu/drm/drm_panel_orientation_quirks.c
>>   create mode 100644 include/drm/drm_utils.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 4d9f21831741..9d005ac98c2b 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -26,6 +26,9 @@ config DRM_MIPI_DSI
>>   	bool
>>   	depends on DRM
>>   
>> +config DRM_PANEL_ORIENTATION_QUIRKS
>> +	tristate
> 
> Why a config option? We don't make the edid quirks optional either ...

You suggested this yourself while discussing v2, since at the end of the
series this is going to be required by both fbcon and the drm series, so
we may want to build it even if the drm subsys is otherwise not used.

>> +
>>   config DRM_DP_AUX_CHARDEV
>>   	bool "DRM DP AUX Interface"
>>   	depends on DRM
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index a3fdc5a68dff..ffb621819bbf 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/
>>   
>>   obj-$(CONFIG_DRM)	+= drm.o
>>   obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
>> +obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
> 
> Since it's a kms thing, probably should be added to the drm_kms_helper.ko
> module. There's panles/bridges which also should be there but aren't, but
> those aren't the best examples.

See my reply above why this cannot be done.

>>   obj-$(CONFIG_DRM_ARM)	+= arm/
>>   obj-$(CONFIG_DRM_TTM)	+= ttm/
>>   obj-$(CONFIG_DRM_TDFX)	+= tdfx/
>> diff --git a/drivers/gpu/drm/drm_panel_orientation_quirks.c b/drivers/gpu/drm/drm_panel_orientation_quirks.c
>> new file mode 100644
>> index 000000000000..cea4d71f4978
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_panel_orientation_quirks.c
>> @@ -0,0 +1,157 @@
>> +/*
>> + *  drm_panel_orientation_quirks.c -- Quirks for non-normal panel orientation
>> + *
>> + *	Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + *  This file is subject to the terms and conditions of the GNU General Public
>> + *  License.  See the file COPYING in the main directory of this archive for
>> + *  more details.
>> + */
>> +
>> +#include <linux/dmi.h>
>> +#include <drm/drm_connector.h>
>> +
>> +#ifdef CONFIG_DMI
>> +
>> +/*
>> + * Some x86 clamshell design devices use portrait tablet screens and a display
>> + * engine which cannot rotate in hardware, so we need to rotate the fbcon to
>> + * compensate. Unfortunately these (cheap) devices also typically have quite
>> + * generic DMI data, so we match on a combination of DMI data, screen resolution
>> + * and a list of known BIOS dates to avoid false positives.
>> + */
>> +
>> +struct drm_dmi_panel_orientation_data {
>> +	int width;
>> +	int height;
>> +	const char * const *bios_dates;
>> +	int orientation;
>> +};
>> +
>> +static const struct drm_dmi_panel_orientation_data asus_t100ha = {
>> +	.width = 800,
>> +	.height = 1280,
>> +	.orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP,
>> +};
>> +
>> +static const struct drm_dmi_panel_orientation_data gpd_pocket = {
>> +	.width = 1200,
>> +	.height = 1920,
>> +	.bios_dates = (const char * const []){ "05/26/2017", "06/28/2017",
>> +		"07/05/2017", "08/07/2017", NULL },
>> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>> +};
>> +
>> +static const struct drm_dmi_panel_orientation_data gpd_win = {
>> +	.width = 720,
>> +	.height = 1280,
>> +	.bios_dates = (const char * const []){
>> +		"10/25/2016", "11/18/2016", "12/23/2016", "12/26/2016",
>> +		"02/21/2017", "03/20/2017", "05/25/2017", NULL },
>> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>> +};
>> +
>> +static const struct drm_dmi_panel_orientation_data itworks_tw891 = {
>> +	.width = 800,
>> +	.height = 1280,
>> +	.bios_dates = (const char * const []){ "10/16/2015", NULL },
>> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>> +};
>> +
>> +static const struct drm_dmi_panel_orientation_data vios_lth17 = {
>> +	.width = 800,
>> +	.height = 1280,
>> +	.orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
>> +};
>> +
>> +static const struct dmi_system_id orientation_data[] = {
>> +	{	/* Asus T100HA */
>> +		.matches = {
>> +		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100HAN"),
>> +		},
>> +		.driver_data = (void *)&asus_t100ha,
>> +	}, {	/*
>> +		 * GPD Pocket, note that the the DMI data is less generic then
>> +		 * it seems, devices with a board-vendor of "AMI Corporation"
>> +		 * are quite rare, as are devices which have both board- *and*
>> +		 * product-id set to "Default String"
>> +		 */
>> +		.matches = {
>> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
>> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
>> +		},
>> +		.driver_data = (void *)&gpd_pocket,
>> +	}, {	/* GPD Win (same note on DMI match as GPD Pocket) */
>> +		.matches = {
>> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
>> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
>> +		},
>> +		.driver_data = (void *)&gpd_win,
>> +	}, {	/* I.T.Works TW891 */
>> +		.matches = {
>> +		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "To be filled by O.E.M."),
>> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "TW891"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "To be filled by O.E.M."),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "TW891"),
>> +		},
>> +		.driver_data = (void *)&itworks_tw891,
>> +	}, {	/* VIOS LTH17 */
>> +		.matches = {
>> +		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "VIOS"),
>> +		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "LTH17"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "VIOS"),
>> +		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "LTH17"),
>> +		},
>> +		.driver_data = (void *)&vios_lth17,
>> +	},
>> +	{}
>> +};
>> +
>> +int drm_get_panel_orientation_quirk(int width, int height)
>> +{
>> +	const struct dmi_system_id *match;
>> +	const struct drm_dmi_panel_orientation_data *data;
>> +	const char *bios_date;
>> +	int i;
>> +
>> +	for (match = dmi_first_match(orientation_data);
>> +	     match;
>> +	     match = dmi_first_match(match + 1)) {
>> +		data = match->driver_data;
>> +
>> +		if (data->width != width ||
>> +		    data->height != height)
>> +			continue;
>> +
>> +		if (!data->bios_dates)
>> +			return data->orientation;
>> +
>> +		bios_date = dmi_get_system_info(DMI_BIOS_DATE);
>> +		if (!bios_date)
>> +			continue;
>> +
>> +		for (i = 0; data->bios_dates[i]; i++) {
>> +			if (!strcmp(data->bios_dates[i], bios_date))
>> +				return data->orientation;
>> +		}
>> +	}
>> +
>> +	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
>> +}
>> +EXPORT_SYMBOL(drm_get_panel_orientation_quirk);
> 
> Can't we integrate this into drm_add_display_info so that it just gets
> auto-added wherever we need it? Maybe there's going to be OF and EDID ways
> os specifying this in the future ...

drm_add_display_info() only gets called from drm_add_edid_modes() and
dsi panels, which are actually the only panels I've so-far seen mounted
non-upright don't have edid info.

Note that the quirks are already automatically used in
drm_connector_init_panel_orientation_property() which the 3th patch:
"drm: Add support for a panel-orientation connector property"
adds, so drivers don't need to call the quirk code themselves.

Regards,

Hans


> -Daniel
> 
>> +
>> +#else
>> +
>> +/* There are no quirks for non x86 devices yet */
>> +int drm_get_panel_orientation_quirk(int width, int height)
>> +{
>> +	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
>> +}
>> +EXPORT_SYMBOL(drm_get_panel_orientation_quirk);
>> +
>> +#endif
>> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
>> new file mode 100644
>> index 000000000000..cea362aeaffe
>> --- /dev/null
>> +++ b/include/drm/drm_utils.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * Function prototypes for misc. drm utility functions.
>> + * Specifically this file is for function prototypes for functions which
>> + * may also be used outside of drm code (e.g. in fbdev drivers).
>> + *
>> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __DRM_UTILS_H__
>> +#define __DRM_UTILS_H__
>> +
>> +int drm_get_panel_orientation_quirk(int width, int height);
>> +
>> +#endif
>> -- 
>> 2.14.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[PATCH v3 3/7] drm: Add support for a panel-orientation connector property

[Hans de Goede]

On some devices the LCD panel is mounted in the casing in such a way that
the up/top side of the panel does not match with the top side of the
device (e.g. it is mounted upside-down).

This commit adds the necessary infra for lcd-panel drm_connector-s to
have a "panel orientation" property to communicate how the panel is
orientated vs the casing.

Userspace can use this property to check for non-normal orientation and
then adjust the displayed image accordingly by rotating it to compensate.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Rebased on 4.14-rc1
-Store panel_orientation in drm_display_info, so that drm_fb_helper.c can
 access it easily
-Have a single drm_connector_init_panel_orientation_property rather then
 create and attach functions. The caller is expected to set
 drm_display_info.panel_orientation before calling this, then this will
 check for platform specific quirks overriding the panel_orientation and if
 the panel_orientation is set after this then it will attach the property.
---
 drivers/gpu/drm/Kconfig         |  1 +
 drivers/gpu/drm/drm_connector.c | 73 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     | 11 +++++++
 include/drm/drm_mode_config.h   |  7 ++++
 include/uapi/drm/drm_mode.h     |  7 ++++
 5 files changed, 99 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 9d005ac98c2b..0b166e626eb6 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -7,6 +7,7 @@
 menuconfig DRM
 	tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
 	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
+	select DRM_PANEL_ORIENTATION_QUIRKS
 	select HDMI
 	select FB_CMDLINE
 	select I2C
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 704fc8934616..129c83a84320 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -24,6 +24,7 @@
 #include <drm/drm_connector.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_utils.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -212,6 +213,8 @@ int drm_connector_init(struct drm_device *dev,
 	mutex_init(&connector->mutex);
 	connector->edid_blob_ptr = NULL;
 	connector->status = connector_status_unknown;
+	connector->display_info.panel_orientation +		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
 
 	drm_connector_get_cmdline_mode(connector);
 
@@ -664,6 +667,13 @@ static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
 	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
 };
 
+static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
+	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	},
+	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside Down"	},
+	{ DRM_MODE_PANEL_ORIENTATION_LEFT_UP,	"Left Side Up"	},
+	{ DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,	"Right Side Up"	},
+};
+
 static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = {
 	{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
 	{ DRM_MODE_SUBCONNECTOR_DVID,      "DVI-D"     }, /* DVI-I  */
@@ -768,6 +778,18 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
  *
  * CRTC_ID:
  * 	Mode object ID of the &drm_crtc this connector should be connected to.
+ *
+ * Connectors for LCD panels may also have one standardized property:
+ *
+ * panel orientation:
+ *	On some devices the LCD panel is mounted in the casing in such a way
+ *	that the up/top side of the panel does not match with the top side of
+ *	the device. Userspace can use this property to check for this.
+ *	Note that input coordinates from touchscreens (input devices with
+ *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
+ *	coordinates, so if userspace rotates the picture to adjust for
+ *	the orientation it must also apply the same transformation to the
+ *	touchscreen input coordinates.
  */
 
 int drm_connector_create_standard_properties(struct drm_device *dev)
@@ -1234,6 +1256,57 @@ void drm_mode_connector_set_link_status_property(struct drm_connector *connector
 }
 EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
 
+/**
+ * drm_connector_init_panel_orientation_property -
+ *	initialize the connecters panel_orientation property
+ * @connector: connector for which to init the panel-orientation property.
+ * @width: width in pixels of the panel, used for panel quirk detection
+ * @height: height in pixels of the panel, used for panel quirk detection
+ *
+ * This function should only be called for built-in panels, after setting
+ * connector->display_info.panel_orientation first (if known).
+ *
+ * This function will check for platform specific (e.g. DMI based) quirks
+ * overriding display_info.panel_orientation first, then if panel_orientation
+ * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
+ * "panel orientation" property to the connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_init_panel_orientation_property(
+	struct drm_connector *connector, int width, int height)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_property *prop;
+	int orientation_quirk;
+
+	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
+	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+		info->panel_orientation = orientation_quirk;
+
+	if (info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+		return 0;
+
+	prop = dev->mode_config.panel_orientation_property;
+	if (!prop) {
+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
+				"panel orientation",
+				drm_panel_orientation_enum_list,
+				ARRAY_SIZE(drm_panel_orientation_enum_list));
+		if (!prop)
+			return -ENOMEM;
+
+		dev->mode_config.panel_orientation_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop,
+				   info->panel_orientation);
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
+
 int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 				    struct drm_property *property,
 				    uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index b4285c40e1e4..e6883065a461 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -222,6 +222,15 @@ struct drm_display_info {
 #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
 #define DRM_COLOR_FORMAT_YCRCB420	(1<<3)
 
+	/**
+	 * @panel_orientation: Read only connector property for built-in panels,
+	 * indicating the orientation of the panel vs the device's casing.
+	 * drm_connector_init() sets this to DRM_MODE_PANEL_ORIENTATION_UNKNOWN.
+	 * When not UNKNOWN this gets used by the drm_fb_helpers to rotate the
+	 * fb to compensate and gets exported as prop to userspace.
+	 */
+	int panel_orientation;
+
 	/**
 	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
 	 * modes. Used DRM_COLOR_FORMAT\_ defines, which are _not_ the same ones
@@ -1019,6 +1028,8 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 					    const struct edid *edid);
 void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
 						 uint64_t link_status);
+int drm_connector_init_panel_orientation_property(
+	struct drm_connector *connector, int width, int height);
 
 /**
  * struct drm_tile_group - Tile group metadata
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 0b4ac2ebc610..7d4ee1726e0a 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -728,6 +728,13 @@ struct drm_mode_config {
 	 */
 	struct drm_property *suggested_y_property;
 
+	/**
+	 * @panel_orientation_property: Optional connector property indicating
+	 * how the lcd-panel is mounted inside the casing (e.g. normal or
+	 * upside-down).
+	 */
+	struct drm_property *panel_orientation_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 34b6bb34b002..f60fae67bc1f 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -127,6 +127,13 @@ extern "C" {
 #define DRM_MODE_LINK_STATUS_GOOD	0
 #define DRM_MODE_LINK_STATUS_BAD	1
 
+/* Panel Orientation options */
+#define DRM_MODE_PANEL_ORIENTATION_UNKNOWN	-1
+#define DRM_MODE_PANEL_ORIENTATION_NORMAL	0
+#define DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP	1
+#define DRM_MODE_PANEL_ORIENTATION_LEFT_UP	2
+#define DRM_MODE_PANEL_ORIENTATION_RIGHT_UP	3
+
 /*
  * DRM_MODE_ROTATE_<degrees>
  *
-- 
2.14.2

Re: [Intel-gfx] [PATCH v3 3/7] drm: Add support for a panel-orientation connector property

[Daniel Vetter]

On Mon, Oct 23, 2017 at 09:14:21AM +0200, Hans de Goede wrote:
> On some devices the LCD panel is mounted in the casing in such a way that
> the up/top side of the panel does not match with the top side of the
> device (e.g. it is mounted upside-down).
> 
> This commit adds the necessary infra for lcd-panel drm_connector-s to
> have a "panel orientation" property to communicate how the panel is
> orientated vs the casing.
> 
> Userspace can use this property to check for non-normal orientation and
> then adjust the displayed image accordingly by rotating it to compensate.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Rebased on 4.14-rc1
> -Store panel_orientation in drm_display_info, so that drm_fb_helper.c can
>  access it easily
> -Have a single drm_connector_init_panel_orientation_property rather then
>  create and attach functions. The caller is expected to set
>  drm_display_info.panel_orientation before calling this, then this will
>  check for platform specific quirks overriding the panel_orientation and if
>  the panel_orientation is set after this then it will attach the property.
> ---
>  drivers/gpu/drm/Kconfig         |  1 +
>  drivers/gpu/drm/drm_connector.c | 73 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h     | 11 +++++++
>  include/drm/drm_mode_config.h   |  7 ++++
>  include/uapi/drm/drm_mode.h     |  7 ++++
>  5 files changed, 99 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9d005ac98c2b..0b166e626eb6 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -7,6 +7,7 @@
>  menuconfig DRM
>  	tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
>  	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
> +	select DRM_PANEL_ORIENTATION_QUIRKS
>  	select HDMI
>  	select FB_CMDLINE
>  	select I2C
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 704fc8934616..129c83a84320 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -24,6 +24,7 @@
>  #include <drm/drm_connector.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_utils.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
> @@ -212,6 +213,8 @@ int drm_connector_init(struct drm_device *dev,
>  	mutex_init(&connector->mutex);
>  	connector->edid_blob_ptr = NULL;
>  	connector->status = connector_status_unknown;
> +	connector->display_info.panel_orientation > +		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
>  
>  	drm_connector_get_cmdline_mode(connector);
>  
> @@ -664,6 +667,13 @@ static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
>  	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
>  };
>  
> +static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
> +	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	},
> +	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside Down"	},
> +	{ DRM_MODE_PANEL_ORIENTATION_LEFT_UP,	"Left Side Up"	},
> +	{ DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,	"Right Side Up"	},
> +};
> +
>  static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = {
>  	{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
>  	{ DRM_MODE_SUBCONNECTOR_DVID,      "DVI-D"     }, /* DVI-I  */
> @@ -768,6 +778,18 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   *
>   * CRTC_ID:
>   * 	Mode object ID of the &drm_crtc this connector should be connected to.
> + *
> + * Connectors for LCD panels may also have one standardized property:
> + *
> + * panel orientation:
> + *	On some devices the LCD panel is mounted in the casing in such a way
> + *	that the up/top side of the panel does not match with the top side of
> + *	the device. Userspace can use this property to check for this.
> + *	Note that input coordinates from touchscreens (input devices with
> + *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
> + *	coordinates, so if userspace rotates the picture to adjust for
> + *	the orientation it must also apply the same transformation to the
> + *	touchscreen input coordinates.
>   */
>  
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1234,6 +1256,57 @@ void drm_mode_connector_set_link_status_property(struct drm_connector *connector
>  }
>  EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
>  
> +/**
> + * drm_connector_init_panel_orientation_property -
> + *	initialize the connecters panel_orientation property
> + * @connector: connector for which to init the panel-orientation property.
> + * @width: width in pixels of the panel, used for panel quirk detection
> + * @height: height in pixels of the panel, used for panel quirk detection
> + *
> + * This function should only be called for built-in panels, after setting
> + * connector->display_info.panel_orientation first (if known).
> + *
> + * This function will check for platform specific (e.g. DMI based) quirks
> + * overriding display_info.panel_orientation first, then if panel_orientation
> + * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
> + * "panel orientation" property to the connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */

Hm, I think our more usual way is to set the prop up first, and then the
parsing mode updates the property (in case it's not quite as stable as we
thought). Not the property init function calling the parsing code.

I know that the panel rotation will probably not change, but I think it'd
be good to be consistent here. Or at least look into whether that makes
sense ...

Besides this bikeshed color question makes all sense.
-Daniel

> +int drm_connector_init_panel_orientation_property(
> +	struct drm_connector *connector, int width, int height)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_property *prop;
> +	int orientation_quirk;
> +
> +	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> +	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> +		info->panel_orientation = orientation_quirk;
> +
> +	if (info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> +		return 0;
> +
> +	prop = dev->mode_config.panel_orientation_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> +				"panel orientation",
> +				drm_panel_orientation_enum_list,
> +				ARRAY_SIZE(drm_panel_orientation_enum_list));
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		dev->mode_config.panel_orientation_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop,
> +				   info->panel_orientation);
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
> +
>  int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  				    struct drm_property *property,
>  				    uint64_t value)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b4285c40e1e4..e6883065a461 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -222,6 +222,15 @@ struct drm_display_info {
>  #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
>  #define DRM_COLOR_FORMAT_YCRCB420	(1<<3)
>  
> +	/**
> +	 * @panel_orientation: Read only connector property for built-in panels,
> +	 * indicating the orientation of the panel vs the device's casing.
> +	 * drm_connector_init() sets this to DRM_MODE_PANEL_ORIENTATION_UNKNOWN.
> +	 * When not UNKNOWN this gets used by the drm_fb_helpers to rotate the
> +	 * fb to compensate and gets exported as prop to userspace.
> +	 */
> +	int panel_orientation;
> +
>  	/**
>  	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
>  	 * modes. Used DRM_COLOR_FORMAT\_ defines, which are _not_ the same ones
> @@ -1019,6 +1028,8 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  					    const struct edid *edid);
>  void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
>  						 uint64_t link_status);
> +int drm_connector_init_panel_orientation_property(
> +	struct drm_connector *connector, int width, int height);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 0b4ac2ebc610..7d4ee1726e0a 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -728,6 +728,13 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *suggested_y_property;
>  
> +	/**
> +	 * @panel_orientation_property: Optional connector property indicating
> +	 * how the lcd-panel is mounted inside the casing (e.g. normal or
> +	 * upside-down).
> +	 */
> +	struct drm_property *panel_orientation_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 34b6bb34b002..f60fae67bc1f 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -127,6 +127,13 @@ extern "C" {
>  #define DRM_MODE_LINK_STATUS_GOOD	0
>  #define DRM_MODE_LINK_STATUS_BAD	1
>  
> +/* Panel Orientation options */
> +#define DRM_MODE_PANEL_ORIENTATION_UNKNOWN	-1
> +#define DRM_MODE_PANEL_ORIENTATION_NORMAL	0
> +#define DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP	1
> +#define DRM_MODE_PANEL_ORIENTATION_LEFT_UP	2
> +#define DRM_MODE_PANEL_ORIENTATION_RIGHT_UP	3
> +
>  /*
>   * DRM_MODE_ROTATE_<degrees>
>   *
> -- 
> 2.14.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v3 3/7] drm: Add support for a panel-orientation connector property

[Hans de Goede]

Hi,

On 30-10-17 10:43, Daniel Vetter wrote:
> On Mon, Oct 23, 2017 at 09:14:21AM +0200, Hans de Goede wrote:
>> On some devices the LCD panel is mounted in the casing in such a way that
>> the up/top side of the panel does not match with the top side of the
>> device (e.g. it is mounted upside-down).
>>
>> This commit adds the necessary infra for lcd-panel drm_connector-s to
>> have a "panel orientation" property to communicate how the panel is
>> orientated vs the casing.
>>
>> Userspace can use this property to check for non-normal orientation and
>> then adjust the displayed image accordingly by rotating it to compensate.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Rebased on 4.14-rc1
>> -Store panel_orientation in drm_display_info, so that drm_fb_helper.c can
>>   access it easily
>> -Have a single drm_connector_init_panel_orientation_property rather then
>>   create and attach functions. The caller is expected to set
>>   drm_display_info.panel_orientation before calling this, then this will
>>   check for platform specific quirks overriding the panel_orientation and if
>>   the panel_orientation is set after this then it will attach the property.
>> ---
>>   drivers/gpu/drm/Kconfig         |  1 +
>>   drivers/gpu/drm/drm_connector.c | 73 +++++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h     | 11 +++++++
>>   include/drm/drm_mode_config.h   |  7 ++++
>>   include/uapi/drm/drm_mode.h     |  7 ++++
>>   5 files changed, 99 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 9d005ac98c2b..0b166e626eb6 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -7,6 +7,7 @@
>>   menuconfig DRM
>>   	tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
>>   	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
>> +	select DRM_PANEL_ORIENTATION_QUIRKS
>>   	select HDMI
>>   	select FB_CMDLINE
>>   	select I2C
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 704fc8934616..129c83a84320 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -24,6 +24,7 @@
>>   #include <drm/drm_connector.h>
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_encoder.h>
>> +#include <drm/drm_utils.h>
>>   
>>   #include "drm_crtc_internal.h"
>>   #include "drm_internal.h"
>> @@ -212,6 +213,8 @@ int drm_connector_init(struct drm_device *dev,
>>   	mutex_init(&connector->mutex);
>>   	connector->edid_blob_ptr = NULL;
>>   	connector->status = connector_status_unknown;
>> +	connector->display_info.panel_orientation >> +		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
>>   
>>   	drm_connector_get_cmdline_mode(connector);
>>   
>> @@ -664,6 +667,13 @@ static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
>>   	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
>>   };
>>   
>> +static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
>> +	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	},
>> +	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside Down"	},
>> +	{ DRM_MODE_PANEL_ORIENTATION_LEFT_UP,	"Left Side Up"	},
>> +	{ DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,	"Right Side Up"	},
>> +};
>> +
>>   static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = {
>>   	{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
>>   	{ DRM_MODE_SUBCONNECTOR_DVID,      "DVI-D"     }, /* DVI-I  */
>> @@ -768,6 +778,18 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>    *
>>    * CRTC_ID:
>>    * 	Mode object ID of the &drm_crtc this connector should be connected to.
>> + *
>> + * Connectors for LCD panels may also have one standardized property:
>> + *
>> + * panel orientation:
>> + *	On some devices the LCD panel is mounted in the casing in such a way
>> + *	that the up/top side of the panel does not match with the top side of
>> + *	the device. Userspace can use this property to check for this.
>> + *	Note that input coordinates from touchscreens (input devices with
>> + *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
>> + *	coordinates, so if userspace rotates the picture to adjust for
>> + *	the orientation it must also apply the same transformation to the
>> + *	touchscreen input coordinates.
>>    */
>>   
>>   int drm_connector_create_standard_properties(struct drm_device *dev)
>> @@ -1234,6 +1256,57 @@ void drm_mode_connector_set_link_status_property(struct drm_connector *connector
>>   }
>>   EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
>>   
>> +/**
>> + * drm_connector_init_panel_orientation_property -
>> + *	initialize the connecters panel_orientation property
>> + * @connector: connector for which to init the panel-orientation property.
>> + * @width: width in pixels of the panel, used for panel quirk detection
>> + * @height: height in pixels of the panel, used for panel quirk detection
>> + *
>> + * This function should only be called for built-in panels, after setting
>> + * connector->display_info.panel_orientation first (if known).
>> + *
>> + * This function will check for platform specific (e.g. DMI based) quirks
>> + * overriding display_info.panel_orientation first, then if panel_orientation
>> + * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
>> + * "panel orientation" property to the connector.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
> 
> Hm, I think our more usual way is to set the prop up first, and then the
> parsing mode updates the property (in case it's not quite as stable as we
> thought). Not the property init function calling the parsing code.
> 
> I know that the panel rotation will probably not change, but I think it'd
> be good to be consistent here. Or at least look into whether that makes
> sense ...

I'm not calling any parsing code here, what I'm calling is the code
checking for quirks, so that that is done in one central place just like
how drm_add_edid_modes() calls edid_get_quirks().

I could create the property in drm_connector_create_standard_properties()
instead of in drm_connector_init_panel_orientation_property and
rename the latter to drm_connector_attach_panel_orientation_property
if you prefer having things split that way.

Auto attaching the property is tricky since we only want it on panels.

Regards,

Hans



> 
> Besides this bikeshed color question makes all sense.
> -Daniel
> 
>> +int drm_connector_init_panel_orientation_property(
>> +	struct drm_connector *connector, int width, int height)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_display_info *info = &connector->display_info;
>> +	struct drm_property *prop;
>> +	int orientation_quirk;
>> +
>> +	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
>> +	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>> +		info->panel_orientation = orientation_quirk;
>> +
>> +	if (info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
>> +		return 0;
>> +
>> +	prop = dev->mode_config.panel_orientation_property;
>> +	if (!prop) {
>> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
>> +				"panel orientation",
>> +				drm_panel_orientation_enum_list,
>> +				ARRAY_SIZE(drm_panel_orientation_enum_list));
>> +		if (!prop)
>> +			return -ENOMEM;
>> +
>> +		dev->mode_config.panel_orientation_property = prop;
>> +	}
>> +
>> +	drm_object_attach_property(&connector->base, prop,
>> +				   info->panel_orientation);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
>> +
>>   int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>>   				    struct drm_property *property,
>>   				    uint64_t value)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index b4285c40e1e4..e6883065a461 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -222,6 +222,15 @@ struct drm_display_info {
>>   #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
>>   #define DRM_COLOR_FORMAT_YCRCB420	(1<<3)
>>   
>> +	/**
>> +	 * @panel_orientation: Read only connector property for built-in panels,
>> +	 * indicating the orientation of the panel vs the device's casing.
>> +	 * drm_connector_init() sets this to DRM_MODE_PANEL_ORIENTATION_UNKNOWN.
>> +	 * When not UNKNOWN this gets used by the drm_fb_helpers to rotate the
>> +	 * fb to compensate and gets exported as prop to userspace.
>> +	 */
>> +	int panel_orientation;
>> +
>>   	/**
>>   	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
>>   	 * modes. Used DRM_COLOR_FORMAT\_ defines, which are _not_ the same ones
>> @@ -1019,6 +1028,8 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>>   					    const struct edid *edid);
>>   void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
>>   						 uint64_t link_status);
>> +int drm_connector_init_panel_orientation_property(
>> +	struct drm_connector *connector, int width, int height);
>>   
>>   /**
>>    * struct drm_tile_group - Tile group metadata
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 0b4ac2ebc610..7d4ee1726e0a 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -728,6 +728,13 @@ struct drm_mode_config {
>>   	 */
>>   	struct drm_property *suggested_y_property;
>>   
>> +	/**
>> +	 * @panel_orientation_property: Optional connector property indicating
>> +	 * how the lcd-panel is mounted inside the casing (e.g. normal or
>> +	 * upside-down).
>> +	 */
>> +	struct drm_property *panel_orientation_property;
>> +
>>   	/* dumb ioctl parameters */
>>   	uint32_t preferred_depth, prefer_shadow;
>>   
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 34b6bb34b002..f60fae67bc1f 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -127,6 +127,13 @@ extern "C" {
>>   #define DRM_MODE_LINK_STATUS_GOOD	0
>>   #define DRM_MODE_LINK_STATUS_BAD	1
>>   
>> +/* Panel Orientation options */
>> +#define DRM_MODE_PANEL_ORIENTATION_UNKNOWN	-1
>> +#define DRM_MODE_PANEL_ORIENTATION_NORMAL	0
>> +#define DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP	1
>> +#define DRM_MODE_PANEL_ORIENTATION_LEFT_UP	2
>> +#define DRM_MODE_PANEL_ORIENTATION_RIGHT_UP	3
>> +
>>   /*
>>    * DRM_MODE_ROTATE_<degrees>
>>    *
>> -- 
>> 2.14.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [Intel-gfx] [PATCH v3 3/7] drm: Add support for a panel-orientation connector property

[Daniel Vetter]

On Mon, Oct 30, 2017 at 11:57:10AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-10-17 10:43, Daniel Vetter wrote:
> > On Mon, Oct 23, 2017 at 09:14:21AM +0200, Hans de Goede wrote:
> > > On some devices the LCD panel is mounted in the casing in such a way that
> > > the up/top side of the panel does not match with the top side of the
> > > device (e.g. it is mounted upside-down).
> > > 
> > > This commit adds the necessary infra for lcd-panel drm_connector-s to
> > > have a "panel orientation" property to communicate how the panel is
> > > orientated vs the casing.
> > > 
> > > Userspace can use this property to check for non-normal orientation and
> > > then adjust the displayed image accordingly by rotating it to compensate.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Changes in v2:
> > > -Rebased on 4.14-rc1
> > > -Store panel_orientation in drm_display_info, so that drm_fb_helper.c can
> > >   access it easily
> > > -Have a single drm_connector_init_panel_orientation_property rather then
> > >   create and attach functions. The caller is expected to set
> > >   drm_display_info.panel_orientation before calling this, then this will
> > >   check for platform specific quirks overriding the panel_orientation and if
> > >   the panel_orientation is set after this then it will attach the property.
> > > ---
> > >   drivers/gpu/drm/Kconfig         |  1 +
> > >   drivers/gpu/drm/drm_connector.c | 73 +++++++++++++++++++++++++++++++++++++++++
> > >   include/drm/drm_connector.h     | 11 +++++++
> > >   include/drm/drm_mode_config.h   |  7 ++++
> > >   include/uapi/drm/drm_mode.h     |  7 ++++
> > >   5 files changed, 99 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index 9d005ac98c2b..0b166e626eb6 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -7,6 +7,7 @@
> > >   menuconfig DRM
> > >   	tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
> > >   	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
> > > +	select DRM_PANEL_ORIENTATION_QUIRKS
> > >   	select HDMI
> > >   	select FB_CMDLINE
> > >   	select I2C
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 704fc8934616..129c83a84320 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -24,6 +24,7 @@
> > >   #include <drm/drm_connector.h>
> > >   #include <drm/drm_edid.h>
> > >   #include <drm/drm_encoder.h>
> > > +#include <drm/drm_utils.h>
> > >   #include "drm_crtc_internal.h"
> > >   #include "drm_internal.h"
> > > @@ -212,6 +213,8 @@ int drm_connector_init(struct drm_device *dev,
> > >   	mutex_init(&connector->mutex);
> > >   	connector->edid_blob_ptr = NULL;
> > >   	connector->status = connector_status_unknown;
> > > +	connector->display_info.panel_orientation > > > +		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > >   	drm_connector_get_cmdline_mode(connector);
> > > @@ -664,6 +667,13 @@ static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
> > >   	{ DRM_MODE_PICTURE_ASPECT_16_9, "16:9" },
> > >   };
> > > +static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
> > > +	{ DRM_MODE_PANEL_ORIENTATION_NORMAL,	"Normal"	},
> > > +	{ DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP,	"Upside Down"	},
> > > +	{ DRM_MODE_PANEL_ORIENTATION_LEFT_UP,	"Left Side Up"	},
> > > +	{ DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,	"Right Side Up"	},
> > > +};
> > > +
> > >   static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = {
> > >   	{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
> > >   	{ DRM_MODE_SUBCONNECTOR_DVID,      "DVI-D"     }, /* DVI-I  */
> > > @@ -768,6 +778,18 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
> > >    *
> > >    * CRTC_ID:
> > >    * 	Mode object ID of the &drm_crtc this connector should be connected to.
> > > + *
> > > + * Connectors for LCD panels may also have one standardized property:
> > > + *
> > > + * panel orientation:
> > > + *	On some devices the LCD panel is mounted in the casing in such a way
> > > + *	that the up/top side of the panel does not match with the top side of
> > > + *	the device. Userspace can use this property to check for this.
> > > + *	Note that input coordinates from touchscreens (input devices with
> > > + *	INPUT_PROP_DIRECT) will still map 1:1 to the actual LCD panel
> > > + *	coordinates, so if userspace rotates the picture to adjust for
> > > + *	the orientation it must also apply the same transformation to the
> > > + *	touchscreen input coordinates.
> > >    */
> > >   int drm_connector_create_standard_properties(struct drm_device *dev)
> > > @@ -1234,6 +1256,57 @@ void drm_mode_connector_set_link_status_property(struct drm_connector *connector
> > >   }
> > >   EXPORT_SYMBOL(drm_mode_connector_set_link_status_property);
> > > +/**
> > > + * drm_connector_init_panel_orientation_property -
> > > + *	initialize the connecters panel_orientation property
> > > + * @connector: connector for which to init the panel-orientation property.
> > > + * @width: width in pixels of the panel, used for panel quirk detection
> > > + * @height: height in pixels of the panel, used for panel quirk detection
> > > + *
> > > + * This function should only be called for built-in panels, after setting
> > > + * connector->display_info.panel_orientation first (if known).
> > > + *
> > > + * This function will check for platform specific (e.g. DMI based) quirks
> > > + * overriding display_info.panel_orientation first, then if panel_orientation
> > > + * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
> > > + * "panel orientation" property to the connector.
> > > + *
> > > + * Returns:
> > > + * Zero on success, negative errno on failure.
> > > + */
> > 
> > Hm, I think our more usual way is to set the prop up first, and then the
> > parsing mode updates the property (in case it's not quite as stable as we
> > thought). Not the property init function calling the parsing code.
> > 
> > I know that the panel rotation will probably not change, but I think it'd
> > be good to be consistent here. Or at least look into whether that makes
> > sense ...
> 
> I'm not calling any parsing code here, what I'm calling is the code
> checking for quirks, so that that is done in one central place just like
> how drm_add_edid_modes() calls edid_get_quirks().
> 
> I could create the property in drm_connector_create_standard_properties()
> instead of in drm_connector_init_panel_orientation_property and
> rename the latter to drm_connector_attach_panel_orientation_property
> if you prefer having things split that way.
> 
> Auto attaching the property is tricky since we only want it on panels.

I just looked a bit backwards, and I hoped we could auto-update the
property when parsing the edid (like we do in a bunch of other places).
But sounds like that's not possible, so please disregard my suggestions.

If it later on turns out we need 2 steps, or have this also updated in the
edid parsing code, we can fix that when there's a real need.
-Daniel
> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> > Besides this bikeshed color question makes all sense.
> > -Daniel
> > 
> > > +int drm_connector_init_panel_orientation_property(
> > > +	struct drm_connector *connector, int width, int height)
> > > +{
> > > +	struct drm_device *dev = connector->dev;
> > > +	struct drm_display_info *info = &connector->display_info;
> > > +	struct drm_property *prop;
> > > +	int orientation_quirk;
> > > +
> > > +	orientation_quirk = drm_get_panel_orientation_quirk(width, height);
> > > +	if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > > +		info->panel_orientation = orientation_quirk;
> > > +
> > > +	if (info->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
> > > +		return 0;
> > > +
> > > +	prop = dev->mode_config.panel_orientation_property;
> > > +	if (!prop) {
> > > +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> > > +				"panel orientation",
> > > +				drm_panel_orientation_enum_list,
> > > +				ARRAY_SIZE(drm_panel_orientation_enum_list));
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +
> > > +		dev->mode_config.panel_orientation_property = prop;
> > > +	}
> > > +
> > > +	drm_object_attach_property(&connector->base, prop,
> > > +				   info->panel_orientation);
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_init_panel_orientation_property);
> > > +
> > >   int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> > >   				    struct drm_property *property,
> > >   				    uint64_t value)
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index b4285c40e1e4..e6883065a461 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -222,6 +222,15 @@ struct drm_display_info {
> > >   #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
> > >   #define DRM_COLOR_FORMAT_YCRCB420	(1<<3)
> > > +	/**
> > > +	 * @panel_orientation: Read only connector property for built-in panels,
> > > +	 * indicating the orientation of the panel vs the device's casing.
> > > +	 * drm_connector_init() sets this to DRM_MODE_PANEL_ORIENTATION_UNKNOWN.
> > > +	 * When not UNKNOWN this gets used by the drm_fb_helpers to rotate the
> > > +	 * fb to compensate and gets exported as prop to userspace.
> > > +	 */
> > > +	int panel_orientation;
> > > +
> > >   	/**
> > >   	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
> > >   	 * modes. Used DRM_COLOR_FORMAT\_ defines, which are _not_ the same ones
> > > @@ -1019,6 +1028,8 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
> > >   					    const struct edid *edid);
> > >   void drm_mode_connector_set_link_status_property(struct drm_connector *connector,
> > >   						 uint64_t link_status);
> > > +int drm_connector_init_panel_orientation_property(
> > > +	struct drm_connector *connector, int width, int height);
> > >   /**
> > >    * struct drm_tile_group - Tile group metadata
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index 0b4ac2ebc610..7d4ee1726e0a 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -728,6 +728,13 @@ struct drm_mode_config {
> > >   	 */
> > >   	struct drm_property *suggested_y_property;
> > > +	/**
> > > +	 * @panel_orientation_property: Optional connector property indicating
> > > +	 * how the lcd-panel is mounted inside the casing (e.g. normal or
> > > +	 * upside-down).
> > > +	 */
> > > +	struct drm_property *panel_orientation_property;
> > > +
> > >   	/* dumb ioctl parameters */
> > >   	uint32_t preferred_depth, prefer_shadow;
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 34b6bb34b002..f60fae67bc1f 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -127,6 +127,13 @@ extern "C" {
> > >   #define DRM_MODE_LINK_STATUS_GOOD	0
> > >   #define DRM_MODE_LINK_STATUS_BAD	1
> > > +/* Panel Orientation options */
> > > +#define DRM_MODE_PANEL_ORIENTATION_UNKNOWN	-1
> > > +#define DRM_MODE_PANEL_ORIENTATION_NORMAL	0
> > > +#define DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP	1
> > > +#define DRM_MODE_PANEL_ORIENTATION_LEFT_UP	2
> > > +#define DRM_MODE_PANEL_ORIENTATION_RIGHT_UP	3
> > > +
> > >   /*
> > >    * DRM_MODE_ROTATE_<degrees>
> > >    *
> > > -- 
> > > 2.14.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 4/7] drm/fb-helper: Apply panel orientation connector prop to the primary plane

[Hans de Goede]

Apply the "panel orientation" drm connector prop to the primary plane so
that fbcon and fbdev using userspace programs display the right way up.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id”894
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-New patch in v2 of this patch-set

Changes in v3:
-Use a rotation member in struct drm_fb_helper_crtc and set that from
 drm_setup_crtcs instead of looping over all crtc's to find the right one
 later
-Since we now no longer look at rotation quirks directly in the fbcon code,
 set fb_info.fbcon_rotate_hint when the panel is not mounted upright and
 we cannot use hardware rotation
---
 drivers/gpu/drm/drm_fb_helper.c | 76 +++++++++++++++++++++++++++++++++++++++--
 include/drm/drm_fb_helper.h     |  8 +++++
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 116d1f1337c7..e0f95f2cc52f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -41,6 +41,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 
+#include "drm_crtc_internal.h"
 #include "drm_crtc_helper_internal.h"
 
 static bool drm_fbdev_emulation = true;
@@ -350,6 +351,7 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
 static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
 {
 	struct drm_device *dev = fb_helper->dev;
+	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	struct drm_atomic_state *state;
 	int i, ret;
@@ -368,8 +370,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
 retry:
 	plane_mask = 0;
 	drm_for_each_plane(plane, dev) {
-		struct drm_plane_state *plane_state;
-
 		plane_state = drm_atomic_get_plane_state(state, plane);
 		if (IS_ERR(plane_state)) {
 			ret = PTR_ERR(plane_state);
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
 
 	for (i = 0; i < fb_helper->crtc_count; i++) {
 		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
+		struct drm_plane *primary = mode_set->crtc->primary;
+
+		/* Cannot fail as we've already gotten the plane state above */
+		plane_state = drm_atomic_get_new_plane_state(state, primary);
+		plane_state->rotation = fb_helper->crtc_info[i].rotation;
 
 		ret = __drm_atomic_helper_set_config(mode_set, state);
 		if (ret != 0)
@@ -2334,6 +2339,57 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	return best_score;
 }
 
+/*
+ * This function checks if rotation is necessary because of panel orientation
+ * and if it is, if it is supported.
+ * If rotation is necessary and supported, its gets set in fb_crtc.rotation.
+ * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
+ * or-ed into fb_helper->rotations. In drm_setup_crtcs_fb() we check if only
+ * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
+ * the unsupported rotation.
+ */
+static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
+				    struct drm_fb_helper_crtc *fb_crtc,
+				    struct drm_connector *connector)
+{
+	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
+	uint64_t valid_mask = 0;
+	int i, rotation;
+
+	fb_crtc->rotation = DRM_MODE_ROTATE_0;
+
+	switch (connector->display_info.panel_orientation) {
+	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
+		rotation = DRM_MODE_ROTATE_180;
+		break;
+	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
+		rotation = DRM_MODE_ROTATE_90;
+		break;
+	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
+		rotation = DRM_MODE_ROTATE_270;
+		break;
+	default:
+		rotation = DRM_MODE_ROTATE_0;
+	}
+
+	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
+		fb_helper->rotations |= rotation;
+		return;
+	}
+
+	for (i = 0; i < plane->rotation_property->num_values; i++)
+		valid_mask |= (1ULL << plane->rotation_property->values[i]);
+
+	if (!(rotation & valid_mask)) {
+		fb_helper->rotations |= rotation;
+		return;
+	}
+
+	fb_crtc->rotation = rotation;
+	/* Rotating in hardware, fbcon should not rotate */
+	fb_helper->rotations |= DRM_MODE_ROTATE_0;
+}
+
 static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 			    u32 width, u32 height)
 {
@@ -2393,6 +2449,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 		drm_fb_helper_modeset_release(fb_helper,
 					      &fb_helper->crtc_info[i].mode_set);
 
+	fb_helper->rotations = 0;
 	drm_fb_helper_for_each_connector(fb_helper, i) {
 		struct drm_display_mode *mode = modes[i];
 		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
@@ -2412,6 +2469,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
 			modeset->mode = drm_mode_duplicate(dev,
 							   fb_crtc->desired_mode);
 			drm_connector_get(connector);
+			drm_setup_crtc_rotation(fb_helper, fb_crtc, connector);
 			modeset->connectors[modeset->num_connectors++] = connector;
 			modeset->x = offset->x;
 			modeset->y = offset->y;
@@ -2453,6 +2511,20 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
 		}
 	}
 	mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+	switch (fb_helper->rotations) {
+	case DRM_MODE_ROTATE_90:
+		info->fbcon_rotate_hint = FB_ROTATE_CCW;
+		break;
+	case DRM_MODE_ROTATE_180:
+		info->fbcon_rotate_hint = FB_ROTATE_UD;
+		break;
+	case DRM_MODE_ROTATE_270:
+		info->fbcon_rotate_hint = FB_ROTATE_CW;
+		break;
+	default:
+		info->fbcon_rotate_hint = FB_ROTATE_UR;
+	}
 }
 
 /* Note: Drops fb_helper->lock before returning. */
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 33fe95927742..4ee834a077a2 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;
+	int rotation;
 };
 
 /**
@@ -158,6 +159,13 @@ struct drm_fb_helper {
 	struct drm_fb_helper_crtc *crtc_info;
 	int connector_count;
 	int connector_info_alloc_count;
+	/**
+	 * @rotations:
+	 * Bitmask of all rotations requested for panel-orientation which
+	 * could not be handled in hardware. If only one bit is set
+	 * fbdev->fbcon_rotate_hint gets set to the requested rotation.
+	 */
+	int rotations;
 	/**
 	 * @connector_info:
 	 *
-- 
2.14.2

Re: [Intel-gfx] [PATCH v3 4/7] drm/fb-helper: Apply panel orientation connector prop to the primary

[Daniel Vetter]

On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
> Apply the "panel orientation" drm connector prop to the primary plane so
> that fbcon and fbdev using userspace programs display the right way up.
> 
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id”894
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -New patch in v2 of this patch-set
> 
> Changes in v3:
> -Use a rotation member in struct drm_fb_helper_crtc and set that from
>  drm_setup_crtcs instead of looping over all crtc's to find the right one
>  later
> -Since we now no longer look at rotation quirks directly in the fbcon code,
>  set fb_info.fbcon_rotate_hint when the panel is not mounted upright and
>  we cannot use hardware rotation
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 76 +++++++++++++++++++++++++++++++++++++++--
>  include/drm/drm_fb_helper.h     |  8 +++++
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 116d1f1337c7..e0f95f2cc52f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -41,6 +41,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  
> +#include "drm_crtc_internal.h"
>  #include "drm_crtc_helper_internal.h"
>  
>  static bool drm_fbdev_emulation = true;
> @@ -350,6 +351,7 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>  {
>  	struct drm_device *dev = fb_helper->dev;
> +	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	struct drm_atomic_state *state;
>  	int i, ret;
> @@ -368,8 +370,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  retry:
>  	plane_mask = 0;
>  	drm_for_each_plane(plane, dev) {
> -		struct drm_plane_state *plane_state;
> -
>  		plane_state = drm_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state)) {
>  			ret = PTR_ERR(plane_state);
> @@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
>  		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> +		struct drm_plane *primary = mode_set->crtc->primary;
> +
> +		/* Cannot fail as we've already gotten the plane state above */
> +		plane_state = drm_atomic_get_new_plane_state(state, primary);
> +		plane_state->rotation = fb_helper->crtc_info[i].rotation;
>  
>  		ret = __drm_atomic_helper_set_config(mode_set, state);
>  		if (ret != 0)
> @@ -2334,6 +2339,57 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	return best_score;
>  }
>  
> +/*
> + * This function checks if rotation is necessary because of panel orientation
> + * and if it is, if it is supported.
> + * If rotation is necessary and supported, its gets set in fb_crtc.rotation.
> + * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
> + * or-ed into fb_helper->rotations. In drm_setup_crtcs_fb() we check if only
> + * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
> + * the unsupported rotation.
> + */
> +static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
> +				    struct drm_fb_helper_crtc *fb_crtc,
> +				    struct drm_connector *connector)
> +{
> +	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
> +	uint64_t valid_mask = 0;
> +	int i, rotation;
> +
> +	fb_crtc->rotation = DRM_MODE_ROTATE_0;
> +
> +	switch (connector->display_info.panel_orientation) {
> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> +		rotation = DRM_MODE_ROTATE_180;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> +		rotation = DRM_MODE_ROTATE_90;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> +		rotation = DRM_MODE_ROTATE_270;
> +		break;

For 90/270 hw rotation you need to flip the coordinates/sizes of the fb.

> +	default:
> +		rotation = DRM_MODE_ROTATE_0;
> +	}
> +
> +	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
> +		fb_helper->rotations |= rotation;
> +		return;
> +	}
> +
> +	for (i = 0; i < plane->rotation_property->num_values; i++)
> +		valid_mask |= (1ULL << plane->rotation_property->values[i]);

This isn't a good enough check for atomic drivers (and not for gen9+ intel
hw), since we might expose 90° rotations, but it only works if you have
the correct tiling format.

For atomic drivers it'd be really good if we could do a TEST_ONLY commit
first, and if that fails, fall back to sw rotation.

But that poses a bit a chicken&egg with creating the framebuffer (we need
one for the TEST_ONLY), so probably a bit too much more for this. And
afaiui your quirk list only applies to older stuff.

At least add a FIXME meanwhile? In a way we have a FIXME already for
multi-pipe, since we don't try to fall back to fewer pipes if the single
atomic commit failed.

Or maybe just don't use 90/270 hw rotation for now since it seems buggy in
your code anyway.

> +
> +	if (!(rotation & valid_mask)) {
> +		fb_helper->rotations |= rotation;
> +		return;
> +	}
> +
> +	fb_crtc->rotation = rotation;
> +	/* Rotating in hardware, fbcon should not rotate */
> +	fb_helper->rotations |= DRM_MODE_ROTATE_0;

Wrong bitopt I think.

Or you're doing some really funny control logic by oring in another value
to hit the default case below which doesn't rotate anything. I think that
should be done explicitly, by explicitly setting to rotation to ROTATE_0
instead of this. Same for the check above.

> +}
> +
>  static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  			    u32 width, u32 height)
>  {
> @@ -2393,6 +2449,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  		drm_fb_helper_modeset_release(fb_helper,
>  					      &fb_helper->crtc_info[i].mode_set);
>  
> +	fb_helper->rotations = 0;
>  	drm_fb_helper_for_each_connector(fb_helper, i) {
>  		struct drm_display_mode *mode = modes[i];
>  		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
> @@ -2412,6 +2469,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>  			modeset->mode = drm_mode_duplicate(dev,
>  							   fb_crtc->desired_mode);
>  			drm_connector_get(connector);
> +			drm_setup_crtc_rotation(fb_helper, fb_crtc, connector);
>  			modeset->connectors[modeset->num_connectors++] = connector;
>  			modeset->x = offset->x;
>  			modeset->y = offset->y;
> @@ -2453,6 +2511,20 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>  		}
>  	}
>  	mutex_unlock(&fb_helper->dev->mode_config.mutex);
> +
> +	switch (fb_helper->rotations) {
> +	case DRM_MODE_ROTATE_90:
> +		info->fbcon_rotate_hint = FB_ROTATE_CCW;
> +		break;
> +	case DRM_MODE_ROTATE_180:
> +		info->fbcon_rotate_hint = FB_ROTATE_UD;
> +		break;
> +	case DRM_MODE_ROTATE_270:
> +		info->fbcon_rotate_hint = FB_ROTATE_CW;
> +		break;
> +	default:
> +		info->fbcon_rotate_hint = FB_ROTATE_UR;
> +	}
>  }
>  
>  /* Note: Drops fb_helper->lock before returning. */
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 33fe95927742..4ee834a077a2 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;
> +	int rotation;
>  };
>  
>  /**
> @@ -158,6 +159,13 @@ struct drm_fb_helper {
>  	struct drm_fb_helper_crtc *crtc_info;
>  	int connector_count;
>  	int connector_info_alloc_count;
> +	/**
> +	 * @rotations:
> +	 * Bitmask of all rotations requested for panel-orientation which
> +	 * could not be handled in hardware. If only one bit is set
> +	 * fbdev->fbcon_rotate_hint gets set to the requested rotation.
> +	 */
> +	int rotations;
>  	/**
>  	 * @connector_info:
>  	 *
> -- 
> 2.14.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v3 4/7] drm/fb-helper: Apply panel orientation connector prop to the primary

[Hans de Goede]

Hi,

On 30-10-17 10:52, Daniel Vetter wrote:
> On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
>> Apply the "panel orientation" drm connector prop to the primary plane so
>> that fbcon and fbdev using userspace programs display the right way up.
>>
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id”894
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -New patch in v2 of this patch-set
>>
>> Changes in v3:
>> -Use a rotation member in struct drm_fb_helper_crtc and set that from
>>   drm_setup_crtcs instead of looping over all crtc's to find the right one
>>   later
>> -Since we now no longer look at rotation quirks directly in the fbcon code,
>>   set fb_info.fbcon_rotate_hint when the panel is not mounted upright and
>>   we cannot use hardware rotation
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 76 +++++++++++++++++++++++++++++++++++++++--
>>   include/drm/drm_fb_helper.h     |  8 +++++
>>   2 files changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 116d1f1337c7..e0f95f2cc52f 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -41,6 +41,7 @@
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>>   
>> +#include "drm_crtc_internal.h"
>>   #include "drm_crtc_helper_internal.h"
>>   
>>   static bool drm_fbdev_emulation = true;
>> @@ -350,6 +351,7 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>>   static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>>   {
>>   	struct drm_device *dev = fb_helper->dev;
>> +	struct drm_plane_state *plane_state;
>>   	struct drm_plane *plane;
>>   	struct drm_atomic_state *state;
>>   	int i, ret;
>> @@ -368,8 +370,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>>   retry:
>>   	plane_mask = 0;
>>   	drm_for_each_plane(plane, dev) {
>> -		struct drm_plane_state *plane_state;
>> -
>>   		plane_state = drm_atomic_get_plane_state(state, plane);
>>   		if (IS_ERR(plane_state)) {
>>   			ret = PTR_ERR(plane_state);
>> @@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>>   
>>   	for (i = 0; i < fb_helper->crtc_count; i++) {
>>   		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
>> +		struct drm_plane *primary = mode_set->crtc->primary;
>> +
>> +		/* Cannot fail as we've already gotten the plane state above */
>> +		plane_state = drm_atomic_get_new_plane_state(state, primary);
>> +		plane_state->rotation = fb_helper->crtc_info[i].rotation;
>>   
>>   		ret = __drm_atomic_helper_set_config(mode_set, state);
>>   		if (ret != 0)
>> @@ -2334,6 +2339,57 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>>   	return best_score;
>>   }
>>   
>> +/*
>> + * This function checks if rotation is necessary because of panel orientation
>> + * and if it is, if it is supported.
>> + * If rotation is necessary and supported, its gets set in fb_crtc.rotation.
>> + * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
>> + * or-ed into fb_helper->rotations. In drm_setup_crtcs_fb() we check if only
>> + * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
>> + * the unsupported rotation.
>> + */
>> +static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>> +				    struct drm_fb_helper_crtc *fb_crtc,
>> +				    struct drm_connector *connector)
>> +{
>> +	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
>> +	uint64_t valid_mask = 0;
>> +	int i, rotation;
>> +
>> +	fb_crtc->rotation = DRM_MODE_ROTATE_0;
>> +
>> +	switch (connector->display_info.panel_orientation) {
>> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
>> +		rotation = DRM_MODE_ROTATE_180;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
>> +		rotation = DRM_MODE_ROTATE_90;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
>> +		rotation = DRM_MODE_ROTATE_270;
>> +		break;
> 
> For 90/270 hw rotation you need to flip the coordinates/sizes of the fb.

You're probably right, I don't have any hardware supporting
270 degree rotation to test this with.

>> +	default:
>> +		rotation = DRM_MODE_ROTATE_0;
>> +	}
>> +
>> +	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
>> +		fb_helper->rotations |= rotation;
>> +		return;
>> +	}
>> +
>> +	for (i = 0; i < plane->rotation_property->num_values; i++)
>> +		valid_mask |= (1ULL << plane->rotation_property->values[i]);
> 
> This isn't a good enough check for atomic drivers (and not for gen9+ intel
> hw), since we might expose 90° rotations, but it only works if you have
> the correct tiling format.
> 
> For atomic drivers it'd be really good if we could do a TEST_ONLY commit
> first, and if that fails, fall back to sw rotation.
> 
> But that poses a bit a chicken&egg with creating the framebuffer (we need
> one for the TEST_ONLY), so probably a bit too much more for this. And
> afaiui your quirk list only applies to older stuff.
> 
> At least add a FIXME meanwhile? In a way we have a FIXME already for
> multi-pipe, since we don't try to fall back to fewer pipes if the single
> atomic commit failed.
> 
> Or maybe just don't use 90/270 hw rotation for now since it seems buggy in
> your code anyway.

I was wondering about this (need for special fb layout) myself too, I
agree also given the above comment that it is probably best to only
support 0/180 degree hardware rotation for now, I will do for the next
version (and add a TODO comment).


>> +
>> +	if (!(rotation & valid_mask)) {
>> +		fb_helper->rotations |= rotation;
>> +		return;
>> +	}
>> +
>> +	fb_crtc->rotation = rotation;
>> +	/* Rotating in hardware, fbcon should not rotate */
>> +	fb_helper->rotations |= DRM_MODE_ROTATE_0;
> 
> Wrong bitopt I think.

No this is intentional, if we've a panel requiring say 90 degree rotation
which we will do in software and another panel (weird example) doing 180
degree rotation in hardware, then we want to or both
DRM_MODE_ROTATE_90 and DRM_MODE_ROTATE_0 into the rotations bitmask
(0 since the hardware rotation requires no sw rotation).
The rotations bitmask is the *combination* of all rotations we need
fbcon to do in software and when that ends up being more then one
rotation we chicken out and just don't do any software rotation,
maybe I should rename rotations to sw_rotations?

A better real world example is a 90 degree rotated panel with
an external monitor, in that case for the panel we hit:

	if (!(rotation & valid_mask)) {
		fb_helper->rotations |= rotation;
		return;
	}

Or-ing in the panel's DRM_MODE_ROTATE_90.

And for the monitor we hit:

	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
		fb_helper->rotations |= rotation;
		return;
	}

Or-ing in the monitors's DRM_MODE_ROTATE_0.

So we end up with 2 bits set in fb_helper->rotations, hitting the
default in the switch case and picking FB_ROTATE_UR, since we cannot
satisfy both rotations in fbcon at the same time.


> Or you're doing some really funny control logic by oring in another value
> to hit the default case below which doesn't rotate anything. I think that
> should be done explicitly, by explicitly setting to rotation to ROTATE_0
> instead of this. Same for the check above.

I hope my explanation above explains why I'm or-ing together rotations,
basically I want to detect if we need more then 1 type of software-rotation
in fbcon and in that case bail out and fallback to FB_ROTATE_UR.

Regards,

Hans


> 
>> +}
>> +
>>   static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>   			    u32 width, u32 height)
>>   {
>> @@ -2393,6 +2449,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>   		drm_fb_helper_modeset_release(fb_helper,
>>   					      &fb_helper->crtc_info[i].mode_set);
>>   
>> +	fb_helper->rotations = 0;
>>   	drm_fb_helper_for_each_connector(fb_helper, i) {
>>   		struct drm_display_mode *mode = modes[i];
>>   		struct drm_fb_helper_crtc *fb_crtc = crtcs[i];
>> @@ -2412,6 +2469,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper,
>>   			modeset->mode = drm_mode_duplicate(dev,
>>   							   fb_crtc->desired_mode);
>>   			drm_connector_get(connector);
>> +			drm_setup_crtc_rotation(fb_helper, fb_crtc, connector);
>>   			modeset->connectors[modeset->num_connectors++] = connector;
>>   			modeset->x = offset->x;
>>   			modeset->y = offset->y;
>> @@ -2453,6 +2511,20 @@ static void drm_setup_crtcs_fb(struct drm_fb_helper *fb_helper)
>>   		}
>>   	}
>>   	mutex_unlock(&fb_helper->dev->mode_config.mutex);
>> +
>> +	switch (fb_helper->rotations) {
>> +	case DRM_MODE_ROTATE_90:
>> +		info->fbcon_rotate_hint = FB_ROTATE_CCW;
>> +		break;
>> +	case DRM_MODE_ROTATE_180:
>> +		info->fbcon_rotate_hint = FB_ROTATE_UD;
>> +		break;
>> +	case DRM_MODE_ROTATE_270:
>> +		info->fbcon_rotate_hint = FB_ROTATE_CW;
>> +		break;
>> +	default:
>> +		info->fbcon_rotate_hint = FB_ROTATE_UR;
>> +	}
>>   }
>>   
>>   /* Note: Drops fb_helper->lock before returning. */
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index 33fe95927742..4ee834a077a2 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;
>> +	int rotation;
>>   };
>>   
>>   /**
>> @@ -158,6 +159,13 @@ struct drm_fb_helper {
>>   	struct drm_fb_helper_crtc *crtc_info;
>>   	int connector_count;
>>   	int connector_info_alloc_count;
>> +	/**
>> +	 * @rotations:
>> +	 * Bitmask of all rotations requested for panel-orientation which
>> +	 * could not be handled in hardware. If only one bit is set
>> +	 * fbdev->fbcon_rotate_hint gets set to the requested rotation.
>> +	 */
>> +	int rotations;
>>   	/**
>>   	 * @connector_info:
>>   	 *
>> -- 
>> 2.14.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [Intel-gfx] [PATCH v3 4/7] drm/fb-helper: Apply panel orientation connector prop to the primary

[Daniel Vetter]

On Mon, Oct 30, 2017 at 12:09:27PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-10-17 10:52, Daniel Vetter wrote:
> > On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
> > > Apply the "panel orientation" drm connector prop to the primary plane so
> > > that fbcon and fbdev using userspace programs display the right way up.
> > > 
> > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id”894
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Changes in v2:
> > > -New patch in v2 of this patch-set
> > > 
> > > Changes in v3:
> > > -Use a rotation member in struct drm_fb_helper_crtc and set that from
> > >   drm_setup_crtcs instead of looping over all crtc's to find the right one
> > >   later
> > > -Since we now no longer look at rotation quirks directly in the fbcon code,
> > >   set fb_info.fbcon_rotate_hint when the panel is not mounted upright and
> > >   we cannot use hardware rotation
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c | 76 +++++++++++++++++++++++++++++++++++++++--
> > >   include/drm/drm_fb_helper.h     |  8 +++++
> > >   2 files changed, 82 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 116d1f1337c7..e0f95f2cc52f 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -41,6 +41,7 @@
> > >   #include <drm/drm_atomic.h>
> > >   #include <drm/drm_atomic_helper.h>
> > > +#include "drm_crtc_internal.h"
> > >   #include "drm_crtc_helper_internal.h"
> > >   static bool drm_fbdev_emulation = true;
> > > @@ -350,6 +351,7 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> > >   static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> > >   {
> > >   	struct drm_device *dev = fb_helper->dev;
> > > +	struct drm_plane_state *plane_state;
> > >   	struct drm_plane *plane;
> > >   	struct drm_atomic_state *state;
> > >   	int i, ret;
> > > @@ -368,8 +370,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >   retry:
> > >   	plane_mask = 0;
> > >   	drm_for_each_plane(plane, dev) {
> > > -		struct drm_plane_state *plane_state;
> > > -
> > >   		plane_state = drm_atomic_get_plane_state(state, plane);
> > >   		if (IS_ERR(plane_state)) {
> > >   			ret = PTR_ERR(plane_state);
> > > @@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > >   	for (i = 0; i < fb_helper->crtc_count; i++) {
> > >   		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> > > +		struct drm_plane *primary = mode_set->crtc->primary;
> > > +
> > > +		/* Cannot fail as we've already gotten the plane state above */
> > > +		plane_state = drm_atomic_get_new_plane_state(state, primary);
> > > +		plane_state->rotation = fb_helper->crtc_info[i].rotation;
> > >   		ret = __drm_atomic_helper_set_config(mode_set, state);
> > >   		if (ret != 0)
> > > @@ -2334,6 +2339,57 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> > >   	return best_score;
> > >   }
> > > +/*
> > > + * This function checks if rotation is necessary because of panel orientation
> > > + * and if it is, if it is supported.
> > > + * If rotation is necessary and supported, its gets set in fb_crtc.rotation.
> > > + * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
> > > + * or-ed into fb_helper->rotations. In drm_setup_crtcs_fb() we check if only
> > > + * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
> > > + * the unsupported rotation.
> > > + */
> > > +static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
> > > +				    struct drm_fb_helper_crtc *fb_crtc,
> > > +				    struct drm_connector *connector)
> > > +{
> > > +	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
> > > +	uint64_t valid_mask = 0;
> > > +	int i, rotation;
> > > +
> > > +	fb_crtc->rotation = DRM_MODE_ROTATE_0;
> > > +
> > > +	switch (connector->display_info.panel_orientation) {
> > > +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> > > +		rotation = DRM_MODE_ROTATE_180;
> > > +		break;
> > > +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> > > +		rotation = DRM_MODE_ROTATE_90;
> > > +		break;
> > > +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> > > +		rotation = DRM_MODE_ROTATE_270;
> > > +		break;
> > 
> > For 90/270 hw rotation you need to flip the coordinates/sizes of the fb.
> 
> You're probably right, I don't have any hardware supporting
> 270 degree rotation to test this with.
> 
> > > +	default:
> > > +		rotation = DRM_MODE_ROTATE_0;
> > > +	}
> > > +
> > > +	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
> > > +		fb_helper->rotations |= rotation;
> > > +		return;
> > > +	}
> > > +
> > > +	for (i = 0; i < plane->rotation_property->num_values; i++)
> > > +		valid_mask |= (1ULL << plane->rotation_property->values[i]);
> > 
> > This isn't a good enough check for atomic drivers (and not for gen9+ intel
> > hw), since we might expose 90° rotations, but it only works if you have
> > the correct tiling format.
> > 
> > For atomic drivers it'd be really good if we could do a TEST_ONLY commit
> > first, and if that fails, fall back to sw rotation.
> > 
> > But that poses a bit a chicken&egg with creating the framebuffer (we need
> > one for the TEST_ONLY), so probably a bit too much more for this. And
> > afaiui your quirk list only applies to older stuff.
> > 
> > At least add a FIXME meanwhile? In a way we have a FIXME already for
> > multi-pipe, since we don't try to fall back to fewer pipes if the single
> > atomic commit failed.
> > 
> > Or maybe just don't use 90/270 hw rotation for now since it seems buggy in
> > your code anyway.
> 
> I was wondering about this (need for special fb layout) myself too, I
> agree also given the above comment that it is probably best to only
> support 0/180 degree hardware rotation for now, I will do for the next
> version (and add a TODO comment).

It'll work on i915 at least, on current hw. Might still be broken on
other, but then that's a larger issue with the fbcon atomic code right
now.
> 
> 
> > > +
> > > +	if (!(rotation & valid_mask)) {
> > > +		fb_helper->rotations |= rotation;
> > > +		return;
> > > +	}
> > > +
> > > +	fb_crtc->rotation = rotation;
> > > +	/* Rotating in hardware, fbcon should not rotate */
> > > +	fb_helper->rotations |= DRM_MODE_ROTATE_0;
> > 
> > Wrong bitopt I think.
> 
> No this is intentional, if we've a panel requiring say 90 degree rotation
> which we will do in software and another panel (weird example) doing 180
> degree rotation in hardware, then we want to or both
> DRM_MODE_ROTATE_90 and DRM_MODE_ROTATE_0 into the rotations bitmask
> (0 since the hardware rotation requires no sw rotation).
> The rotations bitmask is the *combination* of all rotations we need
> fbcon to do in software and when that ends up being more then one
> rotation we chicken out and just don't do any software rotation,
> maybe I should rename rotations to sw_rotations?
> 
> A better real world example is a 90 degree rotated panel with
> an external monitor, in that case for the panel we hit:
> 
> 	if (!(rotation & valid_mask)) {
> 		fb_helper->rotations |= rotation;
> 		return;
> 	}
> 
> Or-ing in the panel's DRM_MODE_ROTATE_90.
> 
> And for the monitor we hit:
> 
> 	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
> 		fb_helper->rotations |= rotation;
> 		return;
> 	}
> 
> Or-ing in the monitors's DRM_MODE_ROTATE_0.
> 
> So we end up with 2 bits set in fb_helper->rotations, hitting the
> default in the switch case and picking FB_ROTATE_UR, since we cannot
> satisfy both rotations in fbcon at the same time.
> 
> 
> > Or you're doing some really funny control logic by oring in another value
> > to hit the default case below which doesn't rotate anything. I think that
> > should be done explicitly, by explicitly setting to rotation to ROTATE_0
> > instead of this. Same for the check above.
> 
> I hope my explanation above explains why I'm or-ing together rotations,
> basically I want to detect if we need more then 1 type of software-rotation
> in fbcon and in that case bail out and fallback to FB_ROTATE_UR.

Yeah I suspected this is what you're trying to do, but imo it's too clear.
Can't we do an explicit check for this instead of uncommented magic that
takes half an hour to understand? I'm thinking of

if (count_bits(fbb_helper->rotation)) {
	/* conflicting rotation requests on different connnectors, fall
	 * back to unrotated. */

	 fb_helper->rotation = ROTATE_0
}

Or something similar at the end of drm_setup_crtcs (plus maybe doing the
resolve in there too). Right now that logic is split between 2 functions,
and no comment explaining what's going on.

But not sure that's actually going to help with readability.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v3 4/7] drm/fb-helper: Apply panel orientation connector prop to the primary

[Hans de Goede]

Hi,

On 31-10-17 11:14, Daniel Vetter wrote:
> On Mon, Oct 30, 2017 at 12:09:27PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-10-17 10:52, Daniel Vetter wrote:
>>> On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
>>>> Apply the "panel orientation" drm connector prop to the primary plane so
>>>> that fbcon and fbdev using userspace programs display the right way up.
>>>>
>>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id”894
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v2:
>>>> -New patch in v2 of this patch-set
>>>>
>>>> Changes in v3:
>>>> -Use a rotation member in struct drm_fb_helper_crtc and set that from
>>>>    drm_setup_crtcs instead of looping over all crtc's to find the right one
>>>>    later
>>>> -Since we now no longer look at rotation quirks directly in the fbcon code,
>>>>    set fb_info.fbcon_rotate_hint when the panel is not mounted upright and
>>>>    we cannot use hardware rotation
>>>> ---
>>>>    drivers/gpu/drm/drm_fb_helper.c | 76 +++++++++++++++++++++++++++++++++++++++--
>>>>    include/drm/drm_fb_helper.h     |  8 +++++
>>>>    2 files changed, 82 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index 116d1f1337c7..e0f95f2cc52f 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -41,6 +41,7 @@
>>>>    #include <drm/drm_atomic.h>
>>>>    #include <drm/drm_atomic_helper.h>
>>>> +#include "drm_crtc_internal.h"
>>>>    #include "drm_crtc_helper_internal.h"
>>>>    static bool drm_fbdev_emulation = true;
>>>> @@ -350,6 +351,7 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>>>>    static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>>>>    {
>>>>    	struct drm_device *dev = fb_helper->dev;
>>>> +	struct drm_plane_state *plane_state;
>>>>    	struct drm_plane *plane;
>>>>    	struct drm_atomic_state *state;
>>>>    	int i, ret;
>>>> @@ -368,8 +370,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>>>>    retry:
>>>>    	plane_mask = 0;
>>>>    	drm_for_each_plane(plane, dev) {
>>>> -		struct drm_plane_state *plane_state;
>>>> -
>>>>    		plane_state = drm_atomic_get_plane_state(state, plane);
>>>>    		if (IS_ERR(plane_state)) {
>>>>    			ret = PTR_ERR(plane_state);
>>>> @@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>>>>    	for (i = 0; i < fb_helper->crtc_count; i++) {
>>>>    		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
>>>> +		struct drm_plane *primary = mode_set->crtc->primary;
>>>> +
>>>> +		/* Cannot fail as we've already gotten the plane state above */
>>>> +		plane_state = drm_atomic_get_new_plane_state(state, primary);
>>>> +		plane_state->rotation = fb_helper->crtc_info[i].rotation;
>>>>    		ret = __drm_atomic_helper_set_config(mode_set, state);
>>>>    		if (ret != 0)
>>>> @@ -2334,6 +2339,57 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>>>>    	return best_score;
>>>>    }
>>>> +/*
>>>> + * This function checks if rotation is necessary because of panel orientation
>>>> + * and if it is, if it is supported.
>>>> + * If rotation is necessary and supported, its gets set in fb_crtc.rotation.
>>>> + * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
>>>> + * or-ed into fb_helper->rotations. In drm_setup_crtcs_fb() we check if only
>>>> + * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
>>>> + * the unsupported rotation.
>>>> + */
>>>> +static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>>>> +				    struct drm_fb_helper_crtc *fb_crtc,
>>>> +				    struct drm_connector *connector)
>>>> +{
>>>> +	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
>>>> +	uint64_t valid_mask = 0;
>>>> +	int i, rotation;
>>>> +
>>>> +	fb_crtc->rotation = DRM_MODE_ROTATE_0;
>>>> +
>>>> +	switch (connector->display_info.panel_orientation) {
>>>> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
>>>> +		rotation = DRM_MODE_ROTATE_180;
>>>> +		break;
>>>> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
>>>> +		rotation = DRM_MODE_ROTATE_90;
>>>> +		break;
>>>> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
>>>> +		rotation = DRM_MODE_ROTATE_270;
>>>> +		break;
>>>
>>> For 90/270 hw rotation you need to flip the coordinates/sizes of the fb.
>>
>> You're probably right, I don't have any hardware supporting
>> 270 degree rotation to test this with.
>>
>>>> +	default:
>>>> +		rotation = DRM_MODE_ROTATE_0;
>>>> +	}
>>>> +
>>>> +	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
>>>> +		fb_helper->rotations |= rotation;
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < plane->rotation_property->num_values; i++)
>>>> +		valid_mask |= (1ULL << plane->rotation_property->values[i]);
>>>
>>> This isn't a good enough check for atomic drivers (and not for gen9+ intel
>>> hw), since we might expose 90° rotations, but it only works if you have
>>> the correct tiling format.
>>>
>>> For atomic drivers it'd be really good if we could do a TEST_ONLY commit
>>> first, and if that fails, fall back to sw rotation.
>>>
>>> But that poses a bit a chicken&egg with creating the framebuffer (we need
>>> one for the TEST_ONLY), so probably a bit too much more for this. And
>>> afaiui your quirk list only applies to older stuff.
>>>
>>> At least add a FIXME meanwhile? In a way we have a FIXME already for
>>> multi-pipe, since we don't try to fall back to fewer pipes if the single
>>> atomic commit failed.
>>>
>>> Or maybe just don't use 90/270 hw rotation for now since it seems buggy in
>>> your code anyway.
>>
>> I was wondering about this (need for special fb layout) myself too, I
>> agree also given the above comment that it is probably best to only
>> support 0/180 degree hardware rotation for now, I will do for the next
>> version (and add a TODO comment).
> 
> It'll work on i915 at least, on current hw. Might still be broken on
> other, but then that's a larger issue with the fbcon atomic code right
> now.
>>
>>
>>>> +
>>>> +	if (!(rotation & valid_mask)) {
>>>> +		fb_helper->rotations |= rotation;
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	fb_crtc->rotation = rotation;
>>>> +	/* Rotating in hardware, fbcon should not rotate */
>>>> +	fb_helper->rotations |= DRM_MODE_ROTATE_0;
>>>
>>> Wrong bitopt I think.
>>
>> No this is intentional, if we've a panel requiring say 90 degree rotation
>> which we will do in software and another panel (weird example) doing 180
>> degree rotation in hardware, then we want to or both
>> DRM_MODE_ROTATE_90 and DRM_MODE_ROTATE_0 into the rotations bitmask
>> (0 since the hardware rotation requires no sw rotation).
>> The rotations bitmask is the *combination* of all rotations we need
>> fbcon to do in software and when that ends up being more then one
>> rotation we chicken out and just don't do any software rotation,
>> maybe I should rename rotations to sw_rotations?
>>
>> A better real world example is a 90 degree rotated panel with
>> an external monitor, in that case for the panel we hit:
>>
>> 	if (!(rotation & valid_mask)) {
>> 		fb_helper->rotations |= rotation;
>> 		return;
>> 	}
>>
>> Or-ing in the panel's DRM_MODE_ROTATE_90.
>>
>> And for the monitor we hit:
>>
>> 	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
>> 		fb_helper->rotations |= rotation;
>> 		return;
>> 	}
>>
>> Or-ing in the monitors's DRM_MODE_ROTATE_0.
>>
>> So we end up with 2 bits set in fb_helper->rotations, hitting the
>> default in the switch case and picking FB_ROTATE_UR, since we cannot
>> satisfy both rotations in fbcon at the same time.
>>
>>
>>> Or you're doing some really funny control logic by oring in another value
>>> to hit the default case below which doesn't rotate anything. I think that
>>> should be done explicitly, by explicitly setting to rotation to ROTATE_0
>>> instead of this. Same for the check above.
>>
>> I hope my explanation above explains why I'm or-ing together rotations,
>> basically I want to detect if we need more then 1 type of software-rotation
>> in fbcon and in that case bail out and fallback to FB_ROTATE_UR.
> 
> Yeah I suspected this is what you're trying to do, but imo it's too clear.
> Can't we do an explicit check for this instead of uncommented magic

Erm, my patch contains a big(ish) comment above drm_setup_crtc_rotation which
tries to explain this already.
> that takes half an hour to understand? I'm thinking of
> 
> if (count_bits(fbb_helper->rotation)) {
> 	/* conflicting rotation requests on different connnectors, fall
> 	 * back to unrotated. */
> 
> 	 fb_helper->rotation = ROTATE_0
> }
> 
> Or something similar at the end of drm_setup_crtcs (plus maybe doing the
> resolve in there too). Right now that logic is split between 2 functions,
> and no comment explaining what's going on.
> 
> But not sure that's actually going to help with readability.

I was thinking of just changing the code setting the fbcon_rotate_hint
to something like this:

         switch (fb_helper->sw_rotations) {
         case DRM_MODE_ROTATE_0:
                 info->fbcon_rotate_hint = FB_ROTATE_UR;
                 break;
         case DRM_MODE_ROTATE_90:
                 info->fbcon_rotate_hint = FB_ROTATE_CCW;
                 break;
         case DRM_MODE_ROTATE_180:
                 info->fbcon_rotate_hint = FB_ROTATE_UD;
                 break;
         case DRM_MODE_ROTATE_270:
                 info->fbcon_rotate_hint = FB_ROTATE_CW;
                 break;
         default:
		/*
		 * Multiple bits are set / multiple rotations requested
		 * fbcon cannot handle separate rotation settings per
		 * output, so fallback to unrotated.
		 */
                 info->fbcon_rotate_hint = FB_ROTATE_UR;
         }

Would that work for you ?

Regards,

Hans

Re: [Intel-gfx] [PATCH v3 4/7] drm/fb-helper: Apply panel orientation connector prop to the primary

[Daniel Vetter]

On Tue, Oct 31, 2017 at 11:24:14AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 31-10-17 11:14, Daniel Vetter wrote:
> > On Mon, Oct 30, 2017 at 12:09:27PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 30-10-17 10:52, Daniel Vetter wrote:
> > > > On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
> > > > > Apply the "panel orientation" drm connector prop to the primary plane so
> > > > > that fbcon and fbdev using userspace programs display the right way up.
> > > > > 
> > > > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id”894
> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > ---
> > > > > Changes in v2:
> > > > > -New patch in v2 of this patch-set
> > > > > 
> > > > > Changes in v3:
> > > > > -Use a rotation member in struct drm_fb_helper_crtc and set that from
> > > > >    drm_setup_crtcs instead of looping over all crtc's to find the right one
> > > > >    later
> > > > > -Since we now no longer look at rotation quirks directly in the fbcon code,
> > > > >    set fb_info.fbcon_rotate_hint when the panel is not mounted upright and
> > > > >    we cannot use hardware rotation
> > > > > ---
> > > > >    drivers/gpu/drm/drm_fb_helper.c | 76 +++++++++++++++++++++++++++++++++++++++--
> > > > >    include/drm/drm_fb_helper.h     |  8 +++++
> > > > >    2 files changed, 82 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > > index 116d1f1337c7..e0f95f2cc52f 100644
> > > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > > @@ -41,6 +41,7 @@
> > > > >    #include <drm/drm_atomic.h>
> > > > >    #include <drm/drm_atomic_helper.h>
> > > > > +#include "drm_crtc_internal.h"
> > > > >    #include "drm_crtc_helper_internal.h"
> > > > >    static bool drm_fbdev_emulation = true;
> > > > > @@ -350,6 +351,7 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> > > > >    static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
> > > > >    {
> > > > >    	struct drm_device *dev = fb_helper->dev;
> > > > > +	struct drm_plane_state *plane_state;
> > > > >    	struct drm_plane *plane;
> > > > >    	struct drm_atomic_state *state;
> > > > >    	int i, ret;
> > > > > @@ -368,8 +370,6 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > > > >    retry:
> > > > >    	plane_mask = 0;
> > > > >    	drm_for_each_plane(plane, dev) {
> > > > > -		struct drm_plane_state *plane_state;
> > > > > -
> > > > >    		plane_state = drm_atomic_get_plane_state(state, plane);
> > > > >    		if (IS_ERR(plane_state)) {
> > > > >    			ret = PTR_ERR(plane_state);
> > > > > @@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
> > > > >    	for (i = 0; i < fb_helper->crtc_count; i++) {
> > > > >    		struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> > > > > +		struct drm_plane *primary = mode_set->crtc->primary;
> > > > > +
> > > > > +		/* Cannot fail as we've already gotten the plane state above */
> > > > > +		plane_state = drm_atomic_get_new_plane_state(state, primary);
> > > > > +		plane_state->rotation = fb_helper->crtc_info[i].rotation;
> > > > >    		ret = __drm_atomic_helper_set_config(mode_set, state);
> > > > >    		if (ret != 0)
> > > > > @@ -2334,6 +2339,57 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> > > > >    	return best_score;
> > > > >    }
> > > > > +/*
> > > > > + * This function checks if rotation is necessary because of panel orientation
> > > > > + * and if it is, if it is supported.
> > > > > + * If rotation is necessary and supported, its gets set in fb_crtc.rotation.
> > > > > + * If rotation is necessary but not supported, a DRM_MODE_ROTATE_* flag gets
> > > > > + * or-ed into fb_helper->rotations. In drm_setup_crtcs_fb() we check if only
> > > > > + * one bit is set and then we set fb_info.fbcon_rotate_hint to make fbcon do
> > > > > + * the unsupported rotation.
> > > > > + */
> > > > > +static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
> > > > > +				    struct drm_fb_helper_crtc *fb_crtc,
> > > > > +				    struct drm_connector *connector)
> > > > > +{
> > > > > +	struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
> > > > > +	uint64_t valid_mask = 0;
> > > > > +	int i, rotation;
> > > > > +
> > > > > +	fb_crtc->rotation = DRM_MODE_ROTATE_0;
> > > > > +
> > > > > +	switch (connector->display_info.panel_orientation) {
> > > > > +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> > > > > +		rotation = DRM_MODE_ROTATE_180;
> > > > > +		break;
> > > > > +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> > > > > +		rotation = DRM_MODE_ROTATE_90;
> > > > > +		break;
> > > > > +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> > > > > +		rotation = DRM_MODE_ROTATE_270;
> > > > > +		break;
> > > > 
> > > > For 90/270 hw rotation you need to flip the coordinates/sizes of the fb.
> > > 
> > > You're probably right, I don't have any hardware supporting
> > > 270 degree rotation to test this with.
> > > 
> > > > > +	default:
> > > > > +		rotation = DRM_MODE_ROTATE_0;
> > > > > +	}
> > > > > +
> > > > > +	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
> > > > > +		fb_helper->rotations |= rotation;
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	for (i = 0; i < plane->rotation_property->num_values; i++)
> > > > > +		valid_mask |= (1ULL << plane->rotation_property->values[i]);
> > > > 
> > > > This isn't a good enough check for atomic drivers (and not for gen9+ intel
> > > > hw), since we might expose 90° rotations, but it only works if you have
> > > > the correct tiling format.
> > > > 
> > > > For atomic drivers it'd be really good if we could do a TEST_ONLY commit
> > > > first, and if that fails, fall back to sw rotation.
> > > > 
> > > > But that poses a bit a chicken&egg with creating the framebuffer (we need
> > > > one for the TEST_ONLY), so probably a bit too much more for this. And
> > > > afaiui your quirk list only applies to older stuff.
> > > > 
> > > > At least add a FIXME meanwhile? In a way we have a FIXME already for
> > > > multi-pipe, since we don't try to fall back to fewer pipes if the single
> > > > atomic commit failed.
> > > > 
> > > > Or maybe just don't use 90/270 hw rotation for now since it seems buggy in
> > > > your code anyway.
> > > 
> > > I was wondering about this (need for special fb layout) myself too, I
> > > agree also given the above comment that it is probably best to only
> > > support 0/180 degree hardware rotation for now, I will do for the next
> > > version (and add a TODO comment).
> > 
> > It'll work on i915 at least, on current hw. Might still be broken on
> > other, but then that's a larger issue with the fbcon atomic code right
> > now.
> > > 
> > > 
> > > > > +
> > > > > +	if (!(rotation & valid_mask)) {
> > > > > +		fb_helper->rotations |= rotation;
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	fb_crtc->rotation = rotation;
> > > > > +	/* Rotating in hardware, fbcon should not rotate */
> > > > > +	fb_helper->rotations |= DRM_MODE_ROTATE_0;
> > > > 
> > > > Wrong bitopt I think.
> > > 
> > > No this is intentional, if we've a panel requiring say 90 degree rotation
> > > which we will do in software and another panel (weird example) doing 180
> > > degree rotation in hardware, then we want to or both
> > > DRM_MODE_ROTATE_90 and DRM_MODE_ROTATE_0 into the rotations bitmask
> > > (0 since the hardware rotation requires no sw rotation).
> > > The rotations bitmask is the *combination* of all rotations we need
> > > fbcon to do in software and when that ends up being more then one
> > > rotation we chicken out and just don't do any software rotation,
> > > maybe I should rename rotations to sw_rotations?
> > > 
> > > A better real world example is a 90 degree rotated panel with
> > > an external monitor, in that case for the panel we hit:
> > > 
> > > 	if (!(rotation & valid_mask)) {
> > > 		fb_helper->rotations |= rotation;
> > > 		return;
> > > 	}
> > > 
> > > Or-ing in the panel's DRM_MODE_ROTATE_90.
> > > 
> > > And for the monitor we hit:
> > > 
> > > 	if (rotation = DRM_MODE_ROTATE_0 || !plane->rotation_property) {
> > > 		fb_helper->rotations |= rotation;
> > > 		return;
> > > 	}
> > > 
> > > Or-ing in the monitors's DRM_MODE_ROTATE_0.
> > > 
> > > So we end up with 2 bits set in fb_helper->rotations, hitting the
> > > default in the switch case and picking FB_ROTATE_UR, since we cannot
> > > satisfy both rotations in fbcon at the same time.
> > > 
> > > 
> > > > Or you're doing some really funny control logic by oring in another value
> > > > to hit the default case below which doesn't rotate anything. I think that
> > > > should be done explicitly, by explicitly setting to rotation to ROTATE_0
> > > > instead of this. Same for the check above.
> > > 
> > > I hope my explanation above explains why I'm or-ing together rotations,
> > > basically I want to detect if we need more then 1 type of software-rotation
> > > in fbcon and in that case bail out and fallback to FB_ROTATE_UR.
> > 
> > Yeah I suspected this is what you're trying to do, but imo it's too clear.
> > Can't we do an explicit check for this instead of uncommented magic
> 
> Erm, my patch contains a big(ish) comment above drm_setup_crtc_rotation which
> tries to explain this already.
> > that takes half an hour to understand? I'm thinking of
> > 
> > if (count_bits(fbb_helper->rotation)) {
> > 	/* conflicting rotation requests on different connnectors, fall
> > 	 * back to unrotated. */
> > 
> > 	 fb_helper->rotation = ROTATE_0
> > }
> > 
> > Or something similar at the end of drm_setup_crtcs (plus maybe doing the
> > resolve in there too). Right now that logic is split between 2 functions,
> > and no comment explaining what's going on.
> > 
> > But not sure that's actually going to help with readability.
> 
> I was thinking of just changing the code setting the fbcon_rotate_hint
> to something like this:
> 
>         switch (fb_helper->sw_rotations) {
>         case DRM_MODE_ROTATE_0:
>                 info->fbcon_rotate_hint = FB_ROTATE_UR;
>                 break;
>         case DRM_MODE_ROTATE_90:
>                 info->fbcon_rotate_hint = FB_ROTATE_CCW;
>                 break;
>         case DRM_MODE_ROTATE_180:
>                 info->fbcon_rotate_hint = FB_ROTATE_UD;
>                 break;
>         case DRM_MODE_ROTATE_270:
>                 info->fbcon_rotate_hint = FB_ROTATE_CW;
>                 break;
>         default:
> 		/*
> 		 * Multiple bits are set / multiple rotations requested
> 		 * fbcon cannot handle separate rotation settings per
> 		 * output, so fallback to unrotated.
> 		 */
>                 info->fbcon_rotate_hint = FB_ROTATE_UR;
>         }
> 
> Would that work for you ?

Ack.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 5/7] drm/i915: Add "panel orientation" property to the panel connector

[Hans de Goede]

Ideally we could use the VBT for this, that would be simple, in
intel_dsi_init() check dev_priv->vbt.dsi.config->rotation, set
connector->display_info.panel_orientation accordingly and call
drm_connector_init_panel_orientation_property(), done.

Unfortunately vbt.dsi.config->rotation is always 0 even on tablets
with an upside down LCD and where the GOP is properly rotating the
EFI fb in hardware.

So instead we end up reading the rotation from the primary plane.
To read the info from the primary plane, we need to know which crtc
the panel is hooked up to, so we do this the first time the panel
encoder's get_config function get called, as by then the encoder
crtc routing has been set up.

This commit only implements the panel orientation property for DSI
panels on BYT / CHT / BXT hardware, as all known non normal oriented
panels are only found on this hardware.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Read back the rotation applied by the GOP from the primary plane
 instead of relying on dev_priv->vbt.dsi.config->rotation, because it
 seems that the VBT rotation filed is always 0 even on devices where the
 GOP does apply a rotation

Changes in v3:
-Rewrite the code to read back the orientation from the primary
 plane to contain all of this in intel_dsi.c instead of poking a bunch
 of holes between all the different layers
---
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_dsi.c   | 48 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dsi.h   |  2 ++
 drivers/gpu/drm/i915/intel_panel.c | 16 +++++++++++++
 4 files changed, 67 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a05ab3a1ab27..29fb7a414bbe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1728,6 +1728,7 @@ void intel_panel_enable_backlight(const struct intel_crtc_state *crtc_state,
 				  const struct drm_connector_state *conn_state);
 void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state);
 void intel_panel_destroy_backlight(struct drm_connector *connector);
+void intel_panel_set_orientation(struct intel_panel *panel, int orientation);
 enum drm_connector_status intel_panel_detect(struct drm_i915_private *dev_priv);
 extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 66bbedc5fa01..86914d2f9f6a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -1084,13 +1084,16 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
 	struct drm_display_mode *adjusted_mode_sw;
 	struct intel_crtc *intel_crtc;
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	struct intel_panel *panel = &intel_dsi->attached_connector->panel;
 	unsigned int lane_count = intel_dsi->lane_count;
 	unsigned int bpp, fmt;
+	int orientation;
 	enum port port;
 	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
 	u16 hfp_sw, hsync_sw, hbp_sw;
 	u16 crtc_htotal_sw, crtc_hsync_start_sw, crtc_hsync_end_sw,
 				crtc_hblank_start_sw, crtc_hblank_end_sw;
+	u32 val;
 
 	/* FIXME: hw readout should not depend on SW state */
 	intel_crtc = to_intel_crtc(encoder->base.crtc);
@@ -1234,6 +1237,49 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
 	if (adjusted_mode->crtc_hblank_end = crtc_hblank_end_sw)
 		adjusted_mode->crtc_hblank_end  					adjusted_mode_sw->crtc_hblank_end;
+
+	if (!intel_dsi->got_panel_orientation) {
+		val = I915_READ(PLANE_CTL(intel_crtc->pipe, 0));
+		/* The rotation is used to correct for the panel orientation */
+		switch (val & PLANE_CTL_ROTATE_MASK) {
+		case PLANE_CTL_ROTATE_0:
+			orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
+			break;
+		case PLANE_CTL_ROTATE_90:
+			orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+			break;
+		case PLANE_CTL_ROTATE_180:
+			orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+			break;
+		case PLANE_CTL_ROTATE_270:
+			orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+			break;
+		}
+		intel_panel_set_orientation(panel, orientation);
+		intel_dsi->got_panel_orientation = true;
+	}
+}
+
+static void vlv_dsi_get_pipe_config(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	struct intel_panel *panel = &intel_dsi->attached_connector->panel;
+	int orientation;
+	u32 val;
+
+	if (!intel_dsi->got_panel_orientation) {
+		val = I915_READ(DSPCNTR(intel_crtc->plane));
+
+		if (val & DISPPLANE_ROTATE_180)
+			orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+		else
+			orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
+
+		intel_panel_set_orientation(panel, orientation);
+		intel_dsi->got_panel_orientation = true;
+	}
 }
 
 static void intel_dsi_get_config(struct intel_encoder *encoder,
@@ -1245,6 +1291,8 @@ static void intel_dsi_get_config(struct intel_encoder *encoder,
 
 	if (IS_GEN9_LP(dev_priv))
 		bxt_dsi_get_pipe_config(encoder, pipe_config);
+	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		vlv_dsi_get_pipe_config(encoder);
 
 	pclk = intel_dsi_get_pclk(encoder, pipe_config->pipe_bpp,
 				  pipe_config);
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 7afeb9580f41..92c2fe31f3fd 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -52,6 +52,8 @@ struct intel_dsi {
 	/* if true, use HS mode, otherwise LP */
 	bool hs;
 
+	bool got_panel_orientation;
+
 	/* virtual channel */
 	int channel;
 
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 37e6211c1f25..536e40a892cb 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1845,6 +1845,22 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
 	panel->backlight.present = false;
 }
 
+void intel_panel_set_orientation(struct intel_panel *panel, int orientation)
+{
+	struct intel_connector *panel_conn;
+	int width = 0, height = 0;
+
+	if (panel->fixed_mode) {
+		width = panel->fixed_mode->hdisplay;
+		height = panel->fixed_mode->vdisplay;
+	}
+
+	panel_conn = container_of(panel, struct intel_connector, panel);
+	panel_conn->base.display_info.panel_orientation = orientation;
+	drm_connector_init_panel_orientation_property(&panel_conn->base,
+						      width, height);
+}
+
 /* Set up chip specific backlight functions */
 static void
 intel_panel_init_backlight_funcs(struct intel_panel *panel)
-- 
2.14.2

[PATCH v3 6/7] efifb: Set info->fbcon_rotate_hint based on drm_get_panel_orientation_quirk

[Hans de Goede]

On some hardware the LCD panel is not mounted upright in the casing,
but rotated by 90 degrees. In this case we want the console to
automatically be rotated to compensate.

The drm subsys has a quirk table for this, use the
drm_get_panel_orientation_quirk function to get the panel orientation
and set info->fbcon_rotate_hint based on this, so that the fbcon console
on top of efifb gets automatically rotated to compensate for the panel
orientation.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/Kconfig |  1 +
 drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 5e58f5ec0a28..c4a90c497839 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -772,6 +772,7 @@ config FB_VESA
 config FB_EFI
 	bool "EFI-based Framebuffer Support"
 	depends on (FB = y) && !IA64 && EFI
+	select DRM_PANEL_ORIENTATION_QUIRKS
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 3a010641f630..8c7f6aeee205 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -15,6 +15,8 @@
 #include <linux/screen_info.h>
 #include <video/vga.h>
 #include <asm/efi.h>
+#include <drm/drm_utils.h> /* For drm_get_panel_orientation_quirk */
+#include <drm/drm_mode.h>  /* For DRM_MODE_PANEL_ORIENTATION_* */
 
 static bool request_mem_succeeded = false;
 static bool nowc = false;
@@ -156,7 +158,7 @@ static u64 bar_offset;
 static int efifb_probe(struct platform_device *dev)
 {
 	struct fb_info *info;
-	int err;
+	int err, orientation;
 	unsigned int size_vmode;
 	unsigned int size_remap;
 	unsigned int size_total;
@@ -328,6 +330,23 @@ static int efifb_probe(struct platform_device *dev)
 	info->fix = efifb_fix;
 	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE;
 
+	orientation = drm_get_panel_orientation_quirk(efifb_defined.xres,
+						      efifb_defined.yres);
+	switch (orientation) {
+	default:
+		info->fbcon_rotate_hint = FB_ROTATE_UR;
+		break;
+	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
+		info->fbcon_rotate_hint = FB_ROTATE_UD;
+		break;
+	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
+		info->fbcon_rotate_hint = FB_ROTATE_CCW;
+		break;
+	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
+		info->fbcon_rotate_hint = FB_ROTATE_CW;
+		break;
+	}
+
 	err = sysfs_create_groups(&dev->dev.kobj, efifb_groups);
 	if (err) {
 		pr_err("efifb: cannot add sysfs attrs\n");
-- 
2.14.2

Re: [Intel-gfx] [PATCH v3 6/7] efifb: Set info->fbcon_rotate_hint based on drm_get_panel_orientation

[Daniel Vetter]

On Mon, Oct 23, 2017 at 09:14:24AM +0200, Hans de Goede wrote:
> On some hardware the LCD panel is not mounted upright in the casing,
> but rotated by 90 degrees. In this case we want the console to
> automatically be rotated to compensate.
> 
> The drm subsys has a quirk table for this, use the
> drm_get_panel_orientation_quirk function to get the panel orientation
> and set info->fbcon_rotate_hint based on this, so that the fbcon console
> on top of efifb gets automatically rotated to compensate for the panel
> orientation.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/video/fbdev/Kconfig |  1 +
>  drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 5e58f5ec0a28..c4a90c497839 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -772,6 +772,7 @@ config FB_VESA
>  config FB_EFI
>  	bool "EFI-based Framebuffer Support"
>  	depends on (FB = y) && !IA64 && EFI
> +	select DRM_PANEL_ORIENTATION_QUIRKS
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 3a010641f630..8c7f6aeee205 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -15,6 +15,8 @@
>  #include <linux/screen_info.h>
>  #include <video/vga.h>
>  #include <asm/efi.h>
> +#include <drm/drm_utils.h> /* For drm_get_panel_orientation_quirk */
> +#include <drm/drm_mode.h>  /* For DRM_MODE_PANEL_ORIENTATION_* */
>  
>  static bool request_mem_succeeded = false;
>  static bool nowc = false;
> @@ -156,7 +158,7 @@ static u64 bar_offset;
>  static int efifb_probe(struct platform_device *dev)
>  {
>  	struct fb_info *info;
> -	int err;
> +	int err, orientation;
>  	unsigned int size_vmode;
>  	unsigned int size_remap;
>  	unsigned int size_total;
> @@ -328,6 +330,23 @@ static int efifb_probe(struct platform_device *dev)
>  	info->fix = efifb_fix;
>  	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE;
>  
> +	orientation = drm_get_panel_orientation_quirk(efifb_defined.xres,
> +						      efifb_defined.yres);

Oh right, that's the reason for the separate function. Still ugh.

Maybe add a comment in the kernel-doc for why it is what it is ...
-Daniel

> +	switch (orientation) {
> +	default:
> +		info->fbcon_rotate_hint = FB_ROTATE_UR;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
> +		info->fbcon_rotate_hint = FB_ROTATE_UD;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
> +		info->fbcon_rotate_hint = FB_ROTATE_CCW;
> +		break;
> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
> +		info->fbcon_rotate_hint = FB_ROTATE_CW;
> +		break;
> +	}
> +
>  	err = sysfs_create_groups(&dev->dev.kobj, efifb_groups);
>  	if (err) {
>  		pr_err("efifb: cannot add sysfs attrs\n");
> -- 
> 2.14.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v3 6/7] efifb: Set info->fbcon_rotate_hint based on drm_get_panel_orientation

[Hans de Goede]

Hi,

On 30-10-17 10:53, Daniel Vetter wrote:
> On Mon, Oct 23, 2017 at 09:14:24AM +0200, Hans de Goede wrote:
>> On some hardware the LCD panel is not mounted upright in the casing,
>> but rotated by 90 degrees. In this case we want the console to
>> automatically be rotated to compensate.
>>
>> The drm subsys has a quirk table for this, use the
>> drm_get_panel_orientation_quirk function to get the panel orientation
>> and set info->fbcon_rotate_hint based on this, so that the fbcon console
>> on top of efifb gets automatically rotated to compensate for the panel
>> orientation.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/video/fbdev/Kconfig |  1 +
>>   drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 5e58f5ec0a28..c4a90c497839 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -772,6 +772,7 @@ config FB_VESA
>>   config FB_EFI
>>   	bool "EFI-based Framebuffer Support"
>>   	depends on (FB = y) && !IA64 && EFI
>> +	select DRM_PANEL_ORIENTATION_QUIRKS
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
>> index 3a010641f630..8c7f6aeee205 100644
>> --- a/drivers/video/fbdev/efifb.c
>> +++ b/drivers/video/fbdev/efifb.c
>> @@ -15,6 +15,8 @@
>>   #include <linux/screen_info.h>
>>   #include <video/vga.h>
>>   #include <asm/efi.h>
>> +#include <drm/drm_utils.h> /* For drm_get_panel_orientation_quirk */
>> +#include <drm/drm_mode.h>  /* For DRM_MODE_PANEL_ORIENTATION_* */
>>   
>>   static bool request_mem_succeeded = false;
>>   static bool nowc = false;
>> @@ -156,7 +158,7 @@ static u64 bar_offset;
>>   static int efifb_probe(struct platform_device *dev)
>>   {
>>   	struct fb_info *info;
>> -	int err;
>> +	int err, orientation;
>>   	unsigned int size_vmode;
>>   	unsigned int size_remap;
>>   	unsigned int size_total;
>> @@ -328,6 +330,23 @@ static int efifb_probe(struct platform_device *dev)
>>   	info->fix = efifb_fix;
>>   	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE;
>>   
>> +	orientation = drm_get_panel_orientation_quirk(efifb_defined.xres,
>> +						      efifb_defined.yres);
> 
> Oh right, that's the reason for the separate function. Still ugh.

Ack.

> Maybe add a comment in the kernel-doc for why it is what it is ...

Ok I will add a comment to the kernel-doc for drm_get_panel_orientation_quirk()
in v4 of this patch-set.

Regards,

Hans



> -Daniel
> 
>> +	switch (orientation) {
>> +	default:
>> +		info->fbcon_rotate_hint = FB_ROTATE_UR;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
>> +		info->fbcon_rotate_hint = FB_ROTATE_UD;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
>> +		info->fbcon_rotate_hint = FB_ROTATE_CCW;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
>> +		info->fbcon_rotate_hint = FB_ROTATE_CW;
>> +		break;
>> +	}
>> +
>>   	err = sysfs_create_groups(&dev->dev.kobj, efifb_groups);
>>   	if (err) {
>>   		pr_err("efifb: cannot add sysfs attrs\n");
>> -- 
>> 2.14.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [Intel-gfx] [PATCH v3 6/7] efifb: Set info->fbcon_rotate_hint based on drm_get_panel_orientation

[Hans de Goede]

Hi,

On 30-10-17 10:53, Daniel Vetter wrote:
> On Mon, Oct 23, 2017 at 09:14:24AM +0200, Hans de Goede wrote:
>> On some hardware the LCD panel is not mounted upright in the casing,
>> but rotated by 90 degrees. In this case we want the console to
>> automatically be rotated to compensate.
>>
>> The drm subsys has a quirk table for this, use the
>> drm_get_panel_orientation_quirk function to get the panel orientation
>> and set info->fbcon_rotate_hint based on this, so that the fbcon console
>> on top of efifb gets automatically rotated to compensate for the panel
>> orientation.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/video/fbdev/Kconfig |  1 +
>>   drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 5e58f5ec0a28..c4a90c497839 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -772,6 +772,7 @@ config FB_VESA
>>   config FB_EFI
>>   	bool "EFI-based Framebuffer Support"
>>   	depends on (FB = y) && !IA64 && EFI
>> +	select DRM_PANEL_ORIENTATION_QUIRKS
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
>> index 3a010641f630..8c7f6aeee205 100644
>> --- a/drivers/video/fbdev/efifb.c
>> +++ b/drivers/video/fbdev/efifb.c
>> @@ -15,6 +15,8 @@
>>   #include <linux/screen_info.h>
>>   #include <video/vga.h>
>>   #include <asm/efi.h>
>> +#include <drm/drm_utils.h> /* For drm_get_panel_orientation_quirk */
>> +#include <drm/drm_mode.h>  /* For DRM_MODE_PANEL_ORIENTATION_* */
>>   
>>   static bool request_mem_succeeded = false;
>>   static bool nowc = false;
>> @@ -156,7 +158,7 @@ static u64 bar_offset;
>>   static int efifb_probe(struct platform_device *dev)
>>   {
>>   	struct fb_info *info;
>> -	int err;
>> +	int err, orientation;
>>   	unsigned int size_vmode;
>>   	unsigned int size_remap;
>>   	unsigned int size_total;
>> @@ -328,6 +330,23 @@ static int efifb_probe(struct platform_device *dev)
>>   	info->fix = efifb_fix;
>>   	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE;
>>   
>> +	orientation = drm_get_panel_orientation_quirk(efifb_defined.xres,
>> +						      efifb_defined.yres);
> 
> Oh right, that's the reason for the separate function. Still ugh.
> 
> Maybe add a comment in the kernel-doc for why it is what it is ...

Actually the drm_get_panel_orientation_quirk() function was missing
a kernel-doc comment documenting it altogether. For v5 of this
patch-set I've added a kernel-doc comment and I've added this bit
to that kernel-doc comment to explain why drm_get_panel_orientation_quirk()
lives in a separate .ko target:

  * Note this function is also used outside of the drm-subsys, by for example
  * the efifb code. Because of this this function gets compiled into its own
  * kernel-module when built as a module.

Regards,

Hans



> -Daniel
> 
>> +	switch (orientation) {
>> +	default:
>> +		info->fbcon_rotate_hint = FB_ROTATE_UR;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:
>> +		info->fbcon_rotate_hint = FB_ROTATE_UD;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_LEFT_UP:
>> +		info->fbcon_rotate_hint = FB_ROTATE_CCW;
>> +		break;
>> +	case DRM_MODE_PANEL_ORIENTATION_RIGHT_UP:
>> +		info->fbcon_rotate_hint = FB_ROTATE_CW;
>> +		break;
>> +	}
>> +
>>   	err = sysfs_create_groups(&dev->dev.kobj, efifb_groups);
>>   	if (err) {
>>   		pr_err("efifb: cannot add sysfs attrs\n");
>> -- 
>> 2.14.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[PATCH v3 7/7] fbcon: Remove dmi quirk table

[Hans de Goede]

This is now all handled in the drivers and communicated through
fb_info.fbcon_rotate_hint.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/core/Makefile           |   3 -
 drivers/video/fbdev/core/fbcon.c            |   4 +-
 drivers/video/fbdev/core/fbcon.h            |   6 --
 drivers/video/fbdev/core/fbcon_dmi_quirks.c | 145 ----------------------------
 4 files changed, 2 insertions(+), 156 deletions(-)
 delete mode 100644 drivers/video/fbdev/core/fbcon_dmi_quirks.c

diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 73493bbd7a15..0214b863ac3f 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -14,9 +14,6 @@ ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE_ROTATION),y)
 fb-y				  += fbcon_rotate.o fbcon_cw.o fbcon_ud.o \
 				     fbcon_ccw.o
 endif
-ifeq ($(CONFIG_DMI),y)
-fb-y				  += fbcon_dmi_quirks.o
-endif
 endif
 fb-objs                           := $(fb-y)
 
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index fb317ed76b45..157a40670a47 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -968,7 +968,7 @@ static const char *fbcon_startup(void)
 	if (p->con_rotate = -1)
 		p->con_rotate = info->fbcon_rotate_hint;
 	if (p->con_rotate = -1)
-		p->con_rotate = fbcon_platform_get_rotate(info);
+		p->con_rotate = FB_ROTATE_UR;
 
 	set_blitting_type(vc, info);
 
@@ -1111,7 +1111,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 	if (p->con_rotate = -1)
 		p->con_rotate = info->fbcon_rotate_hint;
 	if (p->con_rotate = -1)
-		p->con_rotate = fbcon_platform_get_rotate(info);
+		p->con_rotate = FB_ROTATE_UR;
 
 	set_blitting_type(vc, info);
 
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index 18f3ac144237..3746828356ed 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -261,10 +261,4 @@ extern void fbcon_set_rotate(struct fbcon_ops *ops);
 #define fbcon_set_rotate(x) do {} while(0)
 #endif /* CONFIG_FRAMEBUFFER_CONSOLE_ROTATION */
 
-#ifdef CONFIG_DMI
-int fbcon_platform_get_rotate(struct fb_info *info);
-#else
-#define fbcon_platform_get_rotate(i) FB_ROTATE_UR
-#endif /* CONFIG_DMI */
-
 #endif /* _VIDEO_FBCON_H */
diff --git a/drivers/video/fbdev/core/fbcon_dmi_quirks.c b/drivers/video/fbdev/core/fbcon_dmi_quirks.c
deleted file mode 100644
index 6904e47d1e51..000000000000
--- a/drivers/video/fbdev/core/fbcon_dmi_quirks.c
+++ /dev/null
@@ -1,145 +0,0 @@
-/*
- *  fbcon_dmi_quirks.c -- DMI based quirk detection for fbcon
- *
- *	Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
- *
- *  This file is subject to the terms and conditions of the GNU General Public
- *  License.  See the file COPYING in the main directory of this archive for
- *  more details.
- */
-
-#include <linux/dmi.h>
-#include <linux/fb.h>
-#include <linux/kernel.h>
-#include "fbcon.h"
-
-/*
- * Some x86 clamshell design devices use portrait tablet screens and a display
- * engine which cannot rotate in hardware, so we need to rotate the fbcon to
- * compensate. Unfortunately these (cheap) devices also typically have quite
- * generic DMI data, so we match on a combination of DMI data, screen resolution
- * and a list of known BIOS dates to avoid false positives.
- */
-
-struct fbcon_dmi_rotate_data {
-	int width;
-	int height;
-	const char * const *bios_dates;
-	int rotate;
-};
-
-static const struct fbcon_dmi_rotate_data rotate_data_asus_t100ha = {
-	.width = 800,
-	.height = 1280,
-	.rotate = FB_ROTATE_CCW,
-};
-
-static const struct fbcon_dmi_rotate_data rotate_data_gpd_pocket = {
-	.width = 1200,
-	.height = 1920,
-	.bios_dates = (const char * const []){ "05/26/2017", "06/28/2017",
-		"07/05/2017", "08/07/2017", NULL },
-	.rotate = FB_ROTATE_CW,
-};
-
-static const struct fbcon_dmi_rotate_data rotate_data_gpd_win = {
-	.width = 720,
-	.height = 1280,
-	.bios_dates = (const char * const []){
-		"10/25/2016", "11/18/2016", "12/23/2016", "12/26/2016",
-		"02/21/2017", "03/20/2017", "05/25/2017", NULL },
-	.rotate = FB_ROTATE_CW,
-};
-
-static const struct fbcon_dmi_rotate_data rotate_data_itworks_tw891 = {
-	.width = 800,
-	.height = 1280,
-	.bios_dates = (const char * const []){ "10/16/2015", NULL },
-	.rotate = FB_ROTATE_CW,
-};
-
-static const struct fbcon_dmi_rotate_data rotate_data_vios_lth17 = {
-	.width = 800,
-	.height = 1280,
-	.rotate = FB_ROTATE_CW,
-};
-
-static const struct dmi_system_id rotate_data[] = {
-	{	/* Asus T100HA */
-		.matches = {
-		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "T100HAN"),
-		},
-		.driver_data = (void *)&rotate_data_asus_t100ha,
-	}, {	/*
-		 * GPD Pocket, note that the the DMI data is less generic then
-		 * it seems, devices with a board-vendor of "AMI Corporation"
-		 * are quite rare, as are devices which have both board- *and*
-		 * product-id set to "Default String"
-		 */
-		.matches = {
-		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
-		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
-		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
-		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
-		},
-		.driver_data = (void *)&rotate_data_gpd_pocket,
-	}, {	/* GPD Win (same note on DMI match as GPD Pocket) */
-		.matches = {
-		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
-		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "Default string"),
-		  DMI_EXACT_MATCH(DMI_BOARD_SERIAL, "Default string"),
-		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Default string"),
-		},
-		.driver_data = (void *)&rotate_data_gpd_win,
-	}, {	/* I.T.Works TW891 */
-		.matches = {
-		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "To be filled by O.E.M."),
-		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "TW891"),
-		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "To be filled by O.E.M."),
-		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "TW891"),
-		},
-		.driver_data = (void *)&rotate_data_itworks_tw891,
-	}, {	/* VIOS LTH17 */
-		.matches = {
-		  DMI_EXACT_MATCH(DMI_SYS_VENDOR, "VIOS"),
-		  DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "LTH17"),
-		  DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "VIOS"),
-		  DMI_EXACT_MATCH(DMI_BOARD_NAME, "LTH17"),
-		},
-		.driver_data = (void *)&rotate_data_vios_lth17,
-	},
-	{}
-};
-
-int fbcon_platform_get_rotate(struct fb_info *info)
-{
-	const struct dmi_system_id *match;
-	const struct fbcon_dmi_rotate_data *data;
-	const char *bios_date;
-	int i;
-
-	for (match = dmi_first_match(rotate_data);
-	     match;
-	     match = dmi_first_match(match + 1)) {
-		data = match->driver_data;
-
-		if (data->width != info->var.xres ||
-		    data->height != info->var.yres)
-			continue;
-
-		if (!data->bios_dates)
-			return data->rotate;
-
-		bios_date = dmi_get_system_info(DMI_BIOS_DATE);
-		if (!bios_date)
-			continue;
-
-		for (i = 0; data->bios_dates[i]; i++) {
-			if (!strcmp(data->bios_dates[i], bios_date))
-				return data->rotate;
-		}
-	}
-
-	return FB_ROTATE_UR;
-}
-- 
2.14.2

Re: [Intel-gfx] [PATCH v3 0/7] drm/fbdev: Panel orientation connector property support

[Daniel Vetter]

On Mon, Oct 23, 2017 at 09:14:18AM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v3 of my series to add a "panel orientation" property to
> the drm-connector for the LCD panel to let userspace know about LCD
> panels which are not mounted upright, as well as detecting upside-down
> panels without needing quirks (like we do for 90 degree rotated screens).
> 
> As requested by Daniel this version moves the quirks over from the fbdev
> subsys to the drm subsys. I've done this by simpy starting with a copy of
> the quirk table and eventually removing the fbdev version.

I think this design makes much more sense. Some nits I spotted and a few
ideas, but otherwise lgtm.

> The 1st patch in this series is a small fbdev/fbcon patch, patches 2-5
> are all drm patches since patches 2-5 depend on patch 1 I believe it
> would be best to merge patches 1-5 through the drm tree.
> 
> For merging patches 6-7 I see 3 options:
> 
> 1) Wait a kernel cycle, things will work fine without them, they are really
> just there to remove the fbdev copy of the quirks
> 
> 2) Merge all 7 patches through the drm tree

My preference. We can do a topic branch that Bartlomiej pulls into fbdev
so that the patches are in both trees. We need them anyway in both for
testing I'd say.

> 3) Use a stable tag in the drm tree which the fbdev tree can merge and then
> merge patches 6-7 through the drm tree.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch