[Intel-gfx] [PATCH 5/5] drm/atomic-helpers: make mode_set hooks optional

[Daniel Vetter]

On Sun, Feb 22, 2015 at 08:17:04PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Sunday 22 February 2015 19:53:23 Laurent Pinchart wrote:
> > On Sunday 22 February 2015 12:24:20 Daniel Vetter wrote:
> > > With runtime PM the hw might still be off while doing the ->mode_set
> > > callbacks - runtime PM get/put should only happen in the
> > > enable/disable hooks to properly support DPMS. Which essentially makes
> > > these callbacks useless for drivers support runtime PM, so make them
> > > optional. Again motivated by discussions with Laurent.
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > 
> > I think we should go one step further and remove .mode_set() completely for
> > drivers converted to atomic updates. There are two cases to consider:
> > 
> > - Drivers that implement runtime PM can't use the .mode_set() callback for
> > the reason explained above. Those drivers will thus not implement
> > .mode_set() and will perform mode setting related hardware configuration in
> > .enable().
> > 
> > - Drivers that don't implement runtime PM (we probably want to discourage
> > this globally, but that's a different topic) can use the .mode_set()
> > callbacks, but they could equally well perform mode setting in .enable() as
> > the runtime PM- enabled drivers, without any drawback.
> > 
> > To increase consistency, I thus believe we should get rid of .mode_set()
> > completely for drivers converted to atomic updates.
> 
> On second thought, I've confused .mode_set() and .mode_set_nofb(). .mode_set() 
> still makes sense for encoders, but the above reasoning should apply in my 
> opinion for the CRTC .mode_set_nofb().

You're reasoning is correct, but we need to keep smooth transitioning in
mind for driver coming from crtc helpers. And since those use
->mode_set(_nofb) all over the place it's imo better to keep this. At
least until we've run out of drivers to convert ;-)

We could add a DRM_INFO_ONCE though to remind drivers that they're using
deprecated hooks and should convert over. I plan to submit such a patch at
least for dpms/prepare/commit, maybe we could throw in ->mode_set into the
mix too.

> 
> > However, this patch is good as a first step, so if you want to apply it
> > already,
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Merged the entire series to drm-misc, thanks for the feedback.
-Daniel

> > 
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_atomic_helper.c | 5 +++--
> > >  include/drm/drm_crtc_helper.h       | 3 ++-
> > >  2 files changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c index 9fd3466bf277..5e10bcb7d98d
> > > 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -723,7 +723,7 @@ crtc_set_mode(struct drm_device *dev, struct
> > > drm_atomic_state *old_state)
> > > 
> > >  		funcs = crtc->helper_private;
> > > 
> > > -		if (crtc->state->enable) {
> > > +		if (crtc->state->enable && funcs->mode_set_nofb) {
> > > 
> > >  			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d]\n",
> > >  			
> > >  					 crtc->base.id);
> > > 
> > > @@ -759,7 +759,8 @@ 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 call mode_set hooks
> > > twice.
> > > 
> > >  		 */
> > > 
> > > -		funcs->mode_set(encoder, mode, adjusted_mode);
> > > +		if (funcs->mode_set)
> > > +			funcs->mode_set(encoder, mode, adjusted_mode);
> > > 
> > >  		if (encoder->bridge && encoder->bridge->funcs->mode_set)
> > >  		
> > >  			encoder->bridge->funcs->mode_set(encoder->bridge,
> > > 
> > > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
> > > index c250a22b39ab..92d5135b55d2 100644
> > > --- a/include/drm/drm_crtc_helper.h
> > > +++ b/include/drm/drm_crtc_helper.h
> > > @@ -89,6 +89,7 @@ struct drm_crtc_helper_funcs {
> > > 
> > >  	int (*mode_set)(struct drm_crtc *crtc, struct drm_display_mode *mode,
> > >  	
> > >  			struct drm_display_mode *adjusted_mode, int x, int y,
> > >  			struct drm_framebuffer *old_fb);
> > > 
> > > +	/* Actually set the mode for atomic helpers, optional */
> > > 
> > >  	void (*mode_set_nofb)(struct drm_crtc *crtc);
> > >  	
> > >  	/* Move the crtc on the current fb to the given position *optional* 
> */
> > > 
> > > @@ -119,7 +120,7 @@ struct drm_crtc_helper_funcs {
> > > 
> > >   * @mode_fixup: try to fixup proposed mode for this connector
> > >   * @prepare: part of the disable sequence, called before the CRTC modeset
> > >   * @commit: called after the CRTC modeset
> > > 
> > > - * @mode_set: set this mode
> > > + * @mode_set: set this mode, optional for atomic helpers
> > > 
> > >   * @get_crtc: return CRTC that the encoder is currently attached to
> > >   * @detect: connection status detection
> > >   * @disable: disable encoder when not in use (overrides DPMS off)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch