Re: [PATCH 1/2] drm/atomic-helper: Add atomic_mode_set version to extend encoder mode_set

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 22, 2016 at 06:53:41PM +0200, Daniel Vetter wrote:
> On Fri, Jul 22, 2016 at 02:09:12PM +0200, Philipp Zabel wrote:
> > Some encoders need more information from crtc and connector state
> > than just the mode. Add an atomic encoder mode setting variant
> > that passes the crtc state (which contains the modes) and the
> > connector state.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c      |  6 +++++-
> >  include/drm/drm_modeset_helper_vtables.h | 20 ++++++++++++++++++++
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index de7fddc..a3c033f 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -880,8 +880,12 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> >  		 * Each encoder has at most one connector (since we always steal
> >  		 * it away), so we won't call mode_set hooks twice.
> >  		 */
> > -		if (funcs && funcs->mode_set)
> > +		if (funcs && funcs->atomic_mode_set) {
> > +			funcs->atomic_mode_set(encoder, new_crtc_state,
> > +					       connector->state);
> > +		} else if (funcs && funcs->mode_set) {
> >  			funcs->mode_set(encoder, mode, adjusted_mode);
> > +		}
> >  
> >  		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> >  	}
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index b55f218..1b15c1f 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -529,6 +529,26 @@ struct drm_encoder_helper_funcs {
> >  			 struct drm_display_mode *adjusted_mode);
> >  
> >  	/**
> > +	 * @atomic_mode_set:
> > +	 *
> > +	 * This callback is used to update the display mode of an encoder.
> > +	 *
> > +	 * Note that the display pipe is completely off when this function is
> > +	 * called. Drivers which need hardware to be running before they program
> > +	 * the new display mode (because they implement runtime PM) should not
> > +	 * use this hook, because the helper library calls it only once and not
> > +	 * every time the display pipeline is suspended using either DPMS or the
> > +	 * new "ACTIVE" property. Such drivers should instead move all their
> > +	 * encoder setup into the ->enable() callback.
> > +	 *
> > +	 * This callback is used by the atomic modeset helpers instead of the
> > +	 * mode_set callback. It is optional in the atomic helpers.
> 
> s/mode_set/@mode_set/ Also pls a bit of text when this should be used (any
> time you need to inspect connector state, since there's no direct way to
> go from the encoder to the current connector). And pls add a note to
> @mode_set that drivers should use @atomic_mode_set in such a case, for
> cross linking.
> 
> Great docs matter ;-)
> 
> While at it I noticed that the kernel-doc for @mode_set is wrong, it says
> it's only used by CRTC helpers. Can you pls fix that too?

Strike that part, I accidentally read the kerneldoc for crtc->mode_fixup.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel