[PATCH v3 10/20] drm: omapdrm: Only commit planes on active CRTCs

[Laurent Pinchart]

Hi Tomi,

On Tuesday 13 Dec 2016 09:58:34 Tomi Valkeinen wrote:
> On 13/12/16 00:53, Laurent Pinchart wrote:
> > On Tuesday 20 Sep 2016 16:51:11 Tomi Valkeinen wrote:
> >> On 19/09/16 15:27, Laurent Pinchart wrote:
> >>> The DRM core supports skipping plane update for inactive CRTCs for
> >>> hardware that don't need it or can't cope with it. That's our case, so
> >>> use the DRM core infrastructure instead of reinventing it.
> >> 
> >> I don't follow this desc. What is omapdrm reinventing? At least this
> >> patch does not remove any of the "reinvention".
> > 
> > There used to be one, but it got removed when I rebased the patch series.
> > I'll reword the commit message.
> > 
> >> What does DRM_PLANE_COMMIT_ACTIVE_ONLY do? Skips plane HW configuration
> >> for planes on crtcs that are disabled? The plane HW config will still be
> >> done when the crtc is about to be enabled, right?
> > 
> > It skips plane update (atomic_begin, atomic_disable, atomic_flush) for
> > disabled CRTCs. The CRTC .begin() operation is still called for those
> > CRTCs, only plane update is skipped.
> 
> I didn't catch that. Skips atomic_begin, but .begin is still called?

Sorry, I meant .disable().

> > Now that I wrote that, I'm not quite sure this change is right. It looks
> > like disabling a plane is shadowed, and without an atomic_flush call the
> > GO bit will never be set. However, the problem predates this patch, as
> > the GO bit will only be set if dispc_mgr_is_enabled() returns true, which
> > shouldn't be the case for disabled CRTCs.
> > 
> > How is this supposed to work, how is plane disable supposed to be
> > synchronized at the hardware level ?
> 
> If a crtc is currently disabled (in the HW), all the new settings
> (including plane enable/disable bit) can be programmed and when the crtc
> is enabled, those settings are taken into use. GO bit is not necessary.
> 
> When the crtc is active, you need to set the GO bit to get the new
> settings into use at next vblank.
> 
> If a crtc is to be disabled, we can just disable the crtc, GO bit is not
> necessary.

Thanks for the clarification.

-- 
Regards,

Laurent Pinchart