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 <sean@xxxxxxxxxx> 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 <seanpaul@xxxxxxxxxxxx>
> >>>>
> >>>> 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 <daniel@xxxxxxxx>
> >>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> >>>> ---
> >>>> 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
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
