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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

Thank you for the patch.

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@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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.

However, this patch is good as a first step, so if you want to apply it 
already,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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

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