Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 3 May 2019 08:34:52 -0400
From:   Sean Paul <sean@poorly.run>
To:     Sean Paul <sean@poorly.run>, dri-devel@lists.freedesktop.org,
        Sean Paul <seanpaul@chromium.org>,
        Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 
        <ville.syrjala@linux.intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 01/10] drm: Add atomic variants of enable/disable to
 encoder helper funcs
Message-ID: <20190503123452.GG17077@art_vandelay>
References: <20190502194956.218441-1-sean@poorly.run>
 <20190502194956.218441-2-sean@poorly.run>
 <20190503075130.GH3271@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190503075130.GH3271@phenom.ffwll.local>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 03, 2019 at 09:51:30AM +0200, Daniel Vetter wrote:
> On Thu, May 02, 2019 at 03:49:43PM -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch adds atomic_enable and atomic_disable callbacks to the
> > encoder helpers. This will allow encoders to make informed decisions in
> > their start-up/shutdown based on the committed state.
> > 
> > Aside from the new hooks, this patch also introduces the new signature
> > for .atomic_* functions going forward. Instead of passing object state
> > (well, encoders don't have atomic state, but let's ignore that), we pass
> > the entire atomic state so the driver can inspect more than what's
> > happening locally.
> > 
> > This is particularly important for the upcoming self refresh helpers.
> > 
> > Changes in v3:
> > - Added patch to the set
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Ville Syrj?l? <ville.syrjala@linux.intel.com>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++-
> >  include/drm/drm_modeset_helper_vtables.h | 45 ++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 1 deletion(-)

/snip

> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 8f3602811eb5..de57fb40cb6e 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -675,6 +675,51 @@ struct drm_encoder_helper_funcs {
> >  	enum drm_connector_status (*detect)(struct drm_encoder *encoder,
> >  					    struct drm_connector *connector);
> >  
> > +	/**
> > +	 * @atomic_disable:
> > +	 *
> > +	 * This callback should be used to disable the encoder. With the atomic
> > +	 * drivers it is called before this encoder's CRTC has been shut off
> > +	 * using their own &drm_crtc_helper_funcs.atomic_disable hook. If that
> > +	 * sequence is too simple drivers can just add their own driver private
> > +	 * encoder hooks and call them from CRTC's callback by looping over all
> > +	 * encoders connected to it using for_each_encoder_on_crtc().
> > +	 *
> > +	 * 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.
> > +	 */
> 
> I'd add something like "For atomic drivers also consider @atomic_disable"
> to the kerneldoc of @disable (before the NOTE: which is only relevant for
> pre-atomic). Same for the enable side.
> 
> > +	void (*atomic_disable)(struct drm_encoder *encoder,
> > +			       struct drm_atomic_state *state);
> > +
> > +	/**
> > +	 * @atomic_enable:
> > +	 *
> > +	 * This callback should be used to enable the encoder. It is called
> > +	 * after this encoder's CRTC has been enabled using their own
> > +	 * &drm_crtc_helper_funcs.atomic_enable hook. If that sequence is
> > +	 * too simple drivers can just add their own driver private encoder
> > +	 * hooks and call them from CRTC's callback by looping over all encoders
> > +	 * connected to it using for_each_encoder_on_crtc().
> > +	 *
> > +	 * 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.
> 
> needs to be adjusted here for "takes priority".

Can you clarify this comment? I'm a little fuzzy on what it means.


> > +	 *
> > +	 * This hook is used only by atomic helpers, for symmetry with @disable.
> > +	 * 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.
> > +	 */
> > +	void (*atomic_enable)(struct drm_encoder *encoder,
> > +			      struct drm_atomic_state *state);
> > +
> 
> With the nits:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks!

Sean

> 
> >  	/**
> >  	 * @disable:
> >  	 *
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Sean Paul, Software Engineer, Google / Chromium OS