[PATCH 0/5] sti fix to enable atomic modesetting

[PATCH 0/5] sti fix to enable atomic modesetting

[Benjamin Gaignard]

Those patches fix the issues found by using atomictest on sti driver.
After testing we are confident to enable atomic modesetting so
we decide to set DRIVER_ATOMIC flag.

Benjamin Gaignard (4):
  drm: sti: fix potential crash in gdp
  drm: sti: add atomic get/set properties for planes
  drm: sti: set CRTC modesetting parameters
  drm: sti: set DRIVER_ATOMIC for sti

Fabien Dessenne (1):
  drm: sti: fix cursor coordinates

 drivers/gpu/drm/sti/sti_crtc.c   |  1 +
 drivers/gpu/drm/sti/sti_cursor.c |  2 +-
 drivers/gpu/drm/sti/sti_drv.c    |  2 +-
 drivers/gpu/drm/sti/sti_gdp.c    | 13 +++++++------
 drivers/gpu/drm/sti/sti_plane.c  | 40 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 8 deletions(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm: sti: fix potential crash in gdp

[Benjamin Gaignard]

In some cases last_close() could be called before sti_gdp_disable()
and make kernel crash because mixer structure has been destroy.
Let's gdp keep a reference on vtg to fix that (like it is already done
in HQVDP)

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/gpu/drm/sti/sti_gdp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index f9a1d92..990b28e 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -97,6 +97,7 @@ struct sti_gdp_node_list {
  * @vtg_field_nb:       callback for VTG FIELD (top or bottom) notification
  * @is_curr_top:        true if the current node processed is the top field
  * @node_list:          array of node list
+ * @vtg:                registered vtg
  */
 struct sti_gdp {
 	struct sti_plane plane;
@@ -108,6 +109,7 @@ struct sti_gdp {
 	struct notifier_block vtg_field_nb;
 	bool is_curr_top;
 	struct sti_gdp_node_list node_list[GDP_NODE_NB_BANK];
+	struct sti_vtg *vtg;
 };
 
 #define to_sti_gdp(x) container_of(x, struct sti_gdp, plane)
@@ -240,9 +242,6 @@ end:
  */
 static void sti_gdp_disable(struct sti_gdp *gdp)
 {
-	struct drm_plane *drm_plane = &gdp->plane.drm_plane;
-	struct sti_mixer *mixer = to_sti_mixer(drm_plane->crtc);
-	struct sti_compositor *compo = dev_get_drvdata(gdp->dev);
 	unsigned int i;
 
 	DRM_DEBUG_DRIVER("%s\n", sti_plane_to_str(&gdp->plane));
@@ -253,8 +252,7 @@ static void sti_gdp_disable(struct sti_gdp *gdp)
 		gdp->node_list[i].btm_field->gam_gdp_ppt |= GAM_GDP_PPT_IGNORE;
 	}
 
-	if (sti_vtg_unregister_client(mixer->id == STI_MIXER_MAIN ?
-			compo->vtg_main : compo->vtg_aux, &gdp->vtg_field_nb))
+	if (sti_vtg_unregister_client(gdp->vtg, &gdp->vtg_field_nb))
 		DRM_DEBUG_DRIVER("Warning: cannot unregister VTG notifier\n");
 
 	if (gdp->clk_pix)
@@ -490,7 +488,10 @@ static void sti_gdp_atomic_update(struct drm_plane *drm_plane,
 
 	if (first_prepare) {
 		/* Register gdp callback */
-		if (sti_vtg_register_client(mixer->id == STI_MIXER_MAIN ?
+		gdp->vtg = mixer->id == STI_MIXER_MAIN ?
+					compo->vtg_main : compo->vtg_aux;
+
+		if (sti_vtg_register_client(gdp->vtg == STI_MIXER_MAIN ?
 				compo->vtg_main : compo->vtg_aux,
 				&gdp->vtg_field_nb, crtc)) {
 			DRM_ERROR("Cannot register VTG notifier\n");
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm: sti: add atomic get/set properties for planes

[Benjamin Gaignard]

Without those function zpos property isn't displayed in atomic mode.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/gpu/drm/sti/sti_plane.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/sti/sti_plane.c b/drivers/gpu/drm/sti/sti_plane.c
index 2e5c751..e739c5a 100644
--- a/drivers/gpu/drm/sti/sti_plane.c
+++ b/drivers/gpu/drm/sti/sti_plane.c
@@ -69,6 +69,44 @@ static int sti_plane_set_property(struct drm_plane *drm_plane,
 	return -EINVAL;
 }
 
+static int sti_plane_atomic_set_property(struct drm_plane *drm_plane,
+					 struct drm_plane_state *state,
+					 struct drm_property *property,
+					 uint64_t val)
+{
+	struct drm_device *dev = drm_plane->dev;
+	struct sti_private *private = dev->dev_private;
+	struct sti_plane *plane = to_sti_plane(drm_plane);
+
+	DRM_DEBUG_DRIVER("\n");
+
+	if (property == private->plane_zorder_property) {
+		plane->zorder = val;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int sti_plane_atomic_get_property(struct drm_plane *drm_plane,
+					 const struct drm_plane_state *state,
+					 struct drm_property *property,
+					 uint64_t *val)
+{
+	struct drm_device *dev = drm_plane->dev;
+	struct sti_private *private = dev->dev_private;
+	struct sti_plane *plane = to_sti_plane(drm_plane);
+
+	DRM_DEBUG_DRIVER("\n");
+
+	if (property == private->plane_zorder_property) {
+		*val = plane->zorder;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static void sti_plane_attach_zorder_property(struct drm_plane *drm_plane)
 {
 	struct drm_device *dev = drm_plane->dev;
@@ -116,4 +154,6 @@ struct drm_plane_funcs sti_plane_helpers_funcs = {
 	.reset = drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+	.atomic_set_property = sti_plane_atomic_set_property,
+	.atomic_get_property = sti_plane_atomic_get_property,
 };
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm: sti: set CRTC modesetting parameters

[Benjamin Gaignard]

Set CRTC modesetting parameters to avoid warnings in atomic mode.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/gpu/drm/sti/sti_crtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c
index de11c7c..505620c 100644
--- a/drivers/gpu/drm/sti/sti_crtc.c
+++ b/drivers/gpu/drm/sti/sti_crtc.c
@@ -56,6 +56,7 @@ static bool sti_crtc_mode_fixup(struct drm_crtc *crtc,
 				struct drm_display_mode *adjusted_mode)
 {
 	/* accept the provided drm_display_mode, do not fix it up */
+	drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
 	return true;
 }
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm: sti: fix cursor coordinates

[Benjamin Gaignard]

From: Fabien Dessenne <fabien.dessenne@st.com>

x/y typo.

Change-Id: I1ddc0a7e59090a10471db45d862e76756b967236
Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
Reviewed-on: https://gerrit.st.com/47166
Reviewed-by: Denis HUMEAU <denis.humeau@st.com>
Tested-by: Denis HUMEAU <denis.humeau@st.com>
Reviewed-by: Romuald JEANNE <romuald.jeanne@st.com>
---
 drivers/gpu/drm/sti/sti_cursor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
index 8078631..a19693a 100644
--- a/drivers/gpu/drm/sti/sti_cursor.c
+++ b/drivers/gpu/drm/sti/sti_cursor.c
@@ -205,7 +205,7 @@ static void sti_cursor_atomic_update(struct drm_plane *drm_plane,
 	writel(cursor->height << 16 | cursor->width, cursor->regs + CUR_SIZE);
 
 	y = sti_vtg_get_line_number(*mode, dst_y);
-	x = sti_vtg_get_pixel_number(*mode, dst_y);
+	x = sti_vtg_get_pixel_number(*mode, dst_x);
 	writel((y << 16) | x, cursor->regs + CUR_VPO);
 
 	plane->status = STI_PLANE_UPDATED;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm: sti: set DRIVER_ATOMIC for sti

[Benjamin Gaignard]

sti now support of atomic modesetting so set the flag to enable it.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/gpu/drm/sti/sti_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
index 1469987..a6f9ff2 100644
--- a/drivers/gpu/drm/sti/sti_drv.c
+++ b/drivers/gpu/drm/sti/sti_drv.c
@@ -191,7 +191,7 @@ static struct dma_buf *sti_gem_prime_export(struct drm_device *dev,
 
 static struct drm_driver sti_driver = {
 	.driver_features = DRIVER_HAVE_IRQ | DRIVER_MODESET |
-	    DRIVER_GEM | DRIVER_PRIME,
+	    DRIVER_GEM | DRIVER_PRIME | DRIVER_ATOMIC,
 	.load = sti_load,
 	.gem_free_object = drm_gem_cma_free_object,
 	.gem_vm_ops = &drm_gem_cma_vm_ops,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: sti: add atomic get/set properties for planes

[Daniel Vetter]

On Thu, Jan 07, 2016 at 02:30:38PM +0100, Benjamin Gaignard wrote:
> Without those function zpos property isn't displayed in atomic mode.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

Oh, another iteration of a driver-private zpos property. This _really_
should be generic, with some consistent cross-driver semantics. Marek from
samsung has started with a patch series for that.

Can you please work together with him, and rebase the sti zpos support on
top of his work? And hold of this patch for now meanwhile?

Also adding Marek.

Thanks, Daniel

> ---
>  drivers/gpu/drm/sti/sti_plane.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sti/sti_plane.c b/drivers/gpu/drm/sti/sti_plane.c
> index 2e5c751..e739c5a 100644
> --- a/drivers/gpu/drm/sti/sti_plane.c
> +++ b/drivers/gpu/drm/sti/sti_plane.c
> @@ -69,6 +69,44 @@ static int sti_plane_set_property(struct drm_plane *drm_plane,
>  	return -EINVAL;
>  }
>  
> +static int sti_plane_atomic_set_property(struct drm_plane *drm_plane,
> +					 struct drm_plane_state *state,
> +					 struct drm_property *property,
> +					 uint64_t val)
> +{
> +	struct drm_device *dev = drm_plane->dev;
> +	struct sti_private *private = dev->dev_private;
> +	struct sti_plane *plane = to_sti_plane(drm_plane);
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	if (property == private->plane_zorder_property) {
> +		plane->zorder = val;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int sti_plane_atomic_get_property(struct drm_plane *drm_plane,
> +					 const struct drm_plane_state *state,
> +					 struct drm_property *property,
> +					 uint64_t *val)
> +{
> +	struct drm_device *dev = drm_plane->dev;
> +	struct sti_private *private = dev->dev_private;
> +	struct sti_plane *plane = to_sti_plane(drm_plane);
> +
> +	DRM_DEBUG_DRIVER("\n");
> +
> +	if (property == private->plane_zorder_property) {
> +		*val = plane->zorder;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static void sti_plane_attach_zorder_property(struct drm_plane *drm_plane)
>  {
>  	struct drm_device *dev = drm_plane->dev;
> @@ -116,4 +154,6 @@ struct drm_plane_funcs sti_plane_helpers_funcs = {
>  	.reset = drm_atomic_helper_plane_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +	.atomic_set_property = sti_plane_atomic_set_property,
> +	.atomic_get_property = sti_plane_atomic_get_property,
>  };
> -- 
> 1.9.1
> 
> _______________________________________________
> 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

Re: [PATCH 3/5] drm: sti: set CRTC modesetting parameters

[Daniel Vetter]

On Thu, Jan 07, 2016 at 02:30:39PM +0100, Benjamin Gaignard wrote:
> Set CRTC modesetting parameters to avoid warnings in atomic mode.

Hm, should we extend the documentation for the mode_fixup hooks that this
should be done? Care to submit a patch against drm-misc/linux-next (since
there's more doc work queued in there)?

Thanks, Daniel

> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/gpu/drm/sti/sti_crtc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c
> index de11c7c..505620c 100644
> --- a/drivers/gpu/drm/sti/sti_crtc.c
> +++ b/drivers/gpu/drm/sti/sti_crtc.c
> @@ -56,6 +56,7 @@ static bool sti_crtc_mode_fixup(struct drm_crtc *crtc,
>  				struct drm_display_mode *adjusted_mode)
>  {
>  	/* accept the provided drm_display_mode, do not fix it up */
> +	drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
>  	return true;
>  }
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> 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

Re: [PATCH 2/5] drm: sti: add atomic get/set properties for planes

[Benjamin Gaignard]

[-- Attachment #1.1: Type: text/plain, Size: 4112 bytes --]

zpos property isn't new in sti driver only the atomic set/get functions are
new.
Without those functions atomictest failed to discover any of the other
atomic planes properties.

I agree with you, zpos should be generic and I could work with Marek for
that
but if this patch isn't merge sti atomic modesetting support will be
limited to primary planes...

Benjamin


2016-01-07 14:50 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Jan 07, 2016 at 02:30:38PM +0100, Benjamin Gaignard wrote:
> > Without those function zpos property isn't displayed in atomic mode.
> >
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>
> Oh, another iteration of a driver-private zpos property. This _really_
> should be generic, with some consistent cross-driver semantics. Marek from
> samsung has started with a patch series for that.
>
> Can you please work together with him, and rebase the sti zpos support on
> top of his work? And hold of this patch for now meanwhile?
>
> Also adding Marek.
>
> Thanks, Daniel
>
> > ---
> >  drivers/gpu/drm/sti/sti_plane.c | 40
> ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/sti/sti_plane.c
> b/drivers/gpu/drm/sti/sti_plane.c
> > index 2e5c751..e739c5a 100644
> > --- a/drivers/gpu/drm/sti/sti_plane.c
> > +++ b/drivers/gpu/drm/sti/sti_plane.c
> > @@ -69,6 +69,44 @@ static int sti_plane_set_property(struct drm_plane
> *drm_plane,
> >       return -EINVAL;
> >  }
> >
> > +static int sti_plane_atomic_set_property(struct drm_plane *drm_plane,
> > +                                      struct drm_plane_state *state,
> > +                                      struct drm_property *property,
> > +                                      uint64_t val)
> > +{
> > +     struct drm_device *dev = drm_plane->dev;
> > +     struct sti_private *private = dev->dev_private;
> > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > +
> > +     DRM_DEBUG_DRIVER("\n");
> > +
> > +     if (property == private->plane_zorder_property) {
> > +             plane->zorder = val;
> > +             return 0;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int sti_plane_atomic_get_property(struct drm_plane *drm_plane,
> > +                                      const struct drm_plane_state
> *state,
> > +                                      struct drm_property *property,
> > +                                      uint64_t *val)
> > +{
> > +     struct drm_device *dev = drm_plane->dev;
> > +     struct sti_private *private = dev->dev_private;
> > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > +
> > +     DRM_DEBUG_DRIVER("\n");
> > +
> > +     if (property == private->plane_zorder_property) {
> > +             *val = plane->zorder;
> > +             return 0;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> >  static void sti_plane_attach_zorder_property(struct drm_plane
> *drm_plane)
> >  {
> >       struct drm_device *dev = drm_plane->dev;
> > @@ -116,4 +154,6 @@ struct drm_plane_funcs sti_plane_helpers_funcs = {
> >       .reset = drm_atomic_helper_plane_reset,
> >       .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >       .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > +     .atomic_set_property = sti_plane_atomic_set_property,
> > +     .atomic_get_property = sti_plane_atomic_get_property,
> >  };
> > --
> > 1.9.1
> >
> > _______________________________________________
> > 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
>



-- 

Benjamin Gaignard

Graphic Working Group

Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs

Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> | Twitter
<http://twitter.com/#!/linaroorg> | Blog
<http://www.linaro.org/linaro-blog/>

[-- Attachment #1.2: Type: text/html, Size: 6832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: sti: add atomic get/set properties for planes

[Daniel Vetter]

On Thu, Jan 07, 2016 at 03:05:02PM +0100, Benjamin Gaignard wrote:
> zpos property isn't new in sti driver only the atomic set/get functions are
> new.
> Without those functions atomictest failed to discover any of the other
> atomic planes properties.
> 
> I agree with you, zpos should be generic and I could work with Marek for
> that
> but if this patch isn't merge sti atomic modesetting support will be
> limited to primary planes...

These two hooks should be optional. The only downside of not adding them
is that zpos property can't be read, everything else should still work ...

And yeah everyone added zpos without a hole lot of thought about
cross-driver consistency, which is not great. Now everyone gets to clean
things up again I'd say.
-Daniel

> 
> Benjamin
> 
> 
> 2016-01-07 14:50 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> 
> > On Thu, Jan 07, 2016 at 02:30:38PM +0100, Benjamin Gaignard wrote:
> > > Without those function zpos property isn't displayed in atomic mode.
> > >
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >
> > Oh, another iteration of a driver-private zpos property. This _really_
> > should be generic, with some consistent cross-driver semantics. Marek from
> > samsung has started with a patch series for that.
> >
> > Can you please work together with him, and rebase the sti zpos support on
> > top of his work? And hold of this patch for now meanwhile?
> >
> > Also adding Marek.
> >
> > Thanks, Daniel
> >
> > > ---
> > >  drivers/gpu/drm/sti/sti_plane.c | 40
> > ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 40 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/sti/sti_plane.c
> > b/drivers/gpu/drm/sti/sti_plane.c
> > > index 2e5c751..e739c5a 100644
> > > --- a/drivers/gpu/drm/sti/sti_plane.c
> > > +++ b/drivers/gpu/drm/sti/sti_plane.c
> > > @@ -69,6 +69,44 @@ static int sti_plane_set_property(struct drm_plane
> > *drm_plane,
> > >       return -EINVAL;
> > >  }
> > >
> > > +static int sti_plane_atomic_set_property(struct drm_plane *drm_plane,
> > > +                                      struct drm_plane_state *state,
> > > +                                      struct drm_property *property,
> > > +                                      uint64_t val)
> > > +{
> > > +     struct drm_device *dev = drm_plane->dev;
> > > +     struct sti_private *private = dev->dev_private;
> > > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > > +
> > > +     DRM_DEBUG_DRIVER("\n");
> > > +
> > > +     if (property == private->plane_zorder_property) {
> > > +             plane->zorder = val;
> > > +             return 0;
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > > +static int sti_plane_atomic_get_property(struct drm_plane *drm_plane,
> > > +                                      const struct drm_plane_state
> > *state,
> > > +                                      struct drm_property *property,
> > > +                                      uint64_t *val)
> > > +{
> > > +     struct drm_device *dev = drm_plane->dev;
> > > +     struct sti_private *private = dev->dev_private;
> > > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > > +
> > > +     DRM_DEBUG_DRIVER("\n");
> > > +
> > > +     if (property == private->plane_zorder_property) {
> > > +             *val = plane->zorder;
> > > +             return 0;
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > >  static void sti_plane_attach_zorder_property(struct drm_plane
> > *drm_plane)
> > >  {
> > >       struct drm_device *dev = drm_plane->dev;
> > > @@ -116,4 +154,6 @@ struct drm_plane_funcs sti_plane_helpers_funcs = {
> > >       .reset = drm_atomic_helper_plane_reset,
> > >       .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >       .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > +     .atomic_set_property = sti_plane_atomic_set_property,
> > > +     .atomic_get_property = sti_plane_atomic_get_property,
> > >  };
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > 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
> >
> 
> 
> 
> -- 
> 
> Benjamin Gaignard
> 
> Graphic Working Group
> 
> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
> 
> Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> | Twitter
> <http://twitter.com/#!/linaroorg> | Blog
> <http://www.linaro.org/linaro-blog/>

-- 
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

Re: [PATCH 2/5] drm: sti: add atomic get/set properties for planes

[Benjamin Gaignard]

[-- Attachment #1.1: Type: text/plain, Size: 5682 bytes --]

Hi,

I have been able to implement zpos in generic way using Marek's patches (v3)
so I'm abandon this patch and wait to see Marek's patches merged to propose
a new one.

Regards,
Benjamin

2016-01-07 15:47 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Jan 07, 2016 at 03:05:02PM +0100, Benjamin Gaignard wrote:
> > zpos property isn't new in sti driver only the atomic set/get functions
> are
> > new.
> > Without those functions atomictest failed to discover any of the other
> > atomic planes properties.
> >
> > I agree with you, zpos should be generic and I could work with Marek for
> > that
> > but if this patch isn't merge sti atomic modesetting support will be
> > limited to primary planes...
>
> These two hooks should be optional. The only downside of not adding them
> is that zpos property can't be read, everything else should still work ...
>
> And yeah everyone added zpos without a hole lot of thought about
> cross-driver consistency, which is not great. Now everyone gets to clean
> things up again I'd say.
> -Daniel
>
> >
> > Benjamin
> >
> >
> > 2016-01-07 14:50 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> >
> > > On Thu, Jan 07, 2016 at 02:30:38PM +0100, Benjamin Gaignard wrote:
> > > > Without those function zpos property isn't displayed in atomic mode.
> > > >
> > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > >
> > > Oh, another iteration of a driver-private zpos property. This _really_
> > > should be generic, with some consistent cross-driver semantics. Marek
> from
> > > samsung has started with a patch series for that.
> > >
> > > Can you please work together with him, and rebase the sti zpos support
> on
> > > top of his work? And hold of this patch for now meanwhile?
> > >
> > > Also adding Marek.
> > >
> > > Thanks, Daniel
> > >
> > > > ---
> > > >  drivers/gpu/drm/sti/sti_plane.c | 40
> > > ++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 40 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/sti/sti_plane.c
> > > b/drivers/gpu/drm/sti/sti_plane.c
> > > > index 2e5c751..e739c5a 100644
> > > > --- a/drivers/gpu/drm/sti/sti_plane.c
> > > > +++ b/drivers/gpu/drm/sti/sti_plane.c
> > > > @@ -69,6 +69,44 @@ static int sti_plane_set_property(struct drm_plane
> > > *drm_plane,
> > > >       return -EINVAL;
> > > >  }
> > > >
> > > > +static int sti_plane_atomic_set_property(struct drm_plane
> *drm_plane,
> > > > +                                      struct drm_plane_state *state,
> > > > +                                      struct drm_property *property,
> > > > +                                      uint64_t val)
> > > > +{
> > > > +     struct drm_device *dev = drm_plane->dev;
> > > > +     struct sti_private *private = dev->dev_private;
> > > > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > > > +
> > > > +     DRM_DEBUG_DRIVER("\n");
> > > > +
> > > > +     if (property == private->plane_zorder_property) {
> > > > +             plane->zorder = val;
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     return -EINVAL;
> > > > +}
> > > > +
> > > > +static int sti_plane_atomic_get_property(struct drm_plane
> *drm_plane,
> > > > +                                      const struct drm_plane_state
> > > *state,
> > > > +                                      struct drm_property *property,
> > > > +                                      uint64_t *val)
> > > > +{
> > > > +     struct drm_device *dev = drm_plane->dev;
> > > > +     struct sti_private *private = dev->dev_private;
> > > > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > > > +
> > > > +     DRM_DEBUG_DRIVER("\n");
> > > > +
> > > > +     if (property == private->plane_zorder_property) {
> > > > +             *val = plane->zorder;
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     return -EINVAL;
> > > > +}
> > > > +
> > > >  static void sti_plane_attach_zorder_property(struct drm_plane
> > > *drm_plane)
> > > >  {
> > > >       struct drm_device *dev = drm_plane->dev;
> > > > @@ -116,4 +154,6 @@ struct drm_plane_funcs sti_plane_helpers_funcs =
> {
> > > >       .reset = drm_atomic_helper_plane_reset,
> > > >       .atomic_duplicate_state =
> drm_atomic_helper_plane_duplicate_state,
> > > >       .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > > +     .atomic_set_property = sti_plane_atomic_set_property,
> > > > +     .atomic_get_property = sti_plane_atomic_get_property,
> > > >  };
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > 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
> > >
> >
> >
> >
> > --
> >
> > Benjamin Gaignard
> >
> > Graphic Working Group
> >
> > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM
> SoCs
> >
> > Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> |
> Twitter
> > <http://twitter.com/#!/linaroorg> | Blog
> > <http://www.linaro.org/linaro-blog/>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>



-- 

Benjamin Gaignard

Graphic Working Group

Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs

Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> | Twitter
<http://twitter.com/#!/linaroorg> | Blog
<http://www.linaro.org/linaro-blog/>

[-- Attachment #1.2: Type: text/html, Size: 9590 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm: sti: add atomic get/set properties for planes

[Daniel Vetter]

On Wed, Jan 13, 2016 at 09:53:32AM +0100, Benjamin Gaignard wrote:
> Hi,
> 
> I have been able to implement zpos in generic way using Marek's patches (v3)
> so I'm abandon this patch and wait to see Marek's patches merged to propose
> a new one.

Imo Marek's patch is pretty much ready. Feel like doing a full review on
it?
-Daniel

> 
> Regards,
> Benjamin
> 
> 2016-01-07 15:47 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> 
> > On Thu, Jan 07, 2016 at 03:05:02PM +0100, Benjamin Gaignard wrote:
> > > zpos property isn't new in sti driver only the atomic set/get functions
> > are
> > > new.
> > > Without those functions atomictest failed to discover any of the other
> > > atomic planes properties.
> > >
> > > I agree with you, zpos should be generic and I could work with Marek for
> > > that
> > > but if this patch isn't merge sti atomic modesetting support will be
> > > limited to primary planes...
> >
> > These two hooks should be optional. The only downside of not adding them
> > is that zpos property can't be read, everything else should still work ...
> >
> > And yeah everyone added zpos without a hole lot of thought about
> > cross-driver consistency, which is not great. Now everyone gets to clean
> > things up again I'd say.
> > -Daniel
> >
> > >
> > > Benjamin
> > >
> > >
> > > 2016-01-07 14:50 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> > >
> > > > On Thu, Jan 07, 2016 at 02:30:38PM +0100, Benjamin Gaignard wrote:
> > > > > Without those function zpos property isn't displayed in atomic mode.
> > > > >
> > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > > >
> > > > Oh, another iteration of a driver-private zpos property. This _really_
> > > > should be generic, with some consistent cross-driver semantics. Marek
> > from
> > > > samsung has started with a patch series for that.
> > > >
> > > > Can you please work together with him, and rebase the sti zpos support
> > on
> > > > top of his work? And hold of this patch for now meanwhile?
> > > >
> > > > Also adding Marek.
> > > >
> > > > Thanks, Daniel
> > > >
> > > > > ---
> > > > >  drivers/gpu/drm/sti/sti_plane.c | 40
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 40 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sti/sti_plane.c
> > > > b/drivers/gpu/drm/sti/sti_plane.c
> > > > > index 2e5c751..e739c5a 100644
> > > > > --- a/drivers/gpu/drm/sti/sti_plane.c
> > > > > +++ b/drivers/gpu/drm/sti/sti_plane.c
> > > > > @@ -69,6 +69,44 @@ static int sti_plane_set_property(struct drm_plane
> > > > *drm_plane,
> > > > >       return -EINVAL;
> > > > >  }
> > > > >
> > > > > +static int sti_plane_atomic_set_property(struct drm_plane
> > *drm_plane,
> > > > > +                                      struct drm_plane_state *state,
> > > > > +                                      struct drm_property *property,
> > > > > +                                      uint64_t val)
> > > > > +{
> > > > > +     struct drm_device *dev = drm_plane->dev;
> > > > > +     struct sti_private *private = dev->dev_private;
> > > > > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > > > > +
> > > > > +     DRM_DEBUG_DRIVER("\n");
> > > > > +
> > > > > +     if (property == private->plane_zorder_property) {
> > > > > +             plane->zorder = val;
> > > > > +             return 0;
> > > > > +     }
> > > > > +
> > > > > +     return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static int sti_plane_atomic_get_property(struct drm_plane
> > *drm_plane,
> > > > > +                                      const struct drm_plane_state
> > > > *state,
> > > > > +                                      struct drm_property *property,
> > > > > +                                      uint64_t *val)
> > > > > +{
> > > > > +     struct drm_device *dev = drm_plane->dev;
> > > > > +     struct sti_private *private = dev->dev_private;
> > > > > +     struct sti_plane *plane = to_sti_plane(drm_plane);
> > > > > +
> > > > > +     DRM_DEBUG_DRIVER("\n");
> > > > > +
> > > > > +     if (property == private->plane_zorder_property) {
> > > > > +             *val = plane->zorder;
> > > > > +             return 0;
> > > > > +     }
> > > > > +
> > > > > +     return -EINVAL;
> > > > > +}
> > > > > +
> > > > >  static void sti_plane_attach_zorder_property(struct drm_plane
> > > > *drm_plane)
> > > > >  {
> > > > >       struct drm_device *dev = drm_plane->dev;
> > > > > @@ -116,4 +154,6 @@ struct drm_plane_funcs sti_plane_helpers_funcs =
> > {
> > > > >       .reset = drm_atomic_helper_plane_reset,
> > > > >       .atomic_duplicate_state =
> > drm_atomic_helper_plane_duplicate_state,
> > > > >       .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > > > > +     .atomic_set_property = sti_plane_atomic_set_property,
> > > > > +     .atomic_get_property = sti_plane_atomic_get_property,
> > > > >  };
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > 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
> > > >
> > >
> > >
> > >
> > > --
> > >
> > > Benjamin Gaignard
> > >
> > > Graphic Working Group
> > >
> > > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM
> > SoCs
> > >
> > > Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> |
> > Twitter
> > > <http://twitter.com/#!/linaroorg> | Blog
> > > <http://www.linaro.org/linaro-blog/>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> >
> 
> 
> 
> -- 
> 
> Benjamin Gaignard
> 
> Graphic Working Group
> 
> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
> 
> Follow *Linaro: *Facebook <http://www.facebook.com/pages/Linaro> | Twitter
> <http://twitter.com/#!/linaroorg> | Blog
> <http://www.linaro.org/linaro-blog/>

-- 
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