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:   Sun, 12 May 2019 19:48:36 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Sean Paul <sean@poorly.run>, dri-devel@lists.freedesktop.org,
        Sean Paul <seanpaul@chromium.org>,
        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 v4 02/11] drm: Add
 drm_atomic_get_(old|new-_connector_for_encoder() helpers
Message-ID: <20190512164836.GA15762@pendragon.ideasonboard.com>
References: <20190508160920.144739-1-sean@poorly.run>
 <20190508160920.144739-3-sean@poorly.run>
 <20190508163327.GZ17751@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20190508163327.GZ17751@phenom.ffwll.local>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Sean,

In the subject line, s/-/)/

On Wed, May 08, 2019 at 06:33:27PM +0200, Daniel Vetter wrote:
> On Wed, May 08, 2019 at 12:09:07PM -0400, Sean Paul wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Add functions to the atomic core to retrieve the old and new connectors
> > associated with an encoder in a drm_atomic_state. This is useful for
> > encoders and bridges that need to access the connector, for instance for
> > the drm_display_info.
> > 
> > The CRTC associated with the encoder can also be retrieved through the
> > connector state, and from it, the old and new CRTC states.
> > 
> > Changed in v4:
> > - Added to the set
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > [seanpaul removed WARNs from helpers and added docs to explain why
> > returning NULL might be valid]
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 70 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic.h     |  7 ++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 5eb40130fafb..936002495523 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -797,6 +797,76 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_get_private_obj_state);
> >  
> > +/**
> > + * drm_atomic_get_old_connector_for_encoder - Get old connector for an encoder
> > + * @state: Atomic state
> > + * @encoder: The encoder to fetch the connector state for
> > + *
> > + * This function finds and returns the connector that was connected to @encoder
> > + * as specified by the @state.
> > + *
> > + * If there is no connector in @state which previously had @encoder connected to
> > + * it, this function will return NULL. While this may seem like an invalid use
> > + * case, it is sometimes a useful to differentiate commits which had no prior

s/a useful/useful/

> > + * connectors attached to @encoder vs ones that did (and to inspect their
> > + * @state). This is especially true in enable hooks because the pipeline has

s/@state/state/, otherwise you refer to the parameter passed to this
function, which is not a connector state.

> > + * changed/will change.
> 
> s/has changed// ... I meant you'll pick the right ver tense :-)
> 
> > + *
> > + * Returns: The old connector connected to @encoder, or NULL if the encoder is
> > + * not connected.
> > + */
> > +struct drm_connector *
> > +drm_atomic_get_old_connector_for_encoder(struct drm_atomic_state *state,
> > +					 struct drm_encoder *encoder)
> > +{
> > +	struct drm_connector_state *conn_state;
> > +	struct drm_connector *connector;
> > +	unsigned int i;
> > +
> > +	for_each_old_connector_in_state(state, connector, conn_state, i) {
> > +		if (conn_state->best_encoder == encoder)
> > +			return connector;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_get_old_connector_for_encoder);
> > +
> > +/**
> > + * drm_atomic_get_new_connector_for_encoder - Get new connector for an encoder
> > + * @state: Atomic state
> > + * @encoder: The encoder to fetch the connector state for
> > + *
> > + * This function finds and returns the connector that will be connected to
> > + * @encoder as specified by the @state.
> > + *
> > + * If there is no connector in @state which will have @encoder connected to it,
> > + * this function will return NULL. While this may seem like an invalid use case,
> > + * it is sometimes a useful to differentiate commits which have no connectors

s/a useful/useful/

> > + * attached to @encoder vs ones that do (and to inspect their state). This is
> > + * especially true in disable hooks because the pipeline has changed/will
> > + * change.
> 
> s/will change//
> 
> > + *
> > + * Returns: The new connector connected to @encoder, or NULL if the encoder is
> > + * not connected.
> > + */
> > +struct drm_connector *
> > +drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
> > +					 struct drm_encoder *encoder)
> > +{
> > +	struct drm_connector_state *conn_state;
> > +	struct drm_connector *connector;
> > +	unsigned int i;
> > +
> > +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> > +		if (conn_state->best_encoder == encoder)
> > +			return connector;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
> 
> Maybe also add a "See also drm_atomic_get_old_connector_for_encoder() and
> drm_atomic_get_new_connector_for_encoder()." to the kerneldoc of
> drm_connector_state.best_encoder.
> 
> With these doc nits:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > +
> >  /**
> >   * drm_atomic_get_connector_state - get connector state
> >   * @state: global atomic state object
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 824a5ed4e216..d6b3acd34e1c 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -453,6 +453,13 @@ struct drm_private_state * __must_check
> >  drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
> >  				 struct drm_private_obj *obj);
> >  
> > +struct drm_connector *
> > +drm_atomic_get_old_connector_for_encoder(struct drm_atomic_state *state,
> > +					 struct drm_encoder *encoder);
> > +struct drm_connector *
> > +drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state,
> > +					 struct drm_encoder *encoder);
> > +
> >  /**
> >   * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> >   * @state: global atomic state object

-- 
Regards,

Laurent Pinchart