drm/omapdrm: Couple of plane related fixes

This reverts commit dadf4659d0608e034b6633f30300c2eff2dafb4c.

If planes are not disabled when the they are not on any crtc anymore they will remain active and may show as "ghosts" when the crtc they were last on is active again.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 8 +-------
 drivers/gpu/drm/omapdrm/omap_drv.c  | 3 +--
 2 files changed, 2 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/omap_crtc.c index dd47dc1..b68c70e 100644

drivers/gpu/drm/omapdrm/omap_crtc.c +1 -5

-	 * mode set are disabled prior plane updates and enabled afterwards.
-	 * They are thus not active (regardless of what their CRTC core state
-	 * reports) and the DRM core could thus call this function even though
-	 * the CRTC is currently disabled. Do nothing in that case.
-	 */
+	/* Only flush the CRTC if it is currently enabled. */
 	if (!omap_crtc->enabled)
 		return;
 

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/omap_drv.c index 00aa214..5e55f1b 100644

drivers/gpu/drm/omapdrm/omap_drv.c +1 -2

@@ -96,8 +96,7 @@ static void omap_atomic_complete(struct
 omap_atomic_state_commit *commit) dispc_runtime_get();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_planes(dev, old_state,
-					DRM_PLANE_COMMIT_ACTIVE_ONLY);
+	drm_atomic_helper_commit_planes(dev, old_state, 0);
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	omap_atomic_wait_for_completion(dev, old_state);

Hi Jyri,

Thank you for the patch.

Quoted 5 lines · On Friday 27 Jan 2017 12:04:54 Jyri Sarha wrote:

This reverts commit dadf4659d0608e034b6633f30300c2eff2dafb4c.

If planes are not disabled when the they are not on any crtc anymore they will remain active and may show as "ghosts" when the crtc they were last on is active again.

Sorry for the breakage.

The drm_atomic_helper_commit_planes() helper documentation states

* Unless otherwise needed, drivers are advised to set the ACTIVE_ONLY flag in
* @flags in order not to receive plane update notifications related to a
* disabled CRTC. This avoids the need to manually ignore plane updates in
* driver code when the driver and/or hardware can't or just don't need to
* deal with updates on disabled CRTCs, for example when supporting runtime
* PM.

I wonder what this implies when CRTCs are being disabled. I see very few cases where the hardware wouldn't need the plane atomic disable operation being called when a plane is being disabled due to its CRTC being disabled. Maybe this should thus be addressed in the core. Daniel, any comment on this ?

Move drm_atomic_helper_commit_modeset_enables() call to before drm_atomic_helper_commit_planes() call and have a omap_atomic_wait_for_completion() call after both.

With the current dss dispc implementation we have to enable the new modeset before we can commit planes. The dispc ovl configuration relies on the video mode configuration been written into the HW when the ovl configuration is calculated.

This approach is not ideal because after a mode change the plane update is executed only after the first vblank interrupt. The dispc implementation should be fixed so that it is able use uncommitted drm state information. information.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/omapdrm/omap_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/omap_drv.c index 5e55f1b..8c15735 100644

drivers/gpu/drm/omapdrm/omap_drv.c +1 -1

@@ -96,8 +96,22 @@ static void omap_atomic_complete(struct
 omap_atomic_state_commit *commit) dispc_runtime_get();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_planes(dev, old_state, 0);
+

drivers/gpu/drm/omapdrm/omap_drv.c +4 -0

+	 */
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
+	omap_atomic_wait_for_completion(dev, old_state);
+
+	drm_atomic_helper_commit_planes(dev, old_state, 0);
 
 	omap_atomic_wait_for_completion(dev, old_state);

Hi Jyri,

Thank you for the patch.

Quoted 26 lines · On Friday 27 Jan 2017 12:04:55 Jyri Sarha wrote:

Move drm_atomic_helper_commit_modeset_enables() call to before drm_atomic_helper_commit_planes() call and have a omap_atomic_wait_for_completion() call after both.

With the current dss dispc implementation we have to enable the new modeset before we can commit planes. The dispc ovl configuration relies on the video mode configuration been written into the HW when the ovl configuration is calculated.

This approach is not ideal because after a mode change the plane update is executed only after the first vblank interrupt. The dispc implementation should be fixed so that it is able use uncommitted drm state information. information.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/omapdrm/omap_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/omap_drv.c index 5e55f1b..8c15735 100644

drivers/gpu/drm/omapdrm/omap_drv.c +0 -0

@@ -96,8 +96,22 @@ static void omap_atomic_complete(struct
 omap_atomic_state_commit *commit) dispc_runtime_get();
 

drivers/gpu/drm/omapdrm/omap_drv.c static void omap_atomic_complete(struct omap_atomic_state_commit *commit) · line 109

Quoted 13 lines

drivers/gpu/drm/omapdrm/omap_drv.c +11 -1

 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_planes(dev, old_state, 0);
+
+	/* With the current dss dispc implementation we have to enable
+	 * the new modeset before we can commit planes. The dispc ovl
+	 * configuration relies on the video mode configuration been
+	 * written into the HW when the ovl configuration is
+	 * calculated.
+	 *
+	 * This approach is not ideal because after a mode change the
+	 * plane update is executed only after the first vblank
+	 * interrupt. The dispc implementation should be fixed so that
+	 * it is able use uncommitted drm state information.

Any chance you could fix the dispc implementation ? ;-) Otherwise, could you

Hi Jyri,

Thank you for the patch.

Quoted 26 lines · On Friday 27 Jan 2017 12:04:55 Jyri Sarha wrote:

Move drm_atomic_helper_commit_modeset_enables() call to before drm_atomic_helper_commit_planes() call and have a omap_atomic_wait_for_completion() call after both.

With the current dss dispc implementation we have to enable the new modeset before we can commit planes. The dispc ovl configuration relies on the video mode configuration been written into the HW when the ovl configuration is calculated.

This approach is not ideal because after a mode change the plane update is executed only after the first vblank interrupt. The dispc implementation should be fixed so that it is able use uncommitted drm state information. information.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/omapdrm/omap_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/omap_drv.c index 5e55f1b..8c15735 100644

drivers/gpu/drm/omapdrm/omap_drv.c +0 -0

@@ -96,8 +96,22 @@ static void omap_atomic_complete(struct
 omap_atomic_state_commit *commit) dispc_runtime_get();
 

drivers/gpu/drm/omapdrm/omap_drv.c static void omap_atomic_complete(struct omap_atomic_state_commit *commit) · line 109

Quoted 13 lines

drivers/gpu/drm/omapdrm/omap_drv.c +11 -1

 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_planes(dev, old_state, 0);
+
+	/* With the current dss dispc implementation we have to enable
+	 * the new modeset before we can commit planes. The dispc ovl
+	 * configuration relies on the video mode configuration been
+	 * written into the HW when the ovl configuration is
+	 * calculated.
+	 *
+	 * This approach is not ideal because after a mode change the
+	 * plane update is executed only after the first vblank
+	 * interrupt. The dispc implementation should be fixed so that
+	 * it is able use uncommitted drm state information.

Any chance you could fix the dispc implementation ? ;-) Otherwise, could you

rebase (and retest) this patch on top of "[PATCH 0/5] omapdrm: fences and zpos" ?

Quoted 30 lines · Laurent Pinchart

Quoted 29 lines · On Friday 27 Jan 2017 12:04:55 Jyri Sarha wrote:

Move drm_atomic_helper_commit_modeset_enables() call to before drm_atomic_helper_commit_planes() call and have a omap_atomic_wait_for_completion() call after both.

With the current dss dispc implementation we have to enable the new modeset before we can commit planes. The dispc ovl configuration relies on the video mode configuration been written into the HW when the ovl configuration is calculated.

This approach is not ideal because after a mode change the plane update is executed only after the first vblank interrupt. The dispc implementation should be fixed so that it is able use uncommitted drm state information. information.

Signed-off-by Jyri Sarha <[email protected]>

---

 drivers/gpu/drm/omapdrm/omap_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/omapdrm/omap_drv.c index 5e55f1b..8c15735 100644

drivers/gpu/drm/omapdrm/omap_drv.c +0 -0

@@ -96,8 +96,22 @@ static void omap_atomic_complete(struct
 omap_atomic_state_commit *commit) dispc_runtime_get();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 

Quoted 15 lines

drivers/gpu/drm/omapdrm/omap_drv.c static void omap_atomic_complete(struct omap_atomic_state_commit *commit) · line 109

Quoted 12 lines

drivers/gpu/drm/omapdrm/omap_drv.c +11 -1

-	drm_atomic_helper_commit_planes(dev, old_state, 0);
+
+	/* With the current dss dispc implementation we have to enable
+	 * the new modeset before we can commit planes. The dispc ovl
+	 * configuration relies on the video mode configuration been
+	 * written into the HW when the ovl configuration is
+	 * calculated.
+	 *
+	 * This approach is not ideal because after a mode change the
+	 * plane update is executed only after the first vblank
+	 * interrupt. The dispc implementation should be fixed so that
+	 * it is able use uncommitted drm state information.

Any chance you could fix the dispc implementation ? ;-) Otherwise, could you

I am currently studying how to do it, but it tricky... really tricky. In DRM terms, the plane configuration is affected by the connector where the crtc is connected to. The whole DSS should have to made understand drm state structures. There certainly wont be any "fix" for 4.10, probably nothing for 4.11 either.

Quoted 2 lines

rebase (and retest) this patch on top of "[PATCH 0/5] omapdrm: fences and zpos" ?

Thanks, I'll do that.

Quoted 3 lines

Quoted 2 lines

rebase (and retest) this patch on top of "[PATCH 0/5] omapdrm: fences and zpos" ?

Thanks, I'll do that.

If you intend on merging this patch as a v4.10 fix then there's no need to rebase. If it targets v4.11, the above-mentioned series will likely go in first.

I hit these two problems while tracking scaling related problems with omapdrm on DSS5 HW.

Jyri Sarha (2):
Revert "drm: omapdrm: Let the DRM core skip plane commit on inactive
CRTCs"
drm/omapdrm: Move commit_modeset_enables() before commit_planes()

 drivers/gpu/drm/omapdrm/omap_crtc.c |  8 +-------
 drivers/gpu/drm/omapdrm/omap_drv.c  | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 9 deletions(-)