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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4310 bytes --]

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 #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel