Messages in this thread Patch in this message | | | Date | Mon, 5 Sep 2022 12:41:38 +0200 | Subject | Re: [PATCH] drm/ssd130x: Replace simple display helpers with the atomic helpers | From | Thomas Zimmermann <> |
| |
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] | |