Date:   Mon, 5 Sep 2022 12:41:38 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     David Airlie <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/ssd130x: Replace simple display helpers with the
 atomic helpers

Hi Javier

Am 28.08.22 um 17:11 schrieb Javier Martinez Canillas:
> The simple display pipeline is a set of helpers that can be used by DRM
> drivers to avoid dealing with all the needed components and just define
> a few functions to operate a simple display device with one full-screen
> scanout buffer feeding a single output.
> 
> But it is arguable that this provides the correct level of abstraction
> for simple drivers, and recently some have been ported from using these
> simple display helpers to use the regular atomic helpers instead.
> 
> The rationale for this is that the simple display pipeline helpers don't
> hide that much of the DRM complexity, while adding an indirection layer
> that conflates the concepts of CRTCs and planes. This makes the helpers
> less flexible and harder to be reused among different graphics drivers.
> 
> Also, for simple drivers, using the full atomic helpers doesn't require
> a lot of additional code. So adding a simple display pipeline layer may
> not be worth it.
> 
> For these reasons, let's follow that trend and make ssd130x a plain DRM
> driver that creates its own primary plane, CRTC, enconder and connector.

Thanks for considering this change.

> 
> Suggested-by: Thomas Zimmermann <tzimmermann@...e.de>
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>

Acked-by: Thomas Zimmermann <tzimmermann@...e.de>

There are a few questions below.

> ---
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 258 +++++++++++++++++++++---------
>   drivers/gpu/drm/solomon/ssd130x.h |   9 +-
>   2 files changed, 187 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index f87f5443e714..0ae17fcceb7c 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -18,6 +18,7 @@
>   #include <linux/pwm.h>
>   #include <linux/regulator/consumer.h>
>   
> +#include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_damage_helper.h>
>   #include <drm/drm_edid.h>
> @@ -564,61 +565,56 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>   	return ret;
>   }
>   
> -static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
> -					   const struct drm_display_mode *mode)
> +static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
> +						     struct drm_atomic_state *new_state)
>   {
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
> +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
> +	struct drm_crtc *new_crtc = new_plane_state->crtc;
> +	struct drm_crtc_state *new_crtc_state = NULL;
>   
> -	if (mode->hdisplay != ssd130x->mode.hdisplay &&
> -	    mode->vdisplay != ssd130x->mode.vdisplay)
> -		return MODE_ONE_SIZE;
> -
> -	if (mode->hdisplay != ssd130x->mode.hdisplay)
> -		return MODE_ONE_WIDTH;
> -
> -	if (mode->vdisplay != ssd130x->mode.vdisplay)
> -		return MODE_ONE_HEIGHT;
> +	if (new_crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_crtc);
>   
> -	return MODE_OK;
> +	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> +						   DRM_PLANE_NO_SCALING,
> +						   DRM_PLANE_NO_SCALING,
> +						   false, false);
>   }
>   
> -static void ssd130x_display_pipe_enable(struct drm_simple_display_pipe *pipe,
> -					struct drm_crtc_state *crtc_state,
> -					struct drm_plane_state *plane_state)
> +static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
> +						       struct drm_atomic_state *old_state)
>   {
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> -	struct drm_device *drm = &ssd130x->drm;
> -	int idx, ret;
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_device *drm = plane->dev;
> +	struct drm_rect src_clip, dst_clip;
> +	int idx;
>   
> -	ret = ssd130x_power_on(ssd130x);
> -	if (ret)
> +	if (!fb)

I know that some other drivers do this check. But from reading 
drm_atomic_helper.c, it shouldn't be necesary. If !fb, the plane has 
been disabled. And because there's an implementation of atomic_disable, 
the helpers should never call atomic_update on disabled planes. I think 
the test can be removed.

>   		return;
>   
> -	ret = ssd130x_init(ssd130x);
> -	if (ret)
> -		goto out_power_off;
> -
> -	if (!drm_dev_enter(drm, &idx))
> -		goto out_power_off;
> +	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
> +		return;
>   
> -	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &plane_state->dst);
> +	dst_clip = plane_state->dst;
> +	if (!drm_rect_intersect(&dst_clip, &src_clip))
> +		return;
>   
> -	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
>   
> -	backlight_enable(ssd130x->bl_dev);
> +	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
>   
>   	drm_dev_exit(idx);
> -
> -	return;
> -out_power_off:
> -	ssd130x_power_off(ssd130x);
>   }
>   
> -static void ssd130x_display_pipe_disable(struct drm_simple_display_pipe *pipe)
> +static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane,
> +							struct drm_atomic_state *old_state)
>   {
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
> -	struct drm_device *drm = &ssd130x->drm;
> +	struct drm_device *drm = plane->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
>   	int idx;
>   
>   	if (!drm_dev_enter(drm, &idx))
> @@ -626,56 +622,114 @@ static void ssd130x_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>   
>   	ssd130x_clear_screen(ssd130x);
>   
> -	backlight_disable(ssd130x->bl_dev);
> +	drm_dev_exit(idx);
> +}
>   
> -	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF);
> +static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = {
> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +	.atomic_check = ssd130x_primary_plane_helper_atomic_check,
> +	.atomic_update = ssd130x_primary_plane_helper_atomic_update,
> +	.atomic_disable = ssd130x_primary_plane_helper_atomic_disable,
> +};
>   
> -	ssd130x_power_off(ssd130x);
> +static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
> +	.update_plane = drm_atomic_helper_update_plane,
> +	.disable_plane = drm_atomic_helper_disable_plane,
> +	.destroy = drm_plane_cleanup,
> +	DRM_GEM_SHADOW_PLANE_FUNCS,
> +};
>   
> -	drm_dev_exit(idx);
> +static enum drm_mode_status ssd130x_crtc_helper_mode_valid(struct drm_crtc *crtc,
> +							   const struct drm_display_mode *mode)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(crtc->dev);
> +
> +	if (mode->hdisplay != ssd130x->mode.hdisplay &&
> +	    mode->vdisplay != ssd130x->mode.vdisplay)
> +		return MODE_ONE_SIZE;
> +	else if (mode->hdisplay != ssd130x->mode.hdisplay)
> +		return MODE_ONE_WIDTH;
> +	else if (mode->vdisplay != ssd130x->mode.vdisplay)
> +		return MODE_ONE_HEIGHT;
> +
> +	return MODE_OK;
>   }
>   
> -static void ssd130x_display_pipe_update(struct drm_simple_display_pipe *pipe,
> -					struct drm_plane_state *old_plane_state)
> +static int ssd130x_crtc_helper_atomic_check(struct drm_crtc *crtc,
> +					    struct drm_atomic_state *new_state)
>   {
> -	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
> -	struct drm_plane_state *plane_state = pipe->plane.state;
> -	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> -	struct drm_framebuffer *fb = plane_state->fb;
> -	struct drm_device *drm = &ssd130x->drm;
> -	struct drm_rect src_clip, dst_clip;
> -	int idx;
> +	struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(new_state, crtc);
> +	int ret;
>   
> -	if (!fb)
> -		return;
> +	ret = drm_atomic_helper_check_crtc_state(new_crtc_state, false);
> +	if (ret)
> +		return ret;
>   
> -	if (!pipe->crtc.state->active)
> -		return;
> +	return drm_atomic_add_affected_planes(new_state, crtc);
> +}
>   
> -	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
> -		return;
> +/*
> + * The CRTC is always enabled. Screen updates are performed by
> + * the primary plane's atomic_update function. Disabling clears
> + * the screen in the primary plane's atomic_disable function.
> + */
> +static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs = {
> +	.mode_valid = ssd130x_crtc_helper_mode_valid,
> +	.atomic_check = ssd130x_crtc_helper_atomic_check,
> +};
>   
> -	dst_clip = plane_state->dst;
> -	if (!drm_rect_intersect(&dst_clip, &src_clip))
> -		return;
> +static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
> +	.reset = drm_atomic_helper_crtc_reset,
> +	.destroy = drm_crtc_cleanup,
> +	.set_config = drm_atomic_helper_set_config,
> +	.page_flip = drm_atomic_helper_page_flip,
> +	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> +};
>   
> -	if (!drm_dev_enter(drm, &idx))
> +static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
> +						 struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = encoder->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	int ret;
> +
> +	ret = ssd130x_power_on(ssd130x);
> +	if (ret)
>   		return;
>   
> -	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
> +	ret = ssd130x_init(ssd130x);
> +	if (ret)
> +		return ssd130x_power_off(ssd130x);

It returns a value from a function returning 'void'?

Is this function the correct place for ssd130x_init() ? It looks a bit 
heavy for a simple enable operation.

Best regards
Thomas

>   
> -	drm_dev_exit(idx);
> +	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
> +
> +	backlight_enable(ssd130x->bl_dev);
>   }
>   
> -static const struct drm_simple_display_pipe_funcs ssd130x_pipe_funcs = {
> -	.mode_valid = ssd130x_display_pipe_mode_valid,
> -	.enable = ssd130x_display_pipe_enable,
> -	.disable = ssd130x_display_pipe_disable,
> -	.update = ssd130x_display_pipe_update,
> -	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
> +static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder,
> +						  struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = encoder->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +
> +	backlight_disable(ssd130x->bl_dev);
> +
> +	ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF);
> +
> +	ssd130x_power_off(ssd130x);
> +}
> +
> +static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs = {
> +	.atomic_enable = ssd130x_encoder_helper_atomic_enable,
> +	.atomic_disable = ssd130x_encoder_helper_atomic_disable,
> +};
> +
> +static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
>   };
>   
> -static int ssd130x_connector_get_modes(struct drm_connector *connector)
> +static int ssd130x_connector_helper_get_modes(struct drm_connector *connector)
>   {
>   	struct ssd130x_device *ssd130x = drm_to_ssd130x(connector->dev);
>   	struct drm_display_mode *mode;
> @@ -695,7 +749,7 @@ static int ssd130x_connector_get_modes(struct drm_connector *connector)
>   }
>   
>   static const struct drm_connector_helper_funcs ssd130x_connector_helper_funcs = {
> -	.get_modes = ssd130x_connector_get_modes,
> +	.get_modes = ssd130x_connector_helper_get_modes,
>   };
>   
>   static const struct drm_connector_funcs ssd130x_connector_funcs = {
> @@ -806,8 +860,16 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>   	struct device *dev = ssd130x->dev;
>   	struct drm_device *drm = &ssd130x->drm;
>   	unsigned long max_width, max_height;
> +	struct drm_plane *primary_plane;
> +	struct drm_crtc *crtc;
> +	struct drm_encoder *encoder;
> +	struct drm_connector *connector;
>   	int ret;
>   
> +	/*
> +	 * Modesetting
> +	 */
> +
>   	ret = drmm_mode_config_init(drm);
>   	if (ret) {
>   		dev_err(dev, "DRM mode config init failed: %d\n", ret);
> @@ -833,25 +895,65 @@ static int ssd130x_init_modeset(struct ssd130x_device *ssd130x)
>   	drm->mode_config.preferred_depth = 32;
>   	drm->mode_config.funcs = &ssd130x_mode_config_funcs;
>   
> -	ret = drm_connector_init(drm, &ssd130x->connector, &ssd130x_connector_funcs,
> +	/* Primary plane */
> +
> +	primary_plane = &ssd130x->primary_plane;
> +	ret = drm_universal_plane_init(drm, primary_plane, 0, &ssd130x_primary_plane_funcs,
> +				       ssd130x_formats, ARRAY_SIZE(ssd130x_formats),
> +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret) {
> +		dev_err(dev, "DRM primary plane init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_plane_helper_add(primary_plane, &ssd130x_primary_plane_helper_funcs);
> +
> +	drm_plane_enable_fb_damage_clips(primary_plane);
> +
> +	/* CRTC */
> +
> +	crtc = &ssd130x->crtc;
> +	ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
> +					&ssd130x_crtc_funcs, NULL);
> +	if (ret) {
> +		dev_err(dev, "DRM crtc init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_crtc_helper_add(crtc, &ssd130x_crtc_helper_funcs);
> +
> +	/* Encoder */
> +
> +	encoder = &ssd130x->encoder;
> +	ret = drm_encoder_init(drm, encoder, &ssd130x_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret) {
> +		dev_err(dev, "DRM encoder init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_encoder_helper_add(encoder, &ssd130x_encoder_helper_funcs);
> +
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> +	/* Connector */
> +
> +	connector = &ssd130x->connector;
> +	ret = drm_connector_init(drm, connector, &ssd130x_connector_funcs,
>   				 DRM_MODE_CONNECTOR_Unknown);
>   	if (ret) {
>   		dev_err(dev, "DRM connector init failed: %d\n", ret);
>   		return ret;
>   	}
>   
> -	drm_connector_helper_add(&ssd130x->connector, &ssd130x_connector_helper_funcs);
> +	drm_connector_helper_add(connector, &ssd130x_connector_helper_funcs);
>   
> -	ret = drm_simple_display_pipe_init(drm, &ssd130x->pipe, &ssd130x_pipe_funcs,
> -					   ssd130x_formats, ARRAY_SIZE(ssd130x_formats),
> -					   NULL, &ssd130x->connector);
> +	ret = drm_connector_attach_encoder(connector, encoder);
>   	if (ret) {
> -		dev_err(dev, "DRM simple display pipeline init failed: %d\n", ret);
> +		dev_err(dev, "DRM attach connector to encoder failed: %d\n", ret);
>   		return ret;
>   	}
>   
> -	drm_plane_enable_fb_damage_clips(&ssd130x->pipe.plane);
> -
>   	drm_mode_config_reset(drm);
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
> index 4c4a84e962e7..03038c1b6476 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.h
> +++ b/drivers/gpu/drm/solomon/ssd130x.h
> @@ -13,8 +13,11 @@
>   #ifndef __SSD1307X_H__
>   #define __SSD1307X_H__
>   
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
>   #include <drm/drm_drv.h>
> -#include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_plane_helper.h>
>   
>   #include <linux/regmap.h>
>   
> @@ -42,8 +45,10 @@ struct ssd130x_deviceinfo {
>   struct ssd130x_device {
>   	struct drm_device drm;
>   	struct device *dev;
> -	struct drm_simple_display_pipe pipe;
>   	struct drm_display_mode mode;
> +	struct drm_plane primary_plane;
> +	struct drm_crtc crtc;
> +	struct drm_encoder encoder;
>   	struct drm_connector connector;
>   	struct i2c_client *client;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists