| Message ID | 20220828151114.1141510-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drm/ssd130x: Replace simple display helpers with the atomic helpers | expand |
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; >
Hello Thomas, Thanks for your feedback and comments. On 9/5/22 12:41, Thomas Zimmermann wrote: > 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. > You are welcome and thanks to you for mentioning this to me. After doing this I'm convinced as well that the simple-KMS / simple display pipeline abstraction doesn't add any value and we should just drop it in favor of the full atomic helpers. >> >> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > Thanks! > There are a few questions below. > [...] >> +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. > Yes, I just added because noticed that others drivers did. I'll drop it when posting a v2. [...] >> +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'? > Right. I'll fix it in v2 as well. > Is this function the correct place for ssd130x_init() ? It looks a bit > heavy for a simple enable operation. > Yes, I was abusing the concept of encoder here just to have a place where I could hook the enable / disable logic, since I was looking at the other DRM objects helper operations structures and found that these were only defined for the encoder. But there is technically no encoder on this device. As you can see, I was using DRM_MODE_ENCODER_NONE when the encoder is initialized. But I notice now that the struct drm_crtc_helper_funcs also have .enable and .disable callbacks, it seems I was just blind and didn't see before. Would having the init and poweroff logic in the CRTC helpers be correct to you or was do you have in mind ? > Best regards > Thomas
On 9/5/22 13:00, Javier Martinez Canillas wrote: >>> +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'? >> > > Right. I'll fix it in v2 as well. > Actually, this return statement is correct since ssd130x_power_off() return value is 'void' as well.
Hi Am 05.09.22 um 13:00 schrieb Javier Martinez Canillas: > Hello Thomas, > > Thanks for your feedback and comments. > > On 9/5/22 12:41, Thomas Zimmermann wrote: >> 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. >> > > You are welcome and thanks to you for mentioning this to me. After doing > this I'm convinced as well that the simple-KMS / simple display pipeline > abstraction doesn't add any value and we should just drop it in favor of > the full atomic helpers. > >>> >>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >> > > Thanks! > >> There are a few questions below. >> > > [...] > >>> +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. >> > > Yes, I just added because noticed that others drivers did. I'll drop it > when posting a v2. > > [...] > >>> +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'? >> > > Right. I'll fix it in v2 as well. > >> Is this function the correct place for ssd130x_init() ? It looks a bit >> heavy for a simple enable operation. >> > > Yes, I was abusing the concept of encoder here just to have a place where > I could hook the enable / disable logic, since I was looking at the other > DRM objects helper operations structures and found that these were only > defined for the encoder. I liked the idea of handling backlighting here. Power on/off also seems sensible. > > But there is technically no encoder on this device. As you can see, I was > using DRM_MODE_ENCODER_NONE when the encoder is initialized. > > But I notice now that the struct drm_crtc_helper_funcs also have .enable > and .disable callbacks, it seems I was just blind and didn't see before. You certainly want to use atomic_enable/atomic_disable. They are mutually exclusive with the other enable/disable functions. > > Would having the init and poweroff logic in the CRTC helpers be correct > to you or was do you have in mind ? There's quite a bit happening in the init function. Does it have to be re-initialized on each enable operation? If it survives the power-off call, the initial init can be done in the CRTC reset function. It's purpose is to set hardware and software to a clean state. Best regards Thomas > >> Best regards >> Thomas
On 9/5/22 13:34, Thomas Zimmermann wrote: [...] >>> >> >> Yes, I was abusing the concept of encoder here just to have a place where >> I could hook the enable / disable logic, since I was looking at the other >> DRM objects helper operations structures and found that these were only >> defined for the encoder. > > I liked the idea of handling backlighting here. Power on/off also seems > sensible. > Ok. I'll keep that then. >> >> But there is technically no encoder on this device. As you can see, I was >> using DRM_MODE_ENCODER_NONE when the encoder is initialized. >> >> But I notice now that the struct drm_crtc_helper_funcs also have .enable >> and .disable callbacks, it seems I was just blind and didn't see before. > > You certainly want to use atomic_enable/atomic_disable. They are > mutually exclusive with the other enable/disable functions. > Ah, then I wasn't blind after all. It was because the encoder was the only DRM object that had .atomic_{en,dis}able. The CRTC only had some .{en,disable} helper callbacks. >> >> Would having the init and poweroff logic in the CRTC helpers be correct >> to you or was do you have in mind ? > > There's quite a bit happening in the init function. Does it have to be > re-initialized on each enable operation? If it survives the power-off > call, the initial init can be done in the CRTC reset function. It's > purpose is to set hardware and software to a clean state. > I need to check if it survives a disable/enable cycle. Specially since on disable the VCC regulator is disabled, which might lead to the chip state to get lost. > Best regards > Thomas > >> >>> Best regards >>> Thomas >
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) 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); - 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;
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. Suggested-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/gpu/drm/solomon/ssd130x.c | 258 +++++++++++++++++++++--------- drivers/gpu/drm/solomon/ssd130x.h | 9 +- 2 files changed, 187 insertions(+), 80 deletions(-)