Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Bastien Nocera]

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 
> > > 
> > > 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 
> > > [hdego...@redhat.com: Split the drm/fb-helper bits out of Ville's
> > >   "drm/fb-helper: Inherit rotation wip" patch]
> > > Tested-by: Hans de Goede 
> > > Signed-off-by: Hans de Goede 
> > > ---
> > >   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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Hans de Goede]

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 

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 
[hdego...@redhat.com: Split the drm/fb-helper bits out of Ville's
  "drm/fb-helper: Inherit rotation wip" patch]
Tested-by: Hans de Goede 
Signed-off-by: Hans de Goede 
---
  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.



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_* */


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Bastien Nocera]

On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote:
> From: Ville Syrjala 
> 
> 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 
> [hdego...@redhat.com: Split the drm/fb-helper bits out of Ville's
>  "drm/fb-helper: Inherit rotation wip" patch]
> Tested-by: Hans de Goede 
> Signed-off-by: Hans de Goede 
> ---
>  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.   


> 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_* */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Hans de Goede]

From: Ville Syrjala 

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 
[hdego...@redhat.com: Split the drm/fb-helper bits out of Ville's
 "drm/fb-helper: Inherit rotation wip" patch]
Tested-by: Hans de Goede 
Signed-off-by: Hans de Goede 
---
 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;
+
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 
*));