[PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Jyri Sarha]

The old implementation of placing planes on the CRTC while configuring
the planes was naive and relied on the order in which the planes were
configured, enabled, and disabled. The situation where a plane's zpos
was changed on the fly was completely broken. The usual symptoms of
this problem was scrambled display and a flood of sync lost errors,
when a plane was active in two layers at the same time, or a missing
plane, in case when a layer was accidentally disabled.

The rewrite takes a more straight forward approach when when HW is
concerned. The plane positioning registers are in the CRTC (or
actually OVR) register space and it is more natural to configure them
in a one go when configuring the CRTC. This is easy since we have
access to the whole atomic state when updating the CRTC configuration.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
 drivers/gpu/drm/tidss/tidss_dispc.c | 66 ++++++++++++++++++++---------
 2 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 032c31ee2820..367efdebe2f8 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -143,7 +143,7 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
 	if (WARN_ON(!crtc->state->event))
 		return;
 
-	/* Write vp properties to HW if needed. */
+	/* Write vp properties and plane positions to HW if needed. */
 	dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false);
 
 	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index eeb160dc047b..cfc230d2a88a 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -20,6 +20,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
@@ -281,11 +282,6 @@ struct dss_vp_data {
 	u32 *gamma_table;
 };
 
-struct dss_plane_data {
-	u32 zorder;
-	u32 hw_videoport;
-};
-
 struct dispc_device {
 	struct tidss_device *tidss;
 	struct device *dev;
@@ -307,8 +303,6 @@ struct dispc_device {
 
 	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
 
-	struct dss_plane_data plane_data[TIDSS_MAX_PLANES];
-
 	u32 *fourccs;
 	u32 num_fourccs;
 
@@ -1301,13 +1295,54 @@ static void dispc_ovr_set_plane(struct dispc_device *dispc,
 	}
 }
 
-static void dispc_ovr_enable_plane(struct dispc_device *dispc,
-				   u32 hw_videoport, u32 zpos, bool enable)
+static void dispc_ovr_enable_layer(struct dispc_device *dispc,
+				   u32 hw_videoport, u32 layer, bool enable)
 {
-	OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
+	if (dispc->feat->subrev == DISPC_K2G)
+		return;
+
+	OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
 			!!enable, 0, 0);
 }
 
+static void dispc_vp_position_planes(struct dispc_device *dispc,
+				     u32 hw_videoport,
+				     const struct drm_crtc_state *cstate,
+				     bool newmodeset)
+{
+	struct drm_device *ddev = &dispc->tidss->ddev;
+	int zpos;
+
+	if (!cstate->zpos_changed && !cstate->planes_changed && !newmodeset)
+		return;
+
+	for (zpos = 0; zpos < dispc->feat->num_planes; zpos++) {
+		struct drm_plane *plane;
+		bool zpos_taken = false;
+
+		drm_for_each_plane_mask(plane, ddev, cstate->plane_mask) {
+			if (WARN_ON(!plane->state))
+				continue;
+
+			if (plane->state->normalized_zpos == zpos) {
+				zpos_taken = true;
+				break;
+			}
+		}
+
+		if (zpos_taken) {
+			struct tidss_plane *tplane = to_tidss_plane(plane);
+			const struct drm_plane_state *pstate = plane->state;
+
+			dispc_ovr_set_plane(dispc, tplane->hw_plane_id,
+					    hw_videoport,
+					    pstate->crtc_x, pstate->crtc_y,
+					    zpos);
+		}
+		dispc_ovr_enable_layer(dispc, hw_videoport, zpos, zpos_taken);
+	}
+}
+
 /* CSC */
 enum csc_ctm {
 	CSC_RR, CSC_RG, CSC_RB,
@@ -2070,21 +2105,11 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
 		VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, 0,
 				28, 28);
 
-	dispc_ovr_set_plane(dispc, hw_plane, hw_videoport,
-			    state->crtc_x, state->crtc_y,
-			    state->normalized_zpos);
-
-	dispc->plane_data[hw_plane].zorder = state->normalized_zpos;
-	dispc->plane_data[hw_plane].hw_videoport = hw_videoport;
-
 	return 0;
 }
 
 int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable)
 {
-	dispc_ovr_enable_plane(dispc, dispc->plane_data[hw_plane].hw_videoport,
-			       dispc->plane_data[hw_plane].zorder, enable);
-
 	VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, !!enable, 0, 0);
 
 	return 0;
@@ -2566,6 +2591,7 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
 {
 	dispc_vp_set_default_color(dispc, hw_videoport, 0);
 	dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
+	dispc_vp_position_planes(dispc, hw_videoport, state, newmodeset);
 }
 
 int dispc_runtime_suspend(struct dispc_device *dispc)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Jyri Sarha]

On 07/02/2020 20:18, Jyri Sarha wrote:
> The old implementation of placing planes on the CRTC while configuring
> the planes was naive and relied on the order in which the planes were
> configured, enabled, and disabled. The situation where a plane's zpos
> was changed on the fly was completely broken. The usual symptoms of
> this problem was scrambled display and a flood of sync lost errors,
> when a plane was active in two layers at the same time, or a missing
> plane, in case when a layer was accidentally disabled.
> 
> The rewrite takes a more straight forward approach when when HW is
> concerned. The plane positioning registers are in the CRTC (or
> actually OVR) register space and it is more natural to configure them
> in a one go when configuring the CRTC. This is easy since we have
> access to the whole atomic state when updating the CRTC configuration.
> 

While implementing this fix it caught me by surprise that
crtc->state->state (pointer up to full atomic state) is NULL when
crtc_enable() or -flush() is called. So I take the plane-state directly
from the plane->state and just assume that it is pointing to the same
atomic state with the crtc state I am having. I that alraight?

Why is the crtc->state->state NULL? Is it a bug or is there some reason
to it?

Best regards,
Jyri

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
>  drivers/gpu/drm/tidss/tidss_dispc.c | 66 ++++++++++++++++++++---------
>  2 files changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 032c31ee2820..367efdebe2f8 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -143,7 +143,7 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>  	if (WARN_ON(!crtc->state->event))
>  		return;
>  
> -	/* Write vp properties to HW if needed. */
> +	/* Write vp properties and plane positions to HW if needed. */
>  	dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false);
>  
>  	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index eeb160dc047b..cfc230d2a88a 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -20,6 +20,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> @@ -281,11 +282,6 @@ struct dss_vp_data {
>  	u32 *gamma_table;
>  };
>  
> -struct dss_plane_data {
> -	u32 zorder;
> -	u32 hw_videoport;
> -};
> -
>  struct dispc_device {
>  	struct tidss_device *tidss;
>  	struct device *dev;
> @@ -307,8 +303,6 @@ struct dispc_device {
>  
>  	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
>  
> -	struct dss_plane_data plane_data[TIDSS_MAX_PLANES];
> -
>  	u32 *fourccs;
>  	u32 num_fourccs;
>  
> @@ -1301,13 +1295,54 @@ static void dispc_ovr_set_plane(struct dispc_device *dispc,
>  	}
>  }
>  
> -static void dispc_ovr_enable_plane(struct dispc_device *dispc,
> -				   u32 hw_videoport, u32 zpos, bool enable)
> +static void dispc_ovr_enable_layer(struct dispc_device *dispc,
> +				   u32 hw_videoport, u32 layer, bool enable)
>  {
> -	OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos),
> +	if (dispc->feat->subrev == DISPC_K2G)
> +		return;
> +
> +	OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
>  			!!enable, 0, 0);
>  }
>  
> +static void dispc_vp_position_planes(struct dispc_device *dispc,
> +				     u32 hw_videoport,
> +				     const struct drm_crtc_state *cstate,
> +				     bool newmodeset)
> +{
> +	struct drm_device *ddev = &dispc->tidss->ddev;
> +	int zpos;
> +
> +	if (!cstate->zpos_changed && !cstate->planes_changed && !newmodeset)
> +		return;
> +
> +	for (zpos = 0; zpos < dispc->feat->num_planes; zpos++) {
> +		struct drm_plane *plane;
> +		bool zpos_taken = false;
> +
> +		drm_for_each_plane_mask(plane, ddev, cstate->plane_mask) {
> +			if (WARN_ON(!plane->state))
> +				continue;
> +
> +			if (plane->state->normalized_zpos == zpos) {
> +				zpos_taken = true;
> +				break;
> +			}
> +		}
> +
> +		if (zpos_taken) {
> +			struct tidss_plane *tplane = to_tidss_plane(plane);
> +			const struct drm_plane_state *pstate = plane->state;
> +
> +			dispc_ovr_set_plane(dispc, tplane->hw_plane_id,
> +					    hw_videoport,
> +					    pstate->crtc_x, pstate->crtc_y,
> +					    zpos);
> +		}
> +		dispc_ovr_enable_layer(dispc, hw_videoport, zpos, zpos_taken);
> +	}
> +}
> +
>  /* CSC */
>  enum csc_ctm {
>  	CSC_RR, CSC_RG, CSC_RB,
> @@ -2070,21 +2105,11 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>  		VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, 0,
>  				28, 28);
>  
> -	dispc_ovr_set_plane(dispc, hw_plane, hw_videoport,
> -			    state->crtc_x, state->crtc_y,
> -			    state->normalized_zpos);
> -
> -	dispc->plane_data[hw_plane].zorder = state->normalized_zpos;
> -	dispc->plane_data[hw_plane].hw_videoport = hw_videoport;
> -
>  	return 0;
>  }
>  
>  int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable)
>  {
> -	dispc_ovr_enable_plane(dispc, dispc->plane_data[hw_plane].hw_videoport,
> -			       dispc->plane_data[hw_plane].zorder, enable);
> -
>  	VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, !!enable, 0, 0);
>  
>  	return 0;
> @@ -2566,6 +2591,7 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
>  {
>  	dispc_vp_set_default_color(dispc, hw_videoport, 0);
>  	dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
> +	dispc_vp_position_planes(dispc, hw_videoport, state, newmodeset);
>  }
>  
>  int dispc_runtime_suspend(struct dispc_device *dispc)
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 08:26:17PM +0200, Jyri Sarha wrote:
> On 07/02/2020 20:18, Jyri Sarha wrote:
> > The old implementation of placing planes on the CRTC while configuring
> > the planes was naive and relied on the order in which the planes were
> > configured, enabled, and disabled. The situation where a plane's zpos
> > was changed on the fly was completely broken. The usual symptoms of
> > this problem was scrambled display and a flood of sync lost errors,
> > when a plane was active in two layers at the same time, or a missing
> > plane, in case when a layer was accidentally disabled.
> > 
> > The rewrite takes a more straight forward approach when when HW is
> > concerned. The plane positioning registers are in the CRTC (or
> > actually OVR) register space and it is more natural to configure them
> > in a one go when configuring the CRTC. This is easy since we have
> > access to the whole atomic state when updating the CRTC configuration.
> > 
> 
> While implementing this fix it caught me by surprise that
> crtc->state->state (pointer up to full atomic state) is NULL when
> crtc_enable() or -flush() is called. So I take the plane-state directly
> from the plane->state and just assume that it is pointing to the same
> atomic state with the crtc state I am having. I that alraight?

IMO you should never use plane->state etc. Better pass down the
full atomic state everywhere. Otherwise you can never even consider
increasing the commit queue depth since you'd end up accessing the
wrong state.

> 
> Why is the crtc->state->state NULL? Is it a bug or is there some reason
> to it?

Currently swap_state() moves that state pointer from the new obj state
to the old obj state, and clears the one in the new obj state. Not entirely
sure why, but maybe just so there isn't a stale ->state pointer hanging 
around in the obj->state after the swap?

I think a better way could be to not clobber the old obj state at
all, leave the new_obj_state->state alone, and just clear the ->state
pointer .duplicate_state(). But that would require reviewing a bunch
of code to find all the places where old_obj_state->state gets used
during the commit.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Jyri Sarha]

On 07/02/2020 20:45, Ville Syrjälä wrote:
> On Fri, Feb 07, 2020 at 08:26:17PM +0200, Jyri Sarha wrote:
>> On 07/02/2020 20:18, Jyri Sarha wrote:
>>> The old implementation of placing planes on the CRTC while configuring
>>> the planes was naive and relied on the order in which the planes were
>>> configured, enabled, and disabled. The situation where a plane's zpos
>>> was changed on the fly was completely broken. The usual symptoms of
>>> this problem was scrambled display and a flood of sync lost errors,
>>> when a plane was active in two layers at the same time, or a missing
>>> plane, in case when a layer was accidentally disabled.
>>>
>>> The rewrite takes a more straight forward approach when when HW is
>>> concerned. The plane positioning registers are in the CRTC (or
>>> actually OVR) register space and it is more natural to configure them
>>> in a one go when configuring the CRTC. This is easy since we have
>>> access to the whole atomic state when updating the CRTC configuration.
>>>
>>
>> While implementing this fix it caught me by surprise that
>> crtc->state->state (pointer up to full atomic state) is NULL when
>> crtc_enable() or -flush() is called. So I take the plane-state directly
>> from the plane->state and just assume that it is pointing to the same
>> atomic state with the crtc state I am having. I that alraight?
> 
> IMO you should never use plane->state etc. Better pass down the
> full atomic state everywhere. Otherwise you can never even consider
> increasing the commit queue depth since you'd end up accessing the
> wrong state.
>

Ok. I did explore this a bit and it starts to look like that I have to
store the planes' zpos values in the driver after all. Only the changes
are available in the drm_atomic_state being commited so I have to
maintain the full state myself. That is if I should not use plane->state
in crtc_enable() or -flush().

>>
>> Why is the crtc->state->state NULL? Is it a bug or is there some reason
>> to it?
> 
> Currently swap_state() moves that state pointer from the new obj state
> to the old obj state, and clears the one in the new obj state. Not entirely
> sure why, but maybe just so there isn't a stale ->state pointer hanging 
> around in the obj->state after the swap?
> 
> I think a better way could be to not clobber the old obj state at
> all, leave the new_obj_state->state alone, and just clear the ->state
> pointer .duplicate_state(). But that would require reviewing a bunch
> of code to find all the places where old_obj_state->state gets used
> during the commit.
> 

I think those places are many, since at least I did not figure out any
other way to access the full commit behind the atomic helpers.

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Ville Syrjälä]

On Sun, Feb 09, 2020 at 02:50:09PM +0200, Jyri Sarha wrote:
> On 07/02/2020 20:45, Ville Syrjälä wrote:
> > On Fri, Feb 07, 2020 at 08:26:17PM +0200, Jyri Sarha wrote:
> >> On 07/02/2020 20:18, Jyri Sarha wrote:
> >>> The old implementation of placing planes on the CRTC while configuring
> >>> the planes was naive and relied on the order in which the planes were
> >>> configured, enabled, and disabled. The situation where a plane's zpos
> >>> was changed on the fly was completely broken. The usual symptoms of
> >>> this problem was scrambled display and a flood of sync lost errors,
> >>> when a plane was active in two layers at the same time, or a missing
> >>> plane, in case when a layer was accidentally disabled.
> >>>
> >>> The rewrite takes a more straight forward approach when when HW is
> >>> concerned. The plane positioning registers are in the CRTC (or
> >>> actually OVR) register space and it is more natural to configure them
> >>> in a one go when configuring the CRTC. This is easy since we have
> >>> access to the whole atomic state when updating the CRTC configuration.
> >>>
> >>
> >> While implementing this fix it caught me by surprise that
> >> crtc->state->state (pointer up to full atomic state) is NULL when
> >> crtc_enable() or -flush() is called. So I take the plane-state directly
> >> from the plane->state and just assume that it is pointing to the same
> >> atomic state with the crtc state I am having. I that alraight?
> > 
> > IMO you should never use plane->state etc. Better pass down the
> > full atomic state everywhere. Otherwise you can never even consider
> > increasing the commit queue depth since you'd end up accessing the
> > wrong state.
> >
> 
> Ok. I did explore this a bit and it starts to look like that I have to
> store the planes' zpos values in the driver after all. Only the changes
> are available in the drm_atomic_state being commited so I have to
> maintain the full state myself. That is if I should not use plane->state
> in crtc_enable() or -flush().

You have the full old and new states around for each
crtc/plane/connector in the state. So not sure what you mean
by having only the changes available?

> 
> >>
> >> Why is the crtc->state->state NULL? Is it a bug or is there some reason
> >> to it?
> > 
> > Currently swap_state() moves that state pointer from the new obj state
> > to the old obj state, and clears the one in the new obj state. Not entirely
> > sure why, but maybe just so there isn't a stale ->state pointer hanging 
> > around in the obj->state after the swap?
> > 
> > I think a better way could be to not clobber the old obj state at
> > all, leave the new_obj_state->state alone, and just clear the ->state
> > pointer .duplicate_state(). But that would require reviewing a bunch
> > of code to find all the places where old_obj_state->state gets used
> > during the commit.
> > 
> 
> I think those places are many, since at least I did not figure out any
> other way to access the full commit behind the atomic helpers.

I haven't examined how many drivers depend on the current behaviour.
But fixing up the core/helpers should be pretty trivial.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Jyri Sarha]

On 10/02/2020 15:21, Ville Syrjälä wrote:
> On Sun, Feb 09, 2020 at 02:50:09PM +0200, Jyri Sarha wrote:
>> On 07/02/2020 20:45, Ville Syrjälä wrote:
>>> On Fri, Feb 07, 2020 at 08:26:17PM +0200, Jyri Sarha wrote:
>>>> On 07/02/2020 20:18, Jyri Sarha wrote:
>>>>> The old implementation of placing planes on the CRTC while configuring
>>>>> the planes was naive and relied on the order in which the planes were
>>>>> configured, enabled, and disabled. The situation where a plane's zpos
>>>>> was changed on the fly was completely broken. The usual symptoms of
>>>>> this problem was scrambled display and a flood of sync lost errors,
>>>>> when a plane was active in two layers at the same time, or a missing
>>>>> plane, in case when a layer was accidentally disabled.
>>>>>
>>>>> The rewrite takes a more straight forward approach when when HW is
>>>>> concerned. The plane positioning registers are in the CRTC (or
>>>>> actually OVR) register space and it is more natural to configure them
>>>>> in a one go when configuring the CRTC. This is easy since we have
>>>>> access to the whole atomic state when updating the CRTC configuration.
>>>>>
>>>>
>>>> While implementing this fix it caught me by surprise that
>>>> crtc->state->state (pointer up to full atomic state) is NULL when
>>>> crtc_enable() or -flush() is called. So I take the plane-state directly
>>>> from the plane->state and just assume that it is pointing to the same
>>>> atomic state with the crtc state I am having. I that alraight?
>>>
>>> IMO you should never use plane->state etc. Better pass down the
>>> full atomic state everywhere. Otherwise you can never even consider
>>> increasing the commit queue depth since you'd end up accessing the
>>> wrong state.
>>>
>>
>> Ok. I did explore this a bit and it starts to look like that I have to
>> store the planes' zpos values in the driver after all. Only the changes
>> are available in the drm_atomic_state being commited so I have to
>> maintain the full state myself. That is if I should not use plane->state
>> in crtc_enable() or -flush().
> 
> You have the full old and new states around for each
> crtc/plane/connector in the state. So not sure what you mean
> by having only the changes available?
> 

If (by using the drm_atomic_state pointer in old_crtc_state paremeter)
I loop the planes trough with for_each_oldnew_plane_in_state(), I will
only see the planes that were part of the drm atomic request sent from
the user-space. I just tested that again.

But is it a requirement that an user-space applications should always
send the full state, and that the driver should assume that all
mode_config objects that are not there in drm_atomic_state should be
disabled?

At least the implementation of drm_atomic_get_plane_state() (used by at
least drm_atomic_normalize_zpos() and drm_mode_config_helper_suspend())
seems to suggest otherwise. When getting the plane state it first tries
drm_atomic_get_existing_plane_state(), but if it can not find the state
from the given drm_atomic_state, it goes down to the actual plane and
calls plane->funcs->atomic_duplicate_state(plane) to get it from the
plane-object itself.

>>
>>>>
>>>> Why is the crtc->state->state NULL? Is it a bug or is there some reason
>>>> to it?
>>>
>>> Currently swap_state() moves that state pointer from the new obj state
>>> to the old obj state, and clears the one in the new obj state. Not entirely
>>> sure why, but maybe just so there isn't a stale ->state pointer hanging 
>>> around in the obj->state after the swap?
>>>
>>> I think a better way could be to not clobber the old obj state at
>>> all, leave the new_obj_state->state alone, and just clear the ->state
>>> pointer .duplicate_state(). But that would require reviewing a bunch
>>> of code to find all the places where old_obj_state->state gets used
>>> during the commit.
>>>
>>
>> I think those places are many, since at least I did not figure out any
>> other way to access the full commit behind the atomic helpers.
> 
> I haven't examined how many drivers depend on the current behaviour.
> But fixing up the core/helpers should be pretty trivial.
> 


-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Ville Syrjälä]

On Mon, Feb 10, 2020 at 05:44:19PM +0200, Jyri Sarha wrote:
> On 10/02/2020 15:21, Ville Syrjälä wrote:
> > On Sun, Feb 09, 2020 at 02:50:09PM +0200, Jyri Sarha wrote:
> >> On 07/02/2020 20:45, Ville Syrjälä wrote:
> >>> On Fri, Feb 07, 2020 at 08:26:17PM +0200, Jyri Sarha wrote:
> >>>> On 07/02/2020 20:18, Jyri Sarha wrote:
> >>>>> The old implementation of placing planes on the CRTC while configuring
> >>>>> the planes was naive and relied on the order in which the planes were
> >>>>> configured, enabled, and disabled. The situation where a plane's zpos
> >>>>> was changed on the fly was completely broken. The usual symptoms of
> >>>>> this problem was scrambled display and a flood of sync lost errors,
> >>>>> when a plane was active in two layers at the same time, or a missing
> >>>>> plane, in case when a layer was accidentally disabled.
> >>>>>
> >>>>> The rewrite takes a more straight forward approach when when HW is
> >>>>> concerned. The plane positioning registers are in the CRTC (or
> >>>>> actually OVR) register space and it is more natural to configure them
> >>>>> in a one go when configuring the CRTC. This is easy since we have
> >>>>> access to the whole atomic state when updating the CRTC configuration.
> >>>>>
> >>>>
> >>>> While implementing this fix it caught me by surprise that
> >>>> crtc->state->state (pointer up to full atomic state) is NULL when
> >>>> crtc_enable() or -flush() is called. So I take the plane-state directly
> >>>> from the plane->state and just assume that it is pointing to the same
> >>>> atomic state with the crtc state I am having. I that alraight?
> >>>
> >>> IMO you should never use plane->state etc. Better pass down the
> >>> full atomic state everywhere. Otherwise you can never even consider
> >>> increasing the commit queue depth since you'd end up accessing the
> >>> wrong state.
> >>>
> >>
> >> Ok. I did explore this a bit and it starts to look like that I have to
> >> store the planes' zpos values in the driver after all. Only the changes
> >> are available in the drm_atomic_state being commited so I have to
> >> maintain the full state myself. That is if I should not use plane->state
> >> in crtc_enable() or -flush().
> > 
> > You have the full old and new states around for each
> > crtc/plane/connector in the state. So not sure what you mean
> > by having only the changes available?
> > 
> 
> If (by using the drm_atomic_state pointer in old_crtc_state paremeter)
> I loop the planes trough with for_each_oldnew_plane_in_state(), I will
> only see the planes that were part of the drm atomic request sent from
> the user-space. I just tested that again.
> 
> But is it a requirement that an user-space applications should always
> send the full state, and that the driver should assume that all
> mode_config objects that are not there in drm_atomic_state should be
> disabled?

No.

The usual approach we follow in i915 for things that affect more
than one plane is is to collect that state into the crtc state. 
That way we get to remember it for the planes that are not part
of the current commit.

And when we have state that affects more than one crtc that again
get collected up one level up in what we call global state
(basically drm_private_obj with less heavy handed locking scheme).

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Tomi Valkeinen]

Hi Ville,

On 10/02/2020 18:03, Ville Syrjälä wrote:

> The usual approach we follow in i915 for things that affect more
> than one plane is is to collect that state into the crtc state.
> That way we get to remember it for the planes that are not part
> of the current commit.
> 
> And when we have state that affects more than one crtc that again
> get collected up one level up in what we call global state
> (basically drm_private_obj with less heavy handed locking scheme).

I'm confused. Don't we always have the full state available? Why do you need to store state into 
custom crtc-state?

Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
state object and duplicating that information there seems a bit silly, as surely that information is 
tracked by DRM?

Or what am I missing here...

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Ville Syrjälä]

On Tue, Feb 11, 2020 at 11:11:34AM +0200, Tomi Valkeinen wrote:
> Hi Ville,
> 
> On 10/02/2020 18:03, Ville Syrjälä wrote:
> 
> > The usual approach we follow in i915 for things that affect more
> > than one plane is is to collect that state into the crtc state.
> > That way we get to remember it for the planes that are not part
> > of the current commit.
> > 
> > And when we have state that affects more than one crtc that again
> > get collected up one level up in what we call global state
> > (basically drm_private_obj with less heavy handed locking scheme).
> 
> I'm confused. Don't we always have the full state available? Why do you need to store state into 
> custom crtc-state?
> 
> Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
> state object and duplicating that information there seems a bit silly, as surely that information is 
> tracked by DRM?

You can have it if you add all the planes to the state, which can be
a bit expensive. Another option would to peek into the planes' states
that aren't in the commit, but that's quite gross due to bypassing
the normal locking rules and instead relying on the crtc mutex to
sufficiently protect the plane states as well. And I suspect trying
to do said peeking during the commit phase when the locks have
already been dropped will end badly.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Daniel Vetter]

On Tue, Feb 11, 2020 at 03:00:30PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 11, 2020 at 11:11:34AM +0200, Tomi Valkeinen wrote:
> > Hi Ville,
> > 
> > On 10/02/2020 18:03, Ville Syrjälä wrote:
> > 
> > > The usual approach we follow in i915 for things that affect more
> > > than one plane is is to collect that state into the crtc state.
> > > That way we get to remember it for the planes that are not part
> > > of the current commit.
> > > 
> > > And when we have state that affects more than one crtc that again
> > > get collected up one level up in what we call global state
> > > (basically drm_private_obj with less heavy handed locking scheme).
> > 
> > I'm confused. Don't we always have the full state available? Why do you need to store state into 
> > custom crtc-state?
> > 
> > Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
> > state object and duplicating that information there seems a bit silly, as surely that information is 
> > tracked by DRM?
> 
> You can have it if you add all the planes to the state, which can be
> a bit expensive. Another option would to peek into the planes' states
> that aren't in the commit, but that's quite gross due to bypassing
> the normal locking rules and instead relying on the crtc mutex to
> sufficiently protect the plane states as well. And I suspect trying
> to do said peeking during the commit phase when the locks have
> already been dropped will end badly.

Yup, don't peek outside of atomic_check.

Also the peeking only works for planes associated to the crtc. Either
because that's how the hw works (i915 has fixed plane routing).

Now if this is only about all the planes currently active on a crtc, then
you the helpers will already add all those plane states for you, and you
can just walk them in your commit function. Not exactly sure what you need
here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Daniel Vetter]

On Tue, Feb 11, 2020 at 04:40:21PM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 03:00:30PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 11, 2020 at 11:11:34AM +0200, Tomi Valkeinen wrote:
> > > Hi Ville,
> > > 
> > > On 10/02/2020 18:03, Ville Syrjälä wrote:
> > > 
> > > > The usual approach we follow in i915 for things that affect more
> > > > than one plane is is to collect that state into the crtc state.
> > > > That way we get to remember it for the planes that are not part
> > > > of the current commit.
> > > > 
> > > > And when we have state that affects more than one crtc that again
> > > > get collected up one level up in what we call global state
> > > > (basically drm_private_obj with less heavy handed locking scheme).
> > > 
> > > I'm confused. Don't we always have the full state available? Why do you need to store state into 
> > > custom crtc-state?
> > > 
> > > Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
> > > state object and duplicating that information there seems a bit silly, as surely that information is 
> > > tracked by DRM?
> > 
> > You can have it if you add all the planes to the state, which can be
> > a bit expensive. Another option would to peek into the planes' states
> > that aren't in the commit, but that's quite gross due to bypassing
> > the normal locking rules and instead relying on the crtc mutex to
> > sufficiently protect the plane states as well. And I suspect trying
> > to do said peeking during the commit phase when the locks have
> > already been dropped will end badly.
> 
> Yup, don't peek outside of atomic_check.
> 
> Also the peeking only works for planes associated to the crtc. Either
> because that's how the hw works (i915 has fixed plane routing).
> 
> Now if this is only about all the planes currently active on a crtc, then
> you the helpers will already add all those plane states for you, and you
> can just walk them in your commit function. Not exactly sure what you need
> here.

See drm_atomic_add_affected_planes() in case you're rolling your own
stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

[Jyri Sarha]

On 11/02/2020 17:41, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 04:40:21PM +0100, Daniel Vetter wrote:
>> On Tue, Feb 11, 2020 at 03:00:30PM +0200, Ville Syrjälä wrote:
>>> On Tue, Feb 11, 2020 at 11:11:34AM +0200, Tomi Valkeinen wrote:
>>>> Hi Ville,
>>>>
>>>> On 10/02/2020 18:03, Ville Syrjälä wrote:
>>>>
>>>>> The usual approach we follow in i915 for things that affect more
>>>>> than one plane is is to collect that state into the crtc state.
>>>>> That way we get to remember it for the planes that are not part
>>>>> of the current commit.
>>>>>
>>>>> And when we have state that affects more than one crtc that again
>>>>> get collected up one level up in what we call global state
>>>>> (basically drm_private_obj with less heavy handed locking scheme).
>>>>
>>>> I'm confused. Don't we always have the full state available? Why do you need to store state into 
>>>> custom crtc-state?
>>>>
>>>> Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
>>>> state object and duplicating that information there seems a bit silly, as surely that information is 
>>>> tracked by DRM?
>>>
>>> You can have it if you add all the planes to the state, which can be
>>> a bit expensive. Another option would to peek into the planes' states
>>> that aren't in the commit, but that's quite gross due to bypassing
>>> the normal locking rules and instead relying on the crtc mutex to
>>> sufficiently protect the plane states as well. And I suspect trying
>>> to do said peeking during the commit phase when the locks have
>>> already been dropped will end badly.
>>
>> Yup, don't peek outside of atomic_check.
>>
>> Also the peeking only works for planes associated to the crtc. Either
>> because that's how the hw works (i915 has fixed plane routing).
>>
>> Now if this is only about all the planes currently active on a crtc, then
>> you the helpers will already add all those plane states for you, and you
>> can just walk them in your commit function. Not exactly sure what you need
>> here.
> 
> See drm_atomic_add_affected_planes() in case you're rolling your own
> stuff.
Thanks,
This looks to be exactly what I needed.

BR,
Jyri
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel