Hi,
On Thu, Jul 22, 2021 at 08:22:43AM +0200, Sam Ravnborg wrote:
> The atomic variants of enable/disable in drm_bridge_funcs are the
> preferred operations - introduce these.
>
> Use of mode_set is deprecated - merge the functionality with
> atomic_enable()
>
> Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Cc: Robert Foss <robert.foss@xxxxxxxxxx>
> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Jonas Karlman <jonas@xxxxxxxxx>
> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> ---
> drivers/gpu/drm/bridge/lontium-lt9611.c | 69 ++++++++++---------------
> 1 file changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
> index 29b1ce2140ab..dfa7baefe2ab 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> @@ -700,9 +700,17 @@ lt9611_connector_mode_valid(struct drm_connector *connector,
> }
>
> /* bridge funcs */
> -static void lt9611_bridge_enable(struct drm_bridge *bridge)
> +static void lt9611_bridge_atomic_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> + const struct drm_display_mode *mode;
> + const struct drm_crtc_state *crtc_state;
> + struct hdmi_avi_infoframe avi_frame;
> + int ret;
> +
> + crtc_state = drm_bridge_new_crtc_state(bridge, old_bridge_state);
> + mode = &crtc_state->mode;
So, yeah, it looks like you can't make the assumption that crtc_state is
going to be valid here.
I'm not entirely clear on how bridge states are allocated, but it looks
to me that they are through drm_atomic_add_encoder_bridges, which is
called for all the affected connectors in a commit here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L744
Then, the atomic_enable call is made through
drm_atomic_bridge_chain_enable(), which is called in
drm_atomic_helper_commit_modeset_enables only if the CRTC is active and
needs a modeset.
I guess this means that we won't have a null pointer for crtc_state
there, but wouldn't that cause some issues? I can imagine a property
like the bpc count or output format where it wouldn't imply a modeset
but would definitely affect the bridges in the chain?
Maxime
