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

Submitted by Souza, Jose on Sept. 11, 2019, 5:56 p.m.

Details

Message ID 20190911175603.30356-1-jose.souza@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Souza, Jose Sept. 11, 2019, 5:56 p.m.
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(-)

Patch hide | download patch | download mbox

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]);
+}
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index a51824a7e7c1..a1f3c388e398 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -48,6 +48,8 @@ 
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
 
+#include "drm_crtc_internal.h"
+
 /**
  * DOC: overview
  *
@@ -625,7 +627,11 @@  int drm_crtc_helper_set_config(struct drm_mode_set *set,
 		new_encoder = connector->encoder;
 		for (ro = 0; ro < set->num_connectors; ro++) {
 			if (set->connectors[ro] == connector) {
-				new_encoder = connector_funcs->best_encoder(connector);
+				if (connector_funcs->best_encoder)
+					new_encoder = connector_funcs->best_encoder(connector);
+				else
+					new_encoder = drm_connector_get_single_encoder(connector);
+
 				/* if we can't get an encoder for a connector
 				   we are setting now - then fail */
 				if (new_encoder == NULL)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index c7d5e4c21423..80ade1fa66e5 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -181,6 +181,8 @@  int drm_connector_set_obj_prop(struct drm_mode_object *obj,
 int drm_connector_create_standard_properties(struct drm_device *dev);
 const char *drm_get_connector_force_name(enum drm_connector_force force);
 void drm_connector_free_work_fn(struct work_struct *work);
+struct drm_encoder *
+drm_connector_get_single_encoder(struct drm_connector *connector);
 
 /* IOCTL */
 int drm_connector_property_set_ioctl(struct drm_device *dev,
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 5e778b5f1a10..68226556044b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1638,16 +1638,6 @@  static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
-static struct drm_encoder *mga_connector_best_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 void mga_connector_destroy(struct drm_connector *connector)
 {
 	struct mga_connector *mga_connector = to_mga_connector(connector);
@@ -1659,7 +1649,6 @@  static void mga_connector_destroy(struct drm_connector *connector)
 static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
 	.get_modes = mga_vga_get_modes,
 	.mode_valid = mga_vga_mode_valid,
-	.best_encoder = mga_connector_best_encoder,
 };
 
 static const struct drm_connector_funcs mga_vga_connector_funcs = {
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index ddb61a60c610..b4ae3e89a7b4 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -90,13 +90,6 @@  udl_detect(struct drm_connector *connector, bool force)
 	return connector_status_connected;
 }
 
-static struct drm_encoder*
-udl_best_single_encoder(struct drm_connector *connector)
-{
-	int enc_id = connector->encoder_ids[0];
-	return drm_encoder_find(connector->dev, NULL, enc_id);
-}
-
 static int udl_connector_set_property(struct drm_connector *connector,
 				      struct drm_property *property,
 				      uint64_t val)
@@ -120,7 +113,6 @@  static void udl_connector_destroy(struct drm_connector *connector)
 static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
 	.get_modes = udl_get_modes,
 	.mode_valid = udl_mode_valid,
-	.best_encoder = udl_best_single_encoder,
 };
 
 static const struct drm_connector_funcs udl_connector_funcs = {
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 6b18c8adfe9d..b55412c6ce3a 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -955,8 +955,8 @@  struct drm_connector_helper_funcs {
 	 * @atomic_best_encoder.
 	 *
 	 * You can leave this function to NULL if the connector is only
-	 * attached to a single encoder and you are using the atomic helpers.
-	 * In this case, the core will call drm_atomic_helper_best_encoder()
+	 * attached to a single encoder.
+	 * In this case, the core will call drm_connector_get_single_encoder()
 	 * for you.
 	 *
 	 * RETURNS:
@@ -977,7 +977,7 @@  struct drm_connector_helper_funcs {
 	 *
 	 * This function is used by drm_atomic_helper_check_modeset().
 	 * If it is not implemented, the core will fallback to @best_encoder
-	 * (or drm_atomic_helper_best_encoder() if @best_encoder is NULL).
+	 * (or drm_connector_get_single_encoder() if @best_encoder is NULL).
 	 *
 	 * NOTE:
 	 *

Comments

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.

> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index a51824a7e7c1..a1f3c388e398 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -48,6 +48,8 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  
> +#include "drm_crtc_internal.h"
> +
>  /**
>   * DOC: overview
>   *
> @@ -625,7 +627,11 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set,
>  		new_encoder = connector->encoder;
>  		for (ro = 0; ro < set->num_connectors; ro++) {
>  			if (set->connectors[ro] == connector) {
> -				new_encoder = connector_funcs->best_encoder(connector);
> +				if (connector_funcs->best_encoder)
> +					new_encoder = connector_funcs->best_encoder(connector);
> +				else
> +					new_encoder = drm_connector_get_single_encoder(connector);
> +
>  				/* if we can't get an encoder for a connector
>  				   we are setting now - then fail */
>  				if (new_encoder == NULL)
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index c7d5e4c21423..80ade1fa66e5 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -181,6 +181,8 @@ int drm_connector_set_obj_prop(struct drm_mode_object *obj,
>  int drm_connector_create_standard_properties(struct drm_device *dev);
>  const char *drm_get_connector_force_name(enum drm_connector_force force);
>  void drm_connector_free_work_fn(struct work_struct *work);
> +struct drm_encoder *
> +drm_connector_get_single_encoder(struct drm_connector *connector);
>  
>  /* IOCTL */
>  int drm_connector_property_set_ioctl(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 5e778b5f1a10..68226556044b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1638,16 +1638,6 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
>  	return MODE_OK;
>  }
>  
> -static struct drm_encoder *mga_connector_best_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 void mga_connector_destroy(struct drm_connector *connector)
>  {
>  	struct mga_connector *mga_connector = to_mga_connector(connector);
> @@ -1659,7 +1649,6 @@ static void mga_connector_destroy(struct drm_connector *connector)
>  static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {
>  	.get_modes = mga_vga_get_modes,
>  	.mode_valid = mga_vga_mode_valid,
> -	.best_encoder = mga_connector_best_encoder,
>  };
>  
>  static const struct drm_connector_funcs mga_vga_connector_funcs = {
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index ddb61a60c610..b4ae3e89a7b4 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -90,13 +90,6 @@ udl_detect(struct drm_connector *connector, bool force)
>  	return connector_status_connected;
>  }
>  
> -static struct drm_encoder*
> -udl_best_single_encoder(struct drm_connector *connector)
> -{
> -	int enc_id = connector->encoder_ids[0];
> -	return drm_encoder_find(connector->dev, NULL, enc_id);
> -}
> -
>  static int udl_connector_set_property(struct drm_connector *connector,
>  				      struct drm_property *property,
>  				      uint64_t val)
> @@ -120,7 +113,6 @@ static void udl_connector_destroy(struct drm_connector *connector)
>  static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>  	.get_modes = udl_get_modes,
>  	.mode_valid = udl_mode_valid,
> -	.best_encoder = udl_best_single_encoder,
>  };
>  
>  static const struct drm_connector_funcs udl_connector_funcs = {
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 6b18c8adfe9d..b55412c6ce3a 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -955,8 +955,8 @@ struct drm_connector_helper_funcs {
>  	 * @atomic_best_encoder.
>  	 *
>  	 * You can leave this function to NULL if the connector is only
> -	 * attached to a single encoder and you are using the atomic helpers.
> -	 * In this case, the core will call drm_atomic_helper_best_encoder()
> +	 * attached to a single encoder.
> +	 * In this case, the core will call drm_connector_get_single_encoder()
>  	 * for you.
>  	 *
>  	 * RETURNS:
> @@ -977,7 +977,7 @@ struct drm_connector_helper_funcs {
>  	 *
>  	 * This function is used by drm_atomic_helper_check_modeset().
>  	 * If it is not implemented, the core will fallback to @best_encoder
> -	 * (or drm_atomic_helper_best_encoder() if @best_encoder is NULL).
> +	 * (or drm_connector_get_single_encoder() if @best_encoder is NULL).
>  	 *
>  	 * NOTE:
>  	 *
> -- 
> 2.23.0
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.

> 

> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c

> > b/drivers/gpu/drm/drm_crtc_helper.c

> > index a51824a7e7c1..a1f3c388e398 100644

> > --- a/drivers/gpu/drm/drm_crtc_helper.c

> > +++ b/drivers/gpu/drm/drm_crtc_helper.c

> > @@ -48,6 +48,8 @@

> >  #include <drm/drm_print.h>

> >  #include <drm/drm_vblank.h>

> >  

> > +#include "drm_crtc_internal.h"

> > +

> >  /**

> >   * DOC: overview

> >   *

> > @@ -625,7 +627,11 @@ int drm_crtc_helper_set_config(struct

> > drm_mode_set *set,

> >  		new_encoder = connector->encoder;

> >  		for (ro = 0; ro < set->num_connectors; ro++) {

> >  			if (set->connectors[ro] == connector) {

> > -				new_encoder = connector_funcs-

> > >best_encoder(connector);

> > +				if (connector_funcs->best_encoder)

> > +					new_encoder = connector_funcs-

> > >best_encoder(connector);

> > +				else

> > +					new_encoder =

> > drm_connector_get_single_encoder(connector);

> > +

> >  				/* if we can't get an encoder for a

> > connector

> >  				   we are setting now - then fail */

> >  				if (new_encoder == NULL)

> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h

> > b/drivers/gpu/drm/drm_crtc_internal.h

> > index c7d5e4c21423..80ade1fa66e5 100644

> > --- a/drivers/gpu/drm/drm_crtc_internal.h

> > +++ b/drivers/gpu/drm/drm_crtc_internal.h

> > @@ -181,6 +181,8 @@ int drm_connector_set_obj_prop(struct

> > drm_mode_object *obj,

> >  int drm_connector_create_standard_properties(struct drm_device

> > *dev);

> >  const char *drm_get_connector_force_name(enum drm_connector_force

> > force);

> >  void drm_connector_free_work_fn(struct work_struct *work);

> > +struct drm_encoder *

> > +drm_connector_get_single_encoder(struct drm_connector *connector);

> >  

> >  /* IOCTL */

> >  int drm_connector_property_set_ioctl(struct drm_device *dev,

> > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c

> > b/drivers/gpu/drm/mgag200/mgag200_mode.c

> > index 5e778b5f1a10..68226556044b 100644

> > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c

> > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c

> > @@ -1638,16 +1638,6 @@ static enum drm_mode_status

> > mga_vga_mode_valid(struct drm_connector *connector,

> >  	return MODE_OK;

> >  }

> >  

> > -static struct drm_encoder *mga_connector_best_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 void mga_connector_destroy(struct drm_connector *connector)

> >  {

> >  	struct mga_connector *mga_connector =

> > to_mga_connector(connector);

> > @@ -1659,7 +1649,6 @@ static void mga_connector_destroy(struct

> > drm_connector *connector)

> >  static const struct drm_connector_helper_funcs

> > mga_vga_connector_helper_funcs = {

> >  	.get_modes = mga_vga_get_modes,

> >  	.mode_valid = mga_vga_mode_valid,

> > -	.best_encoder = mga_connector_best_encoder,

> >  };

> >  

> >  static const struct drm_connector_funcs mga_vga_connector_funcs =

> > {

> > diff --git a/drivers/gpu/drm/udl/udl_connector.c

> > b/drivers/gpu/drm/udl/udl_connector.c

> > index ddb61a60c610..b4ae3e89a7b4 100644

> > --- a/drivers/gpu/drm/udl/udl_connector.c

> > +++ b/drivers/gpu/drm/udl/udl_connector.c

> > @@ -90,13 +90,6 @@ udl_detect(struct drm_connector *connector, bool

> > force)

> >  	return connector_status_connected;

> >  }

> >  

> > -static struct drm_encoder*

> > -udl_best_single_encoder(struct drm_connector *connector)

> > -{

> > -	int enc_id = connector->encoder_ids[0];

> > -	return drm_encoder_find(connector->dev, NULL, enc_id);

> > -}

> > -

> >  static int udl_connector_set_property(struct drm_connector

> > *connector,

> >  				      struct drm_property *property,

> >  				      uint64_t val)

> > @@ -120,7 +113,6 @@ static void udl_connector_destroy(struct

> > drm_connector *connector)

> >  static const struct drm_connector_helper_funcs

> > udl_connector_helper_funcs = {

> >  	.get_modes = udl_get_modes,

> >  	.mode_valid = udl_mode_valid,

> > -	.best_encoder = udl_best_single_encoder,

> >  };

> >  

> >  static const struct drm_connector_funcs udl_connector_funcs = {

> > diff --git a/include/drm/drm_modeset_helper_vtables.h

> > b/include/drm/drm_modeset_helper_vtables.h

> > index 6b18c8adfe9d..b55412c6ce3a 100644

> > --- a/include/drm/drm_modeset_helper_vtables.h

> > +++ b/include/drm/drm_modeset_helper_vtables.h

> > @@ -955,8 +955,8 @@ struct drm_connector_helper_funcs {

> >  	 * @atomic_best_encoder.

> >  	 *

> >  	 * You can leave this function to NULL if the connector is only

> > -	 * attached to a single encoder and you are using the atomic

> > helpers.

> > -	 * In this case, the core will call

> > drm_atomic_helper_best_encoder()

> > +	 * attached to a single encoder.

> > +	 * In this case, the core will call

> > drm_connector_get_single_encoder()

> >  	 * for you.

> >  	 *

> >  	 * RETURNS:

> > @@ -977,7 +977,7 @@ struct drm_connector_helper_funcs {

> >  	 *

> >  	 * This function is used by drm_atomic_helper_check_modeset().

> >  	 * If it is not implemented, the core will fallback to

> > @best_encoder

> > -	 * (or drm_atomic_helper_best_encoder() if @best_encoder is

> > NULL).

> > +	 * (or drm_connector_get_single_encoder() if @best_encoder is

> > NULL).

> >  	 *

> >  	 * NOTE:

> >  	 *

> > -- 

> > 2.23.0
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.