[PATCH v2 3/6] drm/plane: Add optional ->atomic_disable() callback

[Daniel Vetter]

On Fri, Jan 16, 2015 at 03:53:04PM +0100, Thierry Reding wrote:
> On Fri, Jan 16, 2015 at 03:35:10PM +0100, Thierry Reding wrote:
> > On Tue, Nov 25, 2014 at 01:26:34PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 25, 2014 at 12:57:04PM +0100, Thierry Reding wrote:
> > > > On Tue, Nov 25, 2014 at 12:45:46PM +0100, Thierry Reding wrote:
> > > > > On Tue, Nov 25, 2014 at 12:22:34PM +0100, Daniel Vetter wrote:
> > > > > > On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> > > > [...]
> > > > > > > +/*
> > > > > > > + * drm_plane_disabled - check whether a plane is being disabled
> > > > > > > + * @plane: plane object
> > > > > > > + * @old_state: previous atomic state
> > > > > > > + *
> > > > > > > + * Checks the atomic state of a plane to determine whether it's being disabled
> > > > > > > + * or not. This also WARNs if it detects an invalid state (both CRTC and FB
> > > > > > > + * need to either both be NULL or both be non-NULL).
> > > > > > > + *
> > > > > > > + * RETURNS:
> > > > > > > + * True if the plane is being disabled, false otherwise.
> > > > > > > + */
> > > > > > > +static inline bool drm_plane_disabled(struct drm_plane *plane,
> > > > > > > +      struct drm_plane_state *old_state)
> > > > > > > +{
> > > > [...]
> > > > > > > + return (!old_state || old_state->crtc) && !plane->state->crtc;
> > > > > >
> > > > > > The !old_state check here confused me a bit, until I've realized that you
> > > > > > use this for transitional helpers too. What about adding
> > > > > >
> > > > > > /* Cope with NULL old_state for transitional helpers. */
> > > > > >
> > > > > > right above?
> > > > >
> > > > > Sounds good.
> > > >
> > > > When I now thought about the reason again it took me a while to
> > > > reconstruct, so I figured I'd be extra verbose and used this:
> > > >
> > > > /*
> > > > * When using the transitional helpers, old_state may be NULL. If so,
> > > > * we know nothing about the current state and have to assume that it
> > > > * might be enabled.
> > > > */
> > > >
> > > > Does that sound accurate and sufficient to you?
> > >
> > > Hm, thinking about this some more this will result in a slight difference
> > > in behaviour, at least when drivers just use the helper ->reset functions
> > > but don't disable everything:
> > > - With transitional helpers we assume we know nothing and call
> > >   ->atomic_disable.
> > > - With atomic old_state->crtc == NULL in the same situation right after
> > >   boot-up, but we asssume the plane is really off and _dont_ call
> > >   ->atomic_disable.
> > >
> > > Should we instead check for (old_state && old_state->crtc) and state that
> > > drivers need to make sure they don't have stuff hanging around?
> >
> > I don't think we can check for old_state because otherwise this will
> > always return false, whereas we really want it to force-disable planes
> > that could be on (lacking any more accurate information). For
> > transitional helpers anyway.
> >
> > For the atomic helpers, old_state will never be NULL, but I'd assume
> > that the driver would reconstruct the current state in ->reset().
>
> By the way, the reason for why old_state can be NULL with transitional
> helpers is the ordering of the steps in the atomic transition. Currently
> the Tegra patches do this (based on your blog post and the Exynos proto-
> type):
>
> 1) atomic conversion, phase 1:
>   - implement ->atomic_{check,update,disable}()
>   - use drm_plane_helper_{update,disable}()
>
> 2) atomic conversion, phase 2:
>   - call drm_mode_config_reset() from ->load()
>   - implement ->reset()
>
> That's only a partial list of what's done in these steps, but that's the
> only relevant pieces for why old_state is NULL.
>
> What happens is that without ->reset() implemented there won't be any
> initial state, hence plane->state (the old_state here) will be NULL the
> first time atomic state is applied.
>
> We could of course reorder the sequence such that drivers are required
> to hook up ->reset() before they can (or at the same as they) hook up
> the transitional helpers. We could add an appropriate WARN_ON to this
> helper to make that more obvious.
>
> However, that will not solve the problem because it only gets rid of the
> special case. We still don't know whether old_state->crtc == NULL is the
> current state or just the initial default.
>
> So no matter which way we do this, I don't see a way to get away without
> requiring specific semantics from drivers. They would be that:
>
> - drivers recreate the correct state in ->reset() so that
>  old_state->crtc != NULL if the plane is really enabled
>
> or
>
> - drivers have to ensure that the real state in fact mirrors the
>  initial default as encoded in the state (plane disabled)
>
> So I think the comment below that I proposed earlier is the best we can
> do.
>
> /*
> * When using the transitional helpers, old_state may be NULL. If so,
> * we know nothing about the current state and have to assume that it
> * might be enabled. This usually happens only on the very first call
> * of the drm_plane_helper_commit() helper.
> *
> * When using the atomic helpers, old_state won't be NULL. Therefore
> * this check assumes that either the driver will have reconstructed
> * the correct state in ->reset() or that the driver will have taken
> * appropriate measures to disable all planes.
> */
>
> Or perhaps I'm missing something?

Yeah, comments sounds very good. Any imo you should quote your two mails
here holesale in the commit message too, so that when someone git blames
the code comment all the details are there.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch