From: Hans de Goede <hidden> Date: 2017-10-23 07:14:18
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
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:19
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 <redacted>
---
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(-)
@@ -464,6 +464,11 @@ struct fb_info {atomic_tcount;intnode;intflags;+/*+*-1bydefault,settoaFB_ROTATE_*valuebythedriver,ifitknows+*alcdisnotmounteduprightandfbconshouldrotatetocompensate.+*/+intfbcon_rotate_hint;structmutexlock;/* Lock for open/release/ioctl funcs */structmutexmm_lock;/* Lock for fb_mmap and smem_* fields */structfb_var_screeninfovar;/* Current var */
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:20
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 <redacted>
---
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
@@ -0,0 +1,157 @@+/*+*drm_panel_orientation_quirks.c--Quirksfornon-normalpanelorientation+*+*Copyright(C)2017HansdeGoede<hdegoede@redhat.com>+*+*ThisfileissubjecttothetermsandconditionsoftheGNUGeneralPublic+*License.SeethefileCOPYINGinthemaindirectoryofthisarchivefor+*moredetails.+*/++#include<linux/dmi.h>+#include<drm/drm_connector.h>++#ifdef CONFIG_DMI++/*+*Somex86clamshelldesigndevicesuseportraittabletscreensandadisplay+*enginewhichcannotrotateinhardware,soweneedtorotatethefbconto+*compensate.Unfortunatelythese(cheap)devicesalsotypicallyhavequite+*genericDMIdata,sowematchonacombinationofDMIdata,screenresolution+*andalistofknownBIOSdatestoavoidfalsepositives.+*/++structdrm_dmi_panel_orientation_data{+intwidth;+intheight;+constchar*const*bios_dates;+intorientation;+};++staticconststructdrm_dmi_panel_orientation_dataasus_t100ha={+.width=800,+.height=1280,+.orientation=DRM_MODE_PANEL_ORIENTATION_LEFT_UP,+};++staticconststructdrm_dmi_panel_orientation_datagpd_pocket={+.width=1200,+.height=1920,+.bios_dates=(constchar*const[]){"05/26/2017","06/28/2017",+"07/05/2017","08/07/2017",NULL},+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdrm_dmi_panel_orientation_datagpd_win={+.width=720,+.height=1280,+.bios_dates=(constchar*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,+};++staticconststructdrm_dmi_panel_orientation_dataitworks_tw891={+.width=800,+.height=1280,+.bios_dates=(constchar*const[]){"10/16/2015",NULL},+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdrm_dmi_panel_orientation_datavios_lth17={+.width=800,+.height=1280,+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdmi_system_idorientation_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,+},{/*+*GPDPocket,notethatthetheDMIdataislessgenericthen+*itseems,deviceswithaboard-vendorof"AMI Corporation"+*arequiterare,asaredeviceswhichhavebothboard-*and*+*product-idsetto"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,+},+{}+};++intdrm_get_panel_orientation_quirk(intwidth,intheight)+{+conststructdmi_system_id*match;+conststructdrm_dmi_panel_orientation_data*data;+constchar*bios_date;+inti;++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)+returndata->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))+returndata->orientation;+}+}++returnDRM_MODE_PANEL_ORIENTATION_UNKNOWN;+}+EXPORT_SYMBOL(drm_get_panel_orientation_quirk);++#else++/* There are no quirks for non x86 devices yet */+intdrm_get_panel_orientation_quirk(intwidth,intheight)+{+returnDRM_MODE_PANEL_ORIENTATION_UNKNOWN;+}+EXPORT_SYMBOL(drm_get_panel_orientation_quirk);++#endif
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:21
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 <redacted>
---
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(+)
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:22
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 <redacted>
---
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(-)
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activfor(i=0;i<fb_helper->crtc_count;i++){structdrm_mode_set*mode_set=&fb_helper->crtc_info[i].mode_set;+structdrm_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,returnbest_score;}+/*+*Thisfunctionchecksifrotationisnecessarybecauseofpanelorientation+*andifitis,ifitissupported.+*Ifrotationisnecessaryandsupported,itsgetssetinfb_crtc.rotation.+*Ifrotationisnecessarybutnotsupported,aDRM_MODE_ROTATE_*flaggets+*or-edintofb_helper->rotations.Indrm_setup_crtcs_fb()wecheckifonly+*onebitissetandthenwesetfb_info.fbcon_rotate_hinttomakefbcondo+*theunsupportedrotation.+*/+staticvoiddrm_setup_crtc_rotation(structdrm_fb_helper*fb_helper,+structdrm_fb_helper_crtc*fb_crtc,+structdrm_connector*connector)+{+structdrm_plane*plane=fb_crtc->mode_set.crtc->primary;+uint64_tvalid_mask=0;+inti,rotation;++fb_crtc->rotation=DRM_MODE_ROTATE_0;++switch(connector->display_info.panel_orientation){+caseDRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:+rotation=DRM_MODE_ROTATE_180;+break;+caseDRM_MODE_PANEL_ORIENTATION_LEFT_UP:+rotation=DRM_MODE_ROTATE_90;+break;+caseDRM_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;+}+staticvoiddrm_setup_crtcs(structdrm_fb_helper*fb_helper,u32width,u32height){
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:23
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 <redacted>
---
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(+)
@@ -1084,13 +1084,16 @@ static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,structdrm_display_mode*adjusted_mode_sw;structintel_crtc*intel_crtc;structintel_dsi*intel_dsi=enc_to_intel_dsi(&encoder->base);+structintel_panel*panel=&intel_dsi->attached_connector->panel;unsignedintlane_count=intel_dsi->lane_count;unsignedintbpp,fmt;+intorientation;enumportport;u16hactive,hfp,hsync,hbp,vfp,vsync,vbp;u16hfp_sw,hsync_sw,hbp_sw;u16crtc_htotal_sw,crtc_hsync_start_sw,crtc_hsync_end_sw,crtc_hblank_start_sw,crtc_hblank_end_sw;+u32val;/* 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_endadjusted_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){+casePLANE_CTL_ROTATE_0:+orientation=DRM_MODE_PANEL_ORIENTATION_NORMAL;+break;+casePLANE_CTL_ROTATE_90:+orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;+break;+casePLANE_CTL_ROTATE_180:+orientation=DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;+break;+casePLANE_CTL_ROTATE_270:+orientation=DRM_MODE_PANEL_ORIENTATION_LEFT_UP;+break;+}+intel_panel_set_orientation(panel,orientation);+intel_dsi->got_panel_orientation=true;+}+}++staticvoidvlv_dsi_get_pipe_config(structintel_encoder*encoder)+{+structdrm_i915_private*dev_priv=to_i915(encoder->base.dev);+structintel_crtc*intel_crtc=to_intel_crtc(encoder->base.crtc);+structintel_dsi*intel_dsi=enc_to_intel_dsi(&encoder->base);+structintel_panel*panel=&intel_dsi->attached_connector->panel;+intorientation;+u32val;++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;+}}staticvoidintel_dsi_get_config(structintel_encoder*encoder,
@@ -1845,6 +1845,22 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)panel->backlight.present=false;}+voidintel_panel_set_orientation(structintel_panel*panel,intorientation)+{+structintel_connector*panel_conn;+intwidth=0,height=0;++if(panel->fixed_mode){+width=panel->fixed_mode->hdisplay;+height=panel->fixed_mode->vdisplay;+}++panel_conn=container_of(panel,structintel_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 */staticvoidintel_panel_init_backlight_funcs(structintel_panel*panel)
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:24
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 <redacted>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
@@ -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_* */staticboolrequest_mem_succeeded=false;staticboolnowc=false;
From: Hans de Goede <hidden> Date: 2017-10-23 07:14:25
This is now all handled in the drivers and communicated through
fb_info.fbcon_rotate_hint.
Signed-off-by: Hans de Goede <redacted>
---
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
From: Sebastian Reichel <hidden> Date: 2017-10-23 12:43:26
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 <redacted>
---
Thanks for your work. I will give it a try with Droid 4 and N950
once I find some time :)
[...]
quoted hunk
+ 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);
[...]
quoted hunk
+ 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
From: Hans de Goede <hidden> Date: 2017-10-23 13:54:25
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:
quoted
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 <redacted>
---
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.
[...]
quoted
+ 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);
[...]
quoted
+ 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
From: Daniel Vetter <hidden> Date: 2017-10-30 09:39:11
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 <redacted>
Found a few organizational bikesheds below, but makes sense I think.
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.
@@ -0,0 +1,157 @@+/*+*drm_panel_orientation_quirks.c--Quirksfornon-normalpanelorientation+*+*Copyright(C)2017HansdeGoede<hdegoede@redhat.com>+*+*ThisfileissubjecttothetermsandconditionsoftheGNUGeneralPublic+*License.SeethefileCOPYINGinthemaindirectoryofthisarchivefor+*moredetails.+*/++#include<linux/dmi.h>+#include<drm/drm_connector.h>++#ifdef CONFIG_DMI++/*+*Somex86clamshelldesigndevicesuseportraittabletscreensandadisplay+*enginewhichcannotrotateinhardware,soweneedtorotatethefbconto+*compensate.Unfortunatelythese(cheap)devicesalsotypicallyhavequite+*genericDMIdata,sowematchonacombinationofDMIdata,screenresolution+*andalistofknownBIOSdatestoavoidfalsepositives.+*/++structdrm_dmi_panel_orientation_data{+intwidth;+intheight;+constchar*const*bios_dates;+intorientation;+};++staticconststructdrm_dmi_panel_orientation_dataasus_t100ha={+.width=800,+.height=1280,+.orientation=DRM_MODE_PANEL_ORIENTATION_LEFT_UP,+};++staticconststructdrm_dmi_panel_orientation_datagpd_pocket={+.width=1200,+.height=1920,+.bios_dates=(constchar*const[]){"05/26/2017","06/28/2017",+"07/05/2017","08/07/2017",NULL},+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdrm_dmi_panel_orientation_datagpd_win={+.width=720,+.height=1280,+.bios_dates=(constchar*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,+};++staticconststructdrm_dmi_panel_orientation_dataitworks_tw891={+.width=800,+.height=1280,+.bios_dates=(constchar*const[]){"10/16/2015",NULL},+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdrm_dmi_panel_orientation_datavios_lth17={+.width=800,+.height=1280,+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdmi_system_idorientation_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,+},{/*+*GPDPocket,notethatthetheDMIdataislessgenericthen+*itseems,deviceswithaboard-vendorof"AMI Corporation"+*arequiterare,asaredeviceswhichhavebothboard-*and*+*product-idsetto"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,+},+{}+};++intdrm_get_panel_orientation_quirk(intwidth,intheight)+{+conststructdmi_system_id*match;+conststructdrm_dmi_panel_orientation_data*data;+constchar*bios_date;+inti;++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)+returndata->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))+returndata->orientation;+}+}++returnDRM_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
quoted hunk
++#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
From: Daniel Vetter <hidden> Date: 2017-10-30 09:43:20
On Mon, Oct 23, 2017 at 09:14:21AM +0200, Hans de Goede wrote:
quoted hunk
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 <redacted>
---
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(+)
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
quoted hunk
+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)
From: Daniel Vetter <hidden> Date: 2017-10-30 09:52:16
On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
quoted hunk
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 <redacted>
---
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(-)
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activfor(i=0;i<fb_helper->crtc_count;i++){structdrm_mode_set*mode_set=&fb_helper->crtc_info[i].mode_set;+structdrm_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,returnbest_score;}+/*+*Thisfunctionchecksifrotationisnecessarybecauseofpanelorientation+*andifitis,ifitissupported.+*Ifrotationisnecessaryandsupported,itsgetssetinfb_crtc.rotation.+*Ifrotationisnecessarybutnotsupported,aDRM_MODE_ROTATE_*flaggets+*or-edintofb_helper->rotations.Indrm_setup_crtcs_fb()wecheckifonly+*onebitissetandthenwesetfb_info.fbcon_rotate_hinttomakefbcondo+*theunsupportedrotation.+*/+staticvoiddrm_setup_crtc_rotation(structdrm_fb_helper*fb_helper,+structdrm_fb_helper_crtc*fb_crtc,+structdrm_connector*connector)+{+structdrm_plane*plane=fb_crtc->mode_set.crtc->primary;+uint64_tvalid_mask=0;+inti,rotation;++fb_crtc->rotation=DRM_MODE_ROTATE_0;++switch(connector->display_info.panel_orientation){+caseDRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:+rotation=DRM_MODE_ROTATE_180;+break;+caseDRM_MODE_PANEL_ORIENTATION_LEFT_UP:+rotation=DRM_MODE_ROTATE_90;+break;+caseDRM_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.
quoted hunk
+ 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.
quoted hunk
++ 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.
From: Daniel Vetter <hidden> Date: 2017-10-30 09:53:45
On Mon, Oct 23, 2017 at 09:14:24AM +0200, Hans de Goede wrote:
quoted hunk
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 <redacted>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
@@ -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_* */staticboolrequest_mem_succeeded=false;staticboolnowc=false;
From: Daniel Vetter <hidden> Date: 2017-10-30 09:58:14
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
From: Hans de Goede <hidden> Date: 2017-10-30 10:52:04
Hi,
On 30-10-17 10:39, Daniel Vetter wrote:
On Mon, Oct 23, 2017 at 09:14:20AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
Found a few organizational bikesheds below, but makes sense I think.
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.
quoted
+ config DRM_DP_AUX_CHARDEV bool "DRM DP AUX Interface" depends on DRM
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.
@@ -0,0 +1,157 @@+/*+*drm_panel_orientation_quirks.c--Quirksfornon-normalpanelorientation+*+*Copyright(C)2017HansdeGoede<hdegoede@redhat.com>+*+*ThisfileissubjecttothetermsandconditionsoftheGNUGeneralPublic+*License.SeethefileCOPYINGinthemaindirectoryofthisarchivefor+*moredetails.+*/++#include<linux/dmi.h>+#include<drm/drm_connector.h>++#ifdef CONFIG_DMI++/*+*Somex86clamshelldesigndevicesuseportraittabletscreensandadisplay+*enginewhichcannotrotateinhardware,soweneedtorotatethefbconto+*compensate.Unfortunatelythese(cheap)devicesalsotypicallyhavequite+*genericDMIdata,sowematchonacombinationofDMIdata,screenresolution+*andalistofknownBIOSdatestoavoidfalsepositives.+*/++structdrm_dmi_panel_orientation_data{+intwidth;+intheight;+constchar*const*bios_dates;+intorientation;+};++staticconststructdrm_dmi_panel_orientation_dataasus_t100ha={+.width=800,+.height=1280,+.orientation=DRM_MODE_PANEL_ORIENTATION_LEFT_UP,+};++staticconststructdrm_dmi_panel_orientation_datagpd_pocket={+.width=1200,+.height=1920,+.bios_dates=(constchar*const[]){"05/26/2017","06/28/2017",+"07/05/2017","08/07/2017",NULL},+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdrm_dmi_panel_orientation_datagpd_win={+.width=720,+.height=1280,+.bios_dates=(constchar*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,+};++staticconststructdrm_dmi_panel_orientation_dataitworks_tw891={+.width=800,+.height=1280,+.bios_dates=(constchar*const[]){"10/16/2015",NULL},+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdrm_dmi_panel_orientation_datavios_lth17={+.width=800,+.height=1280,+.orientation=DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,+};++staticconststructdmi_system_idorientation_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,+},{/*+*GPDPocket,notethatthetheDMIdataislessgenericthen+*itseems,deviceswithaboard-vendorof"AMI Corporation"+*arequiterare,asaredeviceswhichhavebothboard-*and*+*product-idsetto"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,+},+{}+};++intdrm_get_panel_orientation_quirk(intwidth,intheight)+{+conststructdmi_system_id*match;+conststructdrm_dmi_panel_orientation_data*data;+constchar*bios_date;+inti;++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)+returndata->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))+returndata->orientation;+}+}++returnDRM_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
quoted
++#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
From: Hans de Goede <hidden> Date: 2017-10-30 10:57:10
Hi,
On 30-10-17 10:43, Daniel Vetter wrote:
On Mon, Oct 23, 2017 at 09:14:21AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
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(+)
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
quoted
+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)
From: Hans de Goede <hidden> Date: 2017-10-30 11:09:27
Hi,
On 30-10-17 10:52, Daniel Vetter wrote:
On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
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(-)
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activfor(i=0;i<fb_helper->crtc_count;i++){structdrm_mode_set*mode_set=&fb_helper->crtc_info[i].mode_set;+structdrm_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,returnbest_score;}+/*+*Thisfunctionchecksifrotationisnecessarybecauseofpanelorientation+*andifitis,ifitissupported.+*Ifrotationisnecessaryandsupported,itsgetssetinfb_crtc.rotation.+*Ifrotationisnecessarybutnotsupported,aDRM_MODE_ROTATE_*flaggets+*or-edintofb_helper->rotations.Indrm_setup_crtcs_fb()wecheckifonly+*onebitissetandthenwesetfb_info.fbcon_rotate_hinttomakefbcondo+*theunsupportedrotation.+*/+staticvoiddrm_setup_crtc_rotation(structdrm_fb_helper*fb_helper,+structdrm_fb_helper_crtc*fb_crtc,+structdrm_connector*connector)+{+structdrm_plane*plane=fb_crtc->mode_set.crtc->primary;+uint64_tvalid_mask=0;+inti,rotation;++fb_crtc->rotation=DRM_MODE_ROTATE_0;++switch(connector->display_info.panel_orientation){+caseDRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:+rotation=DRM_MODE_ROTATE_180;+break;+caseDRM_MODE_PANEL_ORIENTATION_LEFT_UP:+rotation=DRM_MODE_ROTATE_90;+break;+caseDRM_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.
quoted
+ 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).
quoted
++ 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
From: Hans de Goede <hidden> Date: 2017-10-30 11:10:55
Hi,
On 30-10-17 10:53, Daniel Vetter wrote:
On Mon, Oct 23, 2017 at 09:14:24AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
@@ -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_* */staticboolrequest_mem_succeeded=false;staticboolnowc=false;
From: Daniel Vetter <hidden> Date: 2017-10-31 10:07:20
On Mon, Oct 30, 2017 at 11:57:10AM +0100, Hans de Goede wrote:
Hi,
On 30-10-17 10:43, Daniel Vetter wrote:
quoted
On Mon, Oct 23, 2017 at 09:14:21AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
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(+)
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
quoted
Besides this bikeshed color question makes all sense.
-Daniel
quoted
+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)
From: Daniel Vetter <hidden> Date: 2017-10-31 10:14:30
On Mon, Oct 30, 2017 at 12:09:27PM +0100, Hans de Goede wrote:
Hi,
On 30-10-17 10:52, Daniel Vetter wrote:
quoted
On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
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(-)
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activfor(i=0;i<fb_helper->crtc_count;i++){structdrm_mode_set*mode_set=&fb_helper->crtc_info[i].mode_set;+structdrm_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,returnbest_score;}+/*+*Thisfunctionchecksifrotationisnecessarybecauseofpanelorientation+*andifitis,ifitissupported.+*Ifrotationisnecessaryandsupported,itsgetssetinfb_crtc.rotation.+*Ifrotationisnecessarybutnotsupported,aDRM_MODE_ROTATE_*flaggets+*or-edintofb_helper->rotations.Indrm_setup_crtcs_fb()wecheckifonly+*onebitissetandthenwesetfb_info.fbcon_rotate_hinttomakefbcondo+*theunsupportedrotation.+*/+staticvoiddrm_setup_crtc_rotation(structdrm_fb_helper*fb_helper,+structdrm_fb_helper_crtc*fb_crtc,+structdrm_connector*connector)+{+structdrm_plane*plane=fb_crtc->mode_set.crtc->primary;+uint64_tvalid_mask=0;+inti,rotation;++fb_crtc->rotation=DRM_MODE_ROTATE_0;++switch(connector->display_info.panel_orientation){+caseDRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:+rotation=DRM_MODE_ROTATE_180;+break;+caseDRM_MODE_PANEL_ORIENTATION_LEFT_UP:+rotation=DRM_MODE_ROTATE_90;+break;+caseDRM_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.
quoted
quoted
+ 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.
quoted
quoted
++ 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.
quoted
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
From: Hans de Goede <hidden> Date: 2017-10-31 10:24:14
Hi,
On 31-10-17 11:14, Daniel Vetter wrote:
On Mon, Oct 30, 2017 at 12:09:27PM +0100, Hans de Goede wrote:
quoted
Hi,
On 30-10-17 10:52, Daniel Vetter wrote:
quoted
On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
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(-)
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activfor(i=0;i<fb_helper->crtc_count;i++){structdrm_mode_set*mode_set=&fb_helper->crtc_info[i].mode_set;+structdrm_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,returnbest_score;}+/*+*Thisfunctionchecksifrotationisnecessarybecauseofpanelorientation+*andifitis,ifitissupported.+*Ifrotationisnecessaryandsupported,itsgetssetinfb_crtc.rotation.+*Ifrotationisnecessarybutnotsupported,aDRM_MODE_ROTATE_*flaggets+*or-edintofb_helper->rotations.Indrm_setup_crtcs_fb()wecheckifonly+*onebitissetandthenwesetfb_info.fbcon_rotate_hinttomakefbcondo+*theunsupportedrotation.+*/+staticvoiddrm_setup_crtc_rotation(structdrm_fb_helper*fb_helper,+structdrm_fb_helper_crtc*fb_crtc,+structdrm_connector*connector)+{+structdrm_plane*plane=fb_crtc->mode_set.crtc->primary;+uint64_tvalid_mask=0;+inti,rotation;++fb_crtc->rotation=DRM_MODE_ROTATE_0;++switch(connector->display_info.panel_orientation){+caseDRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:+rotation=DRM_MODE_ROTATE_180;+break;+caseDRM_MODE_PANEL_ORIENTATION_LEFT_UP:+rotation=DRM_MODE_ROTATE_90;+break;+caseDRM_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.
quoted
quoted
+ 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.
quoted
quoted
quoted
++ 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.
quoted
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
From: Daniel Vetter <hidden> Date: 2017-10-31 10:41:02
On Tue, Oct 31, 2017 at 11:24:14AM +0100, Hans de Goede wrote:
Hi,
On 31-10-17 11:14, Daniel Vetter wrote:
quoted
On Mon, Oct 30, 2017 at 12:09:27PM +0100, Hans de Goede wrote:
quoted
Hi,
On 30-10-17 10:52, Daniel Vetter wrote:
quoted
On Mon, Oct 23, 2017 at 09:14:22AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
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(-)
@@ -392,6 +392,11 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activfor(i=0;i<fb_helper->crtc_count;i++){structdrm_mode_set*mode_set=&fb_helper->crtc_info[i].mode_set;+structdrm_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,returnbest_score;}+/*+*Thisfunctionchecksifrotationisnecessarybecauseofpanelorientation+*andifitis,ifitissupported.+*Ifrotationisnecessaryandsupported,itsgetssetinfb_crtc.rotation.+*Ifrotationisnecessarybutnotsupported,aDRM_MODE_ROTATE_*flaggets+*or-edintofb_helper->rotations.Indrm_setup_crtcs_fb()wecheckifonly+*onebitissetandthenwesetfb_info.fbcon_rotate_hinttomakefbcondo+*theunsupportedrotation.+*/+staticvoiddrm_setup_crtc_rotation(structdrm_fb_helper*fb_helper,+structdrm_fb_helper_crtc*fb_crtc,+structdrm_connector*connector)+{+structdrm_plane*plane=fb_crtc->mode_set.crtc->primary;+uint64_tvalid_mask=0;+inti,rotation;++fb_crtc->rotation=DRM_MODE_ROTATE_0;++switch(connector->display_info.panel_orientation){+caseDRM_MODE_PANEL_ORIENTATION_BOTTOM_UP:+rotation=DRM_MODE_ROTATE_180;+break;+caseDRM_MODE_PANEL_ORIENTATION_LEFT_UP:+rotation=DRM_MODE_ROTATE_90;+break;+caseDRM_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.
quoted
quoted
+ 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.
quoted
quoted
quoted
++ 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.
quoted
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.
quoted
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
From: Hans de Goede <hidden> Date: 2017-11-04 13:13:21
Hi,
On 30-10-17 10:53, Daniel Vetter wrote:
On Mon, Oct 23, 2017 at 09:14:24AM +0200, Hans de Goede wrote:
quoted
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 <redacted>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/efifb.c | 21 ++++++++++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
@@ -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_* */staticboolrequest_mem_succeeded=false;staticboolnowc=false;
@@ -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