[v3,02/10] drm: Add drm_atomic_crtc_state_for_encoder helper

Commit Message

Sean Paul May 2, 2019, 7:49 p.m. UTC

From: Sean Paul <[e-mail]>

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.

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 <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Signed-off-by: Sean Paul <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c | 48 +++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  6 ++++
 2 files changed, 54 insertions(+)

Comments

Daniel Vetter May 3, 2019, 8:18 a.m. UTC | #1

On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> From: Sean Paul <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Signed-off-by: Sean Paul <[e-mail]>
> ---
>  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
>

Sean Paul May 3, 2019, 12:47 p.m. UTC | #2

On Fri, May 03, 2019 at 10:18:51AM +0200, Daniel Vetter wrote:
> On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> > From: Sean Paul <[e-mail]>
> > 
> > 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.

No longer relevant with the changes below, but for completeness, I was trying to
refer to drm_atomic_crtc_state_for_each_plane and friends. I see now that I
wasn't terribly clear :)


> > 
> > 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 <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Signed-off-by: Sean Paul <[e-mail]>
> > ---
> >  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).

Yeah, I wasn't particularly interested in encoders not in state. I had
considered going the connector route, but since you can have multiple connectors
per encoder, going through crtc seemed a bit more direct. 

> 
> 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.

It seems like we could just tweak this function a bit to get the new or old crtc
for an encoder. Any particular reason for going through connector instead? Is it
to avoid the encoder_mask which is a helper thing? In that case, perhaps this
should use connector links and live in drm_atomic.c?

Thanks for the review!

Sean

> 
> 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

Daniel Vetter May 3, 2019, 2:06 p.m. UTC | #3

On Fri, May 3, 2019 at 2:47 PM Sean Paul <[e-mail]> wrote:
> On Fri, May 03, 2019 at 10:18:51AM +0200, Daniel Vetter wrote:
> > On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> > > From: Sean Paul <[e-mail]>
> > >
> > > 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.
>
> No longer relevant with the changes below, but for completeness, I was trying to
> refer to drm_atomic_crtc_state_for_each_plane and friends. I see now that I
> wasn't terribly clear :)
>
>
> > >
> > > 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 <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Signed-off-by: Sean Paul <[e-mail]>
> > > ---
> > >  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).
>
> Yeah, I wasn't particularly interested in encoders not in state. I had
> considered going the connector route, but since you can have multiple connectors
> per encoder, going through crtc seemed a bit more direct.

You can have multiple possible connectors for a given encoder, and
multiple possible encoders for a given connector. In both cases the
driver picks for you. But for active encoders and connectors the
relationship is 1:1. That's what the helpers exploit by looping over
connectors to get at encoders.

> > 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.
>
> It seems like we could just tweak this function a bit to get the new or old crtc
> for an encoder. Any particular reason for going through connector instead? Is it
> to avoid the encoder_mask which is a helper thing? In that case, perhaps this
> should use connector links and live in drm_atomic.c?

Well as explained, there's 3 ways you can achieve the same really. I
do think the "loop over connectors in drm_atomic_state, WARN() if we
haven't found the encoder you want the crtc for" is probably the most
solid aproach since it picks up a core atomic concept. But the others
(looping the connector list, or looping encoder_mask) all work too.

Aside: plane/encoder_mask was added to go from crtc to
planes/encoders, not really to go the other way round.

Another solution would be to pass the connector_state to all atomic
encoder hooks, then you could just look at connector_state->crtc. Plus
you can get at all the other interesting bits and pieces of
information. In a way this is fallout from us keeping encoders as a
meaningful concept for easier transition of legacy drivers, while
still keeping them entirely irrelevant for the actual userspace api
semantics.

So maybe we want drm_atomic_encoder_get_old/new_connector here. Or
maybe we even want the full set of functions, i.e.
drm_atomic_encoder_get_old/new_connector/crtc.

In all cases I think only returning the object, not it's state is
simplest, since then you avoid the confusion of old/new state for
old/new obj.
-Daniel


> Thanks for the review!
>
> Sean
>
> >
> > 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
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter May 5, 2019, 9:15 p.m. UTC | #4

On Fri, May 03, 2019 at 04:06:28PM +0200, Daniel Vetter wrote:
> On Fri, May 3, 2019 at 2:47 PM Sean Paul <[e-mail]> wrote:
> > On Fri, May 03, 2019 at 10:18:51AM +0200, Daniel Vetter wrote:
> > > On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> > > > From: Sean Paul <[e-mail]>
> > > >
> > > > 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.
> >
> > No longer relevant with the changes below, but for completeness, I was trying to
> > refer to drm_atomic_crtc_state_for_each_plane and friends. I see now that I
> > wasn't terribly clear :)
> >
> >
> > > >
> > > > 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 <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Signed-off-by: Sean Paul <[e-mail]>
> > > > ---
> > > >  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).
> >
> > Yeah, I wasn't particularly interested in encoders not in state. I had
> > considered going the connector route, but since you can have multiple connectors
> > per encoder, going through crtc seemed a bit more direct.
> 
> You can have multiple possible connectors for a given encoder, and
> multiple possible encoders for a given connector. In both cases the
> driver picks for you. But for active encoders and connectors the
> relationship is 1:1. That's what the helpers exploit by looping over
> connectors to get at encoders.
> 
> > > 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.
> >
> > It seems like we could just tweak this function a bit to get the new or old crtc
> > for an encoder. Any particular reason for going through connector instead? Is it
> > to avoid the encoder_mask which is a helper thing? In that case, perhaps this
> > should use connector links and live in drm_atomic.c?
> 
> Well as explained, there's 3 ways you can achieve the same really. I
> do think the "loop over connectors in drm_atomic_state, WARN() if we
> haven't found the encoder you want the crtc for" is probably the most
> solid aproach since it picks up a core atomic concept. But the others
> (looping the connector list, or looping encoder_mask) all work too.
> 
> Aside: plane/encoder_mask was added to go from crtc to
> planes/encoders, not really to go the other way round.
> 
> Another solution would be to pass the connector_state to all atomic
> encoder hooks, then you could just look at connector_state->crtc. Plus
> you can get at all the other interesting bits and pieces of
> information. In a way this is fallout from us keeping encoders as a
> meaningful concept for easier transition of legacy drivers, while
> still keeping them entirely irrelevant for the actual userspace api
> semantics.
> 
> So maybe we want drm_atomic_encoder_get_old/new_connector here. Or
> maybe we even want the full set of functions, i.e.
> drm_atomic_encoder_get_old/new_connector/crtc.

Laurent just asked me on irc how to get from a bridge to connector
information. Bridge knows its encoder (it's static), so a encode2connector
function would already have a 2nd user with this.

I.e. I'd say +1 on that approach, and then you can get at the connector
state and whatever else you feel like from there?

Adding Laurent.
-Daniel

> 
> In all cases I think only returning the object, not it's state is
> simplest, since then you avoid the confusion of old/new state for
> old/new obj.
> -Daniel
> 
> 
> > Thanks for the review!
> >
> > Sean
> >
> > >
> > > 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
> >
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Laurent Pinchart May 5, 2019, 10:01 p.m. UTC | #5

Hello,

On Sun, May 05, 2019 at 11:15:36PM +0200, Daniel Vetter wrote:
> On Fri, May 03, 2019 at 04:06:28PM +0200, Daniel Vetter wrote:
> > On Fri, May 3, 2019 at 2:47 PM Sean Paul <[e-mail]> wrote:
> >> On Fri, May 03, 2019 at 10:18:51AM +0200, Daniel Vetter wrote:
> >>> On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> >>>> From: Sean Paul <[e-mail]>
> >>>>
> >>>> 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.
> >>
> >> No longer relevant with the changes below, but for completeness, I was trying to
> >> refer to drm_atomic_crtc_state_for_each_plane and friends. I see now that I
> >> wasn't terribly clear :)
> >>
> >>>> 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 <[e-mail]>
> >>>> Cc: Ville Syrjälä <[e-mail]>
> >>>> Signed-off-by: Sean Paul <[e-mail]>
> >>>> ---
> >>>>  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).
> >>
> >> Yeah, I wasn't particularly interested in encoders not in state. I had
> >> considered going the connector route, but since you can have multiple connectors
> >> per encoder, going through crtc seemed a bit more direct.
> > 
> > You can have multiple possible connectors for a given encoder, and
> > multiple possible encoders for a given connector. In both cases the
> > driver picks for you. But for active encoders and connectors the
> > relationship is 1:1. That's what the helpers exploit by looping over
> > connectors to get at encoders.
> > 
> >>> 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.
> >>
> >> It seems like we could just tweak this function a bit to get the new or old crtc
> >> for an encoder. Any particular reason for going through connector instead? Is it
> >> to avoid the encoder_mask which is a helper thing? In that case, perhaps this
> >> should use connector links and live in drm_atomic.c?
> > 
> > Well as explained, there's 3 ways you can achieve the same really. I
> > do think the "loop over connectors in drm_atomic_state, WARN() if we
> > haven't found the encoder you want the crtc for" is probably the most
> > solid aproach since it picks up a core atomic concept. But the others
> > (looping the connector list, or looping encoder_mask) all work too.
> > 
> > Aside: plane/encoder_mask was added to go from crtc to
> > planes/encoders, not really to go the other way round.
> > 
> > Another solution would be to pass the connector_state to all atomic
> > encoder hooks, then you could just look at connector_state->crtc. Plus
> > you can get at all the other interesting bits and pieces of
> > information. In a way this is fallout from us keeping encoders as a
> > meaningful concept for easier transition of legacy drivers, while
> > still keeping them entirely irrelevant for the actual userspace api
> > semantics.
> > 
> > So maybe we want drm_atomic_encoder_get_old/new_connector here. Or
> > maybe we even want the full set of functions, i.e.
> > drm_atomic_encoder_get_old/new_connector/crtc.
> 
> Laurent just asked me on irc how to get from a bridge to connector
> information. Bridge knows its encoder (it's static), so a encode2connector
> function would already have a 2nd user with this.
> 
> I.e. I'd say +1 on that approach, and then you can get at the connector
> state and whatever else you feel like from there?
> 
> Adding Laurent.

Thank you. I'll experiment with this, but please don't wait for me to
post a v4.

> > In all cases I think only returning the object, not it's state is
> > simplest, since then you avoid the confusion of old/new state for
> > old/new obj.
> >
> >>> tldr; this is a lot more tricky than it looks like ...
> >>>
> >>>> +           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

Patch

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) {
+		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