On Fri, Apr 24, 2026 at 08:49:50PM -0300, Val Packett wrote:
> Modern DSI panels support various modes (resolutions and refresh rates),
> which usually requires sending mode-specific DCS commands to configure the
> DDIC during operations like prepare(). However existing drm_panel_funcs
> only take the panel itself as an argument, so there's no way to access
> relevant state (such as the mode being set) inside of these functions.
>
> To allow panel drivers access to the state, introduce new atomic_*
> versions of the prepare/enable/disable/unprepare ops, and make the
> corresponding drm_panel_* functions call them if available, falling back
> to the original ops otherwise. To avoid the need to modify all consumers
> at once, the original single-argument drm_panel_* functions are redefined
> as aliases to the new ones, passing NULLs for the new extra arguments.
>
> The atomic ops pass the CRTC as well as the atomic state because getting
> the mode being set is part of per-CRTC state.
>
> Signed-off-by: Val Packett <[email protected]>
> ---
>
> As discussed in my recent RFC [1] and an older thread I was pointed to
> there [2], the right way to expose the mode to panel drivers is to expose
> the entire atomic state and let them find the mode there. (Turns out,
> the drm_crtc must also be passed along with the state as it's necessary
> for drm_atomic_get_new_crtc_state).
>
> Hopefully this first "real" attempt is close enough! :)
>
> I have tested this with the WIP nt37701 driver from the linked RFC,
> only slightly updated to receive the new args and not directly the mode.
>
> [1]: https://lore.kernel.org/all/[email protected]/
> [2]:
> https://lore.kernel.org/dri-devel/nfc6ih43gjpi5u67fpkkxgwwygv53grdldq7tfp5iiukrkiy2u@53fsrtezzkyt/
>
> Thanks,
> ~val
>
> ---
> drivers/gpu/drm/drm_panel.c | 88 +++++++++++++++--------
> include/drm/drm_panel.h | 137 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 193 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index d1e6598ea3bc..48fce5e89bca 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -102,16 +102,20 @@ void drm_panel_remove(struct drm_panel *panel)
> EXPORT_SYMBOL(drm_panel_remove);
>
> /**
> - * drm_panel_prepare - power on a panel
> + * drm_panel_atomic_prepare - power on a panel
> * @panel: DRM panel
> + * @crtc: the CRTC used to drive the panel, may be NULL
What does NULL mean here? How would panel's atomic helpers behave if
they receive NULL?
If you want to see a nicer way to migrate to atomic funcs, see
5ade071ba13e ("drm: Add atomic variants for bridge enable/disable").
So, you can't replace drm_panel_foo() with a stub calling
drm_atomic_panel_foo().
> + * @state: current atomic commit state, may be NULL
> *
> * Calling this function will enable power and deassert any reset signals to
> * the panel. After this has completed it is possible to communicate with any
> * integrated circuitry via a command bus. This function cannot fail (as it
> is
> * called from the pre_enable call chain). There will always be a call to
> - * drm_panel_disable() afterwards.
> + * drm_panel_atomic_disable() afterwards.
> */
> -void drm_panel_prepare(struct drm_panel *panel)
> +void drm_panel_atomic_prepare(struct drm_panel *panel,
> + struct drm_crtc *crtc,
Thinking about it... We can have several panels being driven by the same
CRTC. Likewise there can be a lot going on between the CRTC and the
panel. After looking at the code actually calling drm_panel functions,
the only thing that we can easily pass here is the encoder. But then,
there also might be bridges between encoder and the panel. Encoders
don't have state per se.
If the panel needs to identify something, it should be getting the
connector. It might be a more cumbersome, but it sounds like a more
logical option here.
Maxime suggested passing drm_connector_state here instead of the
complete state + connector. I don't have a strong preference here, but I
think that the latter might be slighly more idiomatic nowadays.
> + struct drm_atomic_state *state)
> {
> struct drm_panel_follower *follower;
> int ret;
[...]
> @@ -330,11 +384,86 @@ void drm_panel_put(struct drm_panel *panel);
> void drm_panel_add(struct drm_panel *panel);
> void drm_panel_remove(struct drm_panel *panel);
>
> -void drm_panel_prepare(struct drm_panel *panel);
> -void drm_panel_unprepare(struct drm_panel *panel);
> +void drm_panel_atomic_prepare(struct drm_panel *panel,
> + struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
>
> -void drm_panel_enable(struct drm_panel *panel);
> -void drm_panel_disable(struct drm_panel *panel);
> +/**
> + * drm_panel_prepare - power on a panel
> + * @panel: DRM panel
> + *
> + * Calling this function will enable power and deassert any reset signals to
> + * the panel. After this has completed it is possible to communicate with any
> + * integrated circuitry via a command bus. This function cannot fail (as it
> is
> + * called from the pre_enable call chain). There will always be a call to
> + * drm_panel_disable() afterwards.
> + *
> + * If atomic commit state is available, call drm_panel_atomic_prepare
> instead.
> + */
> +static inline void drm_panel_prepare(struct drm_panel *panel)
> +{
> + drm_panel_atomic_prepare(panel, NULL, NULL);
> +}
> +
> +void drm_panel_atomic_unprepare(struct drm_panel *panel,
> + struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
> +
> +/**
> + * drm_panel_unprepare - power off a panel
> + * @panel: DRM panel
> + *
> + * Calling this function will completely power off a panel (assert the
> panel's
> + * reset, turn off power supplies, ...). After this function has completed,
> it
> + * is usually no longer possible to communicate with the panel until another
> + * call to drm_panel_prepare().
> + *
> + * If atomic commit state is available, call drm_panel_atomic_unprepare
> instead.
> + */
> +static inline void drm_panel_unprepare(struct drm_panel *panel)
> +{
> + drm_panel_atomic_unprepare(panel, NULL, NULL);
> +}
> +
> +void drm_panel_atomic_enable(struct drm_panel *panel,
> + struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
> +
> +/**
> + * drm_panel_enable - enable a panel
> + * @panel: DRM panel
> + *
> + * Calling this function will cause the panel display drivers to be turned on
> + * and the backlight to be enabled. Content will be visible on screen after
> + * this call completes. This function cannot fail (as it is called from the
> + * enable call chain). There will always be a call to drm_panel_disable()
> + * afterwards.
> + *
> + * If atomic commit state is available, call drm_panel_atomic_enable instead.
> + */
> +static inline void drm_panel_enable(struct drm_panel *panel)
> +{
> + drm_panel_atomic_enable(panel, NULL, NULL);
> +}
> +
> +void drm_panel_atomic_disable(struct drm_panel *panel,
> + struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
> +
> +/**
> + * drm_panel_disable - disable a panel
> + * @panel: DRM panel
> + *
> + * This will typically turn off the panel's backlight or disable the display
> + * drivers. For smart panels it should still be possible to communicate with
> + * the integrated circuitry via any command bus after this call.
> + *
> + * If atomic commit state is available, call drm_panel_atomic_disable
> instead.
> + */
> +static inline void drm_panel_disable(struct drm_panel *panel)
> +{
> + drm_panel_atomic_disable(panel, NULL, NULL);
> +}
>
> int drm_panel_get_modes(struct drm_panel *panel, struct drm_connector
> *connector);
>
> --
> 2.53.0
>
--
With best wishes
Dmitry
