Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 3 May 2019 10:18:51 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Sean Paul <sean@poorly.run>
Cc:     dri-devel@lists.freedesktop.org, Sean Paul <seanpaul@chromium.org>,
        Daniel Vetter <daniel@ffwll.ch>,
        Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 
        <ville.syrjala@linux.intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 02/10] drm: Add drm_atomic_crtc_state_for_encoder
 helper
Message-ID: <20190503081851.GI3271@phenom.ffwll.local>
Mail-Followup-To: Sean Paul <sean@poorly.run>,
        dri-devel@lists.freedesktop.org, Sean Paul <seanpaul@chromium.org>,
        Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org
References: <20190502194956.218441-1-sean@poorly.run>
 <20190502194956.218441-3-sean@poorly.run>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190502194956.218441-3-sean@poorly.run>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 02, 2019 at 03:49:44PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@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@ffwll.ch>
> Cc: Ville Syrj?l? <ville.syrjala@linux.intel.com>
> Signed-off-by: Sean Paul <seanpaul@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