Re: [PATCH 1/2] drm/connector: Share with non-atomic drivers the function to get the single encoder

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 11, 2019 at 08:01:55PM +0000, Souza, Jose wrote:
> On Wed, 2019-09-11 at 21:10 +0300, Ville Syrjälä wrote:
> > On Wed, Sep 11, 2019 at 10:56:02AM -0700, José Roberto de Souza
> > wrote:
> > > This 3 non-atomic drivers all have the same function getting the
> > > only encoder available in the connector, also atomic drivers have
> > > this fallback. So moving it a common place and sharing between
> > > atomic
> > > and non-atomic drivers.
> > > 
> > > While at it I also removed the mention of
> > > drm_atomic_helper_best_encoder() that was renamed in
> > > commit 297e30b5d9b6 ("drm/atomic-helper: Unexport
> > > drm_atomic_helper_best_encoder").
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/ast/ast_mode.c           | 12 ------------
> > >  drivers/gpu/drm/drm_atomic_helper.c      | 15 ++-------------
> > >  drivers/gpu/drm/drm_connector.c          | 11 +++++++++++
> > >  drivers/gpu/drm/drm_crtc_helper.c        |  8 +++++++-
> > >  drivers/gpu/drm/drm_crtc_internal.h      |  2 ++
> > >  drivers/gpu/drm/mgag200/mgag200_mode.c   | 11 -----------
> > >  drivers/gpu/drm/udl/udl_connector.c      |  8 --------
> > >  include/drm/drm_modeset_helper_vtables.h |  6 +++---
> > >  8 files changed, 25 insertions(+), 48 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/ast/ast_mode.c
> > > b/drivers/gpu/drm/ast/ast_mode.c
> > > index d349c721501c..eef95e1af06b 100644
> > > --- a/drivers/gpu/drm/ast/ast_mode.c
> > > +++ b/drivers/gpu/drm/ast/ast_mode.c
> > > @@ -687,17 +687,6 @@ static void ast_encoder_destroy(struct
> > > drm_encoder *encoder)
> > >  	kfree(encoder);
> > >  }
> > >  
> > > -
> > > -static struct drm_encoder *ast_best_single_encoder(struct
> > > drm_connector *connector)
> > > -{
> > > -	int enc_id = connector->encoder_ids[0];
> > > -	/* pick the encoder ids */
> > > -	if (enc_id)
> > > -		return drm_encoder_find(connector->dev, NULL, enc_id);
> > > -	return NULL;
> > > -}
> > > -
> > > -
> > >  static const struct drm_encoder_funcs ast_enc_funcs = {
> > >  	.destroy = ast_encoder_destroy,
> > >  };
> > > @@ -847,7 +836,6 @@ static void ast_connector_destroy(struct
> > > drm_connector *connector)
> > >  static const struct drm_connector_helper_funcs
> > > ast_connector_helper_funcs = {
> > >  	.mode_valid = ast_mode_valid,
> > >  	.get_modes = ast_get_modes,
> > > -	.best_encoder = ast_best_single_encoder,
> > >  };
> > >  
> > >  static const struct drm_connector_funcs ast_connector_funcs = {
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 4706439fb490..9d7e4da6c292 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -97,17 +97,6 @@ drm_atomic_helper_plane_changed(struct
> > > drm_atomic_state *state,
> > >  	}
> > >  }
> > >  
> > > -/*
> > > - * For connectors that support multiple encoders, either the
> > > - * .atomic_best_encoder() or .best_encoder() operation must be
> > > implemented.
> > > - */
> > > -static struct drm_encoder *
> > > -pick_single_encoder_for_connector(struct drm_connector *connector)
> > > -{
> > > -	WARN_ON(connector->encoder_ids[1]);
> > > -	return drm_encoder_find(connector->dev, NULL, connector-
> > > >encoder_ids[0]);
> > > -}
> > > -
> > >  static int handle_conflicting_encoders(struct drm_atomic_state
> > > *state,
> > >  				       bool
> > > disable_conflicting_encoders)
> > >  {
> > > @@ -135,7 +124,7 @@ static int handle_conflicting_encoders(struct
> > > drm_atomic_state *state,
> > >  		else if (funcs->best_encoder)
> > >  			new_encoder = funcs->best_encoder(connector);
> > >  		else
> > > -			new_encoder =
> > > pick_single_encoder_for_connector(connector);
> > > +			new_encoder =
> > > drm_connector_get_single_encoder(connector);
> > >  
> > >  		if (new_encoder) {
> > >  			if (encoder_mask &
> > > drm_encoder_mask(new_encoder)) {
> > > @@ -359,7 +348,7 @@ update_connector_routing(struct
> > > drm_atomic_state *state,
> > >  	else if (funcs->best_encoder)
> > >  		new_encoder = funcs->best_encoder(connector);
> > >  	else
> > > -		new_encoder =
> > > pick_single_encoder_for_connector(connector);
> > > +		new_encoder =
> > > drm_connector_get_single_encoder(connector);
> > >  
> > >  	if (!new_encoder) {
> > >  		DRM_DEBUG_ATOMIC("No suitable encoder found for
> > > [CONNECTOR:%d:%s]\n",
> > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > b/drivers/gpu/drm/drm_connector.c
> > > index 4c766624b20d..3e2a632cf861 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -2334,3 +2334,14 @@ struct drm_tile_group
> > > *drm_mode_create_tile_group(struct drm_device *dev,
> > >  	return tg;
> > >  }
> > >  EXPORT_SYMBOL(drm_mode_create_tile_group);
> > > +
> > > +/*
> > > + * For connectors that support multiple encoders, either the
> > > + * .atomic_best_encoder() or .best_encoder() operation must be
> > > implemented.
> > > + */
> > > +struct drm_encoder *
> > > +drm_connector_get_single_encoder(struct drm_connector *connector)
> > > +{
> > > +	WARN_ON(connector->encoder_ids[1]);
> > > +	return drm_encoder_find(connector->dev, NULL, connector-
> > > >encoder_ids[0]);
> > > +}
> > 
> > I believe we need EXPORT_SYMBOL.
> 
> I don't think we should allow drivers to call this function.

Not drivers. But you already call it from the other module.
In fact all your calls seem to be from the other module, so
perhaps you should move the function into that module.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx