Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
Hans de Goedewrites: > HI, > > On 08-05-17 14:27, Chris Wilson wrote: >> On Sun, May 07, 2017 at 11:10:56AM +0200, Hans de Goede wrote: >>> On some (Bay Trail) devices the LCD panel is mounted upside-down. >>> >>> This commit uses the code to read back the initial rotation of the >>> primary plane in get_initial_plane_config from Ville Syrjala's >>> "drm/fb-helper: Inherit rotation wip" patch and when re-using the >>> initial fb it stores that in intel_crtc.initial_rotation. >>> >>> It adds an intel_plane_get_rotation helper which combines this >>> initial_rotation with any rotation requested by userspace and >>> uses this in all places which look at a planes rotation, thus >>> transparently dealing with upside-down LCD panels without requiring >>> any user-space or fbcon changes. >>> >>> This fixes the kernel boot messages switching from being shown the right >>> way up in efifb to being shown upside down as soon as a native kms driver >>> loads, as well as any graphics displayed by userspace being upside-down. >>> >>> Note this only deals with upside-down LCD panels / 180 degrees >>> rotation as the hardware in question only supports 180 degrees >>> rotation in hardware. The one model known which has a panel mounted >>> with 90/270 degrees rotation will need to be fully dealt with in >>> userspace, even the firmware boot screen / menus are rotated 90 degrees >>> on this one and there simply is nothing the kernel can do about this. >> >> I shall just mention a concern with hiding the transformation on the >> connection, we do expose the subpixel layout to userspace and that >> should be adjusted based on this lie. There are probably other places >> where the orientation of the output leaks through the current interface. >> >> The commit message fails to explain how userspace, which should already >> have the tools available to it, is unable to reapply the existing >> rotation - i.e. why this is a kernel problem, > > This is a kernel problem because one of the things the kernel does > is hardware abstraction, as I mentioned in my reply to Ville, there is > not single userspace to fix here, there is a large supply of userspace > consumers of the kms API which need fixing to fix this in userspace. > > For touchscreens which are mounted with a wrong orientation the > input layer fixes things up, I don't see how this is any different > really. Now if it was impossible or very complicated to fix this > in the kernel that would be a different story, but as this patch > shows it is quite doable to fix this in the kernel. FWIW, Raspberry Pi has a similar problem: The panel has an appropriate orientation due to its viewing angle, but the cabling is such that many people mount it upside down and use a firmware configuration to have everything flipped. It would be nice if I could do readback of the panel's orientation reg (I haven't tested) and retain automatically that after boot, as a default policy. signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
HI, On 08-05-17 14:27, Chris Wilson wrote: On Sun, May 07, 2017 at 11:10:56AM +0200, Hans de Goede wrote: On some (Bay Trail) devices the LCD panel is mounted upside-down. This commit uses the code to read back the initial rotation of the primary plane in get_initial_plane_config from Ville Syrjala's "drm/fb-helper: Inherit rotation wip" patch and when re-using the initial fb it stores that in intel_crtc.initial_rotation. It adds an intel_plane_get_rotation helper which combines this initial_rotation with any rotation requested by userspace and uses this in all places which look at a planes rotation, thus transparently dealing with upside-down LCD panels without requiring any user-space or fbcon changes. This fixes the kernel boot messages switching from being shown the right way up in efifb to being shown upside down as soon as a native kms driver loads, as well as any graphics displayed by userspace being upside-down. Note this only deals with upside-down LCD panels / 180 degrees rotation as the hardware in question only supports 180 degrees rotation in hardware. The one model known which has a panel mounted with 90/270 degrees rotation will need to be fully dealt with in userspace, even the firmware boot screen / menus are rotated 90 degrees on this one and there simply is nothing the kernel can do about this. I shall just mention a concern with hiding the transformation on the connection, we do expose the subpixel layout to userspace and that should be adjusted based on this lie. There are probably other places where the orientation of the output leaks through the current interface. The commit message fails to explain how userspace, which should already have the tools available to it, is unable to reapply the existing rotation - i.e. why this is a kernel problem, This is a kernel problem because one of the things the kernel does is hardware abstraction, as I mentioned in my reply to Ville, there is not single userspace to fix here, there is a large supply of userspace consumers of the kms API which need fixing to fix this in userspace. For touchscreens which are mounted with a wrong orientation the input layer fixes things up, I don't see how this is any different really. Now if it was impossible or very complicated to fix this in the kernel that would be a different story, but as this patch shows it is quite doable to fix this in the kernel. Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
On Sun, May 07, 2017 at 11:10:56AM +0200, Hans de Goede wrote: > On some (Bay Trail) devices the LCD panel is mounted upside-down. > > This commit uses the code to read back the initial rotation of the > primary plane in get_initial_plane_config from Ville Syrjala's > "drm/fb-helper: Inherit rotation wip" patch and when re-using the > initial fb it stores that in intel_crtc.initial_rotation. > > It adds an intel_plane_get_rotation helper which combines this > initial_rotation with any rotation requested by userspace and > uses this in all places which look at a planes rotation, thus > transparently dealing with upside-down LCD panels without requiring > any user-space or fbcon changes. > > This fixes the kernel boot messages switching from being shown the right > way up in efifb to being shown upside down as soon as a native kms driver > loads, as well as any graphics displayed by userspace being upside-down. > > Note this only deals with upside-down LCD panels / 180 degrees > rotation as the hardware in question only supports 180 degrees > rotation in hardware. The one model known which has a panel mounted > with 90/270 degrees rotation will need to be fully dealt with in > userspace, even the firmware boot screen / menus are rotated 90 degrees > on this one and there simply is nothing the kernel can do about this. I shall just mention a concern with hiding the transformation on the connection, we do expose the subpixel layout to userspace and that should be adjusted based on this lie. There are probably other places where the orientation of the output leaks through the current interface. The commit message fails to explain how userspace, which should already have the tools available to it, is unable to reapply the existing rotation - i.e. why this is a kernel problem, and whether by hiding this from userspace we are not trapping ourselves with warts in the ABI. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
Hi, On 08-05-17 12:44, Ville Syrjälä wrote: On Sun, May 07, 2017 at 11:10:56AM +0200, Hans de Goede wrote: On some (Bay Trail) devices the LCD panel is mounted upside-down. This commit uses the code to read back the initial rotation of the primary plane in get_initial_plane_config from Ville Syrjala's "drm/fb-helper: Inherit rotation wip" patch and when re-using the initial fb it stores that in intel_crtc.initial_rotation. It adds an intel_plane_get_rotation helper which combines this initial_rotation with any rotation requested by userspace and uses this in all places which look at a planes rotation, thus transparently dealing with upside-down LCD panels without requiring any user-space or fbcon changes. This fixes the kernel boot messages switching from being shown the right way up in efifb to being shown upside down as soon as a native kms driver loads, as well as any graphics displayed by userspace being upside-down. Note this only deals with upside-down LCD panels / 180 degrees rotation as the hardware in question only supports 180 degrees rotation in hardware. The one model known which has a panel mounted with 90/270 degrees rotation will need to be fully dealt with in userspace, even the firmware boot screen / menus are rotated 90 degrees on this one and there simply is nothing the kernel can do about this. This pretty much looks like a very limited attempt at full pipe rotation. Correct, it is limited to 180 degree rotation because that is what almost all hardware out there which needs rotation to correct for LCD-panel mounting needs and all pipes on GEN4+ support 180 degree rotation and on GEN < 4 we will never set initial_rotation so this patch is a nop. I have posted a more generic version of that in the past but it was pretty much shot down by everyone else. I agree that trying to deal with 90/270 degree rotation in the kernel is not a good idea, but 180 degree rotation is much easier to deal with and all devices I know of where the kernel can even detect it needs to do rotation use 180 degree rotation. So I'm not convinced this apporach is really the way to go. This ways in at about the same amount of lines added as the previous 2 patches, but unlike the previous 2 patches it actually fully solves the problem. As my testing has shown we really should not punt this problem to userspace because then we will get a never ending job of needing to fix the userspace kms consumer of the week. I'm aware of at least 6 implementations which would need fixing without even trying: intel-ddx modesetting-ddx gnome-wayland-compositor kde-wayland-compositor, sway-wayland-compositor, weston. There are a few limitations even for the 180 degree rotation so trying to hide it inside the kernel isn't 100% possible. Looking at supported_rotations for all planes I don't see any limitations, or are the maybe stride limitations or some such ? The only limitation I see is that on Cherry Trail Pipe B supports reflect-x but not when combined with 180 degree rotation, this will get caught in intel_plane_atomic_check_with_state just as before only now reflect cannot be combined with 0 degree rotation instead of 180 degree rotation. Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
On Sun, May 07, 2017 at 11:10:56AM +0200, Hans de Goede wrote: > On some (Bay Trail) devices the LCD panel is mounted upside-down. > > This commit uses the code to read back the initial rotation of the > primary plane in get_initial_plane_config from Ville Syrjala's > "drm/fb-helper: Inherit rotation wip" patch and when re-using the > initial fb it stores that in intel_crtc.initial_rotation. > > It adds an intel_plane_get_rotation helper which combines this > initial_rotation with any rotation requested by userspace and > uses this in all places which look at a planes rotation, thus > transparently dealing with upside-down LCD panels without requiring > any user-space or fbcon changes. > > This fixes the kernel boot messages switching from being shown the right > way up in efifb to being shown upside down as soon as a native kms driver > loads, as well as any graphics displayed by userspace being upside-down. > > Note this only deals with upside-down LCD panels / 180 degrees > rotation as the hardware in question only supports 180 degrees > rotation in hardware. The one model known which has a panel mounted > with 90/270 degrees rotation will need to be fully dealt with in > userspace, even the firmware boot screen / menus are rotated 90 degrees > on this one and there simply is nothing the kernel can do about this. This pretty much looks like a very limited attempt at full pipe rotation. I have posted a more generic version of that in the past but it was pretty much shot down by everyone else. So I'm not convinced this apporach is really the way to go. There are a few limitations even for the 180 degree rotation so trying to hide it inside the kernel isn't 100% possible. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
Hi, On 08-05-17 10:25, Jani Nikula wrote: On Sun, 07 May 2017, Hans de Goedewrote: @@ -1403,6 +1410,31 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state) return i915_ggtt_offset(state->vma); } +static inline unsigned int +intel_plane_get_rotation(const struct intel_plane_state *plane_state) +{ Random drive-by bikeshed, is this really worth the inline? It is small and this seemed a convenient way to answer the "where to put this" question, but I'm fine with having it as a regular function instead. I guess it should go to intel_atomic_plane.c then? Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
On Sun, 07 May 2017, Hans de Goedewrote: > @@ -1403,6 +1410,31 @@ static inline u32 intel_plane_ggtt_offset(const struct > intel_plane_state *state) > return i915_ggtt_offset(state->vma); > } > > +static inline unsigned int > +intel_plane_get_rotation(const struct intel_plane_state *plane_state) > +{ Random drive-by bikeshed, is this really worth the inline? BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels
On some (Bay Trail) devices the LCD panel is mounted upside-down. This commit uses the code to read back the initial rotation of the primary plane in get_initial_plane_config from Ville Syrjala's "drm/fb-helper: Inherit rotation wip" patch and when re-using the initial fb it stores that in intel_crtc.initial_rotation. It adds an intel_plane_get_rotation helper which combines this initial_rotation with any rotation requested by userspace and uses this in all places which look at a planes rotation, thus transparently dealing with upside-down LCD panels without requiring any user-space or fbcon changes. This fixes the kernel boot messages switching from being shown the right way up in efifb to being shown upside down as soon as a native kms driver loads, as well as any graphics displayed by userspace being upside-down. Note this only deals with upside-down LCD panels / 180 degrees rotation as the hardware in question only supports 180 degrees rotation in hardware. The one model known which has a panel mounted with 90/270 degrees rotation will need to be fully dealt with in userspace, even the firmware boot screen / menus are rotated 90 degrees on this one and there simply is nothing the kernel can do about this. BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894 Cc: Ville SyrjalaSigned-off-by: Hans de Goede --- drivers/gpu/drm/i915/intel_atomic_plane.c | 7 +-- drivers/gpu/drm/i915/intel_display.c | 80 +++ drivers/gpu/drm/i915/intel_drv.h | 32 + drivers/gpu/drm/i915/intel_fbc.c | 2 +- drivers/gpu/drm/i915/intel_pm.c | 16 --- drivers/gpu/drm/i915/intel_sprite.c | 14 +++--- 6 files changed, 113 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index 41fd94e62d3c..db681bc4a9ca 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -130,6 +130,7 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, struct drm_i915_private *dev_priv = to_i915(plane->dev); struct drm_plane_state *state = _state->base; struct intel_plane *intel_plane = to_intel_plane(plane); + unsigned int rotation = intel_plane_get_rotation(intel_state); int ret; /* @@ -149,7 +150,7 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, intel_state->clip.y2 = crtc_state->base.enable ? crtc_state->pipe_src_h : 0; - if (state->fb && drm_rotation_90_or_270(state->rotation)) { + if (state->fb && drm_rotation_90_or_270(rotation)) { struct drm_format_name_buf format_name; if (state->fb->modifier != I915_FORMAT_MOD_Y_TILED && @@ -178,8 +179,8 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, /* CHV ignores the mirror bit when the rotate bit is set :( */ if (IS_CHERRYVIEW(dev_priv) && - state->rotation & DRM_ROTATE_180 && - state->rotation & DRM_REFLECT_X) { + rotation & DRM_ROTATE_180 && + rotation & DRM_REFLECT_X) { DRM_DEBUG_KMS("Cannot rotate and reflect at the same time\n"); return -EINVAL; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3664707950c7..d4824702be4e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2286,7 +2286,7 @@ void intel_add_fb_offsets(int *x, int *y, { const struct intel_framebuffer *intel_fb = to_intel_framebuffer(state->base.fb); - unsigned int rotation = state->base.rotation; + unsigned int rotation = intel_plane_get_rotation(state); if (drm_rotation_90_or_270(rotation)) { *x += intel_fb->rotated[plane].x; @@ -2339,7 +2339,7 @@ static u32 intel_adjust_tile_offset(int *x, int *y, const struct drm_i915_private *dev_priv = to_i915(state->base.plane->dev); const struct drm_framebuffer *fb = state->base.fb; unsigned int cpp = fb->format->cpp[plane]; - unsigned int rotation = state->base.rotation; + unsigned int rotation = intel_plane_get_rotation(state); unsigned int pitch = intel_fb_pitch(fb, plane, rotation); WARN_ON(new_offset > old_offset); @@ -2444,7 +2444,7 @@ u32 intel_compute_tile_offset(int *x, int *y, { const struct drm_i915_private *dev_priv = to_i915(state->base.plane->dev); const struct drm_framebuffer *fb = state->base.fb; - unsigned int rotation = state->base.rotation; + unsigned int rotation = intel_plane_get_rotation(state); int pitch = intel_fb_pitch(fb, plane, rotation); u32 alignment; @@ -2798,9 +2798,10 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, return;