Re: [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs

From: Sean Paul <sean@poorly.run>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	David Airlie <airlied@linux.ie>, Sean Paul <sean@poorly.run>,
	Sean Paul <seanpaul@chromium.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v5 01/11] drm: Add atomic variants of enable/disable to encoder helper funcs
Date: Tue, 11 Jun 2019 16:22:19 -0400	[thread overview]
Message-ID: <20190611202219.GD179831@art_vandelay> (raw)
In-Reply-To: <20190611185352.GA16305@ravnborg.org>

On Tue, Jun 11, 2019 at 08:53:52PM +0200, Sam Ravnborg wrote:
> Hi Sean.
> 
> Nits below.
> 
> >  
> > +	/**
> > +	 * @atomic_disable:
> > +	 *
> ...
> > +	 *
> > +	 * This callback is a variant of @disable that provides the atomic state
> > +	 * to the driver. It takes priority over @disable during atomic commits.
> > +	 *
> > +	 * This hook is used only by atomic helpers. Atomic drivers don't need
> > +	 * to implement it if there's no need to disable anything at the encoder
> > +	 * level. To ensure that runtime PM handling (using either DPMS or the
> > +	 * new "ACTIVE" property) works @atomic_disable must be the inverse of
> > +	 * @atomic_enable.
> > +	 */
> > +	void (*atomic_disable)(struct drm_encoder *encoder,
> > +			       struct drm_atomic_state *state);
> 
> 
> 
> > +
> > +	/**
> > +	 * @atomic_enable:
> > +	 *
> ...
> > +	 *
> > +	 * This callback is a variant of @enable that provides the atomic state
> > +	 * to the driver. It is called in place of @enable during atomic
> > +	 * commits.
> 
> The wording in this paragrap is not the same as the similar paragraph
> above.
> 
> One says "it takes priority over"
> Another says "called in place of"
> 
> Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
> define then {dis,en}able is not called?

Sure, can make that change

> 
> 
> > +	 *
> > +	 * This hook is used only by atomic helpers, for symmetry with @disable.
> I do not get the "for symmetry with @disable."?

Copypasta from @enable kerneldoc. I'll change this up and submit a new patch to
fix up @enable to match.

> 
> > +	 * Atomic drivers don't need to implement it if there's no need to
> > +	 * enable anything at the encoder level. To ensure that runtime PM
> > +	 * handling (using either DPMS or the new "ACTIVE" property) works
> > +	 * @enable must be the inverse of @disable for atomic drivers.
> Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?

Yes I did. I'll also clean up some of the surrounding text in the next version.

> 
> > +	 */
> > +	void (*atomic_enable)(struct drm_encoder *encoder,
> > +			      struct drm_atomic_state *state);
> > +
> >  	/**
> >  	 * @disable:
> >  	 *
> > @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
> >  	 * handling (using either DPMS or the new "ACTIVE" property) works
> >  	 * @disable must be the inverse of @enable for atomic drivers.
> >  	 *
> > +	 * For atomic drivers also consider @atomic_disable and save yourself
> > +	 * from having to read the NOTE below!
> Maybe, if this is so, say that atomic drivers shall alwyas use the
> atomic_* variants?
> And then add a TODO entry to make it so for the other atomic drivers?

I don't think that's quite what we want. For atomic drivers it might be
preferable to use enable and disable if they don't need to inspect state. 

The NOTE only applies to drivers using the legacy helpers, which isn't really
applicable for anyone making the decision between disable and atomic_disable.

Sean

> > +	 *
> >  	 * NOTE:
> >  	 *
> >  	 * With legacy CRTC helpers there's a big semantic difference between
> 
> 
> 	Sam

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel