| Message ID | 20170423161106.20103-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote: > From: Ville Syrjala <ville.syrjala@linux.intel.com> > > If a connector added through drm_fb_helper_add_one_connector() has > a crtc attached and that crtc has a rotation configured make the > fbdev inherit the crtc's rotation. > > This is useful on e.g. some tablets which have their lcd panel > mounted > upside down, which before this commit would result in the kernel boot > messages switching from being shown the right way up in efifb to > being > shown upside down as soon as a native kms driver loads. > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894 > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's > "drm/fb-helper: Inherit rotation wip" patch] > Tested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 51 > ++++++++++++++++++++++++++++++++++++++--- > include/drm/drm_fb_helper.h | 2 ++ > 2 files changed, 50 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > b/drivers/gpu/drm/drm_fb_helper.c > index 324a688..c97e00ab 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct > drm_fb_helper *fb_helper, struct drm_ > { > struct drm_fb_helper_connector **temp; > struct drm_fb_helper_connector *fb_helper_connector; > + struct drm_crtc *crtc = connector->encoder ? > + connector->encoder->crtc : NULL; > > if (!drm_fbdev_emulation) > return 0; > @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct > drm_fb_helper *fb_helper, struct drm_ > > drm_connector_reference(connector); > fb_helper_connector->connector = connector; > + if (crtc && crtc->primary->state) > + fb_helper_connector->rotation = crtc->primary- > >state->rotation; > + if (!fb_helper_connector->rotation) > + fb_helper_connector->rotation = DRM_ROTATE_0; That's equivalent to: if (fb_helper_connector->rotation = DRM_ROTATE_0) fb_helper_connector->rotation = DRM_ROTATE_0; Maybe: if (crtc && crtc->primary->state) ... else ...->rotation = DRM_ROTATE_0; would be clearer? Or simply omit it if the connector is zero'ed. <snip> > diff --git a/include/drm/drm_fb_helper.h > b/include/drm/drm_fb_helper.h > index 6f5aceb..19fc313 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -48,6 +48,7 @@ struct drm_fb_helper_crtc { > struct drm_mode_set mode_set; > struct drm_display_mode *desired_mode; > int x, y; > + uint8_t rotation; > }; > > /** > @@ -159,6 +160,7 @@ struct drm_fb_helper_funcs { > > struct drm_fb_helper_connector { > struct drm_connector *connector; > + uint8_t rotation; In both those cases, I'd add a mention of the type of enum/mask etc. for the rotation, for example: uint8_t rotation; /* DRM_ROTATE_* */
Hi, On 26-04-17 14:13, Bastien Nocera wrote: > On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote: >> From: Ville Syrjala <ville.syrjala@linux.intel.com> >> >> If a connector added through drm_fb_helper_add_one_connector() has >> a crtc attached and that crtc has a rotation configured make the >> fbdev inherit the crtc's rotation. >> >> This is useful on e.g. some tablets which have their lcd panel >> mounted >> upside down, which before this commit would result in the kernel boot >> messages switching from being shown the right way up in efifb to >> being >> shown upside down as soon as a native kms driver loads. >> >> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894 >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's >> "drm/fb-helper: Inherit rotation wip" patch] >> Tested-by: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 51 >> ++++++++++++++++++++++++++++++++++++++--- >> include/drm/drm_fb_helper.h | 2 ++ >> 2 files changed, 50 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c >> b/drivers/gpu/drm/drm_fb_helper.c >> index 324a688..c97e00ab 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct >> drm_fb_helper *fb_helper, struct drm_ >> { >> struct drm_fb_helper_connector **temp; >> struct drm_fb_helper_connector *fb_helper_connector; >> + struct drm_crtc *crtc = connector->encoder ? >> + connector->encoder->crtc : NULL; >> >> if (!drm_fbdev_emulation) >> return 0; >> @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct >> drm_fb_helper *fb_helper, struct drm_ >> >> drm_connector_reference(connector); >> fb_helper_connector->connector = connector; >> + if (crtc && crtc->primary->state) >> + fb_helper_connector->rotation = crtc->primary- >>> state->rotation; >> + if (!fb_helper_connector->rotation) >> + fb_helper_connector->rotation = DRM_ROTATE_0; > > That's equivalent to: > if (fb_helper_connector->rotation = DRM_ROTATE_0) > fb_helper_connector->rotation = DRM_ROTATE_0; No it is not equivalent: include/drm/drm_blend.h 40:#define DRM_ROTATE_0 BIT(0) and BIT(0) is (1 << 0) Regards, Hans > > Maybe: > if (crtc && crtc->primary->state) > ... > else > ...->rotation = DRM_ROTATE_0; > would be clearer? Or simply omit it if the connector is zero'ed. > > <snip> >> diff --git a/include/drm/drm_fb_helper.h >> b/include/drm/drm_fb_helper.h >> index 6f5aceb..19fc313 100644 >> --- a/include/drm/drm_fb_helper.h >> +++ b/include/drm/drm_fb_helper.h >> @@ -48,6 +48,7 @@ struct drm_fb_helper_crtc { >> struct drm_mode_set mode_set; >> struct drm_display_mode *desired_mode; >> int x, y; >> + uint8_t rotation; >> }; >> >> /** >> @@ -159,6 +160,7 @@ struct drm_fb_helper_funcs { >> >> struct drm_fb_helper_connector { >> struct drm_connector *connector; >> + uint8_t rotation; > > In both those cases, I'd add a mention of the type of enum/mask etc. > for the rotation, for example: > uint8_t rotation; /* DRM_ROTATE_* */ >
On Sun, 2017-04-30 at 21:22 +0200, Hans de Goede wrote: > Hi, > > On 26-04-17 14:13, Bastien Nocera wrote: > > On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote: > > > From: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > > If a connector added through drm_fb_helper_add_one_connector() > > > has > > > a crtc attached and that crtc has a rotation configured make the > > > fbdev inherit the crtc's rotation. > > > > > > This is useful on e.g. some tablets which have their lcd panel > > > mounted > > > upside down, which before this commit would result in the kernel > > > boot > > > messages switching from being shown the right way up in efifb to > > > being > > > shown upside down as soon as a native kms driver loads. > > > > > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894 > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's > > > "drm/fb-helper: Inherit rotation wip" patch] > > > Tested-by: Hans de Goede <hdegoede@redhat.com> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/gpu/drm/drm_fb_helper.c | 51 > > > ++++++++++++++++++++++++++++++++++++++--- > > > include/drm/drm_fb_helper.h | 2 ++ > > > 2 files changed, 50 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > > b/drivers/gpu/drm/drm_fb_helper.c > > > index 324a688..c97e00ab 100644 > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct > > > drm_fb_helper *fb_helper, struct drm_ > > > { > > > struct drm_fb_helper_connector **temp; > > > struct drm_fb_helper_connector *fb_helper_connector; > > > + struct drm_crtc *crtc = connector->encoder ? > > > + connector->encoder->crtc : NULL; > > > > > > if (!drm_fbdev_emulation) > > > return 0; > > > @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct > > > drm_fb_helper *fb_helper, struct drm_ > > > > > > drm_connector_reference(connector); > > > fb_helper_connector->connector = connector; > > > + if (crtc && crtc->primary->state) > > > + fb_helper_connector->rotation = crtc->primary- > > > > state->rotation; > > > > > > + if (!fb_helper_connector->rotation) > > > + fb_helper_connector->rotation = DRM_ROTATE_0; > > > > That's equivalent to: > > if (fb_helper_connector->rotation = DRM_ROTATE_0) > > fb_helper_connector->rotation = DRM_ROTATE_0; > > No it is not equivalent: > > include/drm/drm_blend.h > 40:#define DRM_ROTATE_0 BIT(0) > > and BIT(0) is (1 << 0) Urgh. I'd have a "DRM_ROTATE_UNSET = 0" added either to the public header, or to this file and compare to it instead of "!rotation". Because this really wasn't clear or obvious. Cheers
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 324a688..c97e00ab 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_ { struct drm_fb_helper_connector **temp; struct drm_fb_helper_connector *fb_helper_connector; + struct drm_crtc *crtc = connector->encoder ? + connector->encoder->crtc : NULL; if (!drm_fbdev_emulation) return 0; @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_ drm_connector_reference(connector); fb_helper_connector->connector = connector; + if (crtc && crtc->primary->state) + fb_helper_connector->rotation = crtc->primary->state->rotation; + if (!fb_helper_connector->rotation) + fb_helper_connector->rotation = DRM_ROTATE_0; + fb_helper->connector_info[fb_helper->connector_count++] = fb_helper_connector; return 0; } @@ -333,6 +340,35 @@ int drm_fb_helper_debug_leave(struct fb_info *info) } EXPORT_SYMBOL(drm_fb_helper_debug_leave); +static int fbdev_plane_index(struct drm_fb_helper *fb_helper, + struct drm_plane *plane) +{ + int i; + + if (plane->type != DRM_PLANE_TYPE_PRIMARY) + return -ENODEV; + + for (i = 0; i < fb_helper->crtc_count; i++) { + struct drm_crtc *crtc = fb_helper->crtc_info[i].mode_set.crtc; + + if (crtc && crtc->primary == plane) + return i; + } + + return -ENODEV; +} + +static unsigned int fbdev_plane_rotation(struct drm_fb_helper *fb_helper, + struct drm_plane *plane) +{ + int i = fbdev_plane_index(fb_helper, plane); + + if (i < 0) + return DRM_ROTATE_0; + else + return fb_helper->crtc_info[i].rotation; +} + static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; @@ -357,7 +393,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper) goto fail; } - plane_state->rotation = DRM_ROTATE_0; + plane_state->rotation = fbdev_plane_rotation(fb_helper, plane); plane->old_fb = plane->fb; plane_mask |= 1 << drm_plane_index(plane); @@ -414,8 +450,8 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) if (plane->rotation_property) drm_mode_plane_set_obj_prop(plane, - plane->rotation_property, - DRM_ROTATE_0); + plane->rotation_property, + fbdev_plane_rotation(fb_helper, plane)); } for (i = 0; i < fb_helper->crtc_count; i++) { @@ -760,6 +796,7 @@ int drm_fb_helper_init(struct drm_device *dev, if (!fb_helper->crtc_info[i].mode_set.connectors) goto out_free; fb_helper->crtc_info[i].mode_set.num_connectors = 0; + fb_helper->crtc_info[i].rotation = DRM_ROTATE_0; } i = 0; @@ -2090,8 +2127,14 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, if (!drm_mode_equal(modes[o], modes[n])) continue; + + if (crtc->rotation && + crtc->rotation != fb_helper_conn->rotation) + continue; } + crtc->rotation = fb_helper_conn->rotation; + crtcs[n] = crtc; memcpy(crtcs, best_crtcs, n * sizeof(struct drm_fb_helper_crtc *)); score = my_score + drm_pick_crtcs(fb_helper, crtcs, modes, n + 1, @@ -2182,6 +2225,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, fb_crtc->desired_mode = mode; fb_crtc->x = offset->x; fb_crtc->y = offset->y; + fb_crtc->rotation = + fb_helper->connector_info[i]->rotation; modeset->mode = drm_mode_duplicate(dev, fb_crtc->desired_mode); drm_connector_reference(connector); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 6f5aceb..19fc313 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -48,6 +48,7 @@ struct drm_fb_helper_crtc { struct drm_mode_set mode_set; struct drm_display_mode *desired_mode; int x, y; + uint8_t rotation; }; /** @@ -159,6 +160,7 @@ struct drm_fb_helper_funcs { struct drm_fb_helper_connector { struct drm_connector *connector; + uint8_t rotation; }; /**