| Message ID | 1450268508-15028-3-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote: > This patch adds all infrastructure to make zpos plane property > configurable from userspace. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Imo zpos should be a generic atomic property with well-defined semantics shared across drivers. So - storead (in decoded form) in drm_plane_state - common functions to register/attach zpos prop to a plane, with full-blown kerneldoc explaining how it should work - generic kms igt to validate that interface One of the big things that always comes up when we talk about zpos is how equal zpos should be handled, and how exactly they should be sorted. I think for that we should have a drm function which computes a normalized zpos. Or the core check code should reject such nonsense. -Daniel > --- > drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 ++- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++--- > 2 files changed, 49 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index 588b6763f9c7..f0827dbebb7d 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -64,6 +64,7 @@ struct exynos_drm_plane_state { > struct exynos_drm_rect src; > unsigned int h_ratio; > unsigned int v_ratio; > + unsigned int zpos; > }; > > static inline struct exynos_drm_plane_state * > @@ -91,11 +92,12 @@ struct exynos_drm_plane { > > #define EXYNOS_DRM_PLANE_CAP_DOUBLE (1 << 0) > #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1) > +#define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2) > > /* > * Exynos DRM plane configuration structure. > * > - * @zpos: z-position of the plane. > + * @zpos: initial z-position of the plane. > * @type: type of the plane (primary, cursor or overlay). > * @pixel_formats: supported pixel formats. > * @num_pixel_formats: number of elements in 'pixel_formats'. > diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c > index fd6cb4cee01a..a2bdab836b50 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c > @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state) > > static void exynos_drm_plane_reset(struct drm_plane *plane) > { > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); > struct exynos_drm_plane_state *exynos_state; > > if (plane->state) { > @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane) > > exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); > if (exynos_state) { > + exynos_state->zpos = exynos_plane->config->zpos; > plane->state = &exynos_state->base; > plane->state->plane = plane; > } > @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane) > return NULL; > > __drm_atomic_helper_plane_duplicate_state(plane, ©->base); > + copy->zpos = exynos_state->zpos; > return ©->base; > } > > @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane, > kfree(old_exynos_state); > } > > +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane, > + struct drm_plane_state *state, > + struct drm_property *property, > + uint64_t val) > +{ > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); > + struct exynos_drm_plane_state *exynos_state = > + to_exynos_plane_state(state); > + struct exynos_drm_private *dev_priv = plane->dev->dev_private; > + const struct exynos_drm_plane_config *config = exynos_plane->config; > + > + if (property == dev_priv->plane_zpos_property && > + (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS)) > + exynos_state->zpos = val; > + else > + return -EINVAL; > + > + return 0; > +} > + > +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane, > + const struct drm_plane_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + const struct exynos_drm_plane_state *exynos_state = > + container_of(state, const struct exynos_drm_plane_state, base); > + struct exynos_drm_private *dev_priv = plane->dev->dev_private; > + > + if (property == dev_priv->plane_zpos_property) > + *val = exynos_state->zpos; > + else > + return -EINVAL; > + > + return 0; > +} > + > static struct drm_plane_funcs exynos_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > .destroy = drm_plane_cleanup, > + .set_property = drm_atomic_helper_plane_set_property, > .reset = exynos_drm_plane_reset, > .atomic_duplicate_state = exynos_drm_plane_duplicate_state, > .atomic_destroy_state = exynos_drm_plane_destroy_state, > + .atomic_set_property = exynos_drm_plane_atomic_set_property, > + .atomic_get_property = exynos_drm_plane_atomic_get_property, > }; > > static int > @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane, > > prop = dev_priv->plane_zpos_property; > if (!prop) { > - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, > - "zpos", 0, MAX_PLANE - 1); > + prop = drm_property_create_range(dev, 0, "zpos", > + 0, MAX_PLANE - 1); > if (!prop) > return; > > @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev, > exynos_plane->index = index; > exynos_plane->config = config; > > - if (config->type == DRM_PLANE_TYPE_OVERLAY) > - exynos_plane_attach_zpos_property(&exynos_plane->base, > - config->zpos); > + exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos); > > return 0; > } > -- > 1.9.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Dec 16, 2015 at 02:28:36PM +0100, Daniel Vetter wrote: > On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote: > > This patch adds all infrastructure to make zpos plane property > > configurable from userspace. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Imo zpos should be a generic atomic property with well-defined semantics > shared across drivers. So > - storead (in decoded form) in drm_plane_state > - common functions to register/attach zpos prop to a plane, with > full-blown kerneldoc explaining how it should work > - generic kms igt to validate that interface > > One of the big things that always comes up when we talk about zpos is how > equal zpos should be handled, and how exactly they should be sorted. I > think for that we should have a drm function which computes a normalized > zpos. Or the core check code should reject such nonsense. Yeah I think just having some core check reject the operation if two planes end up with the same zpos. And zpos should probably just be in the [0-number_of_planes_on_the_crtc_currently] range? Or maybe [0-max_number_of_planes_possible_on_the_crtc], or just [0-total_max_number_of_planes] ? One of the latter two might be sensible because you could then enable/disable some planes on the crtc without necessarily touching the zpos for the other planes. Another complication is how you think about color keying. Eg. if you use dst keying between the primary plane and some other plane, then it may make sense to think of the primary being above the other plane and the colorkey just punches a hole into the primary. But if the planes otherwise are always ordered with the primary at the botton, then I'm not sure if exposing zpos and requiring it to be reconfigured for colorkeying to work would just make things more confusing. But this decisions might also depend on what happens to pixels on the other plane that fall outside of the primary plane (assuming the primary can be non-fullscreen). I don't remember off the top of my head how this works on Intel hw. The other option I suppose is to define color keying as just being slightly magic that it can effectively reorder the planes even though zpos says something else. Not quite sure which is best. Dst color keying is rather confusing in any case since it may work only between certain planes (and exactly which planes may depend which planes are active on the same crtc). > -Daniel > > > --- > > drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 ++- > > drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++--- > > 2 files changed, 49 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h > > index 588b6763f9c7..f0827dbebb7d 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > > @@ -64,6 +64,7 @@ struct exynos_drm_plane_state { > > struct exynos_drm_rect src; > > unsigned int h_ratio; > > unsigned int v_ratio; > > + unsigned int zpos; > > }; > > > > static inline struct exynos_drm_plane_state * > > @@ -91,11 +92,12 @@ struct exynos_drm_plane { > > > > #define EXYNOS_DRM_PLANE_CAP_DOUBLE (1 << 0) > > #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1) > > +#define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2) > > > > /* > > * Exynos DRM plane configuration structure. > > * > > - * @zpos: z-position of the plane. > > + * @zpos: initial z-position of the plane. > > * @type: type of the plane (primary, cursor or overlay). > > * @pixel_formats: supported pixel formats. > > * @num_pixel_formats: number of elements in 'pixel_formats'. > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c > > index fd6cb4cee01a..a2bdab836b50 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c > > @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state) > > > > static void exynos_drm_plane_reset(struct drm_plane *plane) > > { > > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); > > struct exynos_drm_plane_state *exynos_state; > > > > if (plane->state) { > > @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane) > > > > exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); > > if (exynos_state) { > > + exynos_state->zpos = exynos_plane->config->zpos; > > plane->state = &exynos_state->base; > > plane->state->plane = plane; > > } > > @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane) > > return NULL; > > > > __drm_atomic_helper_plane_duplicate_state(plane, ©->base); > > + copy->zpos = exynos_state->zpos; > > return ©->base; > > } > > > > @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane, > > kfree(old_exynos_state); > > } > > > > +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane, > > + struct drm_plane_state *state, > > + struct drm_property *property, > > + uint64_t val) > > +{ > > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); > > + struct exynos_drm_plane_state *exynos_state = > > + to_exynos_plane_state(state); > > + struct exynos_drm_private *dev_priv = plane->dev->dev_private; > > + const struct exynos_drm_plane_config *config = exynos_plane->config; > > + > > + if (property == dev_priv->plane_zpos_property && > > + (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS)) > > + exynos_state->zpos = val; > > + else > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane, > > + const struct drm_plane_state *state, > > + struct drm_property *property, > > + uint64_t *val) > > +{ > > + const struct exynos_drm_plane_state *exynos_state = > > + container_of(state, const struct exynos_drm_plane_state, base); > > + struct exynos_drm_private *dev_priv = plane->dev->dev_private; > > + > > + if (property == dev_priv->plane_zpos_property) > > + *val = exynos_state->zpos; > > + else > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > static struct drm_plane_funcs exynos_plane_funcs = { > > .update_plane = drm_atomic_helper_update_plane, > > .disable_plane = drm_atomic_helper_disable_plane, > > .destroy = drm_plane_cleanup, > > + .set_property = drm_atomic_helper_plane_set_property, > > .reset = exynos_drm_plane_reset, > > .atomic_duplicate_state = exynos_drm_plane_duplicate_state, > > .atomic_destroy_state = exynos_drm_plane_destroy_state, > > + .atomic_set_property = exynos_drm_plane_atomic_set_property, > > + .atomic_get_property = exynos_drm_plane_atomic_get_property, > > }; > > > > static int > > @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane, > > > > prop = dev_priv->plane_zpos_property; > > if (!prop) { > > - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, > > - "zpos", 0, MAX_PLANE - 1); > > + prop = drm_property_create_range(dev, 0, "zpos", > > + 0, MAX_PLANE - 1); > > if (!prop) > > return; > > > > @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev, > > exynos_plane->index = index; > > exynos_plane->config = config; > > > > - if (config->type == DRM_PLANE_TYPE_OVERLAY) > > - exynos_plane_attach_zpos_property(&exynos_plane->base, > > - config->zpos); > > + exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos); > > > > return 0; > > } > > -- > > 1.9.2 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hello, On 2015-12-16 14:28, Daniel Vetter wrote: > On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote: >> This patch adds all infrastructure to make zpos plane property >> configurable from userspace. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Imo zpos should be a generic atomic property with well-defined semantics > shared across drivers. So > - storead (in decoded form) in drm_plane_state > - common functions to register/attach zpos prop to a plane, with > full-blown kerneldoc explaining how it should work > - generic kms igt to validate that interface > > One of the big things that always comes up when we talk about zpos is how > equal zpos should be handled, and how exactly they should be sorted. I > think for that we should have a drm function which computes a normalized > zpos. Or the core check code should reject such nonsense. IMHO it will be enough to state that the case of equal zpos is HW specific. This might simplify some driver logic. For example, in case of Exynos Mixer, equal priority (zpos) means that HW predefined order will be used, so there is no need to normalize zpos values. If you want I can move zpos to drm core and add a function to normalize zpos, although for this particular driver normalization is not needed. It should be quite easy to convert other drivers to use the generic zpos property. The only problem I see is how to handle omap driver, which use 'zorder' property. What about some other typical properties related to blending: - global plane alpha, - colorkey, - alpha mode (standard or pre-multiplied) for alpha-enabled modes, - crtc background color. Do you also want to handle them as generic or driver-specific properties? I plan to add support for them to Exynos Mixer and I would like to avoid doing this twice. Best regards
On Wed, Dec 16, 2015 at 02:54:04PM +0100, Marek Szyprowski wrote: > Hello, > > On 2015-12-16 14:28, Daniel Vetter wrote: > >On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote: > >>This patch adds all infrastructure to make zpos plane property > >>configurable from userspace. > >> > >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >Imo zpos should be a generic atomic property with well-defined semantics > >shared across drivers. So > >- storead (in decoded form) in drm_plane_state > >- common functions to register/attach zpos prop to a plane, with > > full-blown kerneldoc explaining how it should work > >- generic kms igt to validate that interface > > > >One of the big things that always comes up when we talk about zpos is how > >equal zpos should be handled, and how exactly they should be sorted. I > >think for that we should have a drm function which computes a normalized > >zpos. Or the core check code should reject such nonsense. > > IMHO it will be enough to state that the case of equal zpos is HW specific. > This might simplify some driver logic. For example, in case of Exynos Mixer, > equal priority (zpos) means that HW predefined order will be used, so there > is no need to normalize zpos values. > > If you want I can move zpos to drm core and add a function to normalize > zpos, > although for this particular driver normalization is not needed. > > It should be quite easy to convert other drivers to use the generic zpos > property. The only problem I see is how to handle omap driver, which use > 'zorder' property. > > What about some other typical properties related to blending: > - global plane alpha, > - colorkey, > - alpha mode (standard or pre-multiplied) for alpha-enabled modes, > - crtc background color. > > Do you also want to handle them as generic or driver-specific properties? Should all be generic really. And it's also kinda ABI, so userspace needed, and preferrably a proper/automated testcase. i915 has infrastructure to use display pipeline CRCs to really measure it's all correct even, and that's the standard I'd like to see for all KMS API extensions like this. Since if we don't do this we'll end up in a giant mess, and it will be impossible to write a kms compositor that's generic and uses all this. And since this stuff is supposed to be generic, fluffy/unspecified semantics aren't good. Especially if your hw can just somehow deal with it all. > I plan to add support for them to Exynos Mixer and I would like to avoid > doing this twice. It's a lot more work than just adding a few members to drm_plane_state. -Daniel
Hello, On 2015-12-16 15:21, Daniel Vetter wrote: > On Wed, Dec 16, 2015 at 02:54:04PM +0100, Marek Szyprowski wrote: >> On 2015-12-16 14:28, Daniel Vetter wrote: >>> On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote: >>>> This patch adds all infrastructure to make zpos plane property >>>> configurable from userspace. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> Imo zpos should be a generic atomic property with well-defined semantics >>> shared across drivers. So >>> - storead (in decoded form) in drm_plane_state >>> - common functions to register/attach zpos prop to a plane, with >>> full-blown kerneldoc explaining how it should work >>> - generic kms igt to validate that interface >>> >>> One of the big things that always comes up when we talk about zpos is how >>> equal zpos should be handled, and how exactly they should be sorted. I >>> think for that we should have a drm function which computes a normalized >>> zpos. Or the core check code should reject such nonsense. >> IMHO it will be enough to state that the case of equal zpos is HW specific. >> This might simplify some driver logic. For example, in case of Exynos Mixer, >> equal priority (zpos) means that HW predefined order will be used, so there >> is no need to normalize zpos values. >> >> If you want I can move zpos to drm core and add a function to normalize >> zpos, >> although for this particular driver normalization is not needed. >> >> It should be quite easy to convert other drivers to use the generic zpos >> property. The only problem I see is how to handle omap driver, which use >> 'zorder' property. >> >> What about some other typical properties related to blending: >> - global plane alpha, >> - colorkey, >> - alpha mode (standard or pre-multiplied) for alpha-enabled modes, >> - crtc background color. >> >> Do you also want to handle them as generic or driver-specific properties? > Should all be generic really. And it's also kinda ABI, so userspace > needed, and preferrably a proper/automated testcase. i915 has > infrastructure to use display pipeline CRCs to really measure it's all > correct even, and that's the standard I'd like to see for all KMS API > extensions like this. Could you tell a bit more about this pipeline CRCs? What is it? > Since if we don't do this we'll end up in a giant mess, and it will be > impossible to write a kms compositor that's generic and uses all this. > > And since this stuff is supposed to be generic, fluffy/unspecified > semantics aren't good. Especially if your hw can just somehow deal with it > all. > >> I plan to add support for them to Exynos Mixer and I would like to avoid >> doing this twice. > It's a lot more work than just adding a few members to drm_plane_state. I see. Could you elaborate a bit more on what you want to have in drm core for handling all the mentioned features? Best regards
+Cc: Boram Park, Hi Marek, On 12/16/2015 09:21 PM, Marek Szyprowski wrote: > This patch adds all infrastructure to make zpos plane property > configurable from userspace. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 ++- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++--- > 2 files changed, 49 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index 588b6763f9c7..f0827dbebb7d 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -64,6 +64,7 @@ struct exynos_drm_plane_state { > struct exynos_drm_rect src; > unsigned int h_ratio; > unsigned int v_ratio; > + unsigned int zpos; > }; > > static inline struct exynos_drm_plane_state * > @@ -91,11 +92,12 @@ struct exynos_drm_plane { > > #define EXYNOS_DRM_PLANE_CAP_DOUBLE (1 << 0) > #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1) > +#define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2) > > /* > * Exynos DRM plane configuration structure. > * > - * @zpos: z-position of the plane. > + * @zpos: initial z-position of the plane. > * @type: type of the plane (primary, cursor or overlay). > * @pixel_formats: supported pixel formats. > * @num_pixel_formats: number of elements in 'pixel_formats'. > diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c > index fd6cb4cee01a..a2bdab836b50 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c > @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state) > > static void exynos_drm_plane_reset(struct drm_plane *plane) > { > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); > struct exynos_drm_plane_state *exynos_state; > > if (plane->state) { > @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane) > > exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); > if (exynos_state) { > + exynos_state->zpos = exynos_plane->config->zpos; > plane->state = &exynos_state->base; > plane->state->plane = plane; > } > @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane) > return NULL; > > __drm_atomic_helper_plane_duplicate_state(plane, ©->base); > + copy->zpos = exynos_state->zpos; > return ©->base; > } > > @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane, > kfree(old_exynos_state); > } > > +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane, > + struct drm_plane_state *state, > + struct drm_property *property, > + uint64_t val) > +{ > + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); > + struct exynos_drm_plane_state *exynos_state = > + to_exynos_plane_state(state); > + struct exynos_drm_private *dev_priv = plane->dev->dev_private; > + const struct exynos_drm_plane_config *config = exynos_plane->config; > + > + if (property == dev_priv->plane_zpos_property && > + (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS)) > + exynos_state->zpos = val; > + else > + return -EINVAL; > + > + return 0; > +} > + > +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane, > + const struct drm_plane_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + const struct exynos_drm_plane_state *exynos_state = > + container_of(state, const struct exynos_drm_plane_state, base); > + struct exynos_drm_private *dev_priv = plane->dev->dev_private; > + > + if (property == dev_priv->plane_zpos_property) > + *val = exynos_state->zpos; > + else > + return -EINVAL; > + > + return 0; > +} > + > static struct drm_plane_funcs exynos_plane_funcs = { > .update_plane = drm_atomic_helper_update_plane, > .disable_plane = drm_atomic_helper_disable_plane, > .destroy = drm_plane_cleanup, > + .set_property = drm_atomic_helper_plane_set_property, > .reset = exynos_drm_plane_reset, > .atomic_duplicate_state = exynos_drm_plane_duplicate_state, > .atomic_destroy_state = exynos_drm_plane_destroy_state, > + .atomic_set_property = exynos_drm_plane_atomic_set_property, > + .atomic_get_property = exynos_drm_plane_atomic_get_property, > }; > > static int > @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane, > > prop = dev_priv->plane_zpos_property; > if (!prop) { > - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, > - "zpos", 0, MAX_PLANE - 1); > + prop = drm_property_create_range(dev, 0, "zpos", > + 0, MAX_PLANE - 1); User are using zpos property as index now, if we make zpos property configurable, i think we need a property immutable like index or a property like type or name that can know this plane is which plane (e.g. video plane or graphic plane). Another way, user may be possible to check plane index as order of plane object id. Thanks. > if (!prop) > return; > > @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev, > exynos_plane->index = index; > exynos_plane->config = config; > > - if (config->type == DRM_PLANE_TYPE_OVERLAY) > - exynos_plane_attach_zpos_property(&exynos_plane->base, > - config->zpos); > + exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos); > > return 0; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 2015-12-17 03:55, Joonyoung Shim wrote: > +Cc: Boram Park, > > Hi Marek, > > On 12/16/2015 09:21 PM, Marek Szyprowski wrote: >> This patch adds all infrastructure to make zpos plane property >> configurable from userspace. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 ++- >> drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++--- >> 2 files changed, 49 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> index 588b6763f9c7..f0827dbebb7d 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state { >> struct exynos_drm_rect src; >> unsigned int h_ratio; >> unsigned int v_ratio; >> + unsigned int zpos; >> }; >> >> static inline struct exynos_drm_plane_state * >> @@ -91,11 +92,12 @@ struct exynos_drm_plane { >> >> #define EXYNOS_DRM_PLANE_CAP_DOUBLE (1 << 0) >> #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1) >> +#define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2) >> >> /* >> * Exynos DRM plane configuration structure. >> * >> - * @zpos: z-position of the plane. >> + * @zpos: initial z-position of the plane. >> * @type: type of the plane (primary, cursor or overlay). >> * @pixel_formats: supported pixel formats. >> * @num_pixel_formats: number of elements in 'pixel_formats'. >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c >> index fd6cb4cee01a..a2bdab836b50 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c >> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state) >> >> static void exynos_drm_plane_reset(struct drm_plane *plane) >> { >> + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); >> struct exynos_drm_plane_state *exynos_state; >> >> if (plane->state) { >> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane) >> >> exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); >> if (exynos_state) { >> + exynos_state->zpos = exynos_plane->config->zpos; >> plane->state = &exynos_state->base; >> plane->state->plane = plane; >> } >> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane) >> return NULL; >> >> __drm_atomic_helper_plane_duplicate_state(plane, ©->base); >> + copy->zpos = exynos_state->zpos; >> return ©->base; >> } >> >> @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane, >> kfree(old_exynos_state); >> } >> >> +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane, >> + struct drm_plane_state *state, >> + struct drm_property *property, >> + uint64_t val) >> +{ >> + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); >> + struct exynos_drm_plane_state *exynos_state = >> + to_exynos_plane_state(state); >> + struct exynos_drm_private *dev_priv = plane->dev->dev_private; >> + const struct exynos_drm_plane_config *config = exynos_plane->config; >> + >> + if (property == dev_priv->plane_zpos_property && >> + (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS)) >> + exynos_state->zpos = val; >> + else >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane, >> + const struct drm_plane_state *state, >> + struct drm_property *property, >> + uint64_t *val) >> +{ >> + const struct exynos_drm_plane_state *exynos_state = >> + container_of(state, const struct exynos_drm_plane_state, base); >> + struct exynos_drm_private *dev_priv = plane->dev->dev_private; >> + >> + if (property == dev_priv->plane_zpos_property) >> + *val = exynos_state->zpos; >> + else >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> static struct drm_plane_funcs exynos_plane_funcs = { >> .update_plane = drm_atomic_helper_update_plane, >> .disable_plane = drm_atomic_helper_disable_plane, >> .destroy = drm_plane_cleanup, >> + .set_property = drm_atomic_helper_plane_set_property, >> .reset = exynos_drm_plane_reset, >> .atomic_duplicate_state = exynos_drm_plane_duplicate_state, >> .atomic_destroy_state = exynos_drm_plane_destroy_state, >> + .atomic_set_property = exynos_drm_plane_atomic_set_property, >> + .atomic_get_property = exynos_drm_plane_atomic_get_property, >> }; >> >> static int >> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane, >> >> prop = dev_priv->plane_zpos_property; >> if (!prop) { >> - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, >> - "zpos", 0, MAX_PLANE - 1); >> + prop = drm_property_create_range(dev, 0, "zpos", >> + 0, MAX_PLANE - 1); > User are using zpos property as index now, if we make zpos property > configurable, i think we need a property immutable like index or a > property like type or name that can know this plane is which plane > (e.g. video plane or graphic plane). I don't get this. zpos property is attached only to OVERLAY planes, PRIMARY and CURSOR don't have it, so userspace cannot rely on this property. In my patch the initial zpos value is same as before, so if application doesn't relies on zpos values, it will get the same values as now. Application can also check the plane type by reading 'type' property (PRIMARY, OVERLAY or CURSOR). However after my patch application will get the possibility to rearrange plane order for the given crtc by chaning zpos values. That's especially useful to configure 2 typical use cases: PRIMARY plane over OVERLAY plane (PRIMARY is used as typical on-screen-display) or OVERLAY plane over PRIMARY (OVERLAY is used as a window for displaying video data). > Another way, user may be possible to check plane index as order of > plane object id. Application can easily find all the planes, which belongs to given crtc by checking 'possible crtcs' plane parameter, I see no additional need for 'index' property in such case. Please note that the initial plane configuration in case of atomic api will be always the same for all application (determined by exynos_drm_plane_reset function) and it will match the current setup. > > Thanks. > >> if (!prop) >> return; >> >> @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev, >> exynos_plane->index = index; >> exynos_plane->config = config; >> >> - if (config->type == DRM_PLANE_TYPE_OVERLAY) >> - exynos_plane_attach_zpos_property(&exynos_plane->base, >> - config->zpos); >> + exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos); >> >> return 0; >> } >> > Best regards
On 12/17/2015 10:05 PM, Marek Szyprowski wrote: > Hello, > > On 2015-12-17 03:55, Joonyoung Shim wrote: >> +Cc: Boram Park, >> >> Hi Marek, >> >> On 12/16/2015 09:21 PM, Marek Szyprowski wrote: >>> This patch adds all infrastructure to make zpos plane property >>> configurable from userspace. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 ++- >>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++--- >>> 2 files changed, 49 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> index 588b6763f9c7..f0827dbebb7d 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >>> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state { >>> struct exynos_drm_rect src; >>> unsigned int h_ratio; >>> unsigned int v_ratio; >>> + unsigned int zpos; >>> }; >>> static inline struct exynos_drm_plane_state * >>> @@ -91,11 +92,12 @@ struct exynos_drm_plane { >>> #define EXYNOS_DRM_PLANE_CAP_DOUBLE (1 << 0) >>> #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1) >>> +#define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2) >>> /* >>> * Exynos DRM plane configuration structure. >>> * >>> - * @zpos: z-position of the plane. >>> + * @zpos: initial z-position of the plane. >>> * @type: type of the plane (primary, cursor or overlay). >>> * @pixel_formats: supported pixel formats. >>> * @num_pixel_formats: number of elements in 'pixel_formats'. >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c >>> index fd6cb4cee01a..a2bdab836b50 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c >>> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state) >>> static void exynos_drm_plane_reset(struct drm_plane *plane) >>> { >>> + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); >>> struct exynos_drm_plane_state *exynos_state; >>> if (plane->state) { >>> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane) >>> exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); >>> if (exynos_state) { >>> + exynos_state->zpos = exynos_plane->config->zpos; >>> plane->state = &exynos_state->base; >>> plane->state->plane = plane; >>> } >>> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane) >>> return NULL; >>> __drm_atomic_helper_plane_duplicate_state(plane, ©->base); >>> + copy->zpos = exynos_state->zpos; >>> return ©->base; >>> } >>> @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane, >>> kfree(old_exynos_state); >>> } >>> +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane, >>> + struct drm_plane_state *state, >>> + struct drm_property *property, >>> + uint64_t val) >>> +{ >>> + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); >>> + struct exynos_drm_plane_state *exynos_state = >>> + to_exynos_plane_state(state); >>> + struct exynos_drm_private *dev_priv = plane->dev->dev_private; >>> + const struct exynos_drm_plane_config *config = exynos_plane->config; >>> + >>> + if (property == dev_priv->plane_zpos_property && >>> + (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS)) >>> + exynos_state->zpos = val; >>> + else >>> + return -EINVAL; >>> + >>> + return 0; >>> +} >>> + >>> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane, >>> + const struct drm_plane_state *state, >>> + struct drm_property *property, >>> + uint64_t *val) >>> +{ >>> + const struct exynos_drm_plane_state *exynos_state = >>> + container_of(state, const struct exynos_drm_plane_state, base); >>> + struct exynos_drm_private *dev_priv = plane->dev->dev_private; >>> + >>> + if (property == dev_priv->plane_zpos_property) >>> + *val = exynos_state->zpos; >>> + else >>> + return -EINVAL; >>> + >>> + return 0; >>> +} >>> + >>> static struct drm_plane_funcs exynos_plane_funcs = { >>> .update_plane = drm_atomic_helper_update_plane, >>> .disable_plane = drm_atomic_helper_disable_plane, >>> .destroy = drm_plane_cleanup, >>> + .set_property = drm_atomic_helper_plane_set_property, >>> .reset = exynos_drm_plane_reset, >>> .atomic_duplicate_state = exynos_drm_plane_duplicate_state, >>> .atomic_destroy_state = exynos_drm_plane_destroy_state, >>> + .atomic_set_property = exynos_drm_plane_atomic_set_property, >>> + .atomic_get_property = exynos_drm_plane_atomic_get_property, >>> }; >>> static int >>> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane, >>> prop = dev_priv->plane_zpos_property; >>> if (!prop) { >>> - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, >>> - "zpos", 0, MAX_PLANE - 1); >>> + prop = drm_property_create_range(dev, 0, "zpos", >>> + 0, MAX_PLANE - 1); >> User are using zpos property as index now, if we make zpos property >> configurable, i think we need a property immutable like index or a >> property like type or name that can know this plane is which plane >> (e.g. video plane or graphic plane). > > I don't get this. zpos property is attached only to OVERLAY planes, PRIMARY and CURSOR don't have it, so userspace cannot rely on this property. In my patch the initial zpos value is same as before, so if application doesn't relies on zpos values, it will get the same values as now. Application can also check the plane type by reading 'type' property (PRIMARY, OVERLAY or CURSOR). However after my patch application will get the possibility to rearrange plane order for the given crtc by chaning zpos values. That's especially useful to configure 2 typical use cases: PRIMARY plane over OVERLAY plane (PRIMARY is used as typical on-screen-display) or OVERLAY plane over PRIMARY (OVERLAY is used as a window for displaying video data). > >> Another way, user may be possible to check plane index as order of >> plane object id. > > Application can easily find all the planes, which belongs to given crtc by checking 'possible crtcs' plane parameter, I see no additional need for 'index' property in such case. Please note that the initial plane configuration in case of atomic api will be always the same for all application (determined by exynos_drm_plane_reset function) and it will match the current setup. > Thing i want to say is user needs to know which plane is for grahpic0, grahpic1 and video if they belong to mixer but it seems to be enough as the information that user decides to use which plane, possible crtcs, type property, pixel formats supported and zpos. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 588b6763f9c7..f0827dbebb7d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -64,6 +64,7 @@ struct exynos_drm_plane_state { struct exynos_drm_rect src; unsigned int h_ratio; unsigned int v_ratio; + unsigned int zpos; }; static inline struct exynos_drm_plane_state * @@ -91,11 +92,12 @@ struct exynos_drm_plane { #define EXYNOS_DRM_PLANE_CAP_DOUBLE (1 << 0) #define EXYNOS_DRM_PLANE_CAP_SCALE (1 << 1) +#define EXYNOS_DRM_PLANE_CAP_ZPOS (1 << 2) /* * Exynos DRM plane configuration structure. * - * @zpos: z-position of the plane. + * @zpos: initial z-position of the plane. * @type: type of the plane (primary, cursor or overlay). * @pixel_formats: supported pixel formats. * @num_pixel_formats: number of elements in 'pixel_formats'. diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c index fd6cb4cee01a..a2bdab836b50 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state) static void exynos_drm_plane_reset(struct drm_plane *plane) { + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); struct exynos_drm_plane_state *exynos_state; if (plane->state) { @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane) exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL); if (exynos_state) { + exynos_state->zpos = exynos_plane->config->zpos; plane->state = &exynos_state->base; plane->state->plane = plane; } @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane) return NULL; __drm_atomic_helper_plane_duplicate_state(plane, ©->base); + copy->zpos = exynos_state->zpos; return ©->base; } @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane, kfree(old_exynos_state); } +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane, + struct drm_plane_state *state, + struct drm_property *property, + uint64_t val) +{ + struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane); + struct exynos_drm_plane_state *exynos_state = + to_exynos_plane_state(state); + struct exynos_drm_private *dev_priv = plane->dev->dev_private; + const struct exynos_drm_plane_config *config = exynos_plane->config; + + if (property == dev_priv->plane_zpos_property && + (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS)) + exynos_state->zpos = val; + else + return -EINVAL; + + return 0; +} + +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane, + const struct drm_plane_state *state, + struct drm_property *property, + uint64_t *val) +{ + const struct exynos_drm_plane_state *exynos_state = + container_of(state, const struct exynos_drm_plane_state, base); + struct exynos_drm_private *dev_priv = plane->dev->dev_private; + + if (property == dev_priv->plane_zpos_property) + *val = exynos_state->zpos; + else + return -EINVAL; + + return 0; +} + static struct drm_plane_funcs exynos_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, .destroy = drm_plane_cleanup, + .set_property = drm_atomic_helper_plane_set_property, .reset = exynos_drm_plane_reset, .atomic_duplicate_state = exynos_drm_plane_duplicate_state, .atomic_destroy_state = exynos_drm_plane_destroy_state, + .atomic_set_property = exynos_drm_plane_atomic_set_property, + .atomic_get_property = exynos_drm_plane_atomic_get_property, }; static int @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane, prop = dev_priv->plane_zpos_property; if (!prop) { - prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, - "zpos", 0, MAX_PLANE - 1); + prop = drm_property_create_range(dev, 0, "zpos", + 0, MAX_PLANE - 1); if (!prop) return; @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev, exynos_plane->index = index; exynos_plane->config = config; - if (config->type == DRM_PLANE_TYPE_OVERLAY) - exynos_plane_attach_zpos_property(&exynos_plane->base, - config->zpos); + exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos); return 0; }
This patch adds all infrastructure to make zpos plane property configurable from userspace. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 ++- drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 6 deletions(-)