Re: [Intel-gfx] [PATCH 3/7] drm: Add support for optional per-plane rotation property

[Joonas Lahtinen]

On ke, 2016-07-20 at 17:08 +0300, Ville Syrjälä wrote:
> On Wed, Jul 20, 2016 at 04:51:57PM +0300, Joonas Lahtinen wrote:
> > 
> > On ke, 2016-07-20 at 16:18 +0300, ville.syrj...@linux.intel.com wrote:
> > > 
> > > From: Ville Syrjälä 
> > > 
> > > Not all planes on the ssytem may support the same rotations/reflections,
> > s/ssytem/system/
> > 
> > > 
> > > so make it possible to create a separate property for each plane.
> > > This way userspace gets told exactly which rotations/reflections are
> > > possible for each plane.
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c| 6 --
> > >  drivers/gpu/drm/drm_fb_helper.c | 6 +-
> > >  include/drm/drm_crtc.h  | 2 ++
> > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 5f0002897862..38b023da6f09 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -710,7 +710,8 @@ int drm_atomic_plane_set_property(struct drm_plane 
> > > *plane,
> > >   state->src_w = val;
> > >   } else if (property == config->prop_src_h) {
> > >   state->src_h = val;
> > > - } else if (property == config->rotation_property) {
> > > + } else if (property == config->rotation_property ||
> > > +    property == plane->rotation_property) {
> > >   if (!is_power_of_2(val & DRM_ROTATE_MASK))
> > >   return -EINVAL;
> > >   state->rotation = val;
> > > @@ -768,7 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> > >   *val = state->src_w;
> > >   } else if (property == config->prop_src_h) {
> > >   *val = state->src_h;
> > > - } else if (property == config->rotation_property) {
> > > + } else if (property == config->rotation_property ||
> > > +    property == plane->rotation_property) {
> > >   *val = state->rotation;
> > >   } else if (plane->funcs->atomic_get_property) {
> > >   return plane->funcs->atomic_get_property(plane, state, 
> > > property, val);
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> > > b/drivers/gpu/drm/drm_fb_helper.c
> > > index ce54e985d91b..ce536c0553e5 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -392,7 +392,11 @@ static int restore_fbdev_mode(struct drm_fb_helper 
> > > *fb_helper)
> > >   if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> > >   drm_plane_force_disable(plane);
> > >  
> > > - if (dev->mode_config.rotation_property) {
> > > + if (plane->rotation_property) {
> > > + drm_mode_plane_set_obj_prop(plane,
> > > + plane->rotation_property,
> > > + BIT(DRM_ROTATE_0));
> > > + } else if (dev->mode_config.rotation_property) {
> > Why not clear the mode_config.rotation_property too, still?
> I would be cleared if any plane uses it. Though I'm not sure mixing
> per-plane and global rotation props in one device makes any real sense.
> 
> Or maybe I should just nuke the global property?

Maybe a vote for nuking, co-existence will unnecessarily complicate the
code. Only scenario I can see it working when planes with the property
are not used, and planes without are used solely. So the legacy
behaviour might not be very useful.

Regards, Joonas

-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm: Add support for optional per-plane rotation property

[Ville Syrjälä]

On Wed, Jul 20, 2016 at 04:51:57PM +0300, Joonas Lahtinen wrote:
> On ke, 2016-07-20 at 16:18 +0300, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > Not all planes on the ssytem may support the same rotations/reflections,
> 
> s/ssytem/system/
> 
> > so make it possible to create a separate property for each plane.
> > This way userspace gets told exactly which rotations/reflections are
> > possible for each plane.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/drm_atomic.c| 6 --
> >  drivers/gpu/drm/drm_fb_helper.c | 6 +-
> >  include/drm/drm_crtc.h  | 2 ++
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 5f0002897862..38b023da6f09 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -710,7 +710,8 @@ int drm_atomic_plane_set_property(struct drm_plane 
> > *plane,
> >     state->src_w = val;
> >     } else if (property == config->prop_src_h) {
> >     state->src_h = val;
> > -   } else if (property == config->rotation_property) {
> > +   } else if (property == config->rotation_property ||
> > +      property == plane->rotation_property) {
> >     if (!is_power_of_2(val & DRM_ROTATE_MASK))
> >     return -EINVAL;
> >     state->rotation = val;
> > @@ -768,7 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >     *val = state->src_w;
> >     } else if (property == config->prop_src_h) {
> >     *val = state->src_h;
> > -   } else if (property == config->rotation_property) {
> > +   } else if (property == config->rotation_property ||
> > +      property == plane->rotation_property) {
> >     *val = state->rotation;
> >     } else if (plane->funcs->atomic_get_property) {
> >     return plane->funcs->atomic_get_property(plane, state, 
> > property, val);
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index ce54e985d91b..ce536c0553e5 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -392,7 +392,11 @@ static int restore_fbdev_mode(struct drm_fb_helper 
> > *fb_helper)
> >     if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> >     drm_plane_force_disable(plane);
> >  
> > -   if (dev->mode_config.rotation_property) {
> > +   if (plane->rotation_property) {
> > +   drm_mode_plane_set_obj_prop(plane,
> > +   plane->rotation_property,
> > +   BIT(DRM_ROTATE_0));
> > +   } else if (dev->mode_config.rotation_property) {
> 
> Why not clear the mode_config.rotation_property too, still?

I would be cleared if any plane uses it. Though I'm not sure mixing
per-plane and global rotation props in one device makes any real sense.

Or maybe I should just nuke the global property?

> 
> Regards, Joonas
> 
> >     drm_mode_plane_set_obj_prop(plane,
> >     
> > dev->mode_config.rotation_property,
> >     BIT(DRM_ROTATE_0));
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index b7ac7dcf52db..8d65e85d6d91 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1737,6 +1737,8 @@ struct drm_plane {
> >     const struct drm_plane_helper_funcs *helper_private;
> >  
> >     struct drm_plane_state *state;
> > +
> > +   struct drm_property *rotation_property;
> >  };
> >  
> >  /**
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation

-- 
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 3/7] drm: Add support for optional per-plane rotation property

[Joonas Lahtinen]

On ke, 2016-07-20 at 16:18 +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Not all planes on the ssytem may support the same rotations/reflections,

s/ssytem/system/

> so make it possible to create a separate property for each plane.
> This way userspace gets told exactly which rotations/reflections are
> possible for each plane.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_atomic.c| 6 --
>  drivers/gpu/drm/drm_fb_helper.c | 6 +-
>  include/drm/drm_crtc.h  | 2 ++
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5f0002897862..38b023da6f09 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -710,7 +710,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> - } else if (property == config->rotation_property) {
> + } else if (property == config->rotation_property ||
> +    property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
>   state->rotation = val;
> @@ -768,7 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> - } else if (property == config->rotation_property) {
> + } else if (property == config->rotation_property ||
> +    property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ce54e985d91b..ce536c0553e5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -392,7 +392,11 @@ static int restore_fbdev_mode(struct drm_fb_helper 
> *fb_helper)
>   if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>   drm_plane_force_disable(plane);
>  
> - if (dev->mode_config.rotation_property) {
> + if (plane->rotation_property) {
> + drm_mode_plane_set_obj_prop(plane,
> + plane->rotation_property,
> + BIT(DRM_ROTATE_0));
> + } else if (dev->mode_config.rotation_property) {

Why not clear the mode_config.rotation_property too, still?

Regards, Joonas

>   drm_mode_plane_set_obj_prop(plane,
>   
> dev->mode_config.rotation_property,
>   BIT(DRM_ROTATE_0));
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b7ac7dcf52db..8d65e85d6d91 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1737,6 +1737,8 @@ struct drm_plane {
>   const struct drm_plane_helper_funcs *helper_private;
>  
>   struct drm_plane_state *state;
> +
> + struct drm_property *rotation_property;
>  };
>  
>  /**
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm: Add support for optional per-plane rotation property

[Chris Wilson]

On Wed, Jul 20, 2016 at 04:18:08PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Not all planes on the ssytem may support the same rotations/reflections,
> so make it possible to create a separate property for each plane.
> This way userspace gets told exactly which rotations/reflections are
> possible for each plane.

Hmm, can't see that it would make much difference here, but I was
expecting something like

drm_plane_lookup_rotation_propery()
{
return plane->rotation_property ?: 
plane->dev->mode_config.rotation_property;
}

so that it is obvious that the plane takes precedence over the global
property when provided.

You've caught all the spots I can see so,
Reviewed-by: Chris Wilson 
-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

[Intel-gfx] [PATCH 3/7] drm: Add support for optional per-plane rotation property

[ville . syrjala]

From: Ville Syrjälä 

Not all planes on the ssytem may support the same rotations/reflections,
so make it possible to create a separate property for each plane.
This way userspace gets told exactly which rotations/reflections are
possible for each plane.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_atomic.c| 6 --
 drivers/gpu/drm/drm_fb_helper.c | 6 +-
 include/drm/drm_crtc.h  | 2 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5f0002897862..38b023da6f09 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -710,7 +710,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
state->src_w = val;
} else if (property == config->prop_src_h) {
state->src_h = val;
-   } else if (property == config->rotation_property) {
+   } else if (property == config->rotation_property ||
+  property == plane->rotation_property) {
if (!is_power_of_2(val & DRM_ROTATE_MASK))
return -EINVAL;
state->rotation = val;
@@ -768,7 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_w;
} else if (property == config->prop_src_h) {
*val = state->src_h;
-   } else if (property == config->rotation_property) {
+   } else if (property == config->rotation_property ||
+  property == plane->rotation_property) {
*val = state->rotation;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ce54e985d91b..ce536c0553e5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -392,7 +392,11 @@ static int restore_fbdev_mode(struct drm_fb_helper 
*fb_helper)
if (plane->type != DRM_PLANE_TYPE_PRIMARY)
drm_plane_force_disable(plane);
 
-   if (dev->mode_config.rotation_property) {
+   if (plane->rotation_property) {
+   drm_mode_plane_set_obj_prop(plane,
+   plane->rotation_property,
+   BIT(DRM_ROTATE_0));
+   } else if (dev->mode_config.rotation_property) {
drm_mode_plane_set_obj_prop(plane,

dev->mode_config.rotation_property,
BIT(DRM_ROTATE_0));
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b7ac7dcf52db..8d65e85d6d91 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1737,6 +1737,8 @@ struct drm_plane {
const struct drm_plane_helper_funcs *helper_private;
 
struct drm_plane_state *state;
+
+   struct drm_property *rotation_property;
 };
 
 /**
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx