| Message ID | 1461702945-14185-11-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/04/16 23:35, Laurent Pinchart wrote: > Instead of conditioning planes update based on the hardware device > state, use the CRTC state stored in the atomic state. This reduces the > dependency from the DRM layer to the DSS layer. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index 6359d7933b93..4c56d6a68390 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, > > WARN_ON(omap_crtc->vblank_irq.registered); > > - if (dispc_mgr_is_enabled(omap_crtc->channel)) { > + /* > + * Only flush the CRTC if it is currently active. CRTCs that require a > + * mode set are disabled prior plane updates and enabled afterwards. > + * They are thus not active, regardless of what their state report. > + */ > + if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state)) > + return; If the DRM core doesn't track whether a CRTC HW is enabled at the moment, maybe omapdrm should? I guess the above works, but that if() makes me a bit uneasy, as it's not really obvious, and the logic behind it could possibly change later... A "if (crtc->is_hw_enabled)" would be much more readable. Tomi
On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote: > > On 26/04/16 23:35, Laurent Pinchart wrote: > > Instead of conditioning planes update based on the hardware device > > state, use the CRTC state stored in the atomic state. This reduces the > > dependency from the DRM layer to the DSS layer. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > > index 6359d7933b93..4c56d6a68390 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > > @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, > > > > WARN_ON(omap_crtc->vblank_irq.registered); > > > > - if (dispc_mgr_is_enabled(omap_crtc->channel)) { > > + /* > > + * Only flush the CRTC if it is currently active. CRTCs that require a > > + * mode set are disabled prior plane updates and enabled afterwards. > > + * They are thus not active, regardless of what their state report. > > + */ > > + if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state)) > > + return; > > If the DRM core doesn't track whether a CRTC HW is enabled at the > moment, maybe omapdrm should? I guess the above works, but that if() > makes me a bit uneasy, as it's not really obvious, and the logic behind > it could possibly change later... > > A "if (crtc->is_hw_enabled)" would be much more readable. Look at the active_only paramater of the planes_commit helper. That should do exactly what you want. -Daniel
Hi Tomi and Daniel, On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote: > On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote: > > On 26/04/16 23:35, Laurent Pinchart wrote: > >> Instead of conditioning planes update based on the hardware device > >> state, use the CRTC state stored in the atomic state. This reduces the > >> dependency from the DRM layer to the DSS layer. > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> > >> drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++--------- > >> 1 file changed, 14 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > >> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390 > >> 100644 > >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > >> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc > >> *crtc, > >> WARN_ON(omap_crtc->vblank_irq.registered); > >> > >> - if (dispc_mgr_is_enabled(omap_crtc->channel)) { > >> + /* > >> + * Only flush the CRTC if it is currently active. CRTCs that require a > >> + * mode set are disabled prior plane updates and enabled afterwards. > >> + * They are thus not active, regardless of what their state report. > >> + */ > >> + if (!crtc->state->active || > >> drm_atomic_crtc_needs_modeset(crtc->state)) > >> + return; > > > > If the DRM core doesn't track whether a CRTC HW is enabled at the > > moment, maybe omapdrm should? I guess the above works, but that if() > > makes me a bit uneasy, as it's not really obvious, and the logic behind > > it could possibly change later... > > > > A "if (crtc->is_hw_enabled)" would be much more readable. The whole point of this patch is to remove local state and rely on DRM core state, so I'd like to avoid that if possible. > Look at the active_only paramater of the planes_commit helper. That should > do exactly what you want. The drm_atomic_helper_commit_planes() helper has this check if (active_only && !crtc->state->active) continue; funcs->atomic_flush(crtc, old_crtc_state); I could thus remove the !crtc->state->active check, but the drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When CRTCs go through a modeset they are disabled prior to plane updates, but their state active status can still be true. Or should that be fixed in the core ?
On 06/06/16 04:14, Laurent Pinchart wrote: >>> If the DRM core doesn't track whether a CRTC HW is enabled at the >>> moment, maybe omapdrm should? I guess the above works, but that if() >>> makes me a bit uneasy, as it's not really obvious, and the logic behind >>> it could possibly change later... >>> >>> A "if (crtc->is_hw_enabled)" would be much more readable. > > The whole point of this patch is to remove local state and rely on DRM core > state, so I'd like to avoid that if possible. Yep, but if DRM core doesn't give that information... Using drm_atomic_crtc_needs_modeset() to check if a crtc is enabled at a particular point in the commit sequence feels a bit risky to me. You do explain it in the comment, so it's not that bad, but I'd still rather see an 'if (is-the-hw-enabled-or-not)' than looking at seemingly unrelated information, and deducing from that if the hw is enabled or not. Tomi
Hi Tomi, On Monday 06 Jun 2016 13:37:13 Tomi Valkeinen wrote: > On 06/06/16 04:14, Laurent Pinchart wrote: > >>> If the DRM core doesn't track whether a CRTC HW is enabled at the > >>> moment, maybe omapdrm should? I guess the above works, but that if() > >>> makes me a bit uneasy, as it's not really obvious, and the logic behind > >>> it could possibly change later... > >>> > >>> A "if (crtc->is_hw_enabled)" would be much more readable. > > > > The whole point of this patch is to remove local state and rely on DRM > > core state, so I'd like to avoid that if possible. > > Yep, but if DRM core doesn't give that information... > > Using drm_atomic_crtc_needs_modeset() to check if a crtc is enabled at a > particular point in the commit sequence feels a bit risky to me. > > You do explain it in the comment, so it's not that bad, but I'd still > rather see an 'if (is-the-hw-enabled-or-not)' than looking at seemingly > unrelated information, and deducing from that if the hw is enabled or not. I agree, but I'd like that "is-the-hw-enabled-or-not" state to be provided by the DRM core, not the omapdrm driver. I'll see how we can get there, but I'd rather do that separately from this patch series as it will require very careful design and review.
Hi Daniel, On Monday 06 Jun 2016 04:14:59 Laurent Pinchart wrote: > On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote: > > On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote: > >> On 26/04/16 23:35, Laurent Pinchart wrote: > >>> Instead of conditioning planes update based on the hardware device > >>> state, use the CRTC state stored in the atomic state. This reduces the > >>> dependency from the DRM layer to the DSS layer. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> > >>> drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++--------- > >>> 1 file changed, 14 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > >>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390 > >>> 100644 > >>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > >>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > >>> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct > >>> drm_crtc *crtc, > >>> > >>> WARN_ON(omap_crtc->vblank_irq.registered); > >>> > >>> - if (dispc_mgr_is_enabled(omap_crtc->channel)) { > >>> + /* > >>> + * Only flush the CRTC if it is currently active. CRTCs that require > >>> a > >>> + * mode set are disabled prior plane updates and enabled afterwards. > >>> + * They are thus not active, regardless of what their state report. > >>> + */ > >>> + if (!crtc->state->active || > >>> drm_atomic_crtc_needs_modeset(crtc->state)) > >>> + return; > >> > >> If the DRM core doesn't track whether a CRTC HW is enabled at the > >> moment, maybe omapdrm should? I guess the above works, but that if() > >> makes me a bit uneasy, as it's not really obvious, and the logic behind > >> it could possibly change later... > >> > >> A "if (crtc->is_hw_enabled)" would be much more readable. > > The whole point of this patch is to remove local state and rely on DRM core > state, so I'd like to avoid that if possible. > > > Look at the active_only paramater of the planes_commit helper. That should > > do exactly what you want. > > The drm_atomic_helper_commit_planes() helper has this check > > if (active_only && !crtc->state->active) > continue; > > funcs->atomic_flush(crtc, old_crtc_state); > > I could thus remove the !crtc->state->active check, but the > drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When > CRTCs go through a modeset they are disabled prior to plane updates, but > their state active status can still be true. Or should that be fixed in the > core ? Any comment on this ? Knowing whether the CRTC hardware is enabled is helpful to implement the flush() function. Would it make sense to add a new "is hardware enabled" flag to the CRTC structure (or possibly the CRTC state structure, but given that there is by definition a single instance of the hardware state the CRTC structure would make more sense) ?
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, WARN_ON(omap_crtc->vblank_irq.registered); - if (dispc_mgr_is_enabled(omap_crtc->channel)) { + /* + * Only flush the CRTC if it is currently active. CRTCs that require a + * mode set are disabled prior plane updates and enabled afterwards. + * They are thus not active, regardless of what their state report. + */ + if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state)) + return; - DBG("%s: GO", omap_crtc->name); + DBG("%s: GO", omap_crtc->name); - rmb(); - WARN_ON(omap_crtc->pending); - omap_crtc->pending = true; - wmb(); + rmb(); + WARN_ON(omap_crtc->pending); + omap_crtc->pending = true; + wmb(); - dispc_mgr_go(omap_crtc->channel); - omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); - } + dispc_mgr_go(omap_crtc->channel); + omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); } static bool omap_crtc_is_plane_prop(struct drm_device *dev,
Instead of conditioning planes update based on the hardware device state, use the CRTC state stored in the atomic state. This reduces the dependency from the DRM layer to the DSS layer. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)