Re: [PATCH 7/8] drm: Add old state pointer to CRTC .enable() helper function

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	m@freedesktop.org, Liviu Dudau <liviu.dudau@arm.com>,
	dri-devel@lists.freedesktop.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Kyungmin Park <kyungmin.park@samsung.co>,
	Boris Brezillon <boris.brezillon@free-electrons.com>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	Alexey Brodkin <abrodkin@synopsys.com>,
	Xinliang Liu <z.liuxinliang@hisilicon.com>,
	Xinwei Kong <kong.kongxinwei@hisilicon.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Mali DP Maintainers <malidp@foss.arm.com>,
	Dave Airlie <airlied@redhat.com>,
	Chen Feng <puck.chen@hisilicon.com>, Jyri Sarha <jsarha@ti.com>,
	Vincent Abriou <vincent.abriou@st.com>,
	VMware Graphics <linux-graphics-maintainer@vmware.com>,
	Alison Wang <alison.w>
Subject: Re: [PATCH 7/8] drm: Add old state pointer to CRTC .enable() helper function
Date: Wed, 28 Jun 2017 14:55:08 +0300	[thread overview]
Message-ID: <1617739.Y0EePd6PWn@avalon> (raw)
In-Reply-To: <20170628074254.puzcyteol3csvpt2@phenom.ffwll.local>

Hi Daniel,

On Wednesday 28 Jun 2017 09:42:54 Daniel Vetter wrote:
> On Wed, Jun 28, 2017 at 12:16:20AM +0300, Laurent Pinchart wrote:
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h index
> > 474a1029ec79..d74a2cafc3de 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -316,24 +316,6 @@ struct drm_crtc_helper_funcs {
> >  	void (*disable)(struct drm_crtc *crtc);
> >  	
> >  	/**
> > -	 * @enable:
> > -	 *
> > -	 * This callback should be used to enable the CRTC. With the atomic
> > -	 * drivers it is called before all encoders connected to this CRTC are
> > -	 * enabled through the encoder's own &drm_encoder_helper_funcs.enable
> > -	 * hook.  If that sequence is too simple drivers can just add their 
own
> > -	 * hooks and call it from this CRTC callback here by looping over all
> > -	 * encoders connected to it using for_each_encoder_on_crtc().
> > -	 *
> > -	 * 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 CRTC 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 (*enable)(struct drm_crtc *crtc);
> > -
> > -	/**
> >  	 * @atomic_check:
> >  	 *
> >  	 * Drivers should check plane-update related CRTC constraints in this
> > @@ -433,6 +415,26 @@ struct drm_crtc_helper_funcs {
> >  			     struct drm_crtc_state *old_crtc_state);
> >  	
> >  	/**
> > +	 * @atomic_enable:
> > +	 *
> > +	 * This callback should be used to enable the CRTC. With the atomic
> > +	 * drivers it is called before all encoders connected to this CRTC are
> > +	 * enabled through the encoder's own &drm_encoder_helper_funcs.enable
> > +	 * hook.  If that sequence is too simple drivers can just add their 
own
> > +	 * hooks and call it from this CRTC callback here by looping over all
> > +	 * encoders connected to it using for_each_encoder_on_crtc().
> > +	 *
> > +	 * This hook is used only by atomic helpers, for symmetry with
> > +	 * @atomic_disable. Atomic drivers don't need to implement it if 
there's
> > +	 * no need to enable anything at the CRTC level. To ensure that 
runtime
> > +	 * PM handling (using either DPMS or the new "ACTIVE" property) works
> > +	 * @atomic_enable must be the inverse of @atomic_disable for atomic
> > +	 * drivers.
> 
> A few lines explaing what @old_crtc_state is good for would be good here
> (like we already have for @atomic_disable). Otherwise lgtm.

I'll fix that in v2.

> > +	void (*atomic_enable)(struct drm_crtc *crtc,
> > +			      struct drm_crtc_state *old_crtc_state);
> > +
> > +	/**
> >  	 * @atomic_disable:
> >  	 *
> >  	 * This callback should be used to disable the CRTC. With the atomic

-- 
Regards,

Laurent Pinchart

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