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@xxxxxxxxxxxxxxxx>
> >>> ---
> >>>
> >>> 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) ?
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
