On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul at chromium.org>
>
> This patch adds a helper to tease out the currently connected crtc for
> an encoder, along with its state. This follows the same pattern as the
> drm_atomic_crtc_*_for_* macros in the atomic helpers. Since the
> relationship of crtc:encoder is 1:n, we don't need a loop since there is
> only one crtc per encoder.
No idea which macros you mean, couldn't find them.
>
> Instead of splitting this into 3 functions which all do the same thing,
> this is presented as one function. Perhaps that's too ugly and it should
> be split to:
> struct drm_crtc *drm_atomic_crtc_for_encoder(state, encoder);
> struct drm_crtc_state *drm_atomic_new_crtc_state_for_encoder(state, encoder);
> struct drm_crtc_state *drm_atomic_old_crtc_state_for_encoder(state, encoder);
>
> Suggestions welcome.
>
> Changes in v3:
> - Added to the set
>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 48 +++++++++++++++++++++++++++++
> include/drm/drm_atomic_helper.h | 6 ++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 71cc7d6b0644..1f81ca8daad7 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3591,3 +3591,51 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> return ret;
> }
> EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
> +
> +/**
> + * drm_atomic_crtc_state_for_encoder - Get crtc and new/old state for an encoder
> + * @state: Atomic state
> + * @encoder: The encoder to fetch the crtc information for
> + * @crtc: If not NULL, receives the currently connected crtc
> + * @old_crtc_state: If not NULL, receives the crtc's old state
> + * @new_crtc_state: If not NULL, receives the crtc's new state
> + *
> + * This function finds the crtc which is currently connected to @encoder and
> + * returns it as well as its old and new state. If there is no crtc currently
> + * connected, the function will clear @crtc, @old_crtc_state, @new_crtc_state.
> + *
> + * All of @crtc, @old_crtc_state, and @new_crtc_state are optional.
> + */
> +void drm_atomic_crtc_state_for_encoder(struct drm_atomic_state *state,
> + struct drm_encoder *encoder,
> + struct drm_crtc **crtc,
> + struct drm_crtc_state **old_crtc_state,
> + struct drm_crtc_state **new_crtc_state)
> +{
> + struct drm_crtc *tmp_crtc;
> + struct drm_crtc_state *tmp_new_crtc_state, *tmp_old_crtc_state;
> + u32 enc_mask = drm_encoder_mask(encoder);
> + int i;
> +
> + for_each_oldnew_crtc_in_state(state, tmp_crtc, tmp_old_crtc_state,
> + tmp_new_crtc_state, i) {
So there's two ways to do this:
- Using encoder_mask, which is a helper thing. In that case I'd rename
this to drm_atomic_helper_crtc_for_encoder.
- By looping over the connectors, and looking at ->best_encoder and
->crtc, see drm_encoder_get_crtc in drm_encoder.c. That's the core way
of doing things. In that case call it drm_atomic_crtc_for_encoder, and
put it into drm_atomic.c.
There's two ways of doing the 2nd one: looping over connectors in a
drm_atomic_state, or the connector list overall. First requires that the
encoder is already in drm_atomic_state (which I think makes sense).
Even more complications on old/new_crtc_state: Is that the old state for
the old crtc, or the old state for the new crtc (that can switch too).
Same for the new crtc state ...
tldr; I'd create 2 functions:
drm_crtc *drm_atomic_encoder_get_new_crtc(drm_atomic_state *state, encoder)
drm_crtc *drm_atomic_encoder_get_old_crtc(drm_atomic_state *state, encoder)
With the requirement that they'll return NULL if the encder isn't in in
@state, and implemented using looping over connectors in @state.
tldr; this is a lot more tricky than it looks like ...
-Daniel
> + if (!(tmp_new_crtc_state->encoder_mask & enc_mask))
> + continue;
> +
> + if (new_crtc_state)
> + *new_crtc_state = tmp_new_crtc_state;
> + if (old_crtc_state)
> + *old_crtc_state = tmp_old_crtc_state;
> + if (crtc)
> + *crtc = tmp_crtc;
> + return;
> + }
> +
> + if (new_crtc_state)
> + *new_crtc_state = NULL;
> + if (old_crtc_state)
> + *old_crtc_state = NULL;
> + if (crtc)
> + *crtc = NULL;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_state_for_encoder);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 58214be3bf3d..2383550a0cc8 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -153,6 +153,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> uint32_t size,
> struct drm_modeset_acquire_ctx *ctx);
>
> +void drm_atomic_crtc_state_for_encoder(struct drm_atomic_state *state,
> + struct drm_encoder *encoder,
> + struct drm_crtc **crtc,
> + struct drm_crtc_state **old_crtc_state,
> + struct drm_crtc_state **new_crtc_state);
> +
> /**
> * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
> * @plane: the loop cursor
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch