On January 17, 2015 4:48:53 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> 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.
Will do. Does this count as Reviewed-by? Also the above discussion should
probably be distilled into the Docbook at some point.
Thierry
