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().
> Or maybe just a note that there's a difference in behaviour here?
One other option would be to split this up into two functions:
- drm_plane_helper_disabling() which is called from
drm_plane_helper_commit() and checks for the validity of
old_state
- drm_atomic_plane_disabling() which is called from
drm_atomic_helper_commit_planes() and doesn't have to check
for the validity of old_state because it's always valid.
On second thought, that doesn't really help because the very first call
would still not know whether old_state->crtc is NULL because it's just
the default or because the plane is really disabled.
So I think the only sane solution to this would be to either have the
drivers reconstruct the correct value for state->crtc in ->reset() or
make sure to comply with the semantics that planes are initially
considered to be disabled.
Maybe complementing the above comment like this would help:
/*
* 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 current state in ->reset() or that the driver will have taken
* measures to disable all planes.
*/
Thierry
Attachment:
pgpenE7m0crgS.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
