Re: [PATCH v4] drm/tidss: dispc: Rewrite naive plane positioning code
On 14/02/2020 13:20, Tomi Valkeinen wrote:
> On 13/02/2020 21:37, 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 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. To do this we need make sure we
>> have all the planes on the updated CRTCs in the new atomic state. The
>> untouched planes on CRTCs that need plane position update are added to
>> the atomic state in tidss_atomic_check().
>
> The subject needs updating. This is a fix for a bug, and subject needs
> to reflect that.
>
Ok
>> Signed-off-by: Jyri Sarha
>> ---
>> drivers/gpu/drm/tidss/tidss_crtc.c | 55 +
>> drivers/gpu/drm/tidss/tidss_crtc.h | 2 ++
>> drivers/gpu/drm/tidss/tidss_dispc.c | 55 +++--
>> drivers/gpu/drm/tidss/tidss_dispc.h | 5 +++
>> drivers/gpu/drm/tidss/tidss_kms.c | 49 -
>> 5 files changed, 130 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c
>> b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 032c31ee2820..631ec61b086a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -17,6 +17,7 @@
>> #include "tidss_dispc.h"
>> #include "tidss_drv.h"
>> #include "tidss_irq.h"
>> +#include "tidss_plane.h"
>> /* Page flip and frame done IRQs */
>> @@ -111,6 +112,54 @@ static int tidss_crtc_atomic_check(struct
>> drm_crtc *crtc,
>> return dispc_vp_bus_check(dispc, hw_videoport, state);
>> }
>> +/*
>> + * This needs all affected planes to be present in the atomic
>> + * state. The untouched planes are added to the state in
>> + * tidss_atomic_check().
>> + */
>> +static void tidss_crtc_position_planes(struct tidss_device *tidss,
>> + struct drm_crtc *crtc,
>> + struct drm_crtc_state *old_state,
>> + bool newmodeset)
>> +{
>> + struct drm_atomic_state *ostate = old_state->state;
>> + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>> + struct drm_crtc_state *cstate = crtc->state;
>> + int zpos;
>> +
>> + if (!newmodeset && !cstate->zpos_changed &&
>> + !to_tidss_crtc_state(cstate)->plane_pos_changed)
>> + return;
>> +
>> + for (zpos = 0; zpos < tidss->feat->num_planes; zpos++) {
>> + struct drm_plane_state *pstate;
>> + struct drm_plane *plane;
>> + bool zpos_taken = false;
>> + int i;
>> +
>> + for_each_new_plane_in_state(ostate, plane, pstate, i) {
>> + if (pstate->crtc != crtc || !pstate->visible)
>> + continue;
>> +
>> + if (pstate->normalized_zpos == zpos) {
>> + zpos_taken = true;
>> + break;
>> + }
>> + }
>> +
>> + if (zpos_taken) {
>> + struct tidss_plane *tplane = to_tidss_plane(plane);
>> +
>> + dispc_ovr_set_plane(tidss->dispc, tplane->hw_plane_id,
>> + tcrtc->hw_videoport,
>> + pstate->crtc_x, pstate->crtc_y,
>> + zpos);
>> + }
>> + dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, zpos,
>> + zpos_taken);
>> + }
>> +}
>
> Nitpicking, but... I think the "zpos" above is really "layer". Even the
> params, to which you pass "zpos", in the ovr functions are named "layer".
>
Well, it is both. But I'll change that.
> "zpos_taken" sounds like it's reserved and not available for us, or
> something like that. Maybe "layer_active" conveys better that we're just
> collecting which layers are active and which are not.
>
Ok, that sounds better.
>> +
>> static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>> struct drm_crtc_state *old_crtc_state)
>> {
>> @@ -146,6 +195,9 @@ static void tidss_crtc_atomic_flush(struct
>> drm_crtc *crtc,
>> /* Write vp properties to HW if needed. */
>> dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state,
>> false);
>> + /* Update plane positions if needed. */
>> + tidss_crtc_position_planes(tidss, crtc, old_crtc_state, false);
>> +
>> WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>> spin_lock_irqsave(>event_lock, flags);
>> @@ -183,6 +235,7 @@ static void tidss_crtc_atomic_enable(struct
>> drm_crtc *crtc,
>> return;
>>
Re: [PATCH v4] drm/tidss: dispc: Rewrite naive plane positioning code
On 13/02/2020 21:37, 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 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. To do this we need make sure we
have all the planes on the updated CRTCs in the new atomic state. The
untouched planes on CRTCs that need plane position update are added to
the atomic state in tidss_atomic_check().
The subject needs updating. This is a fix for a bug, and subject needs to
reflect that.
Signed-off-by: Jyri Sarha
---
drivers/gpu/drm/tidss/tidss_crtc.c | 55 +
drivers/gpu/drm/tidss/tidss_crtc.h | 2 ++
drivers/gpu/drm/tidss/tidss_dispc.c | 55 +++--
drivers/gpu/drm/tidss/tidss_dispc.h | 5 +++
drivers/gpu/drm/tidss/tidss_kms.c | 49 -
5 files changed, 130 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c
b/drivers/gpu/drm/tidss/tidss_crtc.c
index 032c31ee2820..631ec61b086a 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -17,6 +17,7 @@
#include "tidss_dispc.h"
#include "tidss_drv.h"
#include "tidss_irq.h"
+#include "tidss_plane.h"
/* Page flip and frame done IRQs */
@@ -111,6 +112,54 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
return dispc_vp_bus_check(dispc, hw_videoport, state);
}
+/*
+ * This needs all affected planes to be present in the atomic
+ * state. The untouched planes are added to the state in
+ * tidss_atomic_check().
+ */
+static void tidss_crtc_position_planes(struct tidss_device *tidss,
+ struct drm_crtc *crtc,
+ struct drm_crtc_state *old_state,
+ bool newmodeset)
+{
+ struct drm_atomic_state *ostate = old_state->state;
+ struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
+ struct drm_crtc_state *cstate = crtc->state;
+ int zpos;
+
+ if (!newmodeset && !cstate->zpos_changed &&
+ !to_tidss_crtc_state(cstate)->plane_pos_changed)
+ return;
+
+ for (zpos = 0; zpos < tidss->feat->num_planes; zpos++) {
+ struct drm_plane_state *pstate;
+ struct drm_plane *plane;
+ bool zpos_taken = false;
+ int i;
+
+ for_each_new_plane_in_state(ostate, plane, pstate, i) {
+ if (pstate->crtc != crtc || !pstate->visible)
+ continue;
+
+ if (pstate->normalized_zpos == zpos) {
+ zpos_taken = true;
+ break;
+ }
+ }
+
+ if (zpos_taken) {
+ struct tidss_plane *tplane = to_tidss_plane(plane);
+
+ dispc_ovr_set_plane(tidss->dispc, tplane->hw_plane_id,
+ tcrtc->hw_videoport,
+ pstate->crtc_x, pstate->crtc_y,
+ zpos);
+ }
+ dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, zpos,
+ zpos_taken);
+ }
+}
Nitpicking, but... I think the "zpos" above is really "layer". Even the params, to which you pass
"zpos", in the ovr functions are named "layer".
"zpos_taken" sounds like it's reserved and not available for us, or something like that. Maybe
"layer_active" conveys better that we're just collecting which layers are active and which are not.
+
static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
@@ -146,6 +195,9 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
/* Write vp properties to HW if needed. */
dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false);
+ /* Update plane positions if needed. */
+ tidss_crtc_position_planes(tidss, crtc, old_crtc_state, false);
+
WARN_ON(drm_crtc_vblank_get(crtc) != 0);
spin_lock_irqsave(>event_lock, flags);
@@ -183,6 +235,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
return;
dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, true);
+
[PATCH v4] drm/tidss: dispc: Rewrite naive plane positioning code
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 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. To do this we need make sure we
have all the planes on the updated CRTCs in the new atomic state. The
untouched planes on CRTCs that need plane position update are added to
the atomic state in tidss_atomic_check().
Signed-off-by: Jyri Sarha
---
drivers/gpu/drm/tidss/tidss_crtc.c | 55 +
drivers/gpu/drm/tidss/tidss_crtc.h | 2 ++
drivers/gpu/drm/tidss/tidss_dispc.c | 55 +++--
drivers/gpu/drm/tidss/tidss_dispc.h | 5 +++
drivers/gpu/drm/tidss/tidss_kms.c | 49 -
5 files changed, 130 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c
b/drivers/gpu/drm/tidss/tidss_crtc.c
index 032c31ee2820..631ec61b086a 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -17,6 +17,7 @@
#include "tidss_dispc.h"
#include "tidss_drv.h"
#include "tidss_irq.h"
+#include "tidss_plane.h"
/* Page flip and frame done IRQs */
@@ -111,6 +112,54 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
return dispc_vp_bus_check(dispc, hw_videoport, state);
}
+/*
+ * This needs all affected planes to be present in the atomic
+ * state. The untouched planes are added to the state in
+ * tidss_atomic_check().
+ */
+static void tidss_crtc_position_planes(struct tidss_device *tidss,
+ struct drm_crtc *crtc,
+ struct drm_crtc_state *old_state,
+ bool newmodeset)
+{
+ struct drm_atomic_state *ostate = old_state->state;
+ struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
+ struct drm_crtc_state *cstate = crtc->state;
+ int zpos;
+
+ if (!newmodeset && !cstate->zpos_changed &&
+ !to_tidss_crtc_state(cstate)->plane_pos_changed)
+ return;
+
+ for (zpos = 0; zpos < tidss->feat->num_planes; zpos++) {
+ struct drm_plane_state *pstate;
+ struct drm_plane *plane;
+ bool zpos_taken = false;
+ int i;
+
+ for_each_new_plane_in_state(ostate, plane, pstate, i) {
+ if (pstate->crtc != crtc || !pstate->visible)
+ continue;
+
+ if (pstate->normalized_zpos == zpos) {
+ zpos_taken = true;
+ break;
+ }
+ }
+
+ if (zpos_taken) {
+ struct tidss_plane *tplane = to_tidss_plane(plane);
+
+ dispc_ovr_set_plane(tidss->dispc, tplane->hw_plane_id,
+ tcrtc->hw_videoport,
+ pstate->crtc_x, pstate->crtc_y,
+ zpos);
+ }
+ dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, zpos,
+ zpos_taken);
+ }
+}
+
static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
@@ -146,6 +195,9 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
/* Write vp properties to HW if needed. */
dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false);
+ /* Update plane positions if needed. */
+ tidss_crtc_position_planes(tidss, crtc, old_crtc_state, false);
+
WARN_ON(drm_crtc_vblank_get(crtc) != 0);
spin_lock_irqsave(>event_lock, flags);
@@ -183,6 +235,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
return;
dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, true);
+ tidss_crtc_position_planes(tidss, crtc, old_state, true);
/* Turn vertical blanking interrupt reporting on. */
drm_crtc_vblank_on(crtc);
@@ -318,6 +371,8 @@ static struct drm_crtc_state
*tidss_crtc_duplicate_state(struct drm_crtc *crtc)
__drm_atomic_helper_crtc_duplicate_state(crtc, >base);
+ state->plane_pos_changed = false;
+
state->bus_format = current_state->bus_format;
state->bus_flags = current_state->bus_flags;
diff --git
