| Message ID | 4e28061a55b9473631f0793e1a056fcda8163508.1485510281.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jyri, Thank you for the patch. 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 ? > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 8 +------- > drivers/gpu/drm/omapdrm/omap_drv.c | 3 +-- > 2 files changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index dd47dc1..b68c70e 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -410,13 +410,7 @@ static void omap_crtc_atomic_flush(struct drm_crtc > *crtc, dispc_mgr_set_gamma(omap_crtc->channel, lut, length); > } > > - /* > - * Only flush the CRTC if it is currently enabled. CRTCs that require a > - * 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; > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c index 00aa214..5e55f1b 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -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);
On 28/01/17 18:11, Laurent Pinchart wrote: > Hi Jyri, > > Thank you for the patch. > > 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 ? Similar change was done to mali in "drm: mali-dp: Don't set DRM_PLANE_COMMIT_ACTIVE_ONLY", so at least omapdrm is not alone here. I also wonder if this behavior is correct. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index dd47dc1..b68c70e 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -410,13 +410,7 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, dispc_mgr_set_gamma(omap_crtc->channel, lut, length); } - /* - * Only flush the CRTC if it is currently enabled. CRTCs that require a - * 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; diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 00aa214..5e55f1b 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -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);
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 <jsarha@ti.com> --- drivers/gpu/drm/omapdrm/omap_crtc.c | 8 +------- drivers/gpu/drm/omapdrm/omap_drv.c | 3 +-- 2 files changed, 2 insertions(+), 9 deletions(-)