| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190503124737.GH17077@art_vandelay>
Date: Fri, 3 May 2019 08:47:37 -0400
From: Sean Paul <sean@...rly.run>
To: Sean Paul <sean@...rly.run>, dri-devel@...ts.freedesktop.org,
Sean Paul <seanpaul@...omium.org>,
Ville Syrjälä
<ville.syrjala@...ux.intel.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <maxime.ripard@...tlin.com>,
David Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 02/10] drm: Add drm_atomic_crtc_state_for_encoder
helper
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@...omium.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.
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@...ll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>
> > Signed-off-by: Sean Paul <seanpaul@...omium.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).
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
--
Sean Paul, Software Engineer, Google / Chromium OS
Powered by blists - more mailing lists