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?
Or maybe just a note that there's a difference in behaviour here?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
