All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess
@ 2018-06-26 17:47 Ville Syrjala
  2018-06-26 17:47 ` [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage Ville Syrjala
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Daniel pointed out that there should be a way to do what userspace does
and not require any .best_encoder() stuff in the fb-helper. So this
series does just that. Well, most of the series is actually just extra
polish that happened by accident. But the real thing is in there
somewhere still :)

The not-so-great previous attempt was this thing:
https://patchwork.freedesktop.org/series/44864/

Ville Syrjälä (8):
  drm/fb-helper: Eliminate the .best_encoder() usage
  drm/i915: Nuke intel_mst_best_encoder()
  drm: Add drm_for_each_connector_encoder_ids()
  drm/amdgpu: Use drm_for_each_connector_encoder_ids()
  drm/msm: Use drm_for_each_connector_encoder_ids()
  drm/nouveau: Use drm_for_each_connector_encoder_ids()
  drm/radeon: Use drm_for_each_connector_encoder_ids()
  drm/tilcdc: Use drm_for_each_connector_encoder_ids()

 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 51 ++++++++-------------
 drivers/gpu/drm/amd/amdgpu/dce_virtual.c       | 15 +++---
 drivers/gpu/drm/drm_connector.c                | 22 ++++-----
 drivers/gpu/drm/drm_fb_helper.c                | 36 ++++++++-------
 drivers/gpu/drm/drm_probe_helper.c             |  9 ++--
 drivers/gpu/drm/i915/intel_dp_mst.c            | 10 ----
 drivers/gpu/drm/msm/dsi/dsi_manager.c          |  5 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c    | 20 +++-----
 drivers/gpu/drm/radeon/radeon_connectors.c     | 63 +++++++++++---------------
 drivers/gpu/drm/tilcdc/tilcdc_external.c       |  5 +-
 include/drm/drm_connector.h                    | 11 +++++
 11 files changed, 111 insertions(+), 136 deletions(-)

-- 
2.16.4

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
@ 2018-06-26 17:47 ` Ville Syrjala
  2018-06-26 17:58   ` Alex Deucher
  2018-06-27  9:03   ` Daniel Vetter
  2018-06-26 17:47 ` [PATCH 2/8] drm/i915: Nuke intel_mst_best_encoder() Ville Syrjala
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter, intel-gfx, Harry Wentland, Dhinakaran Pandiyan

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Instead of using the .best_encoder() hook to figure out whether a given
connector+crtc combo will work, let's instead do what userspace does and
just iterate over all the encoders for the connector, and then check
each crtc against each encoder's possible_crtcs bitmask.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index cab14f253384..61c39cd75a27 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2323,6 +2323,23 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
 	return true;
 }
 
+static bool connector_crtc_ok(struct drm_connector *connector,
+			      struct drm_crtc *crtc)
+{
+	int i;
+
+	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+		struct drm_encoder *encoder =
+			drm_encoder_find(connector->dev, NULL,
+					 connector->encoder_ids[i]);
+
+		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
+			return true;
+	}
+
+	return false;
+}
+
 static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 			  struct drm_fb_helper_crtc **best_crtcs,
 			  struct drm_display_mode **modes,
@@ -2331,7 +2348,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	int c, o;
 	struct drm_connector *connector;
 	const struct drm_connector_helper_funcs *connector_funcs;
-	struct drm_encoder *encoder;
 	int my_score, best_score, score;
 	struct drm_fb_helper_crtc **crtcs, *crtc;
 	struct drm_fb_helper_connector *fb_helper_conn;
@@ -2362,20 +2378,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 
 	connector_funcs = connector->helper_private;
 
-	/*
-	 * If the DRM device implements atomic hooks and ->best_encoder() is
-	 * NULL we fallback to the default drm_atomic_helper_best_encoder()
-	 * helper.
-	 */
-	if (drm_drv_uses_atomic_modeset(fb_helper->dev) &&
-	    !connector_funcs->best_encoder)
-		encoder = drm_atomic_helper_best_encoder(connector);
-	else
-		encoder = connector_funcs->best_encoder(connector);
-
-	if (!encoder)
-		goto out;
-
 	/*
 	 * select a crtc for this connector and then attempt to configure
 	 * remaining connectors
@@ -2383,7 +2385,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	for (c = 0; c < fb_helper->crtc_count; c++) {
 		crtc = &fb_helper->crtc_info[c];
 
-		if ((encoder->possible_crtcs & (1 << c)) == 0)
+		if (!connector_crtc_ok(connector, crtc->mode_set.crtc))
 			continue;
 
 		for (o = 0; o < n; o++)
@@ -2410,7 +2412,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 			       sizeof(struct drm_fb_helper_crtc *));
 		}
 	}
-out:
+
 	kfree(crtcs);
 	return best_score;
 }
-- 
2.16.4

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/8] drm/i915: Nuke intel_mst_best_encoder()
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
  2018-06-26 17:47 ` [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage Ville Syrjala
@ 2018-06-26 17:47 ` Ville Syrjala
  2018-06-27  9:03   ` Daniel Vetter
  2018-06-26 17:47 ` [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids() Ville Syrjala
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx, Dhinakaran Pandiyan

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

With the fb-helper no longer relying on the non-atomic .best_encoder()
we can eliminate the hook from the MST encoder.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 5890500a3a8b..0f012fbe34eb 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -403,20 +403,10 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
 	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
 }
 
-static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connector)
-{
-	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct intel_dp *intel_dp = intel_connector->mst_port;
-	if (!intel_dp)
-		return NULL;
-	return &intel_dp->mst_encoders[0]->base.base;
-}
-
 static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = {
 	.get_modes = intel_dp_mst_get_modes,
 	.mode_valid = intel_dp_mst_mode_valid,
 	.atomic_best_encoder = intel_mst_atomic_best_encoder,
-	.best_encoder = intel_mst_best_encoder,
 	.atomic_check = intel_dp_mst_atomic_check,
 };
 
-- 
2.16.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids()
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
  2018-06-26 17:47 ` [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage Ville Syrjala
  2018-06-26 17:47 ` [PATCH 2/8] drm/i915: Nuke intel_mst_best_encoder() Ville Syrjala
@ 2018-06-26 17:47 ` Ville Syrjala
  2018-06-27  9:08   ` Daniel Vetter
       [not found] ` <20180626174714.32012-1-ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a convenience macro for iterating connector->encoder_ids[].
Isolates the users from the implementation details.

Also use ARRAY_SIZE() when populating the array to avoid spreading
knowledge about the array size all over.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_connector.c    | 22 ++++++++++------------
 drivers/gpu/drm/drm_fb_helper.c    |  6 +++---
 drivers/gpu/drm/drm_probe_helper.c |  9 +++++----
 include/drm/drm_connector.h        | 11 +++++++++++
 4 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2f9ebddd178e..c43646cb8145 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -321,7 +321,7 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 	if (WARN_ON(connector->encoder))
 		return -EINVAL;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+	for (i = 0; i < ARRAY_SIZE(connector->encoder_ids); i++) {
 		if (connector->encoder_ids[i] == 0) {
 			connector->encoder_ids[i] = encoder->base.id;
 			return 0;
@@ -1693,6 +1693,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	int encoders_count = 0;
 	int ret = 0;
 	int copied = 0;
+	u32 encoder_id;
 	int i;
 	struct drm_mode_modeinfo u_mode;
 	struct drm_mode_modeinfo __user *mode_ptr;
@@ -1708,22 +1709,19 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	if (!connector)
 		return -ENOENT;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
-		if (connector->encoder_ids[i] != 0)
-			encoders_count++;
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i)
+		encoders_count++;
 
 	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
 		copied = 0;
 		encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
-		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-			if (connector->encoder_ids[i] != 0) {
-				if (put_user(connector->encoder_ids[i],
-					     encoder_ptr + copied)) {
-					ret = -EFAULT;
-					goto out;
-				}
-				copied++;
+
+		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+			if (put_user(encoder_id, encoder_ptr + copied)) {
+				ret = -EFAULT;
+				goto out;
 			}
+			copied++;
 		}
 	}
 	out_resp->count_encoders = encoders_count;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 61c39cd75a27..e086b08748f4 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2326,12 +2326,12 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
 static bool connector_crtc_ok(struct drm_connector *connector,
 			      struct drm_crtc *crtc)
 {
+	u32 encoder_id;
 	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
 		struct drm_encoder *encoder =
-			drm_encoder_find(connector->dev, NULL,
-					 connector->encoder_ids[i]);
+			drm_encoder_find(connector->dev, NULL, encoder_id);
 
 		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
 			return true;
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 527743394150..0239f76c52fb 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -88,9 +88,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
 			    struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
-	uint32_t *ids = connector->encoder_ids;
 	enum drm_mode_status ret = MODE_OK;
-	unsigned int i;
+	u32 encoder_id;
+	int i;
 
 	/* Step 1: Validate against connector */
 	ret = drm_connector_mode_valid(connector, mode);
@@ -98,8 +98,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
 		return ret;
 
 	/* Step 2: Validate against encoders and crtcs */
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		struct drm_encoder *encoder = drm_encoder_find(dev, NULL, ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		struct drm_encoder *encoder =
+			drm_encoder_find(dev, NULL, encoder_id);
 		struct drm_crtc *crtc;
 
 		if (!encoder)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 14ab58ade87f..4a0363f7dd8c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1193,4 +1193,15 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
 #define drm_for_each_connector_iter(connector, iter) \
 	while ((connector = drm_connector_list_iter_next(iter)))
 
+/**
+ * drm_for_each_connector_encoder_ids - iterate connector encoder_ids[]
+ * @connector: &struct drm_connector pointer used as cursor
+ * @encoder_id: the encoder ID
+ * @__i: int iteration cursor, for macro-internal use
+ */
+#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
+	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
+		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
+		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])
+
 #endif
-- 
2.16.4

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/8] drm/amdgpu: Use drm_for_each_connector_encoder_ids()
       [not found] ` <20180626174714.32012-1-ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
@ 2018-06-26 17:47   ` Ville Syrjala
  2018-06-26 17:47   ` [PATCH 5/8] drm/msm: " Ville Syrjala
  2018-06-26 17:47   ` [PATCH 7/8] drm/radeon: " Ville Syrjala
  2 siblings, 0 replies; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: David (ChunMing) Zhou,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Alex Deucher,
	Harry Wentland, Christian König

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use drm_for_each_connector_encoder_ids() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 51 ++++++++++----------------
 drivers/gpu/drm/amd/amdgpu/dce_virtual.c       | 15 ++++----
 2 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 8e66851eb427..dcbe45f6d941 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -216,16 +216,13 @@ amdgpu_connector_update_scratch_regs(struct drm_connector *connector,
 	struct drm_encoder *encoder = NULL;
 	const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private;
 	bool connected;
+	u32 encoder_id;
 	int i;
 
 	best_encoder = connector_funcs->best_encoder(connector);
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL,
-					connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -244,13 +241,11 @@ amdgpu_connector_find_encoder(struct drm_connector *connector,
 			       int encoder_type)
 {
 	struct drm_encoder *encoder;
+	u32 encoder_id;
 	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-		encoder = drm_encoder_find(connector->dev, NULL,
-					connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -987,7 +982,7 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force)
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
 	struct drm_encoder *encoder = NULL;
 	const struct drm_encoder_helper_funcs *encoder_funcs;
-	int i, r;
+	int r;
 	enum drm_connector_status ret = connector_status_disconnected;
 	bool dret = false, broken_edid = false;
 
@@ -1077,11 +1072,11 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force)
 
 	/* find analog encoder */
 	if (amdgpu_connector->dac_load_detect) {
-		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-			if (connector->encoder_ids[i] == 0)
-				break;
+		u32 encoder_id;
+		int i;
 
-			encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+			encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 			if (!encoder)
 				continue;
 
@@ -1135,12 +1130,11 @@ amdgpu_connector_dvi_encoder(struct drm_connector *connector)
 	int enc_id = connector->encoder_ids[0];
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
 	struct drm_encoder *encoder;
+	u32 encoder_id;
 	int i;
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
 
-		encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -1294,14 +1288,11 @@ u16 amdgpu_connector_encoder_get_dp_bridge_encoder_id(struct drm_connector *conn
 {
 	struct drm_encoder *encoder;
 	struct amdgpu_encoder *amdgpu_encoder;
+	u32 encoder_id;
 	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL,
-					connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -1323,14 +1314,12 @@ static bool amdgpu_connector_encoder_is_hbr2(struct drm_connector *connector)
 {
 	struct drm_encoder *encoder;
 	struct amdgpu_encoder *amdgpu_encoder;
+	u32 encoder_id;
 	int i;
 	bool found = false;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-		encoder = drm_encoder_find(connector->dev, NULL,
-					connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_virtual.c b/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
index dbf2ccd0c744..f8d6318ddbba 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_virtual.c
@@ -269,15 +269,12 @@ static int dce_virtual_early_init(void *handle)
 static struct drm_encoder *
 dce_virtual_encoder(struct drm_connector *connector)
 {
-	int enc_id = connector->encoder_ids[0];
 	struct drm_encoder *encoder;
+	u32 encoder_id;
 	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -286,8 +283,10 @@ dce_virtual_encoder(struct drm_connector *connector)
 	}
 
 	/* pick the first one */
-	if (enc_id)
-		return drm_encoder_find(connector->dev, NULL, enc_id);
+	encoder_id = connector->encoder_ids[0];
+	if (encoder_id)
+		return drm_encoder_find(connector->dev, NULL, encoder_id);
+
 	return NULL;
 }
 
-- 
2.16.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/8] drm/msm: Use drm_for_each_connector_encoder_ids()
       [not found] ` <20180626174714.32012-1-ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
  2018-06-26 17:47   ` [PATCH 4/8] drm/amdgpu: Use drm_for_each_connector_encoder_ids() Ville Syrjala
@ 2018-06-26 17:47   ` Ville Syrjala
  2018-06-26 17:47   ` [PATCH 7/8] drm/radeon: " Ville Syrjala
  2 siblings, 0 replies; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Clark

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use drm_for_each_connector_encoder_ids() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

Cc: Rob Clark <robdclark@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 4cb1cb68878b..26337be9a1ec 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -751,10 +751,11 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
 	connector_list = &dev->mode_config.connector_list;
 
 	list_for_each_entry(connector, connector_list, head) {
+		u32 encoder_id;
 		int i;
 
-		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-			if (connector->encoder_ids[i] == encoder->base.id)
+		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+			if (encoder_id == encoder->base.id)
 				return connector;
 		}
 	}
-- 
2.16.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/8] drm/nouveau: Use drm_for_each_connector_encoder_ids()
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
                   ` (3 preceding siblings ...)
       [not found] ` <20180626174714.32012-1-ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
@ 2018-06-26 17:47 ` Ville Syrjala
  2018-06-26 20:55   ` [Intel-gfx] " kbuild test robot
  2018-06-26 17:47 ` [PATCH 8/8] drm/tilcdc: " Ville Syrjala
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel; +Cc: nouveau, intel-gfx, Ben Skeggs

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use drm_for_each_connector_encoder_ids() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 7b557c354307..ea12fbbea92e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -366,14 +366,11 @@ find_encoder(struct drm_connector *connector, int type)
 	struct drm_device *dev = connector->dev;
 	struct nouveau_encoder *nv_encoder;
 	struct drm_encoder *enc;
-	int i, id;
+	u32 encoder_id;
+	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		id = connector->encoder_ids[i];
-		if (!id)
-			break;
-
-		enc = drm_encoder_find(dev, NULL, id);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		enc = drm_encoder_find(dev, NULL, encoder_id);
 		if (!enc)
 			continue;
 		nv_encoder = nouveau_encoder(enc);
@@ -423,6 +420,7 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 	struct nouveau_encoder *nv_encoder;
 	struct drm_encoder *encoder;
 	int i, panel = -ENODEV;
+	u32 encoder_id;
 
 	/* eDP panels need powering on by us (if the VBIOS doesn't default it
 	 * to on) before doing any AUX channel transactions.  LVDS panel power
@@ -436,12 +434,8 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 		}
 	}
 
-	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		int id = connector->encoder_ids[i];
-		if (id == 0)
-			break;
-
-		encoder = drm_encoder_find(dev, NULL, id);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 		nv_encoder = nouveau_encoder(encoder);
-- 
2.16.4

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 7/8] drm/radeon: Use drm_for_each_connector_encoder_ids()
       [not found] ` <20180626174714.32012-1-ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
  2018-06-26 17:47   ` [PATCH 4/8] drm/amdgpu: Use drm_for_each_connector_encoder_ids() Ville Syrjala
  2018-06-26 17:47   ` [PATCH 5/8] drm/msm: " Ville Syrjala
@ 2018-06-26 17:47   ` Ville Syrjala
  2 siblings, 0 replies; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: David (ChunMing) Zhou,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Alex Deucher,
	Harry Wentland, Christian König

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use drm_for_each_connector_encoder_ids() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 63 ++++++++++++------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 2aea2bdff99b..7fc7e72a075f 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -248,16 +248,13 @@ radeon_connector_update_scratch_regs(struct drm_connector *connector, enum drm_c
 	struct drm_encoder *encoder = NULL;
 	const struct drm_connector_helper_funcs *connector_funcs = connector->helper_private;
 	bool connected;
+	u32 encoder_id;
 	int i;
 
 	best_encoder = connector_funcs->best_encoder(connector);
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL,
-					   connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -277,13 +274,11 @@ radeon_connector_update_scratch_regs(struct drm_connector *connector, enum drm_c
 static struct drm_encoder *radeon_find_encoder(struct drm_connector *connector, int encoder_type)
 {
 	struct drm_encoder *encoder;
+	u32 encoder_id;
 	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -436,6 +431,7 @@ radeon_connector_analog_encoder_conflict_solve(struct drm_connector *connector,
 	struct drm_device *dev = connector->dev;
 	struct drm_connector *conflict;
 	struct radeon_connector *radeon_conflict;
+	u32 encoder_id;
 	int i;
 
 	list_for_each_entry(conflict, &dev->mode_config.connector_list, head) {
@@ -443,12 +439,10 @@ radeon_connector_analog_encoder_conflict_solve(struct drm_connector *connector,
 			continue;
 
 		radeon_conflict = to_radeon_connector(conflict);
-		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-			if (conflict->encoder_ids[i] == 0)
-				break;
 
+		drm_for_each_connector_encoder_ids(conflict, encoder_id, i) {
 			/* if the IDs match */
-			if (conflict->encoder_ids[i] == encoder->base.id) {
+			if (encoder_id == encoder->base.id) {
 				if (conflict->status != connector_status_connected)
 					continue;
 
@@ -1256,7 +1250,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
 	struct drm_encoder *encoder = NULL;
 	const struct drm_encoder_helper_funcs *encoder_funcs;
-	int i, r;
+	int r;
 	enum drm_connector_status ret = connector_status_disconnected;
 	bool dret = false, broken_edid = false;
 
@@ -1374,12 +1368,12 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 
 	/* find analog encoder */
 	if (radeon_connector->dac_load_detect) {
-		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-			if (connector->encoder_ids[i] == 0)
-				break;
+		u32 encoder_id;
+		int i;
 
+		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
 			encoder = drm_encoder_find(connector->dev, NULL,
-						   connector->encoder_ids[i]);
+						   encoder_id);
 			if (!encoder)
 				continue;
 
@@ -1458,15 +1452,13 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 /* okay need to be smart in here about which encoder to pick */
 static struct drm_encoder *radeon_dvi_encoder(struct drm_connector *connector)
 {
-	int enc_id = connector->encoder_ids[0];
 	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
 	struct drm_encoder *encoder;
+	u32 encoder_id;
 	int i;
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
 
-		encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -1484,8 +1476,9 @@ static struct drm_encoder *radeon_dvi_encoder(struct drm_connector *connector)
 
 	/* then check use digitial */
 	/* pick the first one */
-	if (enc_id)
-		return drm_encoder_find(connector->dev, NULL, enc_id);
+	encoder_id = connector->encoder_ids[0];
+	if (encoder_id)
+		return drm_encoder_find(connector->dev, NULL, encoder_id);
 	return NULL;
 }
 
@@ -1626,13 +1619,11 @@ u16 radeon_connector_encoder_get_dp_bridge_encoder_id(struct drm_connector *conn
 {
 	struct drm_encoder *encoder;
 	struct radeon_encoder *radeon_encoder;
+	u32 encoder_id;
 	int i;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
@@ -1654,14 +1645,12 @@ static bool radeon_connector_encoder_is_hbr2(struct drm_connector *connector)
 {
 	struct drm_encoder *encoder;
 	struct radeon_encoder *radeon_encoder;
+	u32 encoder_id;
 	int i;
 	bool found = false;
 
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == 0)
-			break;
-
-		encoder = drm_encoder_find(connector->dev, NULL, connector->encoder_ids[i]);
+	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
+		encoder = drm_encoder_find(connector->dev, NULL, encoder_id);
 		if (!encoder)
 			continue;
 
-- 
2.16.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 8/8] drm/tilcdc: Use drm_for_each_connector_encoder_ids()
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
                   ` (4 preceding siblings ...)
  2018-06-26 17:47 ` [PATCH 6/8] drm/nouveau: " Ville Syrjala
@ 2018-06-26 17:47 ` Ville Syrjala
  2018-06-26 18:25 ` ✗ Fi.CI.CHECKPATCH: warning for drm: Second attempt at fixing the fb-helper .best_encoder() mess Patchwork
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Ville Syrjala @ 2018-06-26 17:47 UTC (permalink / raw)
  To: dri-devel; +Cc: intel-gfx, Tomi Valkeinen, Jyri Sarha

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use drm_for_each_connector_encoder_ids() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

Cc: Jyri Sarha <jsarha@ti.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_external.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c
index d651bdd6597e..ed8af21e75b0 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_external.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c
@@ -103,11 +103,12 @@ struct drm_connector *tilcdc_encoder_find_connector(struct drm_device *ddev,
 						    struct drm_encoder *encoder)
 {
 	struct drm_connector *connector;
+	u32 encoder_id;
 	int i;
 
 	list_for_each_entry(connector, &ddev->mode_config.connector_list, head)
-		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
-			if (connector->encoder_ids[i] == encoder->base.id)
+		drm_for_each_connector_encoder_ids(connector, encoder_id, i)
+			if (encoder_id == encoder->base.id)
 				return connector;
 
 	dev_err(ddev->dev, "No connector found for %s encoder (id %d)\n",
-- 
2.16.4

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage
  2018-06-26 17:47 ` [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage Ville Syrjala
@ 2018-06-26 17:58   ` Alex Deucher
  2018-06-27  9:03   ` Daniel Vetter
  1 sibling, 0 replies; 21+ messages in thread
From: Alex Deucher @ 2018-06-26 17:58 UTC (permalink / raw)
  To: Ville Syrjala
  Cc: Daniel Vetter, Intel Graphics Development, Dhinakaran Pandiyan,
	Maling list - DRI developers

On Tue, Jun 26, 2018 at 1:47 PM, Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Instead of using the .best_encoder() hook to figure out whether a given
> connector+crtc combo will work, let's instead do what userspace does and
> just iterate over all the encoders for the connector, and then check
> each crtc against each encoder's possible_crtcs bitmask.
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index cab14f253384..61c39cd75a27 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2323,6 +2323,23 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
>         return true;
>  }
>
> +static bool connector_crtc_ok(struct drm_connector *connector,
> +                             struct drm_crtc *crtc)
> +{
> +       int i;
> +
> +       for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +               struct drm_encoder *encoder =
> +                       drm_encoder_find(connector->dev, NULL,
> +                                        connector->encoder_ids[i]);
> +
> +               if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>                           struct drm_fb_helper_crtc **best_crtcs,
>                           struct drm_display_mode **modes,
> @@ -2331,7 +2348,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>         int c, o;
>         struct drm_connector *connector;
>         const struct drm_connector_helper_funcs *connector_funcs;
> -       struct drm_encoder *encoder;
>         int my_score, best_score, score;
>         struct drm_fb_helper_crtc **crtcs, *crtc;
>         struct drm_fb_helper_connector *fb_helper_conn;
> @@ -2362,20 +2378,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>
>         connector_funcs = connector->helper_private;
>
> -       /*
> -        * If the DRM device implements atomic hooks and ->best_encoder() is
> -        * NULL we fallback to the default drm_atomic_helper_best_encoder()
> -        * helper.
> -        */
> -       if (drm_drv_uses_atomic_modeset(fb_helper->dev) &&
> -           !connector_funcs->best_encoder)
> -               encoder = drm_atomic_helper_best_encoder(connector);
> -       else
> -               encoder = connector_funcs->best_encoder(connector);
> -
> -       if (!encoder)
> -               goto out;
> -
>         /*
>          * select a crtc for this connector and then attempt to configure
>          * remaining connectors
> @@ -2383,7 +2385,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>         for (c = 0; c < fb_helper->crtc_count; c++) {
>                 crtc = &fb_helper->crtc_info[c];
>
> -               if ((encoder->possible_crtcs & (1 << c)) == 0)
> +               if (!connector_crtc_ok(connector, crtc->mode_set.crtc))
>                         continue;
>
>                 for (o = 0; o < n; o++)
> @@ -2410,7 +2412,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>                                sizeof(struct drm_fb_helper_crtc *));
>                 }
>         }
> -out:
> +
>         kfree(crtcs);
>         return best_score;
>  }
> --
> 2.16.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm: Second attempt at fixing the fb-helper .best_encoder() mess
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
                   ` (5 preceding siblings ...)
  2018-06-26 17:47 ` [PATCH 8/8] drm/tilcdc: " Ville Syrjala
@ 2018-06-26 18:25 ` Patchwork
  2018-06-26 18:40 ` ✓ Fi.CI.BAT: success " Patchwork
  2018-06-26 20:47 ` ✓ Fi.CI.IGT: " Patchwork
  8 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2018-06-26 18:25 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

== Series Details ==

Series: drm: Second attempt at fixing the fb-helper .best_encoder() mess
URL   : https://patchwork.freedesktop.org/series/45422/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
eebb696c2139 drm/fb-helper: Eliminate the .best_encoder() usage
b75ca3e84ab2 drm/i915: Nuke intel_mst_best_encoder()
111eeb2a13e6 drm: Add drm_for_each_connector_encoder_ids()
-:131: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'connector' - possible side-effects?
#131: FILE: include/drm/drm_connector.h:1202:
+#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
+	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
+		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
+		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])

-:131: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__i' - possible side-effects?
#131: FILE: include/drm/drm_connector.h:1202:
+#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
+	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
+		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
+		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])

total: 0 errors, 0 warnings, 2 checks, 97 lines checked
80d66565fb4e drm/amdgpu: Use drm_for_each_connector_encoder_ids()
5665f4e07eb7 drm/msm: Use drm_for_each_connector_encoder_ids()
f1b7ac77b2fc drm/nouveau: Use drm_for_each_connector_encoder_ids()
d955c57fd754 drm/radeon: Use drm_for_each_connector_encoder_ids()
03e3457fbf75 drm/tilcdc: Use drm_for_each_connector_encoder_ids()

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* ✓ Fi.CI.BAT: success for drm: Second attempt at fixing the fb-helper .best_encoder() mess
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
                   ` (6 preceding siblings ...)
  2018-06-26 18:25 ` ✗ Fi.CI.CHECKPATCH: warning for drm: Second attempt at fixing the fb-helper .best_encoder() mess Patchwork
@ 2018-06-26 18:40 ` Patchwork
  2018-06-26 20:47 ` ✓ Fi.CI.IGT: " Patchwork
  8 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2018-06-26 18:40 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

== Series Details ==

Series: drm: Second attempt at fixing the fb-helper .best_encoder() mess
URL   : https://patchwork.freedesktop.org/series/45422/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4381 -> Patchwork_9428 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45422/revisions/1/mbox/


== Changes ==

  No changes found


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4381 -> Patchwork_9428

  CI_DRM_4381: ed0d219201c3fd3eb430b712d6ceb51b423daefc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9428: 03e3457fbf755417391ae680b5ad5e2f7fced9b3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

03e3457fbf75 drm/tilcdc: Use drm_for_each_connector_encoder_ids()
d955c57fd754 drm/radeon: Use drm_for_each_connector_encoder_ids()
f1b7ac77b2fc drm/nouveau: Use drm_for_each_connector_encoder_ids()
5665f4e07eb7 drm/msm: Use drm_for_each_connector_encoder_ids()
80d66565fb4e drm/amdgpu: Use drm_for_each_connector_encoder_ids()
111eeb2a13e6 drm: Add drm_for_each_connector_encoder_ids()
b75ca3e84ab2 drm/i915: Nuke intel_mst_best_encoder()
eebb696c2139 drm/fb-helper: Eliminate the .best_encoder() usage

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9428/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* ✓ Fi.CI.IGT: success for drm: Second attempt at fixing the fb-helper .best_encoder() mess
  2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
                   ` (7 preceding siblings ...)
  2018-06-26 18:40 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-06-26 20:47 ` Patchwork
  8 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2018-06-26 20:47 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx

== Series Details ==

Series: drm: Second attempt at fixing the fb-helper .best_encoder() mess
URL   : https://patchwork.freedesktop.org/series/45422/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4381_full -> Patchwork_9428_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9428_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9428_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9428_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          SKIP -> PASS +2

    igt@gem_linear_blits@interruptible:
      shard-apl:          SKIP -> PASS

    igt@gem_mocs_settings@mocs-rc6-bsd2:
      shard-kbl:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9428_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105189)

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@perf@blocking:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@gem_exec_await@wide-contexts:
      shard-glk:          FAIL (fdo#105900) -> PASS

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions-varying-size:
      shard-hsw:          FAIL (fdo#103355) -> PASS

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS +2

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4381 -> Patchwork_9428

  CI_DRM_4381: ed0d219201c3fd3eb430b712d6ceb51b423daefc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9428: 03e3457fbf755417391ae680b5ad5e2f7fced9b3 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9428/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Intel-gfx] [PATCH 6/8] drm/nouveau: Use drm_for_each_connector_encoder_ids()
  2018-06-26 17:47 ` [PATCH 6/8] drm/nouveau: " Ville Syrjala
@ 2018-06-26 20:55   ` kbuild test robot
  0 siblings, 0 replies; 21+ messages in thread
From: kbuild test robot @ 2018-06-26 20:55 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: nouveau, intel-gfx, kbuild-all, dri-devel, Ben Skeggs

[-- Attachment #1: Type: text/plain, Size: 10080 bytes --]

Hi Ville,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.18-rc2 next-20180626]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ville-Syrjala/drm-Second-attempt-at-fixing-the-fb-helper-best_encoder-mess/20180627-024018
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x012-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/gpu//drm/nouveau/nouveau_connector.c: In function 'nouveau_connector_detect':
>> drivers/gpu//drm/nouveau/nouveau_connector.c:606:33: warning: 'nv_encoder' may be used uninitialized in this function [-Wmaybe-uninitialized]
      if (nv_partner && ((nv_encoder->dcb->type == DCB_OUTPUT_ANALOG &&
                          ~~~~~~~~~~^~~~~

vim +/nv_encoder +606 drivers/gpu//drm/nouveau/nouveau_connector.c

6ee73861 Ben Skeggs        2009-12-11  546  
6ee73861 Ben Skeggs        2009-12-11  547  static enum drm_connector_status
930a9e28 Chris Wilson      2010-09-14  548  nouveau_connector_detect(struct drm_connector *connector, bool force)
6ee73861 Ben Skeggs        2009-12-11  549  {
6ee73861 Ben Skeggs        2009-12-11  550  	struct drm_device *dev = connector->dev;
77145f1c Ben Skeggs        2012-07-31  551  	struct nouveau_drm *drm = nouveau_drm(dev);
6ee73861 Ben Skeggs        2009-12-11  552  	struct nouveau_connector *nv_connector = nouveau_connector(connector);
6ee73861 Ben Skeggs        2009-12-11  553  	struct nouveau_encoder *nv_encoder = NULL;
e19b20bb Ben Skeggs        2011-07-12  554  	struct nouveau_encoder *nv_partner;
2aa5eac5 Ben Skeggs        2015-08-20  555  	struct i2c_adapter *i2c;
03cd06ca Francisco Jerez   2010-07-20  556  	int type;
5addcf0a Dave Airlie       2012-09-10  557  	int ret;
5addcf0a Dave Airlie       2012-09-10  558  	enum drm_connector_status conn_status = connector_status_disconnected;
6ee73861 Ben Skeggs        2009-12-11  559  
b8780e2a Francisco Jerez   2010-01-14  560  	/* Cleanup the previous EDID block. */
b8780e2a Francisco Jerez   2010-01-14  561  	if (nv_connector->edid) {
b8780e2a Francisco Jerez   2010-01-14  562  		drm_mode_connector_update_edid_property(connector, NULL);
c8ebe275 Xavier Chantry    2010-01-11  563  		kfree(nv_connector->edid);
c8ebe275 Xavier Chantry    2010-01-11  564  		nv_connector->edid = NULL;
b8780e2a Francisco Jerez   2010-01-14  565  	}
c8ebe275 Xavier Chantry    2010-01-11  566  
d61a5c10 Lukas Wunner      2018-02-11  567  	/* Outputs are only polled while runtime active, so acquiring a
d61a5c10 Lukas Wunner      2018-02-11  568  	 * runtime PM ref here is unnecessary (and would deadlock upon
d61a5c10 Lukas Wunner      2018-02-11  569  	 * runtime suspend because it waits for polling to finish).
d61a5c10 Lukas Wunner      2018-02-11  570  	 */
d61a5c10 Lukas Wunner      2018-02-11  571  	if (!drm_kms_helper_is_poll_worker()) {
5addcf0a Dave Airlie       2012-09-10  572  		ret = pm_runtime_get_sync(connector->dev->dev);
b6c4285a Alexandre Courbot 2014-02-12  573  		if (ret < 0 && ret != -EACCES)
5addcf0a Dave Airlie       2012-09-10  574  			return conn_status;
d61a5c10 Lukas Wunner      2018-02-11  575  	}
5addcf0a Dave Airlie       2012-09-10  576  
8777c5c1 Ben Skeggs        2014-06-06  577  	nv_encoder = nouveau_connector_ddc_detect(connector);
8777c5c1 Ben Skeggs        2014-06-06  578  	if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) {
39c1c901 Lukas Wunner      2016-01-11  579  		if ((vga_switcheroo_handler_flags() &
39c1c901 Lukas Wunner      2016-01-11  580  		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
39c1c901 Lukas Wunner      2016-01-11  581  		    nv_connector->type == DCB_CONNECTOR_LVDS)
39c1c901 Lukas Wunner      2016-01-11  582  			nv_connector->edid = drm_get_edid_switcheroo(connector,
39c1c901 Lukas Wunner      2016-01-11  583  								     i2c);
39c1c901 Lukas Wunner      2016-01-11  584  		else
2aa5eac5 Ben Skeggs        2015-08-20  585  			nv_connector->edid = drm_get_edid(connector, i2c);
39c1c901 Lukas Wunner      2016-01-11  586  
6ee73861 Ben Skeggs        2009-12-11  587  		drm_mode_connector_update_edid_property(connector,
6ee73861 Ben Skeggs        2009-12-11  588  							nv_connector->edid);
6ee73861 Ben Skeggs        2009-12-11  589  		if (!nv_connector->edid) {
77145f1c Ben Skeggs        2012-07-31  590  			NV_ERROR(drm, "DDC responded, but no EDID for %s\n",
8c6c361a Jani Nikula       2014-06-03  591  				 connector->name);
0ed3165e Francisco Jerez   2010-01-14  592  			goto detect_analog;
6ee73861 Ben Skeggs        2009-12-11  593  		}
6ee73861 Ben Skeggs        2009-12-11  594  
6ee73861 Ben Skeggs        2009-12-11  595  		/* Override encoder type for DVI-I based on whether EDID
6ee73861 Ben Skeggs        2009-12-11  596  		 * says the display is digital or analog, both use the
6ee73861 Ben Skeggs        2009-12-11  597  		 * same i2c channel so the value returned from ddc_detect
6ee73861 Ben Skeggs        2009-12-11  598  		 * isn't necessarily correct.
6ee73861 Ben Skeggs        2009-12-11  599  		 */
e19b20bb Ben Skeggs        2011-07-12  600  		nv_partner = NULL;
cb75d97e Ben Skeggs        2012-07-11  601  		if (nv_encoder->dcb->type == DCB_OUTPUT_TMDS)
cb75d97e Ben Skeggs        2012-07-11  602  			nv_partner = find_encoder(connector, DCB_OUTPUT_ANALOG);
cb75d97e Ben Skeggs        2012-07-11  603  		if (nv_encoder->dcb->type == DCB_OUTPUT_ANALOG)
cb75d97e Ben Skeggs        2012-07-11  604  			nv_partner = find_encoder(connector, DCB_OUTPUT_TMDS);
cb75d97e Ben Skeggs        2012-07-11  605  
cb75d97e Ben Skeggs        2012-07-11 @606  		if (nv_partner && ((nv_encoder->dcb->type == DCB_OUTPUT_ANALOG &&
cb75d97e Ben Skeggs        2012-07-11  607  				    nv_partner->dcb->type == DCB_OUTPUT_TMDS) ||
cb75d97e Ben Skeggs        2012-07-11  608  				   (nv_encoder->dcb->type == DCB_OUTPUT_TMDS &&
cb75d97e Ben Skeggs        2012-07-11  609  				    nv_partner->dcb->type == DCB_OUTPUT_ANALOG))) {
6ee73861 Ben Skeggs        2009-12-11  610  			if (nv_connector->edid->input & DRM_EDID_INPUT_DIGITAL)
cb75d97e Ben Skeggs        2012-07-11  611  				type = DCB_OUTPUT_TMDS;
6ee73861 Ben Skeggs        2009-12-11  612  			else
cb75d97e Ben Skeggs        2012-07-11  613  				type = DCB_OUTPUT_ANALOG;
6ee73861 Ben Skeggs        2009-12-11  614  
e19b20bb Ben Skeggs        2011-07-12  615  			nv_encoder = find_encoder(connector, type);
6ee73861 Ben Skeggs        2009-12-11  616  		}
6ee73861 Ben Skeggs        2009-12-11  617  
6ee73861 Ben Skeggs        2009-12-11  618  		nouveau_connector_set_encoder(connector, nv_encoder);
5addcf0a Dave Airlie       2012-09-10  619  		conn_status = connector_status_connected;
5addcf0a Dave Airlie       2012-09-10  620  		goto out;
6ee73861 Ben Skeggs        2009-12-11  621  	}
6ee73861 Ben Skeggs        2009-12-11  622  
c16c5707 Francisco Jerez   2010-09-09  623  	nv_encoder = nouveau_connector_of_detect(connector);
c16c5707 Francisco Jerez   2010-09-09  624  	if (nv_encoder) {
c16c5707 Francisco Jerez   2010-09-09  625  		nouveau_connector_set_encoder(connector, nv_encoder);
5addcf0a Dave Airlie       2012-09-10  626  		conn_status = connector_status_connected;
5addcf0a Dave Airlie       2012-09-10  627  		goto out;
c16c5707 Francisco Jerez   2010-09-09  628  	}
c16c5707 Francisco Jerez   2010-09-09  629  
0ed3165e Francisco Jerez   2010-01-14  630  detect_analog:
cb75d97e Ben Skeggs        2012-07-11  631  	nv_encoder = find_encoder(connector, DCB_OUTPUT_ANALOG);
f4053509 Ben Skeggs        2010-03-15  632  	if (!nv_encoder && !nouveau_tv_disable)
cb75d97e Ben Skeggs        2012-07-11  633  		nv_encoder = find_encoder(connector, DCB_OUTPUT_TV);
84b8081c Francisco Jerez   2010-10-26  634  	if (nv_encoder && force) {
6ee73861 Ben Skeggs        2009-12-11  635  		struct drm_encoder *encoder = to_drm_encoder(nv_encoder);
d58ded76 Jani Nikula       2015-03-11  636  		const struct drm_encoder_helper_funcs *helper =
6ee73861 Ben Skeggs        2009-12-11  637  						encoder->helper_private;
6ee73861 Ben Skeggs        2009-12-11  638  
6ee73861 Ben Skeggs        2009-12-11  639  		if (helper->detect(encoder, connector) ==
6ee73861 Ben Skeggs        2009-12-11  640  						connector_status_connected) {
6ee73861 Ben Skeggs        2009-12-11  641  			nouveau_connector_set_encoder(connector, nv_encoder);
5addcf0a Dave Airlie       2012-09-10  642  			conn_status = connector_status_connected;
5addcf0a Dave Airlie       2012-09-10  643  			goto out;
6ee73861 Ben Skeggs        2009-12-11  644  		}
6ee73861 Ben Skeggs        2009-12-11  645  
6ee73861 Ben Skeggs        2009-12-11  646  	}
6ee73861 Ben Skeggs        2009-12-11  647  
5addcf0a Dave Airlie       2012-09-10  648   out:
5addcf0a Dave Airlie       2012-09-10  649  
d61a5c10 Lukas Wunner      2018-02-11  650  	if (!drm_kms_helper_is_poll_worker()) {
5addcf0a Dave Airlie       2012-09-10  651  		pm_runtime_mark_last_busy(connector->dev->dev);
5addcf0a Dave Airlie       2012-09-10  652  		pm_runtime_put_autosuspend(connector->dev->dev);
d61a5c10 Lukas Wunner      2018-02-11  653  	}
5addcf0a Dave Airlie       2012-09-10  654  
5addcf0a Dave Airlie       2012-09-10  655  	return conn_status;
6ee73861 Ben Skeggs        2009-12-11  656  }
6ee73861 Ben Skeggs        2009-12-11  657  

:::::: The code at line 606 was first introduced by commit
:::::: cb75d97e9c77743ecfcc43375be135a55a4d9b25 drm/nouveau: implement devinit subdev, and new init table parser

:::::: TO: Ben Skeggs <bskeggs@redhat.com>
:::::: CC: Ben Skeggs <bskeggs@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27704 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage
  2018-06-26 17:47 ` [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage Ville Syrjala
  2018-06-26 17:58   ` Alex Deucher
@ 2018-06-27  9:03   ` Daniel Vetter
  2018-06-27 12:03     ` Ville Syrjälä
  1 sibling, 1 reply; 21+ messages in thread
From: Daniel Vetter @ 2018-06-27  9:03 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: Daniel Vetter, intel-gfx, Dhinakaran Pandiyan, dri-devel

On Tue, Jun 26, 2018 at 08:47:07PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Instead of using the .best_encoder() hook to figure out whether a given
> connector+crtc combo will work, let's instead do what userspace does and
> just iterate over all the encoders for the connector, and then check
> each crtc against each encoder's possible_crtcs bitmask.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index cab14f253384..61c39cd75a27 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2323,6 +2323,23 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
>  	return true;
>  }
>  
> +static bool connector_crtc_ok(struct drm_connector *connector,
> +			      struct drm_crtc *crtc)
> +{
> +	int i;
> +
> +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +		struct drm_encoder *encoder =
> +			drm_encoder_find(connector->dev, NULL,
> +					 connector->encoder_ids[i]);
> +

Shouldn't we also check for encoder != NULL here? Just for the case where
a connector does not work on the given crtc, where we do expect to
actually run off the end of the valid entries in the array.

With that fixed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  			  struct drm_fb_helper_crtc **best_crtcs,
>  			  struct drm_display_mode **modes,
> @@ -2331,7 +2348,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	int c, o;
>  	struct drm_connector *connector;
>  	const struct drm_connector_helper_funcs *connector_funcs;
> -	struct drm_encoder *encoder;
>  	int my_score, best_score, score;
>  	struct drm_fb_helper_crtc **crtcs, *crtc;
>  	struct drm_fb_helper_connector *fb_helper_conn;
> @@ -2362,20 +2378,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  
>  	connector_funcs = connector->helper_private;
>  
> -	/*
> -	 * If the DRM device implements atomic hooks and ->best_encoder() is
> -	 * NULL we fallback to the default drm_atomic_helper_best_encoder()
> -	 * helper.
> -	 */
> -	if (drm_drv_uses_atomic_modeset(fb_helper->dev) &&
> -	    !connector_funcs->best_encoder)
> -		encoder = drm_atomic_helper_best_encoder(connector);
> -	else
> -		encoder = connector_funcs->best_encoder(connector);
> -
> -	if (!encoder)
> -		goto out;
> -
>  	/*
>  	 * select a crtc for this connector and then attempt to configure
>  	 * remaining connectors
> @@ -2383,7 +2385,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	for (c = 0; c < fb_helper->crtc_count; c++) {
>  		crtc = &fb_helper->crtc_info[c];
>  
> -		if ((encoder->possible_crtcs & (1 << c)) == 0)
> +		if (!connector_crtc_ok(connector, crtc->mode_set.crtc))
>  			continue;
>  
>  		for (o = 0; o < n; o++)
> @@ -2410,7 +2412,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  			       sizeof(struct drm_fb_helper_crtc *));
>  		}
>  	}
> -out:
> +
>  	kfree(crtcs);
>  	return best_score;
>  }
> -- 
> 2.16.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/8] drm/i915: Nuke intel_mst_best_encoder()
  2018-06-26 17:47 ` [PATCH 2/8] drm/i915: Nuke intel_mst_best_encoder() Ville Syrjala
@ 2018-06-27  9:03   ` Daniel Vetter
  0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2018-06-27  9:03 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx, Dhinakaran Pandiyan, dri-devel

On Tue, Jun 26, 2018 at 08:47:08PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> With the fb-helper no longer relying on the non-atomic .best_encoder()
> we can eliminate the hook from the MST encoder.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yay!

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 5890500a3a8b..0f012fbe34eb 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -403,20 +403,10 @@ static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *c
>  	return &intel_dp->mst_encoders[crtc->pipe]->base.base;
>  }
>  
> -static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connector)
> -{
> -	struct intel_connector *intel_connector = to_intel_connector(connector);
> -	struct intel_dp *intel_dp = intel_connector->mst_port;
> -	if (!intel_dp)
> -		return NULL;
> -	return &intel_dp->mst_encoders[0]->base.base;
> -}
> -
>  static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = {
>  	.get_modes = intel_dp_mst_get_modes,
>  	.mode_valid = intel_dp_mst_mode_valid,
>  	.atomic_best_encoder = intel_mst_atomic_best_encoder,
> -	.best_encoder = intel_mst_best_encoder,
>  	.atomic_check = intel_dp_mst_atomic_check,
>  };
>  
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids()
  2018-06-26 17:47 ` [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids() Ville Syrjala
@ 2018-06-27  9:08   ` Daniel Vetter
  2018-06-27  9:11     ` Daniel Vetter
  0 siblings, 1 reply; 21+ messages in thread
From: Daniel Vetter @ 2018-06-27  9:08 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx, dri-devel

On Tue, Jun 26, 2018 at 08:47:09PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a convenience macro for iterating connector->encoder_ids[].
> Isolates the users from the implementation details.
> 
> Also use ARRAY_SIZE() when populating the array to avoid spreading
> knowledge about the array size all over.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c    | 22 ++++++++++------------
>  drivers/gpu/drm/drm_fb_helper.c    |  6 +++---
>  drivers/gpu/drm/drm_probe_helper.c |  9 +++++----
>  include/drm/drm_connector.h        | 11 +++++++++++
>  4 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 2f9ebddd178e..c43646cb8145 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -321,7 +321,7 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>  	if (WARN_ON(connector->encoder))
>  		return -EINVAL;
>  
> -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +	for (i = 0; i < ARRAY_SIZE(connector->encoder_ids); i++) {
>  		if (connector->encoder_ids[i] == 0) {
>  			connector->encoder_ids[i] = encoder->base.id;
>  			return 0;
> @@ -1693,6 +1693,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	int encoders_count = 0;
>  	int ret = 0;
>  	int copied = 0;
> +	u32 encoder_id;
>  	int i;
>  	struct drm_mode_modeinfo u_mode;
>  	struct drm_mode_modeinfo __user *mode_ptr;
> @@ -1708,22 +1709,19 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	if (!connector)
>  		return -ENOENT;
>  
> -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> -		if (connector->encoder_ids[i] != 0)
> -			encoders_count++;
> +	drm_for_each_connector_encoder_ids(connector, encoder_id, i)
> +		encoders_count++;
>  
>  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
>  		copied = 0;
>  		encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> -		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> -			if (connector->encoder_ids[i] != 0) {
> -				if (put_user(connector->encoder_ids[i],
> -					     encoder_ptr + copied)) {
> -					ret = -EFAULT;
> -					goto out;
> -				}
> -				copied++;
> +
> +		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> +			if (put_user(encoder_id, encoder_ptr + copied)) {
> +				ret = -EFAULT;
> +				goto out;
>  			}
> +			copied++;
>  		}
>  	}
>  	out_resp->count_encoders = encoders_count;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 61c39cd75a27..e086b08748f4 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2326,12 +2326,12 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
>  static bool connector_crtc_ok(struct drm_connector *connector,
>  			      struct drm_crtc *crtc)
>  {
> +	u32 encoder_id;
>  	int i;
>  
> -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
>  		struct drm_encoder *encoder =
> -			drm_encoder_find(connector->dev, NULL,
> -					 connector->encoder_ids[i]);
> +			drm_encoder_find(connector->dev, NULL, encoder_id);
>  
>  		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
>  			return true;
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 527743394150..0239f76c52fb 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -88,9 +88,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
>  			    struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
> -	uint32_t *ids = connector->encoder_ids;
>  	enum drm_mode_status ret = MODE_OK;
> -	unsigned int i;
> +	u32 encoder_id;
> +	int i;
>  
>  	/* Step 1: Validate against connector */
>  	ret = drm_connector_mode_valid(connector, mode);
> @@ -98,8 +98,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
>  		return ret;
>  
>  	/* Step 2: Validate against encoders and crtcs */
> -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> -		struct drm_encoder *encoder = drm_encoder_find(dev, NULL, ids[i]);
> +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> +		struct drm_encoder *encoder =
> +			drm_encoder_find(dev, NULL, encoder_id);
>  		struct drm_crtc *crtc;
>  
>  		if (!encoder)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 14ab58ade87f..4a0363f7dd8c 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1193,4 +1193,15 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
>  #define drm_for_each_connector_iter(connector, iter) \
>  	while ((connector = drm_connector_list_iter_next(iter)))
>  
> +/**
> + * drm_for_each_connector_encoder_ids - iterate connector encoder_ids[]

&drm_connector.encoder_ids would give you a (fairly direct) link. Just a
nitpick.

Wrt the api: I think including the drm_encoder_find would be useful here -
it's what 2/3 users want anyway, and the ioctl can easily look up the id
again by following encoder->base.id. And then maybe call the function a
bit differently, I think usually we go with
$parent_object_for_each_$iterated_thing, so
drm_connector_for_each_encoder(). There's also the
for_each_$thing_in_$parent_object pattern (used for atomic states), but I
think that's less fitting here.

Apologies for the interface bikeshedding ...
-Daniel

> + * @connector: &struct drm_connector pointer used as cursor
> + * @encoder_id: the encoder ID
> + * @__i: int iteration cursor, for macro-internal use
> + */
> +#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
> +	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
> +		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
> +		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])
> +
>  #endif
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids()
  2018-06-27  9:08   ` Daniel Vetter
@ 2018-06-27  9:11     ` Daniel Vetter
  2018-06-27 15:07       ` Ville Syrjälä
  0 siblings, 1 reply; 21+ messages in thread
From: Daniel Vetter @ 2018-06-27  9:11 UTC (permalink / raw)
  To: Ville Syrjala; +Cc: intel-gfx, dri-devel

On Wed, Jun 27, 2018 at 11:08:48AM +0200, Daniel Vetter wrote:
> On Tue, Jun 26, 2018 at 08:47:09PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a convenience macro for iterating connector->encoder_ids[].
> > Isolates the users from the implementation details.
> > 
> > Also use ARRAY_SIZE() when populating the array to avoid spreading
> > knowledge about the array size all over.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_connector.c    | 22 ++++++++++------------
> >  drivers/gpu/drm/drm_fb_helper.c    |  6 +++---
> >  drivers/gpu/drm/drm_probe_helper.c |  9 +++++----
> >  include/drm/drm_connector.h        | 11 +++++++++++
> >  4 files changed, 29 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 2f9ebddd178e..c43646cb8145 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -321,7 +321,7 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
> >  	if (WARN_ON(connector->encoder))
> >  		return -EINVAL;
> >  
> > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > +	for (i = 0; i < ARRAY_SIZE(connector->encoder_ids); i++) {
> >  		if (connector->encoder_ids[i] == 0) {
> >  			connector->encoder_ids[i] = encoder->base.id;
> >  			return 0;
> > @@ -1693,6 +1693,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >  	int encoders_count = 0;
> >  	int ret = 0;
> >  	int copied = 0;
> > +	u32 encoder_id;
> >  	int i;
> >  	struct drm_mode_modeinfo u_mode;
> >  	struct drm_mode_modeinfo __user *mode_ptr;
> > @@ -1708,22 +1709,19 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> >  	if (!connector)
> >  		return -ENOENT;
> >  
> > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> > -		if (connector->encoder_ids[i] != 0)
> > -			encoders_count++;
> > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i)
> > +		encoders_count++;
> >  
> >  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
> >  		copied = 0;
> >  		encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> > -		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > -			if (connector->encoder_ids[i] != 0) {
> > -				if (put_user(connector->encoder_ids[i],
> > -					     encoder_ptr + copied)) {
> > -					ret = -EFAULT;
> > -					goto out;
> > -				}
> > -				copied++;
> > +
> > +		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > +			if (put_user(encoder_id, encoder_ptr + copied)) {
> > +				ret = -EFAULT;
> > +				goto out;
> >  			}
> > +			copied++;
> >  		}
> >  	}
> >  	out_resp->count_encoders = encoders_count;
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 61c39cd75a27..e086b08748f4 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2326,12 +2326,12 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> >  static bool connector_crtc_ok(struct drm_connector *connector,
> >  			      struct drm_crtc *crtc)
> >  {
> > +	u32 encoder_id;
> >  	int i;
> >  
> > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> >  		struct drm_encoder *encoder =
> > -			drm_encoder_find(connector->dev, NULL,
> > -					 connector->encoder_ids[i]);
> > +			drm_encoder_find(connector->dev, NULL, encoder_id);
> >  
> >  		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> >  			return true;
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 527743394150..0239f76c52fb 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -88,9 +88,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> >  			    struct drm_connector *connector)
> >  {
> >  	struct drm_device *dev = connector->dev;
> > -	uint32_t *ids = connector->encoder_ids;
> >  	enum drm_mode_status ret = MODE_OK;
> > -	unsigned int i;
> > +	u32 encoder_id;
> > +	int i;
> >  
> >  	/* Step 1: Validate against connector */
> >  	ret = drm_connector_mode_valid(connector, mode);
> > @@ -98,8 +98,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> >  		return ret;
> >  
> >  	/* Step 2: Validate against encoders and crtcs */
> > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > -		struct drm_encoder *encoder = drm_encoder_find(dev, NULL, ids[i]);
> > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > +		struct drm_encoder *encoder =
> > +			drm_encoder_find(dev, NULL, encoder_id);
> >  		struct drm_crtc *crtc;
> >  
> >  		if (!encoder)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 14ab58ade87f..4a0363f7dd8c 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -1193,4 +1193,15 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
> >  #define drm_for_each_connector_iter(connector, iter) \
> >  	while ((connector = drm_connector_list_iter_next(iter)))
> >  
> > +/**
> > + * drm_for_each_connector_encoder_ids - iterate connector encoder_ids[]
> 
> &drm_connector.encoder_ids would give you a (fairly direct) link. Just a
> nitpick.
> 
> Wrt the api: I think including the drm_encoder_find would be useful here -
> it's what 2/3 users want anyway, and the ioctl can easily look up the id
> again by following encoder->base.id. And then maybe call the function a
> bit differently, I think usually we go with
> $parent_object_for_each_$iterated_thing, so
> drm_connector_for_each_encoder(). There's also the
> for_each_$thing_in_$parent_object pattern (used for atomic states), but I
> think that's less fitting here.
> 
> Apologies for the interface bikeshedding ...

Looking at all the patches it's 15 places that want the drm_encoder_find
integrated vs. 4 that don't want it (without further refactoring). So even
more reasons for drm_connector_for_each_encoder imo.
-Daniel

> -Daniel
> 
> > + * @connector: &struct drm_connector pointer used as cursor
> > + * @encoder_id: the encoder ID
> > + * @__i: int iteration cursor, for macro-internal use
> > + */
> > +#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
> > +	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
> > +		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
> > +		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])
> > +
> >  #endif
> > -- 
> > 2.16.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage
  2018-06-27  9:03   ` Daniel Vetter
@ 2018-06-27 12:03     ` Ville Syrjälä
  0 siblings, 0 replies; 21+ messages in thread
From: Ville Syrjälä @ 2018-06-27 12:03 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, Dhinakaran Pandiyan, dri-devel

On Wed, Jun 27, 2018 at 11:03:31AM +0200, Daniel Vetter wrote:
> On Tue, Jun 26, 2018 at 08:47:07PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Instead of using the .best_encoder() hook to figure out whether a given
> > connector+crtc combo will work, let's instead do what userspace does and
> > just iterate over all the encoders for the connector, and then check
> > each crtc against each encoder's possible_crtcs bitmask.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 36 +++++++++++++++++++-----------------
> >  1 file changed, 19 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index cab14f253384..61c39cd75a27 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2323,6 +2323,23 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> >  	return true;
> >  }
> >  
> > +static bool connector_crtc_ok(struct drm_connector *connector,
> > +			      struct drm_crtc *crtc)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > +		struct drm_encoder *encoder =
> > +			drm_encoder_find(connector->dev, NULL,
> > +					 connector->encoder_ids[i]);
> > +
> 
> Shouldn't we also check for encoder != NULL here? Just for the case where
> a connector does not work on the given crtc, where we do expect to
> actually run off the end of the valid entries in the array.

Yeah. This one is obviously crap. Originally I had it after the
for_each_encoder_ids() thing so didn't need any checks, but then
thought I should move it before just in case people don't like
that particular macro. And apparently forgot to think when doing
that.

> 
> With that fixed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > +		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> >  			  struct drm_fb_helper_crtc **best_crtcs,
> >  			  struct drm_display_mode **modes,
> > @@ -2331,7 +2348,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> >  	int c, o;
> >  	struct drm_connector *connector;
> >  	const struct drm_connector_helper_funcs *connector_funcs;
> > -	struct drm_encoder *encoder;
> >  	int my_score, best_score, score;
> >  	struct drm_fb_helper_crtc **crtcs, *crtc;
> >  	struct drm_fb_helper_connector *fb_helper_conn;
> > @@ -2362,20 +2378,6 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> >  
> >  	connector_funcs = connector->helper_private;
> >  
> > -	/*
> > -	 * If the DRM device implements atomic hooks and ->best_encoder() is
> > -	 * NULL we fallback to the default drm_atomic_helper_best_encoder()
> > -	 * helper.
> > -	 */
> > -	if (drm_drv_uses_atomic_modeset(fb_helper->dev) &&
> > -	    !connector_funcs->best_encoder)
> > -		encoder = drm_atomic_helper_best_encoder(connector);
> > -	else
> > -		encoder = connector_funcs->best_encoder(connector);
> > -
> > -	if (!encoder)
> > -		goto out;
> > -
> >  	/*
> >  	 * select a crtc for this connector and then attempt to configure
> >  	 * remaining connectors
> > @@ -2383,7 +2385,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> >  	for (c = 0; c < fb_helper->crtc_count; c++) {
> >  		crtc = &fb_helper->crtc_info[c];
> >  
> > -		if ((encoder->possible_crtcs & (1 << c)) == 0)
> > +		if (!connector_crtc_ok(connector, crtc->mode_set.crtc))
> >  			continue;
> >  
> >  		for (o = 0; o < n; o++)
> > @@ -2410,7 +2412,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
> >  			       sizeof(struct drm_fb_helper_crtc *));
> >  		}
> >  	}
> > -out:
> > +
> >  	kfree(crtcs);
> >  	return best_score;
> >  }
> > -- 
> > 2.16.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids()
  2018-06-27  9:11     ` Daniel Vetter
@ 2018-06-27 15:07       ` Ville Syrjälä
  2018-06-28  6:04         ` Daniel Vetter
  0 siblings, 1 reply; 21+ messages in thread
From: Ville Syrjälä @ 2018-06-27 15:07 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, dri-devel

On Wed, Jun 27, 2018 at 11:11:57AM +0200, Daniel Vetter wrote:
> On Wed, Jun 27, 2018 at 11:08:48AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 26, 2018 at 08:47:09PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add a convenience macro for iterating connector->encoder_ids[].
> > > Isolates the users from the implementation details.
> > > 
> > > Also use ARRAY_SIZE() when populating the array to avoid spreading
> > > knowledge about the array size all over.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_connector.c    | 22 ++++++++++------------
> > >  drivers/gpu/drm/drm_fb_helper.c    |  6 +++---
> > >  drivers/gpu/drm/drm_probe_helper.c |  9 +++++----
> > >  include/drm/drm_connector.h        | 11 +++++++++++
> > >  4 files changed, 29 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 2f9ebddd178e..c43646cb8145 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -321,7 +321,7 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
> > >  	if (WARN_ON(connector->encoder))
> > >  		return -EINVAL;
> > >  
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > +	for (i = 0; i < ARRAY_SIZE(connector->encoder_ids); i++) {
> > >  		if (connector->encoder_ids[i] == 0) {
> > >  			connector->encoder_ids[i] = encoder->base.id;
> > >  			return 0;
> > > @@ -1693,6 +1693,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  	int encoders_count = 0;
> > >  	int ret = 0;
> > >  	int copied = 0;
> > > +	u32 encoder_id;
> > >  	int i;
> > >  	struct drm_mode_modeinfo u_mode;
> > >  	struct drm_mode_modeinfo __user *mode_ptr;
> > > @@ -1708,22 +1709,19 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > >  	if (!connector)
> > >  		return -ENOENT;
> > >  
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> > > -		if (connector->encoder_ids[i] != 0)
> > > -			encoders_count++;
> > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i)
> > > +		encoders_count++;
> > >  
> > >  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
> > >  		copied = 0;
> > >  		encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> > > -		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > -			if (connector->encoder_ids[i] != 0) {
> > > -				if (put_user(connector->encoder_ids[i],
> > > -					     encoder_ptr + copied)) {
> > > -					ret = -EFAULT;
> > > -					goto out;
> > > -				}
> > > -				copied++;
> > > +
> > > +		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > +			if (put_user(encoder_id, encoder_ptr + copied)) {
> > > +				ret = -EFAULT;
> > > +				goto out;
> > >  			}
> > > +			copied++;
> > >  		}
> > >  	}
> > >  	out_resp->count_encoders = encoders_count;
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 61c39cd75a27..e086b08748f4 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -2326,12 +2326,12 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> > >  static bool connector_crtc_ok(struct drm_connector *connector,
> > >  			      struct drm_crtc *crtc)
> > >  {
> > > +	u32 encoder_id;
> > >  	int i;
> > >  
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > >  		struct drm_encoder *encoder =
> > > -			drm_encoder_find(connector->dev, NULL,
> > > -					 connector->encoder_ids[i]);
> > > +			drm_encoder_find(connector->dev, NULL, encoder_id);
> > >  
> > >  		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> > >  			return true;
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 527743394150..0239f76c52fb 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -88,9 +88,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > >  			    struct drm_connector *connector)
> > >  {
> > >  	struct drm_device *dev = connector->dev;
> > > -	uint32_t *ids = connector->encoder_ids;
> > >  	enum drm_mode_status ret = MODE_OK;
> > > -	unsigned int i;
> > > +	u32 encoder_id;
> > > +	int i;
> > >  
> > >  	/* Step 1: Validate against connector */
> > >  	ret = drm_connector_mode_valid(connector, mode);
> > > @@ -98,8 +98,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > >  		return ret;
> > >  
> > >  	/* Step 2: Validate against encoders and crtcs */
> > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > -		struct drm_encoder *encoder = drm_encoder_find(dev, NULL, ids[i]);
> > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > +		struct drm_encoder *encoder =
> > > +			drm_encoder_find(dev, NULL, encoder_id);
> > >  		struct drm_crtc *crtc;
> > >  
> > >  		if (!encoder)
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 14ab58ade87f..4a0363f7dd8c 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -1193,4 +1193,15 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
> > >  #define drm_for_each_connector_iter(connector, iter) \
> > >  	while ((connector = drm_connector_list_iter_next(iter)))
> > >  
> > > +/**
> > > + * drm_for_each_connector_encoder_ids - iterate connector encoder_ids[]
> > 
> > &drm_connector.encoder_ids would give you a (fairly direct) link. Just a
> > nitpick.
> > 
> > Wrt the api: I think including the drm_encoder_find would be useful here -
> > it's what 2/3 users want anyway, and the ioctl can easily look up the id
> > again by following encoder->base.id. And then maybe call the function a
> > bit differently, I think usually we go with
> > $parent_object_for_each_$iterated_thing, so
> > drm_connector_for_each_encoder(). There's also the
> > for_each_$thing_in_$parent_object pattern (used for atomic states), but I
> > think that's less fitting here.
> > 
> > Apologies for the interface bikeshedding ...
> 
> Looking at all the patches it's 15 places that want the drm_encoder_find
> integrated vs. 4 that don't want it (without further refactoring). So even
> more reasons for drm_connector_for_each_encoder imo.

I had that same thought initially, but then forgot about it for
some reason. It does indeed lead to an even neater result.

Although it should be somewhat obvious that there can only be one
currently used encoder for a connector, I think we may want to name
this eg. drm_connector_for_each_possible_encoder() to make it super
clear what we're talking about.

> -Daniel
> 
> > -Daniel
> > 
> > > + * @connector: &struct drm_connector pointer used as cursor
> > > + * @encoder_id: the encoder ID
> > > + * @__i: int iteration cursor, for macro-internal use
> > > + */
> > > +#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
> > > +	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
> > > +		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
> > > +		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])
> > > +
> > >  #endif
> > > -- 
> > > 2.16.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids()
  2018-06-27 15:07       ` Ville Syrjälä
@ 2018-06-28  6:04         ` Daniel Vetter
  0 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2018-06-28  6:04 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, dri-devel

On Wed, Jun 27, 2018 at 06:07:39PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 27, 2018 at 11:11:57AM +0200, Daniel Vetter wrote:
> > On Wed, Jun 27, 2018 at 11:08:48AM +0200, Daniel Vetter wrote:
> > > On Tue, Jun 26, 2018 at 08:47:09PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Add a convenience macro for iterating connector->encoder_ids[].
> > > > Isolates the users from the implementation details.
> > > > 
> > > > Also use ARRAY_SIZE() when populating the array to avoid spreading
> > > > knowledge about the array size all over.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_connector.c    | 22 ++++++++++------------
> > > >  drivers/gpu/drm/drm_fb_helper.c    |  6 +++---
> > > >  drivers/gpu/drm/drm_probe_helper.c |  9 +++++----
> > > >  include/drm/drm_connector.h        | 11 +++++++++++
> > > >  4 files changed, 29 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > index 2f9ebddd178e..c43646cb8145 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -321,7 +321,7 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
> > > >  	if (WARN_ON(connector->encoder))
> > > >  		return -EINVAL;
> > > >  
> > > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > > +	for (i = 0; i < ARRAY_SIZE(connector->encoder_ids); i++) {
> > > >  		if (connector->encoder_ids[i] == 0) {
> > > >  			connector->encoder_ids[i] = encoder->base.id;
> > > >  			return 0;
> > > > @@ -1693,6 +1693,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > > >  	int encoders_count = 0;
> > > >  	int ret = 0;
> > > >  	int copied = 0;
> > > > +	u32 encoder_id;
> > > >  	int i;
> > > >  	struct drm_mode_modeinfo u_mode;
> > > >  	struct drm_mode_modeinfo __user *mode_ptr;
> > > > @@ -1708,22 +1709,19 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> > > >  	if (!connector)
> > > >  		return -ENOENT;
> > > >  
> > > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> > > > -		if (connector->encoder_ids[i] != 0)
> > > > -			encoders_count++;
> > > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i)
> > > > +		encoders_count++;
> > > >  
> > > >  	if ((out_resp->count_encoders >= encoders_count) && encoders_count) {
> > > >  		copied = 0;
> > > >  		encoder_ptr = (uint32_t __user *)(unsigned long)(out_resp->encoders_ptr);
> > > > -		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > > -			if (connector->encoder_ids[i] != 0) {
> > > > -				if (put_user(connector->encoder_ids[i],
> > > > -					     encoder_ptr + copied)) {
> > > > -					ret = -EFAULT;
> > > > -					goto out;
> > > > -				}
> > > > -				copied++;
> > > > +
> > > > +		drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > > +			if (put_user(encoder_id, encoder_ptr + copied)) {
> > > > +				ret = -EFAULT;
> > > > +				goto out;
> > > >  			}
> > > > +			copied++;
> > > >  		}
> > > >  	}
> > > >  	out_resp->count_encoders = encoders_count;
> > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > > index 61c39cd75a27..e086b08748f4 100644
> > > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > > @@ -2326,12 +2326,12 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper,
> > > >  static bool connector_crtc_ok(struct drm_connector *connector,
> > > >  			      struct drm_crtc *crtc)
> > > >  {
> > > > +	u32 encoder_id;
> > > >  	int i;
> > > >  
> > > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > >  		struct drm_encoder *encoder =
> > > > -			drm_encoder_find(connector->dev, NULL,
> > > > -					 connector->encoder_ids[i]);
> > > > +			drm_encoder_find(connector->dev, NULL, encoder_id);
> > > >  
> > > >  		if (encoder->possible_crtcs & drm_crtc_mask(crtc))
> > > >  			return true;
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > index 527743394150..0239f76c52fb 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -88,9 +88,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > > >  			    struct drm_connector *connector)
> > > >  {
> > > >  	struct drm_device *dev = connector->dev;
> > > > -	uint32_t *ids = connector->encoder_ids;
> > > >  	enum drm_mode_status ret = MODE_OK;
> > > > -	unsigned int i;
> > > > +	u32 encoder_id;
> > > > +	int i;
> > > >  
> > > >  	/* Step 1: Validate against connector */
> > > >  	ret = drm_connector_mode_valid(connector, mode);
> > > > @@ -98,8 +98,9 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,
> > > >  		return ret;
> > > >  
> > > >  	/* Step 2: Validate against encoders and crtcs */
> > > > -	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> > > > -		struct drm_encoder *encoder = drm_encoder_find(dev, NULL, ids[i]);
> > > > +	drm_for_each_connector_encoder_ids(connector, encoder_id, i) {
> > > > +		struct drm_encoder *encoder =
> > > > +			drm_encoder_find(dev, NULL, encoder_id);
> > > >  		struct drm_crtc *crtc;
> > > >  
> > > >  		if (!encoder)
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 14ab58ade87f..4a0363f7dd8c 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -1193,4 +1193,15 @@ void drm_connector_list_iter_end(struct drm_connector_list_iter *iter);
> > > >  #define drm_for_each_connector_iter(connector, iter) \
> > > >  	while ((connector = drm_connector_list_iter_next(iter)))
> > > >  
> > > > +/**
> > > > + * drm_for_each_connector_encoder_ids - iterate connector encoder_ids[]
> > > 
> > > &drm_connector.encoder_ids would give you a (fairly direct) link. Just a
> > > nitpick.
> > > 
> > > Wrt the api: I think including the drm_encoder_find would be useful here -
> > > it's what 2/3 users want anyway, and the ioctl can easily look up the id
> > > again by following encoder->base.id. And then maybe call the function a
> > > bit differently, I think usually we go with
> > > $parent_object_for_each_$iterated_thing, so
> > > drm_connector_for_each_encoder(). There's also the
> > > for_each_$thing_in_$parent_object pattern (used for atomic states), but I
> > > think that's less fitting here.
> > > 
> > > Apologies for the interface bikeshedding ...
> > 
> > Looking at all the patches it's 15 places that want the drm_encoder_find
> > integrated vs. 4 that don't want it (without further refactoring). So even
> > more reasons for drm_connector_for_each_encoder imo.
> 
> I had that same thought initially, but then forgot about it for
> some reason. It does indeed lead to an even neater result.
> 
> Although it should be somewhat obvious that there can only be one
> currently used encoder for a connector, I think we may want to name
> this eg. drm_connector_for_each_possible_encoder() to make it super
> clear what we're talking about.

Yeah, that sounds like an even better name.
-Daniel

> 
> > -Daniel
> > 
> > > -Daniel
> > > 
> > > > + * @connector: &struct drm_connector pointer used as cursor
> > > > + * @encoder_id: the encoder ID
> > > > + * @__i: int iteration cursor, for macro-internal use
> > > > + */
> > > > +#define drm_for_each_connector_encoder_ids(connector, encoder_id, __i) \
> > > > +	for ((__i) = 0; (__i) < ARRAY_SIZE((connector)->encoder_ids) && \
> > > > +		     (connector)->encoder_ids[(__i)] != 0; (__i)++) \
> > > > +		for_each_if((encoder_id) = (connector)->encoder_ids[(__i)])
> > > > +
> > > >  #endif
> > > > -- 
> > > > 2.16.4
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2018-06-28  6:04 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 17:47 [PATCH 0/8] drm: Second attempt at fixing the fb-helper .best_encoder() mess Ville Syrjala
2018-06-26 17:47 ` [PATCH 1/8] drm/fb-helper: Eliminate the .best_encoder() usage Ville Syrjala
2018-06-26 17:58   ` Alex Deucher
2018-06-27  9:03   ` Daniel Vetter
2018-06-27 12:03     ` Ville Syrjälä
2018-06-26 17:47 ` [PATCH 2/8] drm/i915: Nuke intel_mst_best_encoder() Ville Syrjala
2018-06-27  9:03   ` Daniel Vetter
2018-06-26 17:47 ` [PATCH 3/8] drm: Add drm_for_each_connector_encoder_ids() Ville Syrjala
2018-06-27  9:08   ` Daniel Vetter
2018-06-27  9:11     ` Daniel Vetter
2018-06-27 15:07       ` Ville Syrjälä
2018-06-28  6:04         ` Daniel Vetter
     [not found] ` <20180626174714.32012-1-ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2018-06-26 17:47   ` [PATCH 4/8] drm/amdgpu: Use drm_for_each_connector_encoder_ids() Ville Syrjala
2018-06-26 17:47   ` [PATCH 5/8] drm/msm: " Ville Syrjala
2018-06-26 17:47   ` [PATCH 7/8] drm/radeon: " Ville Syrjala
2018-06-26 17:47 ` [PATCH 6/8] drm/nouveau: " Ville Syrjala
2018-06-26 20:55   ` [Intel-gfx] " kbuild test robot
2018-06-26 17:47 ` [PATCH 8/8] drm/tilcdc: " Ville Syrjala
2018-06-26 18:25 ` ✗ Fi.CI.CHECKPATCH: warning for drm: Second attempt at fixing the fb-helper .best_encoder() mess Patchwork
2018-06-26 18:40 ` ✓ Fi.CI.BAT: success " Patchwork
2018-06-26 20:47 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.