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