lkml.org 
[lkml]   [2022]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] drm/ssd130x: Replace simple display helpers with the atomic helpers
From
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@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-09-05 12:43    [W:0.078 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site