All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v10 1/2] drm: Add connector property to limit max bpc
@ 2018-09-24 21:08 Radhakrishna Sripada
  2018-09-24 21:08 ` [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp Radhakrishna Sripada
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Radhakrishna Sripada @ 2018-09-24 21:08 UTC (permalink / raw)
  To: intel-gfx
  Cc: Sunpeng Li, Daniel Vetter, Kishore Kadiyala, dri-devel, Rodrigo Vivi

At times 12bpc HDMI cannot be driven due to faulty cables, dongles
level shifters etc. To workaround them we may need to drive the output
at a lower bpc. Currently the user space does not have a way to limit
the bpc. The default bpc to be programmed is decided by the driver and
is run against connector limitations.

Creating a new connector property "max bpc" in order to limit the bpc.
xrandr can make use of this connector property to make sure that bpc does
not exceed the configured value. This property can be used by userspace to
set the bpc.

V2: Initialize max_bpc to satisfy kms_properties
V3: Move the property to drm_connector
V4: Split drm and i915 components(Ville)
V5: Make the property per connector(Ville)
V6: Compare the requested bpc to connector bpc(Daniel)
    Move the attach_property function to core(Ville)
V7: Fix checkpatch warnings
V8: Simplify the connector check code(Ville)
V9: Const display_info(Ville)
V10: Fix CI issues.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Sunpeng Li <sunpeng.li@amd.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        |  5 +++++
 drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
 drivers/gpu/drm/drm_atomic_uapi.c   |  4 ++++
 drivers/gpu/drm/drm_connector.c     | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h         | 20 ++++++++++++++++++++
 5 files changed, 66 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2870ae205237..f328bcca84a8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -390,6 +390,7 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
 {
 	struct drm_crtc_state *crtc_state;
 	struct drm_writeback_job *writeback_job = state->writeback_job;
+	const struct drm_display_info *info = &connector->display_info;
 
 	if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) || !writeback_job)
 		return 0;
@@ -417,6 +418,10 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
 		return -EINVAL;
 	}
 
+	state->max_bpc = info->bpc ? info->bpc : 8;
+	if (connector->max_bpc_property)
+		state->max_bpc = min(state->max_bpc, state->max_requested_bpc);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index e49b22381048..75aeca35f6d9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -639,6 +639,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			if (old_connector_state->link_status !=
 			    new_connector_state->link_status)
 				new_crtc_state->connectors_changed = true;
+
+			if (old_connector_state->max_requested_bpc !=
+			    new_connector_state->max_requested_bpc)
+				new_crtc_state->connectors_changed = true;
 		}
 
 		if (funcs->atomic_check)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index d5b7f315098c..86ac33922b09 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -740,6 +740,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 
 		return set_out_fence_for_connector(state->state, connector,
 						   fence_ptr);
+	} else if (property == connector->max_bpc_property) {
+		state->max_requested_bpc = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -804,6 +806,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == config->writeback_out_fence_ptr_property) {
 		*val = 0;
+	} else if (property == connector->max_bpc_property) {
+		*val = state->max_requested_bpc;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1e40e5decbe9..65e22c1b37a5 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1583,6 +1583,39 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
 EXPORT_SYMBOL(drm_connector_set_link_status_property);
 
 /**
+ * drm_connector_attach_max_bpc_property - attach "max bpc" property
+ * @connector: connector to attach max bpc property on.
+ * @min: The minimum bit depth supported by the connector.
+ * @max: The maximum bit depth supported by the connector.
+ *
+ * This is used to add support for limiting the bit depth on a connector.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
+					  int min, int max)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = connector->max_bpc_property;
+	if (!prop) {
+		prop = drm_property_create_range(dev, 0, "max bpc", min, max);
+		if (!prop)
+			return -ENOMEM;
+
+		connector->max_bpc_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, max);
+	connector->state->max_requested_bpc = max;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
+
+/**
  * drm_connector_init_panel_orientation_property -
  *	initialize the connecters panel_orientation property
  * @connector: connector for which to init the panel-orientation property.
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 91a877fa00cb..bfc3e698cda6 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -461,6 +461,18 @@ struct drm_connector_state {
 	 * drm_writeback_signal_completion()
 	 */
 	struct drm_writeback_job *writeback_job;
+
+	/**
+	 * @max_requested_bpc: Connector property to limit the maximum bit
+	 * depth of the pixels.
+	 */
+	u8 max_requested_bpc;
+
+	/**
+	 * @max_bpc: Connector max_bpc based on the requested max_bpc property
+	 * and the connector bpc limitations obtained from edid.
+	 */
+	u8 max_bpc;
 };
 
 /**
@@ -924,6 +936,12 @@ struct drm_connector {
 	 */
 	struct drm_property_blob *path_blob_ptr;
 
+	/**
+	 * @max_bpc_property: Default connector property for the max bpc to be
+	 * driven out of the connector.
+	 */
+	struct drm_property *max_bpc_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1201,6 +1219,8 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
 					    uint64_t link_status);
 int drm_connector_init_panel_orientation_property(
 	struct drm_connector *connector, int width, int height);
+int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
+					  int min, int max);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.9.3

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

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

* [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp
  2018-09-24 21:08 [PATCH v10 1/2] drm: Add connector property to limit max bpc Radhakrishna Sripada
@ 2018-09-24 21:08 ` Radhakrishna Sripada
  2018-10-01 13:48   ` Ville Syrjälä
  2018-09-24 21:36 ` ✗ Fi.CI.SPARSE: warning for series starting with [v10,1/2] drm: Add connector property to limit max bpc Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Radhakrishna Sripada @ 2018-09-24 21:08 UTC (permalink / raw)
  To: intel-gfx
  Cc: Stanislav Lisovskiy, Radhakrishna Sripada, Daniel Vetter,
	Kishore Kadiyala, Manasi Navare, dri-devel, Rodrigo Vivi

Use the newly added "max bpc" connector property to limit pipe bpp.

V3: Use drm_connector_state to access the "max bpc" property
V4: Initialize the drm property, add suuport to DP(Ville)
V5: Use the property in the connector and fix CI failure(Ville)
V6: Use the core function to attach max_bpc property, remove the redundant
    clamping of pipe bpp based on connector info
V7: Fix Checkpatch warnings
V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_dp.c      |  4 +++
 drivers/gpu/drm/i915/intel_hdmi.c    |  5 ++++
 3 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 931898013506..057abfd77cc3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10839,30 +10839,38 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 	drm_connector_list_iter_end(&conn_iter);
 }
 
-static void
-connected_sink_compute_bpp(struct intel_connector *connector,
-			   struct intel_crtc_state *pipe_config)
+static int
+connected_sink_max_bpp(const struct drm_connector_state *conn_state,
+		       struct intel_crtc_state *pipe_config)
 {
-	const struct drm_display_info *info = &connector->base.display_info;
-	int bpp = pipe_config->pipe_bpp;
+	int bpp;
+	struct drm_display_info *info = &conn_state->connector->display_info;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp constrains\n",
-		      connector->base.base.id,
-		      connector->base.name);
+	bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3);
 
-	/* Don't use an invalid EDID bpc value */
-	if (info->bpc != 0 && info->bpc * 3 < bpp) {
-		DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n",
-			      bpp, info->bpc * 3);
-		pipe_config->pipe_bpp = info->bpc * 3;
+	switch (conn_state->max_bpc) {
+	case 6 ... 7:
+		pipe_config->pipe_bpp = 6 * 3;
+	case 8 ... 9:
+		pipe_config->pipe_bpp = 8 * 3;
+		break;
+	case 10 ... 11:
+		pipe_config->pipe_bpp = 10 * 3;
+		break;
+	case 12:
+		pipe_config->pipe_bpp = 12 * 3;
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	/* Clamp bpp to 8 on screens without EDID 1.4 */
-	if (info->bpc == 0 && bpp > 24) {
-		DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n",
-			      bpp);
-		pipe_config->pipe_bpp = 24;
+	if (bpp != pipe_config->pipe_bpp) {
+		DRM_DEBUG_KMS("Limiting display bpp to %d instead of requested "
+			      "bpp %d, Edid bpp %d\n", bpp, 3 * info->bpc,
+			      3 * conn_state->max_requested_bpc);
+		pipe_config->pipe_bpp = bpp;
 	}
+	return 0;
 }
 
 static int
@@ -10893,8 +10901,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
 		if (connector_state->crtc != &crtc->base)
 			continue;
 
-		connected_sink_compute_bpp(to_intel_connector(connector),
-					   pipe_config);
+		if (connected_sink_max_bpp(connector_state, pipe_config) < 0)
+			return -EINVAL;
 	}
 
 	return bpp;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6b4c19123f2a..d8e128e771a1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5719,6 +5719,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
 		intel_attach_force_audio_property(connector);
 
 	intel_attach_broadcast_rgb_property(connector);
+	if (HAS_GMCH_DISPLAY(dev_priv))
+		drm_connector_attach_max_bpc_property(connector, 6, 10);
+	else if (INTEL_GEN(dev_priv) >= 5)
+		drm_connector_attach_max_bpc_property(connector, 6, 12);
 
 	if (intel_dp_is_edp(intel_dp)) {
 		u32 allowed_scalers;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a2dab0b6bde6..2b432c7e4f8a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2109,11 +2109,16 @@ static const struct drm_encoder_funcs intel_hdmi_enc_funcs = {
 static void
 intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector)
 {
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+
 	intel_attach_force_audio_property(connector);
 	intel_attach_broadcast_rgb_property(connector);
 	intel_attach_aspect_ratio_property(connector);
 	drm_connector_attach_content_type_property(connector);
 	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+
+	if (!HAS_GMCH_DISPLAY(dev_priv))
+		drm_connector_attach_max_bpc_property(connector, 8, 12);
 }
 
 /*
-- 
2.9.3

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

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

* ✗ Fi.CI.SPARSE: warning for series starting with [v10,1/2] drm: Add connector property to limit max bpc
  2018-09-24 21:08 [PATCH v10 1/2] drm: Add connector property to limit max bpc Radhakrishna Sripada
  2018-09-24 21:08 ` [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp Radhakrishna Sripada
@ 2018-09-24 21:36 ` Patchwork
  2018-09-24 21:53 ` ✗ Fi.CI.BAT: failure " Patchwork
  2018-10-01  7:23 ` [PATCH v10 1/2] " Daniel Vetter
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-24 21:36 UTC (permalink / raw)
  To: Radhakrishna Sripada; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v10,1/2] drm: Add connector property to limit max bpc
URL   : https://patchwork.freedesktop.org/series/50110/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm: Add connector property to limit max bpc
+drivers/gpu/drm/drm_atomic.c:423:34: warning: expression using sizeof(void)
+drivers/gpu/drm/drm_atomic.c:423:34: warning: expression using sizeof(void)

Commit: drm/i915: Allow "max bpc" property to limit pipe_bpp
+drivers/gpu/drm/i915/intel_display.c:10849:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_display.c:10849:15: warning: expression using sizeof(void)

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

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

* ✗ Fi.CI.BAT: failure for series starting with [v10,1/2] drm: Add connector property to limit max bpc
  2018-09-24 21:08 [PATCH v10 1/2] drm: Add connector property to limit max bpc Radhakrishna Sripada
  2018-09-24 21:08 ` [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp Radhakrishna Sripada
  2018-09-24 21:36 ` ✗ Fi.CI.SPARSE: warning for series starting with [v10,1/2] drm: Add connector property to limit max bpc Patchwork
@ 2018-09-24 21:53 ` Patchwork
  2018-10-01  7:23 ` [PATCH v10 1/2] " Daniel Vetter
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-09-24 21:53 UTC (permalink / raw)
  To: Radhakrishna Sripada; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v10,1/2] drm: Add connector property to limit max bpc
URL   : https://patchwork.freedesktop.org/series/50110/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4868 -> Patchwork_10264 =

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@debugfs_test@read_all_entries:
      fi-skl-iommu:       PASS -> FAIL +34
      fi-bdw-samus:       PASS -> FAIL
      fi-blb-e6850:       PASS -> FAIL
      fi-kbl-soraka:      PASS -> FAIL

    igt@drv_hangman@error-state-basic:
      fi-pnv-d510:        PASS -> DMESG-WARN
      fi-blb-e6850:       PASS -> DMESG-WARN
      fi-bwr-2160:        PASS -> DMESG-WARN

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-r:           PASS -> DMESG-WARN
      fi-skl-6770hq:      PASS -> DMESG-WARN
      fi-byt-n2820:       PASS -> DMESG-WARN
      fi-cfl-8109u:       PASS -> DMESG-WARN
      fi-cfl-s3:          PASS -> DMESG-WARN
      fi-snb-2600:        PASS -> DMESG-WARN
      fi-whl-u:           PASS -> DMESG-WARN
      fi-skl-caroline:    PASS -> DMESG-WARN
      fi-elk-e7500:       PASS -> DMESG-WARN
      fi-cfl-guc:         PASS -> DMESG-WARN
      fi-skl-iommu:       PASS -> DMESG-WARN
      fi-kbl-7567u:       PASS -> DMESG-WARN
      fi-skl-guc:         PASS -> DMESG-WARN
      fi-glk-j4005:       PASS -> DMESG-WARN
      fi-glk-dsi:         PASS -> DMESG-WARN
      fi-snb-2520m:       PASS -> DMESG-WARN +1
      fi-cfl-8700k:       PASS -> DMESG-WARN
      fi-bsw-kefka:       PASS -> DMESG-WARN
      fi-bxt-dsi:         PASS -> DMESG-WARN
      fi-byt-clapper:     PASS -> DMESG-WARN
      fi-bsw-n3050:       PASS -> DMESG-WARN
      fi-hsw-4770:        PASS -> DMESG-WARN
      fi-kbl-7560u:       PASS -> DMESG-WARN
      fi-bxt-j4205:       PASS -> DMESG-WARN
      fi-skl-6700hq:      PASS -> DMESG-WARN
      fi-cnl-u:           PASS -> DMESG-WARN
      fi-ivb-3770:        PASS -> DMESG-WARN
      fi-skl-6700k2:      PASS -> DMESG-WARN
      fi-hsw-4770r:       NOTRUN -> DMESG-WARN
      fi-skl-6600u:       PASS -> DMESG-WARN
      fi-hsw-peppy:       PASS -> DMESG-WARN

    igt@kms_busy@basic-flip-a:
      fi-kbl-7567u:       PASS -> FAIL +34
      fi-whl-u:           PASS -> FAIL +39

    igt@kms_chamelium@hdmi-crc-fast:
      fi-skl-6700k2:      PASS -> FAIL +35

    igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
      fi-glk-j4005:       PASS -> FAIL +34

    igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
      fi-hsw-4770r:       NOTRUN -> FAIL +34
      fi-pnv-d510:        PASS -> FAIL +20
      fi-skl-6600u:       PASS -> FAIL +39
      fi-cfl-8700k:       PASS -> FAIL +34

    igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
      fi-skl-guc:         PASS -> FAIL +34
      fi-snb-2600:        PASS -> FAIL +24

    igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
      fi-snb-2520m:       PASS -> FAIL +25

    igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
      fi-skl-caroline:    PASS -> FAIL +39

    igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
      fi-bsw-n3050:       PASS -> FAIL +20
      fi-hsw-4770:        PASS -> FAIL +35

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6770hq:      PASS -> FAIL +34

    igt@kms_flip@basic-flip-vs-modeset:
      fi-cnl-u:           PASS -> FAIL +38

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-hsw-peppy:       PASS -> FAIL +34

    igt@kms_frontbuffer_tracking@basic:
      fi-elk-e7500:       SKIP -> FAIL

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
      fi-ivb-3770:        PASS -> FAIL +35

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-bxt-j4205:       PASS -> FAIL +34

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-elk-e7500:       PASS -> FAIL +20
      fi-byt-n2820:       PASS -> FAIL +23

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-bsw-kefka:       PASS -> FAIL +24

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-bdw-gvtdvm:      PASS -> FAIL +34
      fi-gdg-551:         PASS -> FAIL +12

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-skl-gvtdvm:      PASS -> FAIL +34

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
      fi-glk-dsi:         PASS -> FAIL +35

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-bwr-2160:        PASS -> FAIL +20

    igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
      fi-cfl-guc:         PASS -> FAIL +34

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL +23
      fi-cfl-8109u:       PASS -> FAIL +34
      fi-cfl-s3:          PASS -> FAIL +39

    igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence:
      fi-bxt-dsi:         PASS -> FAIL +35
      fi-kbl-7560u:       PASS -> FAIL +39

    igt@kms_psr@cursor_plane_move:
      fi-kbl-r:           PASS -> FAIL +39

    igt@kms_psr@sprite_plane_onoff:
      fi-skl-6700hq:      PASS -> FAIL +39

    
    ==== Warnings ====

    igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
      fi-gdg-551:         PASS -> SKIP +7

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-WARN (fdo#102614) -> FAIL

    igt@pm_rpm@basic-pci-d3-state:
      fi-skl-6600u:       PASS -> SKIP +1
      fi-kbl-7560u:       PASS -> SKIP +1
      fi-whl-u:           PASS -> SKIP +1
      fi-byt-n2820:       PASS -> SKIP +2
      fi-skl-6770hq:      PASS -> SKIP +2
      fi-skl-6700k2:      PASS -> SKIP +2
      fi-bxt-j4205:       PASS -> SKIP +2
      fi-cnl-u:           PASS -> SKIP +1
      fi-skl-caroline:    PASS -> SKIP +1
      fi-skl-6700hq:      PASS -> SKIP +1

    igt@pm_rpm@basic-rte:
      fi-skl-iommu:       PASS -> SKIP +2
      fi-glk-j4005:       PASS -> SKIP +2
      fi-cfl-guc:         PASS -> SKIP +2
      fi-skl-guc:         PASS -> SKIP +2
      fi-kbl-7567u:       PASS -> SKIP +2
      fi-hsw-peppy:       PASS -> SKIP +1
      fi-kbl-r:           PASS -> SKIP +1
      fi-cfl-8109u:       PASS -> SKIP +2
      fi-cfl-s3:          PASS -> SKIP +1
      fi-bxt-dsi:         PASS -> SKIP +1
      fi-bsw-n3050:       PASS -> SKIP +1
      fi-bsw-kefka:       PASS -> SKIP +1
      fi-glk-dsi:         PASS -> SKIP +1
      fi-byt-clapper:     PASS -> SKIP +1

    igt@prime_vgem@basic-fence-flip:
      fi-cfl-8700k:       PASS -> SKIP +2
      fi-bdw-gvtdvm:      PASS -> SKIP
      fi-ivb-3770:        PASS -> SKIP
      fi-skl-gvtdvm:      PASS -> SKIP
      fi-hsw-4770:        PASS -> SKIP +2
      fi-snb-2600:        PASS -> SKIP
      fi-bwr-2160:        PASS -> SKIP
      fi-pnv-d510:        PASS -> SKIP
      fi-elk-e7500:       PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (46 -> 42) ==

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


== Build changes ==

    * Linux: CI_DRM_4868 -> Patchwork_10264

  CI_DRM_4868: dd42062d73b630f0ef0e8891bcc6438c14dae9dc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10264: 7369c5dbf163531ebb2bf65d09ae454e41786e65 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7369c5dbf163 drm/i915: Allow "max bpc" property to limit pipe_bpp
ffe87d224d06 drm: Add connector property to limit max bpc

== Logs ==

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

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

* Re: [PATCH v10 1/2] drm: Add connector property to limit max bpc
  2018-09-24 21:08 [PATCH v10 1/2] drm: Add connector property to limit max bpc Radhakrishna Sripada
                   ` (2 preceding siblings ...)
  2018-09-24 21:53 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2018-10-01  7:23 ` Daniel Vetter
  2018-10-03 22:24   ` Radhakrishna Sripada
  3 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2018-10-01  7:23 UTC (permalink / raw)
  To: Radhakrishna Sripada
  Cc: Stanislav Lisovskiy, Sunpeng Li, Daniel Vetter, intel-gfx,
	Kishore Kadiyala, Manasi Navare, dri-devel, Rodrigo Vivi

On Mon, Sep 24, 2018 at 02:08:14PM -0700, Radhakrishna Sripada wrote:
> At times 12bpc HDMI cannot be driven due to faulty cables, dongles
> level shifters etc. To workaround them we may need to drive the output
> at a lower bpc. Currently the user space does not have a way to limit
> the bpc. The default bpc to be programmed is decided by the driver and
> is run against connector limitations.
> 
> Creating a new connector property "max bpc" in order to limit the bpc.
> xrandr can make use of this connector property to make sure that bpc does
> not exceed the configured value. This property can be used by userspace to
> set the bpc.
> 
> V2: Initialize max_bpc to satisfy kms_properties
> V3: Move the property to drm_connector
> V4: Split drm and i915 components(Ville)
> V5: Make the property per connector(Ville)
> V6: Compare the requested bpc to connector bpc(Daniel)
>     Move the attach_property function to core(Ville)
> V7: Fix checkpatch warnings
> V8: Simplify the connector check code(Ville)
> V9: Const display_info(Ville)
> V10: Fix CI issues.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Sunpeng Li <sunpeng.li@amd.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>

Skimming this, I think it looks good now at a high-level.

What's missing is now kernel-doc for these new prorties, needs to be added
here:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties

With that I'm happy with the high-level design:

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

No full review since I didn't look at the igt side for this. Userspace I
think is ok, since it's just another connector prop.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c        |  5 +++++
>  drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
>  drivers/gpu/drm/drm_atomic_uapi.c   |  4 ++++
>  drivers/gpu/drm/drm_connector.c     | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         | 20 ++++++++++++++++++++
>  5 files changed, 66 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2870ae205237..f328bcca84a8 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -390,6 +390,7 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
>  {
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_writeback_job *writeback_job = state->writeback_job;
> +	const struct drm_display_info *info = &connector->display_info;
>  
>  	if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) || !writeback_job)
>  		return 0;
> @@ -417,6 +418,10 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
>  		return -EINVAL;
>  	}
>  
> +	state->max_bpc = info->bpc ? info->bpc : 8;
> +	if (connector->max_bpc_property)
> +		state->max_bpc = min(state->max_bpc, state->max_requested_bpc);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e49b22381048..75aeca35f6d9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -639,6 +639,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  			if (old_connector_state->link_status !=
>  			    new_connector_state->link_status)
>  				new_crtc_state->connectors_changed = true;
> +
> +			if (old_connector_state->max_requested_bpc !=
> +			    new_connector_state->max_requested_bpc)
> +				new_crtc_state->connectors_changed = true;
>  		}
>  
>  		if (funcs->atomic_check)
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f315098c..86ac33922b09 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -740,6 +740,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  
>  		return set_out_fence_for_connector(state->state, connector,
>  						   fence_ptr);
> +	} else if (property == connector->max_bpc_property) {
> +		state->max_requested_bpc = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -804,6 +806,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = 0;
>  	} else if (property == config->writeback_out_fence_ptr_property) {
>  		*val = 0;
> +	} else if (property == connector->max_bpc_property) {
> +		*val = state->max_requested_bpc;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1e40e5decbe9..65e22c1b37a5 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1583,6 +1583,39 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_connector_set_link_status_property);
>  
>  /**
> + * drm_connector_attach_max_bpc_property - attach "max bpc" property
> + * @connector: connector to attach max bpc property on.
> + * @min: The minimum bit depth supported by the connector.
> + * @max: The maximum bit depth supported by the connector.
> + *
> + * This is used to add support for limiting the bit depth on a connector.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> +					  int min, int max)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = connector->max_bpc_property;
> +	if (!prop) {
> +		prop = drm_property_create_range(dev, 0, "max bpc", min, max);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->max_bpc_property = prop;
> +	}
> +
> +	drm_object_attach_property(&connector->base, prop, max);
> +	connector->state->max_requested_bpc = max;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> +
> +/**
>   * drm_connector_init_panel_orientation_property -
>   *	initialize the connecters panel_orientation property
>   * @connector: connector for which to init the panel-orientation property.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 91a877fa00cb..bfc3e698cda6 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -461,6 +461,18 @@ struct drm_connector_state {
>  	 * drm_writeback_signal_completion()
>  	 */
>  	struct drm_writeback_job *writeback_job;
> +
> +	/**
> +	 * @max_requested_bpc: Connector property to limit the maximum bit
> +	 * depth of the pixels.
> +	 */
> +	u8 max_requested_bpc;
> +
> +	/**
> +	 * @max_bpc: Connector max_bpc based on the requested max_bpc property
> +	 * and the connector bpc limitations obtained from edid.
> +	 */
> +	u8 max_bpc;
>  };
>  
>  /**
> @@ -924,6 +936,12 @@ struct drm_connector {
>  	 */
>  	struct drm_property_blob *path_blob_ptr;
>  
> +	/**
> +	 * @max_bpc_property: Default connector property for the max bpc to be
> +	 * driven out of the connector.
> +	 */
> +	struct drm_property *max_bpc_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1201,6 +1219,8 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
>  					    uint64_t link_status);
>  int drm_connector_init_panel_orientation_property(
>  	struct drm_connector *connector, int width, int height);
> +int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> +					  int min, int max);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata
> -- 
> 2.9.3
> 

-- 
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] 10+ messages in thread

* Re: [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp
  2018-09-24 21:08 ` [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp Radhakrishna Sripada
@ 2018-10-01 13:48   ` Ville Syrjälä
  2018-10-03 18:42     ` Radhakrishna Sripada
  0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2018-10-01 13:48 UTC (permalink / raw)
  To: Radhakrishna Sripada
  Cc: Stanislav Lisovskiy, Daniel Vetter, intel-gfx, Kishore Kadiyala,
	Manasi Navare, dri-devel, Rodrigo Vivi

On Mon, Sep 24, 2018 at 02:08:15PM -0700, Radhakrishna Sripada wrote:
> Use the newly added "max bpc" connector property to limit pipe bpp.
> 
> V3: Use drm_connector_state to access the "max bpc" property
> V4: Initialize the drm property, add suuport to DP(Ville)
> V5: Use the property in the connector and fix CI failure(Ville)
> V6: Use the core function to attach max_bpc property, remove the redundant
>     clamping of pipe bpp based on connector info
> V7: Fix Checkpatch warnings
> V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_dp.c      |  4 +++
>  drivers/gpu/drm/i915/intel_hdmi.c    |  5 ++++
>  3 files changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 931898013506..057abfd77cc3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10839,30 +10839,38 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>  	drm_connector_list_iter_end(&conn_iter);
>  }
>  
> -static void
> -connected_sink_compute_bpp(struct intel_connector *connector,
> -			   struct intel_crtc_state *pipe_config)
> +static int
> +connected_sink_max_bpp(const struct drm_connector_state *conn_state,
> +		       struct intel_crtc_state *pipe_config)
>  {
> -	const struct drm_display_info *info = &connector->base.display_info;
> -	int bpp = pipe_config->pipe_bpp;
> +	int bpp;
> +	struct drm_display_info *info = &conn_state->connector->display_info;
>  
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp constrains\n",
> -		      connector->base.base.id,
> -		      connector->base.name);
> +	bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3);

Needs a check to make sure the connector has the property.

>  
> -	/* Don't use an invalid EDID bpc value */
> -	if (info->bpc != 0 && info->bpc * 3 < bpp) {
> -		DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n",
> -			      bpp, info->bpc * 3);
> -		pipe_config->pipe_bpp = info->bpc * 3;
> +	switch (conn_state->max_bpc) {
> +	case 6 ... 7:
> +		pipe_config->pipe_bpp = 6 * 3;
> +	case 8 ... 9:
> +		pipe_config->pipe_bpp = 8 * 3;
> +		break;
> +	case 10 ... 11:
> +		pipe_config->pipe_bpp = 10 * 3;
> +		break;
> +	case 12:
> +		pipe_config->pipe_bpp = 12 * 3;
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> -	/* Clamp bpp to 8 on screens without EDID 1.4 */
> -	if (info->bpc == 0 && bpp > 24) {
> -		DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n",
> -			      bpp);
> -		pipe_config->pipe_bpp = 24;
> +	if (bpp != pipe_config->pipe_bpp) {
> +		DRM_DEBUG_KMS("Limiting display bpp to %d instead of requested "
> +			      "bpp %d, Edid bpp %d\n", bpp, 3 * info->bpc,
> +			      3 * conn_state->max_requested_bpc);

The format string doesn't seem to match the arguments.

> +		pipe_config->pipe_bpp = bpp;
>  	}
> +	return 0;
>  }
>  
>  static int
> @@ -10893,8 +10901,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
>  
> -		connected_sink_compute_bpp(to_intel_connector(connector),
> -					   pipe_config);
> +		if (connected_sink_max_bpp(connector_state, pipe_config) < 0)
> +			return -EINVAL;
>  	}
>  
>  	return bpp;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6b4c19123f2a..d8e128e771a1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5719,6 +5719,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>  		intel_attach_force_audio_property(connector);
>  
>  	intel_attach_broadcast_rgb_property(connector);
> +	if (HAS_GMCH_DISPLAY(dev_priv))
> +		drm_connector_attach_max_bpc_property(connector, 6, 10);
> +	else if (INTEL_GEN(dev_priv) >= 5)
> +		drm_connector_attach_max_bpc_property(connector, 6, 12);
>  
>  	if (intel_dp_is_edp(intel_dp)) {
>  		u32 allowed_scalers;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a2dab0b6bde6..2b432c7e4f8a 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2109,11 +2109,16 @@ static const struct drm_encoder_funcs intel_hdmi_enc_funcs = {
>  static void
>  intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +
>  	intel_attach_force_audio_property(connector);
>  	intel_attach_broadcast_rgb_property(connector);
>  	intel_attach_aspect_ratio_property(connector);
>  	drm_connector_attach_content_type_property(connector);
>  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +
> +	if (!HAS_GMCH_DISPLAY(dev_priv))
> +		drm_connector_attach_max_bpc_property(connector, 8, 12);
>  }
>  
>  /*
> -- 
> 2.9.3

-- 
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] 10+ messages in thread

* Re: [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp
  2018-10-01 13:48   ` Ville Syrjälä
@ 2018-10-03 18:42     ` Radhakrishna Sripada
  2018-10-03 18:48       ` Ville Syrjälä
  0 siblings, 1 reply; 10+ messages in thread
From: Radhakrishna Sripada @ 2018-10-03 18:42 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Daniel Vetter, intel-gfx, Kishore Kadiyala, dri-devel, Rodrigo Vivi

On Mon, Oct 01, 2018 at 04:48:01PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 02:08:15PM -0700, Radhakrishna Sripada wrote:
> > Use the newly added "max bpc" connector property to limit pipe bpp.
> > 
> > V3: Use drm_connector_state to access the "max bpc" property
> > V4: Initialize the drm property, add suuport to DP(Ville)
> > V5: Use the property in the connector and fix CI failure(Ville)
> > V6: Use the core function to attach max_bpc property, remove the redundant
> >     clamping of pipe bpp based on connector info
> > V7: Fix Checkpatch warnings
> > V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++---------------
> >  drivers/gpu/drm/i915/intel_dp.c      |  4 +++
> >  drivers/gpu/drm/i915/intel_hdmi.c    |  5 ++++
> >  3 files changed, 37 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 931898013506..057abfd77cc3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10839,30 +10839,38 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
> >  	drm_connector_list_iter_end(&conn_iter);
> >  }
> >  
> > -static void
> > -connected_sink_compute_bpp(struct intel_connector *connector,
> > -			   struct intel_crtc_state *pipe_config)
> > +static int
> > +connected_sink_max_bpp(const struct drm_connector_state *conn_state,
> > +		       struct intel_crtc_state *pipe_config)
> >  {
> > -	const struct drm_display_info *info = &connector->base.display_info;
> > -	int bpp = pipe_config->pipe_bpp;
> > +	int bpp;
> > +	struct drm_display_info *info = &conn_state->connector->display_info;
> >  
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp constrains\n",
> > -		      connector->base.base.id,
> > -		      connector->base.name);
> > +	bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3);
> 
> Needs a check to make sure the connector has the property.
We do make a check for the connector property in the drm core. In the absence 
of the property, conn_state->max_bpc carries the limit imposed from 
connector->display_info.bpc and would be requiring the codepath below.

Thoughts?

-- Radhakrishna Sripada
> 
> >  
> > -	/* Don't use an invalid EDID bpc value */
> > -	if (info->bpc != 0 && info->bpc * 3 < bpp) {
> > -		DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n",
> > -			      bpp, info->bpc * 3);
> > -		pipe_config->pipe_bpp = info->bpc * 3;
> > +	switch (conn_state->max_bpc) {
> > +	case 6 ... 7:
> > +		pipe_config->pipe_bpp = 6 * 3;
> > +	case 8 ... 9:
> > +		pipe_config->pipe_bpp = 8 * 3;
> > +		break;
> > +	case 10 ... 11:
> > +		pipe_config->pipe_bpp = 10 * 3;
> > +		break;
> > +	case 12:
> > +		pipe_config->pipe_bpp = 12 * 3;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> >  	}
> >  
> > -	/* Clamp bpp to 8 on screens without EDID 1.4 */
> > -	if (info->bpc == 0 && bpp > 24) {
> > -		DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n",
> > -			      bpp);
> > -		pipe_config->pipe_bpp = 24;
> > +	if (bpp != pipe_config->pipe_bpp) {
> > +		DRM_DEBUG_KMS("Limiting display bpp to %d instead of requested "
> > +			      "bpp %d, Edid bpp %d\n", bpp, 3 * info->bpc,
> > +			      3 * conn_state->max_requested_bpc);
> 
> The format string doesn't seem to match the arguments.
> 
> > +		pipe_config->pipe_bpp = bpp;
> >  	}
> > +	return 0;
> >  }
> >  
> >  static int
> > @@ -10893,8 +10901,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> >  		if (connector_state->crtc != &crtc->base)
> >  			continue;
> >  
> > -		connected_sink_compute_bpp(to_intel_connector(connector),
> > -					   pipe_config);
> > +		if (connected_sink_max_bpp(connector_state, pipe_config) < 0)
> > +			return -EINVAL;
> >  	}
> >  
> >  	return bpp;
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6b4c19123f2a..d8e128e771a1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5719,6 +5719,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> >  		intel_attach_force_audio_property(connector);
> >  
> >  	intel_attach_broadcast_rgb_property(connector);
> > +	if (HAS_GMCH_DISPLAY(dev_priv))
> > +		drm_connector_attach_max_bpc_property(connector, 6, 10);
> > +	else if (INTEL_GEN(dev_priv) >= 5)
> > +		drm_connector_attach_max_bpc_property(connector, 6, 12);
> >  
> >  	if (intel_dp_is_edp(intel_dp)) {
> >  		u32 allowed_scalers;
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index a2dab0b6bde6..2b432c7e4f8a 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2109,11 +2109,16 @@ static const struct drm_encoder_funcs intel_hdmi_enc_funcs = {
> >  static void
> >  intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > +
> >  	intel_attach_force_audio_property(connector);
> >  	intel_attach_broadcast_rgb_property(connector);
> >  	intel_attach_aspect_ratio_property(connector);
> >  	drm_connector_attach_content_type_property(connector);
> >  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > +
> > +	if (!HAS_GMCH_DISPLAY(dev_priv))
> > +		drm_connector_attach_max_bpc_property(connector, 8, 12);
> >  }
> >  
> >  /*
> > -- 
> > 2.9.3
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp
  2018-10-03 18:42     ` Radhakrishna Sripada
@ 2018-10-03 18:48       ` Ville Syrjälä
  0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-10-03 18:48 UTC (permalink / raw)
  To: Radhakrishna Sripada
  Cc: Daniel Vetter, intel-gfx, Kishore Kadiyala, dri-devel, Rodrigo Vivi

On Wed, Oct 03, 2018 at 11:42:14AM -0700, Radhakrishna Sripada wrote:
> On Mon, Oct 01, 2018 at 04:48:01PM +0300, Ville Syrjälä wrote:
> > On Mon, Sep 24, 2018 at 02:08:15PM -0700, Radhakrishna Sripada wrote:
> > > Use the newly added "max bpc" connector property to limit pipe bpp.
> > > 
> > > V3: Use drm_connector_state to access the "max bpc" property
> > > V4: Initialize the drm property, add suuport to DP(Ville)
> > > V5: Use the property in the connector and fix CI failure(Ville)
> > > V6: Use the core function to attach max_bpc property, remove the redundant
> > >     clamping of pipe bpp based on connector info
> > > V7: Fix Checkpatch warnings
> > > V9: Cleanup connected_sink_max_bpp and fix initial value in DP(Ville)
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++---------------
> > >  drivers/gpu/drm/i915/intel_dp.c      |  4 +++
> > >  drivers/gpu/drm/i915/intel_hdmi.c    |  5 ++++
> > >  3 files changed, 37 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 931898013506..057abfd77cc3 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -10839,30 +10839,38 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
> > >  	drm_connector_list_iter_end(&conn_iter);
> > >  }
> > >  
> > > -static void
> > > -connected_sink_compute_bpp(struct intel_connector *connector,
> > > -			   struct intel_crtc_state *pipe_config)
> > > +static int
> > > +connected_sink_max_bpp(const struct drm_connector_state *conn_state,
> > > +		       struct intel_crtc_state *pipe_config)
> > >  {
> > > -	const struct drm_display_info *info = &connector->base.display_info;
> > > -	int bpp = pipe_config->pipe_bpp;
> > > +	int bpp;
> > > +	struct drm_display_info *info = &conn_state->connector->display_info;
> > >  
> > > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] checking for sink bpp constrains\n",
> > > -		      connector->base.base.id,
> > > -		      connector->base.name);
> > > +	bpp = min(pipe_config->pipe_bpp, conn_state->max_bpc * 3);
> > 
> > Needs a check to make sure the connector has the property.
> We do make a check for the connector property in the drm core. In the absence 
> of the property, conn_state->max_bpc carries the limit imposed from 
> connector->display_info.bpc and would be requiring the codepath below.

Ah yes. That part is perfectly good then.

> 
> Thoughts?
> 
> -- Radhakrishna Sripada
> > 
> > >  
> > > -	/* Don't use an invalid EDID bpc value */
> > > -	if (info->bpc != 0 && info->bpc * 3 < bpp) {
> > > -		DRM_DEBUG_KMS("clamping display bpp (was %d) to EDID reported max of %d\n",
> > > -			      bpp, info->bpc * 3);
> > > -		pipe_config->pipe_bpp = info->bpc * 3;
> > > +	switch (conn_state->max_bpc) {
> > > +	case 6 ... 7:
> > > +		pipe_config->pipe_bpp = 6 * 3;
> > > +	case 8 ... 9:
> > > +		pipe_config->pipe_bpp = 8 * 3;
> > > +		break;
> > > +	case 10 ... 11:
> > > +		pipe_config->pipe_bpp = 10 * 3;
> > > +		break;
> > > +	case 12:
> > > +		pipe_config->pipe_bpp = 12 * 3;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > >  	}
> > >  
> > > -	/* Clamp bpp to 8 on screens without EDID 1.4 */
> > > -	if (info->bpc == 0 && bpp > 24) {
> > > -		DRM_DEBUG_KMS("clamping display bpp (was %d) to default limit of 24\n",
> > > -			      bpp);
> > > -		pipe_config->pipe_bpp = 24;
> > > +	if (bpp != pipe_config->pipe_bpp) {
> > > +		DRM_DEBUG_KMS("Limiting display bpp to %d instead of requested "
> > > +			      "bpp %d, Edid bpp %d\n", bpp, 3 * info->bpc,
> > > +			      3 * conn_state->max_requested_bpc);
> > 
> > The format string doesn't seem to match the arguments.
> > 
> > > +		pipe_config->pipe_bpp = bpp;
> > >  	}
> > > +	return 0;
> > >  }
> > >  
> > >  static int
> > > @@ -10893,8 +10901,8 @@ compute_baseline_pipe_bpp(struct intel_crtc *crtc,
> > >  		if (connector_state->crtc != &crtc->base)
> > >  			continue;
> > >  
> > > -		connected_sink_compute_bpp(to_intel_connector(connector),
> > > -					   pipe_config);
> > > +		if (connected_sink_max_bpp(connector_state, pipe_config) < 0)
> > > +			return -EINVAL;
> > >  	}
> > >  
> > >  	return bpp;
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 6b4c19123f2a..d8e128e771a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5719,6 +5719,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
> > >  		intel_attach_force_audio_property(connector);
> > >  
> > >  	intel_attach_broadcast_rgb_property(connector);
> > > +	if (HAS_GMCH_DISPLAY(dev_priv))
> > > +		drm_connector_attach_max_bpc_property(connector, 6, 10);
> > > +	else if (INTEL_GEN(dev_priv) >= 5)
> > > +		drm_connector_attach_max_bpc_property(connector, 6, 12);
> > >  
> > >  	if (intel_dp_is_edp(intel_dp)) {
> > >  		u32 allowed_scalers;
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index a2dab0b6bde6..2b432c7e4f8a 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -2109,11 +2109,16 @@ static const struct drm_encoder_funcs intel_hdmi_enc_funcs = {
> > >  static void
> > >  intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > > +
> > >  	intel_attach_force_audio_property(connector);
> > >  	intel_attach_broadcast_rgb_property(connector);
> > >  	intel_attach_aspect_ratio_property(connector);
> > >  	drm_connector_attach_content_type_property(connector);
> > >  	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > > +
> > > +	if (!HAS_GMCH_DISPLAY(dev_priv))
> > > +		drm_connector_attach_max_bpc_property(connector, 8, 12);
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.9.3
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

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

* Re: [PATCH v10 1/2] drm: Add connector property to limit max bpc
  2018-10-01  7:23 ` [PATCH v10 1/2] " Daniel Vetter
@ 2018-10-03 22:24   ` Radhakrishna Sripada
  2018-10-04  7:52     ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Radhakrishna Sripada @ 2018-10-03 22:24 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Stanislav Lisovskiy, Sunpeng Li, Daniel Vetter, intel-gfx,
	Kishore Kadiyala, Manasi Navare, dri-devel, Rodrigo Vivi

On Mon, Oct 01, 2018 at 09:23:38AM +0200, Daniel Vetter wrote:
> On Mon, Sep 24, 2018 at 02:08:14PM -0700, Radhakrishna Sripada wrote:
> > At times 12bpc HDMI cannot be driven due to faulty cables, dongles
> > level shifters etc. To workaround them we may need to drive the output
> > at a lower bpc. Currently the user space does not have a way to limit
> > the bpc. The default bpc to be programmed is decided by the driver and
> > is run against connector limitations.
> > 
> > Creating a new connector property "max bpc" in order to limit the bpc.
> > xrandr can make use of this connector property to make sure that bpc does
> > not exceed the configured value. This property can be used by userspace to
> > set the bpc.
> > 
> > V2: Initialize max_bpc to satisfy kms_properties
> > V3: Move the property to drm_connector
> > V4: Split drm and i915 components(Ville)
> > V5: Make the property per connector(Ville)
> > V6: Compare the requested bpc to connector bpc(Daniel)
> >     Move the attach_property function to core(Ville)
> > V7: Fix checkpatch warnings
> > V8: Simplify the connector check code(Ville)
> > V9: Const display_info(Ville)
> > V10: Fix CI issues.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Cc: Sunpeng Li <sunpeng.li@amd.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> 
> Skimming this, I think it looks good now at a high-level.
> 
> What's missing is now kernel-doc for these new prorties, needs to be added
> here:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties
> 
> With that I'm happy with the high-level design:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> No full review since I didn't look at the igt side for this. Userspace I
> think is ok, since it's just another connector prop.
kms_properties would test this newly added property as part of the connector properties.
Do you suggest me to add a new subtest to try checking the different values for the 
newly added property?

--Radhakrishna Sripada
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        |  5 +++++
> >  drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
> >  drivers/gpu/drm/drm_atomic_uapi.c   |  4 ++++
> >  drivers/gpu/drm/drm_connector.c     | 33 +++++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h         | 20 ++++++++++++++++++++
> >  5 files changed, 66 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 2870ae205237..f328bcca84a8 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -390,6 +390,7 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
> >  {
> >  	struct drm_crtc_state *crtc_state;
> >  	struct drm_writeback_job *writeback_job = state->writeback_job;
> > +	const struct drm_display_info *info = &connector->display_info;
> >  
> >  	if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) || !writeback_job)
> >  		return 0;
> > @@ -417,6 +418,10 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
> >  		return -EINVAL;
> >  	}
> >  
> > +	state->max_bpc = info->bpc ? info->bpc : 8;
> > +	if (connector->max_bpc_property)
> > +		state->max_bpc = min(state->max_bpc, state->max_requested_bpc);
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index e49b22381048..75aeca35f6d9 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -639,6 +639,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  			if (old_connector_state->link_status !=
> >  			    new_connector_state->link_status)
> >  				new_crtc_state->connectors_changed = true;
> > +
> > +			if (old_connector_state->max_requested_bpc !=
> > +			    new_connector_state->max_requested_bpc)
> > +				new_crtc_state->connectors_changed = true;
> >  		}
> >  
> >  		if (funcs->atomic_check)
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index d5b7f315098c..86ac33922b09 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -740,6 +740,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  
> >  		return set_out_fence_for_connector(state->state, connector,
> >  						   fence_ptr);
> > +	} else if (property == connector->max_bpc_property) {
> > +		state->max_requested_bpc = val;
> >  	} else if (connector->funcs->atomic_set_property) {
> >  		return connector->funcs->atomic_set_property(connector,
> >  				state, property, val);
> > @@ -804,6 +806,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = 0;
> >  	} else if (property == config->writeback_out_fence_ptr_property) {
> >  		*val = 0;
> > +	} else if (property == connector->max_bpc_property) {
> > +		*val = state->max_requested_bpc;
> >  	} else if (connector->funcs->atomic_get_property) {
> >  		return connector->funcs->atomic_get_property(connector,
> >  				state, property, val);
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 1e40e5decbe9..65e22c1b37a5 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1583,6 +1583,39 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
> >  EXPORT_SYMBOL(drm_connector_set_link_status_property);
> >  
> >  /**
> > + * drm_connector_attach_max_bpc_property - attach "max bpc" property
> > + * @connector: connector to attach max bpc property on.
> > + * @min: The minimum bit depth supported by the connector.
> > + * @max: The maximum bit depth supported by the connector.
> > + *
> > + * This is used to add support for limiting the bit depth on a connector.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> > +					  int min, int max)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *prop;
> > +
> > +	prop = connector->max_bpc_property;
> > +	if (!prop) {
> > +		prop = drm_property_create_range(dev, 0, "max bpc", min, max);
> > +		if (!prop)
> > +			return -ENOMEM;
> > +
> > +		connector->max_bpc_property = prop;
> > +	}
> > +
> > +	drm_object_attach_property(&connector->base, prop, max);
> > +	connector->state->max_requested_bpc = max;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> > +
> > +/**
> >   * drm_connector_init_panel_orientation_property -
> >   *	initialize the connecters panel_orientation property
> >   * @connector: connector for which to init the panel-orientation property.
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 91a877fa00cb..bfc3e698cda6 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -461,6 +461,18 @@ struct drm_connector_state {
> >  	 * drm_writeback_signal_completion()
> >  	 */
> >  	struct drm_writeback_job *writeback_job;
> > +
> > +	/**
> > +	 * @max_requested_bpc: Connector property to limit the maximum bit
> > +	 * depth of the pixels.
> > +	 */
> > +	u8 max_requested_bpc;
> > +
> > +	/**
> > +	 * @max_bpc: Connector max_bpc based on the requested max_bpc property
> > +	 * and the connector bpc limitations obtained from edid.
> > +	 */
> > +	u8 max_bpc;
> >  };
> >  
> >  /**
> > @@ -924,6 +936,12 @@ struct drm_connector {
> >  	 */
> >  	struct drm_property_blob *path_blob_ptr;
> >  
> > +	/**
> > +	 * @max_bpc_property: Default connector property for the max bpc to be
> > +	 * driven out of the connector.
> > +	 */
> > +	struct drm_property *max_bpc_property;
> > +
> >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> >  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> >  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > @@ -1201,6 +1219,8 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
> >  					    uint64_t link_status);
> >  int drm_connector_init_panel_orientation_property(
> >  	struct drm_connector *connector, int width, int height);
> > +int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> > +					  int min, int max);
> >  
> >  /**
> >   * struct drm_tile_group - Tile group metadata
> > -- 
> > 2.9.3
> > 
> 
> -- 
> 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] 10+ messages in thread

* Re: [PATCH v10 1/2] drm: Add connector property to limit max bpc
  2018-10-03 22:24   ` Radhakrishna Sripada
@ 2018-10-04  7:52     ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-10-04  7:52 UTC (permalink / raw)
  To: Radhakrishna Sripada
  Cc: Stanislav Lisovskiy, Sunpeng Li, Daniel Vetter, intel-gfx,
	Kishore Kadiyala, Manasi Navare, dri-devel, Rodrigo Vivi

On Wed, Oct 03, 2018 at 03:24:40PM -0700, Radhakrishna Sripada wrote:
> On Mon, Oct 01, 2018 at 09:23:38AM +0200, Daniel Vetter wrote:
> > On Mon, Sep 24, 2018 at 02:08:14PM -0700, Radhakrishna Sripada wrote:
> > > At times 12bpc HDMI cannot be driven due to faulty cables, dongles
> > > level shifters etc. To workaround them we may need to drive the output
> > > at a lower bpc. Currently the user space does not have a way to limit
> > > the bpc. The default bpc to be programmed is decided by the driver and
> > > is run against connector limitations.
> > > 
> > > Creating a new connector property "max bpc" in order to limit the bpc.
> > > xrandr can make use of this connector property to make sure that bpc does
> > > not exceed the configured value. This property can be used by userspace to
> > > set the bpc.
> > > 
> > > V2: Initialize max_bpc to satisfy kms_properties
> > > V3: Move the property to drm_connector
> > > V4: Split drm and i915 components(Ville)
> > > V5: Make the property per connector(Ville)
> > > V6: Compare the requested bpc to connector bpc(Daniel)
> > >     Move the attach_property function to core(Ville)
> > > V7: Fix checkpatch warnings
> > > V8: Simplify the connector check code(Ville)
> > > V9: Const display_info(Ville)
> > > V10: Fix CI issues.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Kishore Kadiyala <kishore.kadiyala@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Cc: Sunpeng Li <sunpeng.li@amd.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > 
> > Skimming this, I think it looks good now at a high-level.
> > 
> > What's missing is now kernel-doc for these new prorties, needs to be added
> > here:
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-connector-properties
> > 
> > With that I'm happy with the high-level design:
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > No full review since I didn't look at the igt side for this. Userspace I
> > think is ok, since it's just another connector prop.
> kms_properties would test this newly added property as part of the connector properties.
> Do you suggest me to add a new subtest to try checking the different values for the 
> newly added property?

kms_properties most definitely doesn't do any kind of functional testing
here. Which means it's not validated in an automatic way.

So yes, we most definitely want a real testcase here. Coming up with a
proper validation strategy here is going to be somewhat challenging
though.

Thanks, Daniel

> 
> --Radhakrishna Sripada
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c        |  5 +++++
> > >  drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
> > >  drivers/gpu/drm/drm_atomic_uapi.c   |  4 ++++
> > >  drivers/gpu/drm/drm_connector.c     | 33 +++++++++++++++++++++++++++++++++
> > >  include/drm/drm_connector.h         | 20 ++++++++++++++++++++
> > >  5 files changed, 66 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 2870ae205237..f328bcca84a8 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -390,6 +390,7 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
> > >  {
> > >  	struct drm_crtc_state *crtc_state;
> > >  	struct drm_writeback_job *writeback_job = state->writeback_job;
> > > +	const struct drm_display_info *info = &connector->display_info;
> > >  
> > >  	if ((connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) || !writeback_job)
> > >  		return 0;
> > > @@ -417,6 +418,10 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	state->max_bpc = info->bpc ? info->bpc : 8;
> > > +	if (connector->max_bpc_property)
> > > +		state->max_bpc = min(state->max_bpc, state->max_requested_bpc);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index e49b22381048..75aeca35f6d9 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -639,6 +639,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > >  			if (old_connector_state->link_status !=
> > >  			    new_connector_state->link_status)
> > >  				new_crtc_state->connectors_changed = true;
> > > +
> > > +			if (old_connector_state->max_requested_bpc !=
> > > +			    new_connector_state->max_requested_bpc)
> > > +				new_crtc_state->connectors_changed = true;
> > >  		}
> > >  
> > >  		if (funcs->atomic_check)
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index d5b7f315098c..86ac33922b09 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -740,6 +740,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> > >  
> > >  		return set_out_fence_for_connector(state->state, connector,
> > >  						   fence_ptr);
> > > +	} else if (property == connector->max_bpc_property) {
> > > +		state->max_requested_bpc = val;
> > >  	} else if (connector->funcs->atomic_set_property) {
> > >  		return connector->funcs->atomic_set_property(connector,
> > >  				state, property, val);
> > > @@ -804,6 +806,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> > >  		*val = 0;
> > >  	} else if (property == config->writeback_out_fence_ptr_property) {
> > >  		*val = 0;
> > > +	} else if (property == connector->max_bpc_property) {
> > > +		*val = state->max_requested_bpc;
> > >  	} else if (connector->funcs->atomic_get_property) {
> > >  		return connector->funcs->atomic_get_property(connector,
> > >  				state, property, val);
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 1e40e5decbe9..65e22c1b37a5 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1583,6 +1583,39 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
> > >  EXPORT_SYMBOL(drm_connector_set_link_status_property);
> > >  
> > >  /**
> > > + * drm_connector_attach_max_bpc_property - attach "max bpc" property
> > > + * @connector: connector to attach max bpc property on.
> > > + * @min: The minimum bit depth supported by the connector.
> > > + * @max: The maximum bit depth supported by the connector.
> > > + *
> > > + * This is used to add support for limiting the bit depth on a connector.
> > > + *
> > > + * Returns:
> > > + * Zero on success, negative errno on failure.
> > > + */
> > > +int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> > > +					  int min, int max)
> > > +{
> > > +	struct drm_device *dev = connector->dev;
> > > +	struct drm_property *prop;
> > > +
> > > +	prop = connector->max_bpc_property;
> > > +	if (!prop) {
> > > +		prop = drm_property_create_range(dev, 0, "max bpc", min, max);
> > > +		if (!prop)
> > > +			return -ENOMEM;
> > > +
> > > +		connector->max_bpc_property = prop;
> > > +	}
> > > +
> > > +	drm_object_attach_property(&connector->base, prop, max);
> > > +	connector->state->max_requested_bpc = max;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> > > +
> > > +/**
> > >   * drm_connector_init_panel_orientation_property -
> > >   *	initialize the connecters panel_orientation property
> > >   * @connector: connector for which to init the panel-orientation property.
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 91a877fa00cb..bfc3e698cda6 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -461,6 +461,18 @@ struct drm_connector_state {
> > >  	 * drm_writeback_signal_completion()
> > >  	 */
> > >  	struct drm_writeback_job *writeback_job;
> > > +
> > > +	/**
> > > +	 * @max_requested_bpc: Connector property to limit the maximum bit
> > > +	 * depth of the pixels.
> > > +	 */
> > > +	u8 max_requested_bpc;
> > > +
> > > +	/**
> > > +	 * @max_bpc: Connector max_bpc based on the requested max_bpc property
> > > +	 * and the connector bpc limitations obtained from edid.
> > > +	 */
> > > +	u8 max_bpc;
> > >  };
> > >  
> > >  /**
> > > @@ -924,6 +936,12 @@ struct drm_connector {
> > >  	 */
> > >  	struct drm_property_blob *path_blob_ptr;
> > >  
> > > +	/**
> > > +	 * @max_bpc_property: Default connector property for the max bpc to be
> > > +	 * driven out of the connector.
> > > +	 */
> > > +	struct drm_property *max_bpc_property;
> > > +
> > >  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> > >  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
> > >  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> > > @@ -1201,6 +1219,8 @@ void drm_connector_set_link_status_property(struct drm_connector *connector,
> > >  					    uint64_t link_status);
> > >  int drm_connector_init_panel_orientation_property(
> > >  	struct drm_connector *connector, int width, int height);
> > > +int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
> > > +					  int min, int max);
> > >  
> > >  /**
> > >   * struct drm_tile_group - Tile group metadata
> > > -- 
> > > 2.9.3
> > > 
> > 
> > -- 
> > 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] 10+ messages in thread

end of thread, other threads:[~2018-10-04  7:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-24 21:08 [PATCH v10 1/2] drm: Add connector property to limit max bpc Radhakrishna Sripada
2018-09-24 21:08 ` [PATCH v10 2/2] drm/i915: Allow "max bpc" property to limit pipe_bpp Radhakrishna Sripada
2018-10-01 13:48   ` Ville Syrjälä
2018-10-03 18:42     ` Radhakrishna Sripada
2018-10-03 18:48       ` Ville Syrjälä
2018-09-24 21:36 ` ✗ Fi.CI.SPARSE: warning for series starting with [v10,1/2] drm: Add connector property to limit max bpc Patchwork
2018-09-24 21:53 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-10-01  7:23 ` [PATCH v10 1/2] " Daniel Vetter
2018-10-03 22:24   ` Radhakrishna Sripada
2018-10-04  7:52     ` Daniel Vetter

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.