[PATCH 0/6] Support change dw-hdmi output color

[PATCH 0/6] Support change dw-hdmi output color

[Algea Cao]

Support userspace to set hdmi output color via hdmi properties.
Add hdmi atomic_begin/atomic_flush to make sure screen don't flash
when updating color.


Algea Cao (6):
  drm: Add connector atomic_begin/atomic_flush
  drm: bridge: dw-hdmi: Implement connector atomic_begin/atomic_flush
  drm: bridge: dw-hdmi: Introduce previous_pixelclock/previous_tmdsclock
  drm/rockchip: dw_hdmi: Add vendor hdmi properties
  drm/rockchip: dw_hdmi: Add get_output_bus_format
  drm: bridge: dw-hdmi: Get output bus format when dw-hdmi is the only
    bridge

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 123 ++++++++++-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.h   |   4 +
 drivers/gpu/drm/drm_atomic_helper.c         |  46 ++++
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 221 ++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h                |  23 ++
 include/drm/drm_modeset_helper_vtables.h    |  19 ++
 6 files changed, 428 insertions(+), 8 deletions(-)

-- 
2.25.1



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

[PATCH 1/6] drm: Add connector atomic_begin/atomic_flush

[Algea Cao]

In some situations, connector should get some work done
when plane is updating. Such as when change output color
format, hdmi should send AVMUTE to make screen black before
crtc updating color format, or screen may flash. After color
updating, hdmi should clear AVMUTE bring screen back to normal.

The process is as follows:
AVMUTE -> Update CRTC -> Update HDMI -> Clear AVMUTE

So we introduce connector atomic_begin/atomic_flush.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>

---

 drivers/gpu/drm/drm_atomic_helper.c      | 46 ++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 19 ++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f68c69a45752..f4abd700d2c4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2471,6 +2471,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 				     struct drm_atomic_state *old_state,
 				     uint32_t flags)
 {
+	struct drm_connector *connector;
+	struct drm_connector_state *old_connector_state, *new_connector_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
@@ -2479,6 +2481,28 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 	bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
 	bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
 
+	for_each_oldnew_connector_in_state(old_state, connector,
+					   old_connector_state,
+					   new_connector_state, i) {
+		const struct drm_connector_helper_funcs *funcs;
+
+		if (!connector->state->crtc)
+			continue;
+
+		if (!connector->state->crtc->state->active)
+			continue;
+
+		funcs = connector->helper_private;
+
+		if (!funcs || !funcs->atomic_begin)
+			continue;
+
+		DRM_DEBUG_ATOMIC("flush beginning [CONNECTOR:%d:%s]\n",
+				 connector->base.id, connector->name);
+
+		funcs->atomic_begin(connector, old_connector_state);
+	}
+
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
@@ -2550,6 +2574,28 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 		funcs->atomic_flush(crtc, old_crtc_state);
 	}
+
+	for_each_oldnew_connector_in_state(old_state, connector,
+					   old_connector_state,
+					   new_connector_state, i) {
+		const struct drm_connector_helper_funcs *funcs;
+
+		if (!connector->state->crtc)
+			continue;
+
+		if (!connector->state->crtc->state->active)
+			continue;
+
+		funcs = connector->helper_private;
+
+		if (!funcs || !funcs->atomic_flush)
+			continue;
+
+		DRM_DEBUG_ATOMIC("flushing [CONNECTOR:%d:%s]\n",
+				 connector->base.id, connector->name);
+
+		funcs->atomic_flush(connector, old_connector_state);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
 
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 421a30f08463..10f3f2e2fe28 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1075,6 +1075,25 @@ struct drm_connector_helper_funcs {
 	void (*atomic_commit)(struct drm_connector *connector,
 			      struct drm_connector_state *state);
 
+	/**
+	 * @atomic_begin:
+	 *
+	 * flush atomic update
+	 *
+	 * This callback is used by the atomic modeset helpers but it is optional.
+	 */
+	void (*atomic_begin)(struct drm_connector *connector,
+			     struct drm_connector_state *state);
+
+	/**
+	 * @atomic_begin:
+	 *
+	 * begin atomic update
+	 *
+	 * This callback is used by the atomic modeset helpers but it is optional.
+	 */
+	void (*atomic_flush)(struct drm_connector *connector,
+			     struct drm_connector_state *state);
 	/**
 	 * @prepare_writeback_job:
 	 *
-- 
2.25.1



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

[PATCH 2/6] drm: bridge: dw-hdmi: Implement connector atomic_begin/atomic_flush

[Algea Cao]

Introduce dw_hdmi_connector_atomic_begin() and
dw_hdmi_connector_atomic_flush() to implement connector
atomic_begin/atomic_flush. When enc_out_bus_format or
enc_in_bus_format changed, dw_hdmi_setup is called.

To avoid screen flash when updating bus format, it's need
to send AVMUTE flag to make screen black, and clear flag
after bus format updated.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
---

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 65 +++++++++++++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |  4 ++
 2 files changed, 69 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 6148a022569a..a1a81fc768c2 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -108,6 +108,8 @@ struct hdmi_vmode {
 };
 
 struct hdmi_data_info {
+	unsigned int prev_enc_in_bus_format;
+	unsigned int prev_enc_out_bus_format;
 	unsigned int enc_in_bus_format;
 	unsigned int enc_out_bus_format;
 	unsigned int enc_in_encoding;
@@ -116,6 +118,7 @@ struct hdmi_data_info {
 	unsigned int hdcp_enable;
 	struct hdmi_vmode video_mode;
 	bool rgb_limited_range;
+	bool update;
 };
 
 struct dw_hdmi_i2c {
@@ -2401,6 +2404,60 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 	return ret;
 }
 
+static void
+dw_hdmi_connector_atomic_begin(struct drm_connector *connector,
+			       struct drm_connector_state *conn_state)
+{
+	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
+					    connector);
+	unsigned int enc_in_bus_fmt = hdmi->hdmi_data.enc_in_bus_format;
+	unsigned int enc_out_bus_fmt = hdmi->hdmi_data.enc_out_bus_format;
+	unsigned int prev_enc_in_bus_fmt =
+		hdmi->hdmi_data.prev_enc_in_bus_format;
+	unsigned int prev_enc_out_bus_fmt =
+		hdmi->hdmi_data.prev_enc_out_bus_format;
+
+	if (!conn_state->crtc)
+		return;
+
+	if (!hdmi->hdmi_data.video_mode.mpixelclock)
+		return;
+
+	if (enc_in_bus_fmt != prev_enc_in_bus_fmt ||
+	    enc_out_bus_fmt != prev_enc_out_bus_fmt) {
+		hdmi->hdmi_data.update = true;
+		hdmi_writeb(hdmi, HDMI_FC_GCP_SET_AVMUTE, HDMI_FC_GCP);
+		/* Add delay to make av mute work on sink*/
+		msleep(50);
+	} else {
+		hdmi->hdmi_data.update = false;
+	}
+}
+
+static void
+dw_hdmi_connector_atomic_flush(struct drm_connector *connector,
+			       struct drm_connector_state *conn_state)
+{
+	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
+					     connector);
+
+	if (!conn_state->crtc)
+		return;
+
+	DRM_DEBUG("%s\n", __func__);
+
+	if (hdmi->hdmi_data.update) {
+		dw_hdmi_setup(hdmi, hdmi->curr_conn, &hdmi->previous_mode);
+		/*
+		 * Before clear AVMUTE, delay is needed to
+		 * prevent display flash.
+		 */
+		msleep(50);
+		hdmi_writeb(hdmi, HDMI_FC_GCP_CLEAR_AVMUTE, HDMI_FC_GCP);
+		hdmi->hdmi_data.update = false;
+	}
+}
+
 static bool hdr_metadata_equal(const struct drm_connector_state *old_state,
 			       const struct drm_connector_state *new_state)
 {
@@ -2465,6 +2522,8 @@ static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
 static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
 	.get_modes = dw_hdmi_connector_get_modes,
 	.atomic_check = dw_hdmi_connector_atomic_check,
+	.atomic_begin = dw_hdmi_connector_atomic_begin,
+	.atomic_flush = dw_hdmi_connector_atomic_flush,
 };
 
 static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
@@ -2778,6 +2837,12 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
 {
 	struct dw_hdmi *hdmi = bridge->driver_private;
 
+	hdmi->hdmi_data.prev_enc_out_bus_format =
+			hdmi->hdmi_data.enc_out_bus_format;
+
+	hdmi->hdmi_data.prev_enc_in_bus_format =
+			hdmi->hdmi_data.enc_in_bus_format;
+
 	hdmi->hdmi_data.enc_out_bus_format =
 			bridge_state->output_bus_cfg.format;
 
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 1999db05bc3b..05182418efbb 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -842,6 +842,10 @@ enum {
 	HDMI_FC_AVICONF3_QUANT_RANGE_LIMITED = 0x00,
 	HDMI_FC_AVICONF3_QUANT_RANGE_FULL = 0x04,
 
+/* HDMI_FC_GCP */
+	HDMI_FC_GCP_SET_AVMUTE = 0x2,
+	HDMI_FC_GCP_CLEAR_AVMUTE = 0x1,
+
 /* FC_DBGFORCE field values */
 	HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
 	HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
-- 
2.25.1



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

[PATCH 3/6] drm: bridge: dw-hdmi: Introduce previous_pixelclock/previous_tmdsclock

[Algea Cao]

Introduce previous_pixelclock/previous_tmdsclock to
determine whether PHY needs initialization. If phy is power off,
or mpixelclock/mtmdsclock is different to previous value, phy is
neet to be reinitialized.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
---

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 50 +++++++++++++++++++----
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index a1a81fc768c2..1eb4736b9b59 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -101,6 +101,8 @@ static const u16 csc_coeff_rgb_full_to_rgb_limited[3][4] = {
 struct hdmi_vmode {
 	bool mdataenablepolarity;
 
+	unsigned int previous_pixelclock;
+	unsigned int previous_tmdsclock;
 	unsigned int mpixelclock;
 	unsigned int mpixelrepetitioninput;
 	unsigned int mpixelrepetitionoutput;
@@ -890,6 +892,32 @@ static int hdmi_bus_fmt_color_depth(unsigned int bus_format)
 	}
 }
 
+static unsigned int
+hdmi_get_tmdsclock(struct dw_hdmi *hdmi, unsigned long mpixelclock)
+{
+	unsigned int tmdsclock = mpixelclock;
+	unsigned int depth =
+		hdmi_bus_fmt_color_depth(hdmi->hdmi_data.enc_out_bus_format);
+
+	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {
+		switch (depth) {
+		case 16:
+			tmdsclock = mpixelclock * 2;
+			break;
+		case 12:
+			tmdsclock = mpixelclock * 3 / 2;
+			break;
+		case 10:
+			tmdsclock = mpixelclock * 5 / 4;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return tmdsclock;
+}
+
 /*
  * this submodule is responsible for the video data synchronization.
  * for example, for RGB 4:4:4 input, the data map is defined as
@@ -1861,11 +1889,13 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
 	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
 	unsigned int vdisplay, hdisplay;
 
+	vmode->previous_pixelclock = vmode->mpixelclock;
 	vmode->mpixelclock = mode->clock * 1000;
 
 	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
 
-	vmode->mtmdsclock = vmode->mpixelclock;
+	vmode->previous_tmdsclock = vmode->mtmdsclock;
+	vmode->mtmdsclock = hdmi_get_tmdsclock(hdmi, vmode->mpixelclock);
 
 	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {
 		switch (hdmi_bus_fmt_color_depth(
@@ -2172,12 +2202,18 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
 	hdmi_av_composer(hdmi, &connector->display_info, mode);
 
 	/* HDMI Initializateion Step B.2 */
-	ret = hdmi->phy.ops->init(hdmi, hdmi->phy.data,
-				  &connector->display_info,
-				  &hdmi->previous_mode);
-	if (ret)
-		return ret;
-	hdmi->phy.enabled = true;
+	if (!hdmi->phy.enabled ||
+	    hdmi->hdmi_data.video_mode.previous_pixelclock !=
+	    hdmi->hdmi_data.video_mode.mpixelclock ||
+	    hdmi->hdmi_data.video_mode.previous_tmdsclock !=
+	    hdmi->hdmi_data.video_mode.mtmdsclock) {
+		ret = hdmi->phy.ops->init(hdmi, hdmi->phy.data,
+					  &connector->display_info,
+					  &hdmi->previous_mode);
+		if (ret)
+			return ret;
+		hdmi->phy.enabled = true;
+	}
 
 	/* HDMI Initialization Step B.3 */
 	dw_hdmi_enable_video_path(hdmi);
-- 
2.25.1



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

[PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Algea Cao]

Introduce struct dw_hdmi_property_ops in plat_data to support
vendor hdmi property.

Implement hdmi vendor properties color_depth_property and
hdmi_output_property to config hdmi output color depth and
color format.

The property "hdmi_output_format", the possible value
could be:
         - RGB
         - YCBCR 444
         - YCBCR 422
         - YCBCR 420

Default value of the property is set to 0 = RGB, so no changes if you
don't set the property.

The property "hdmi_output_depth" possible value could be
         - Automatic
           This indicates prefer highest color depth, it is
           30bit on rockcip platform.
         - 24bit
         - 30bit
The default value of property is 24bit.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
---

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h                |  22 +++
 2 files changed, 196 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 23de359a1dec..8f22d9a566db 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -52,6 +52,27 @@
 
 #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
 
+/* HDMI output pixel format */
+enum drm_hdmi_output_type {
+	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
+	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
+	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
+	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
+	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
+	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
+	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
+};
+
+enum dw_hdmi_rockchip_color_depth {
+	ROCKCHIP_HDMI_DEPTH_8,
+	ROCKCHIP_HDMI_DEPTH_10,
+	ROCKCHIP_HDMI_DEPTH_12,
+	ROCKCHIP_HDMI_DEPTH_16,
+	ROCKCHIP_HDMI_DEPTH_420_10,
+	ROCKCHIP_HDMI_DEPTH_420_12,
+	ROCKCHIP_HDMI_DEPTH_420_16
+};
+
 /**
  * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips
  * @lcdsel_grf_reg: grf register offset of lcdc select
@@ -73,6 +94,12 @@ struct rockchip_hdmi {
 	struct clk *grf_clk;
 	struct dw_hdmi *hdmi;
 	struct phy *phy;
+
+	struct drm_property *color_depth_property;
+	struct drm_property *hdmi_output_property;
+
+	unsigned int colordepth;
+	enum drm_hdmi_output_type hdmi_output;
 };
 
 #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
@@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data)
 	phy_power_off(hdmi->phy);
 }
 
+static const struct drm_prop_enum_list color_depth_enum_list[] = {
+	{ 0, "Automatic" }, /* Prefer highest color depth */
+	{ 8, "24bit" },
+	{ 10, "30bit" },
+};
+
+static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = {
+	{ DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" },
+	{ DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" },
+	{ DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" },
+	{ DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" },
+	{ DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" },
+	{ DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" },
+	{ DRM_HDMI_OUTPUT_INVALID, "invalid_output" },
+};
+
+static void
+dw_hdmi_rockchip_attach_properties(struct drm_connector *connector,
+				   unsigned int color, int version,
+				   void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+	struct drm_property *prop;
+
+	switch (color) {
+	case MEDIA_BUS_FMT_RGB101010_1X30:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
+		hdmi->colordepth = 10;
+		break;
+	case MEDIA_BUS_FMT_YUV8_1X24:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
+		hdmi->colordepth = 8;
+		break;
+	case MEDIA_BUS_FMT_YUV10_1X30:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
+		hdmi->colordepth = 10;
+		break;
+	case MEDIA_BUS_FMT_UYVY10_1X20:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
+		hdmi->colordepth = 10;
+		break;
+	case MEDIA_BUS_FMT_UYVY8_1X16:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
+		hdmi->colordepth = 8;
+		break;
+	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
+		hdmi->colordepth = 8;
+		break;
+	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
+		hdmi->colordepth = 10;
+		break;
+	default:
+		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
+		hdmi->colordepth = 8;
+	}
+
+	prop = drm_property_create_enum(connector->dev, 0,
+					"hdmi_output_depth",
+					color_depth_enum_list,
+					ARRAY_SIZE(color_depth_enum_list));
+	if (prop) {
+		hdmi->color_depth_property = prop;
+		drm_object_attach_property(&connector->base, prop, 0);
+	}
+
+	prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format",
+					drm_hdmi_output_enum_list,
+					ARRAY_SIZE(drm_hdmi_output_enum_list));
+	if (prop) {
+		hdmi->hdmi_output_property = prop;
+		drm_object_attach_property(&connector->base, prop, 0);
+	}
+}
+
+static void
+dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector,
+				    void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+
+	if (hdmi->color_depth_property) {
+		drm_property_destroy(connector->dev,
+				     hdmi->color_depth_property);
+		hdmi->color_depth_property = NULL;
+	}
+
+	if (hdmi->hdmi_output_property) {
+		drm_property_destroy(connector->dev,
+				     hdmi->hdmi_output_property);
+		hdmi->hdmi_output_property = NULL;
+	}
+}
+
+static int
+dw_hdmi_rockchip_set_property(struct drm_connector *connector,
+			      struct drm_connector_state *state,
+			      struct drm_property *property,
+			      u64 val,
+			      void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+
+	if (property == hdmi->color_depth_property) {
+		hdmi->colordepth = val;
+		return 0;
+	} else if (property == hdmi->hdmi_output_property) {
+		hdmi->hdmi_output = val;
+		return 0;
+	}
+
+	DRM_ERROR("failed to set rockchip hdmi connector property\n");
+	return -EINVAL;
+}
+
+static int
+dw_hdmi_rockchip_get_property(struct drm_connector *connector,
+			      const struct drm_connector_state *state,
+			      struct drm_property *property,
+			      u64 *val,
+			      void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+
+	if (property == hdmi->color_depth_property) {
+		*val = hdmi->colordepth;
+		return 0;
+	} else if (property == hdmi->hdmi_output_property) {
+		*val = hdmi->hdmi_output;
+		return 0;
+	}
+
+	DRM_ERROR("failed to get rockchip hdmi connector property\n");
+	return -EINVAL;
+}
+
+static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = {
+	.attach_properties	= dw_hdmi_rockchip_attach_properties,
+	.destroy_properties	= dw_hdmi_rockchip_destroy_properties,
+	.set_property		= dw_hdmi_rockchip_set_property,
+	.get_property		= dw_hdmi_rockchip_get_property,
+};
+
 static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
@@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	hdmi->dev = &pdev->dev;
 	hdmi->chip_data = plat_data->phy_data;
 	plat_data->phy_data = hdmi;
+
+	plat_data->property_ops = &dw_hdmi_rockchip_property_ops;
+
 	encoder = &hdmi->encoder;
 
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index ea34ca146b82..dc561ebe7a9b 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -6,6 +6,7 @@
 #ifndef __DW_HDMI__
 #define __DW_HDMI__
 
+#include <drm/drm_property.h>
 #include <sound/hdmi-codec.h>
 
 struct drm_display_info;
@@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops {
 	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
 };
 
+struct dw_hdmi_property_ops {
+	void (*attach_properties)(struct drm_connector *connector,
+				  unsigned int color, int version,
+				  void *data);
+	void (*destroy_properties)(struct drm_connector *connector,
+				   void *data);
+	int (*set_property)(struct drm_connector *connector,
+			    struct drm_connector_state *state,
+			    struct drm_property *property,
+			    u64 val,
+			    void *data);
+	int (*get_property)(struct drm_connector *connector,
+			    const struct drm_connector_state *state,
+			    struct drm_property *property,
+			    u64 *val,
+			    void *data);
+};
+
 struct dw_hdmi_plat_data {
 	struct regmap *regm;
 
@@ -141,6 +160,9 @@ struct dw_hdmi_plat_data {
 					   const struct drm_display_info *info,
 					   const struct drm_display_mode *mode);
 
+	/* Vendor Property support */
+	const struct dw_hdmi_property_ops *property_ops;
+
 	/* Vendor PHY support */
 	const struct dw_hdmi_phy_ops *phy_ops;
 	const char *phy_name;
-- 
2.25.1



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

[PATCH 5/6] drm/rockchip: dw_hdmi: Add get_output_bus_format

[Algea Cao]

Add get_output_bus_format in dw_hdmi_plat_data to get
hdmi output bus format. The output bus format is determined
by color format and depth, which can be set by rockchip
hdmi properties.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
---

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 47 +++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h                |  1 +
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 8f22d9a566db..a602d25639a7 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -100,6 +100,7 @@ struct rockchip_hdmi {
 
 	unsigned int colordepth;
 	enum drm_hdmi_output_type hdmi_output;
+	unsigned long output_bus_format;
 };
 
 #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
@@ -498,6 +499,50 @@ static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = {
 	.get_property		= dw_hdmi_rockchip_get_property,
 };
 
+static void
+dw_hdmi_rockchip_output_bus_format_select(struct rockchip_hdmi *hdmi)
+{
+	unsigned long bus_format;
+
+	if (hdmi->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420) {
+		if (hdmi->colordepth > 8)
+			bus_format = MEDIA_BUS_FMT_UYYVYY10_0_5X30;
+		else
+			bus_format = MEDIA_BUS_FMT_UYYVYY8_0_5X24;
+	} else if (hdmi->hdmi_output == DRM_HDMI_OUTPUT_YCBCR422) {
+		if (hdmi->colordepth == 12)
+			bus_format = MEDIA_BUS_FMT_UYVY12_1X24;
+		else if (hdmi->colordepth == 10)
+			bus_format = MEDIA_BUS_FMT_UYVY10_1X20;
+		else
+			bus_format = MEDIA_BUS_FMT_UYVY8_1X16;
+	} else {
+		if (hdmi->colordepth > 8) {
+			if (hdmi->hdmi_output != DRM_HDMI_OUTPUT_DEFAULT_RGB)
+				bus_format = MEDIA_BUS_FMT_YUV10_1X30;
+			else
+				bus_format = MEDIA_BUS_FMT_RGB101010_1X30;
+		} else {
+			if (hdmi->hdmi_output != DRM_HDMI_OUTPUT_DEFAULT_RGB)
+				bus_format = MEDIA_BUS_FMT_YUV8_1X24;
+			else
+				bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+		}
+	}
+
+	hdmi->output_bus_format = bus_format;
+}
+
+static unsigned long
+dw_hdmi_rockchip_get_output_bus_format(void *data)
+{
+	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
+
+	dw_hdmi_rockchip_output_bus_format_select(hdmi);
+
+	return hdmi->output_bus_format;
+}
+
 static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data)
 {
 	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
@@ -685,6 +730,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 
 	plat_data->property_ops = &dw_hdmi_rockchip_property_ops;
 
+	plat_data->get_output_bus_format =
+		dw_hdmi_rockchip_get_output_bus_format;
 	encoder = &hdmi->encoder;
 
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index dc561ebe7a9b..13495f810328 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -175,6 +175,7 @@ struct dw_hdmi_plat_data {
 	const struct dw_hdmi_phy_config *phy_config;
 	int (*configure_phy)(struct dw_hdmi *hdmi, void *data,
 			     unsigned long mpixelclock);
+	unsigned long (*get_output_bus_format)(void *data);
 };
 
 struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
-- 
2.25.1



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

[PATCH 6/6] drm: bridge: dw-hdmi: Get output bus format when dw-hdmi is the only bridge

[Algea Cao]

If plat_data->get_output_bus_format() is exist, we can
use it to get hdmi output bus format when dw-hdmi is the
only bridge. The hdmi output bus format can be set by vendor
properties.

Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
---

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 1eb4736b9b59..878e9e506963 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2644,6 +2644,8 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
 					unsigned int *num_output_fmts)
 {
 	struct drm_connector *conn = conn_state->connector;
+	struct dw_hdmi *hdmi = bridge->driver_private;
+	void *data = hdmi->plat_data->phy_data;
 	struct drm_display_info *info = &conn->display_info;
 	struct drm_display_mode *mode = &crtc_state->mode;
 	u8 max_bpc = conn_state->max_requested_bpc;
@@ -2662,7 +2664,11 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
 	/* If dw-hdmi is the only bridge, avoid negociating with ourselves */
 	if (list_is_singular(&bridge->encoder->bridge_chain)) {
 		*num_output_fmts = 1;
-		output_fmts[0] = MEDIA_BUS_FMT_FIXED;
+		if (hdmi->plat_data->get_output_bus_format)
+			output_fmts[0] =
+				hdmi->plat_data->get_output_bus_format(data);
+		else
+			output_fmts[0] = MEDIA_BUS_FMT_FIXED;
 
 		return output_fmts;
 	}
-- 
2.25.1



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

Re: [PATCH 2/6] drm: bridge: dw-hdmi: Implement connector atomic_begin/atomic_flush

[Laurent Pinchart]

Hi Algea,

Thank you for the patch.

On Wed, Aug 12, 2020 at 04:34:33PM +0800, Algea Cao wrote:
> Introduce dw_hdmi_connector_atomic_begin() and
> dw_hdmi_connector_atomic_flush() to implement connector
> atomic_begin/atomic_flush. When enc_out_bus_format or
> enc_in_bus_format changed, dw_hdmi_setup is called.
> 
> To avoid screen flash when updating bus format, it's need
> to send AVMUTE flag to make screen black, and clear flag
> after bus format updated.
> 
> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 65 +++++++++++++++++++++++
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |  4 ++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 6148a022569a..a1a81fc768c2 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -108,6 +108,8 @@ struct hdmi_vmode {
>  };
>  
>  struct hdmi_data_info {
> +	unsigned int prev_enc_in_bus_format;
> +	unsigned int prev_enc_out_bus_format;
>  	unsigned int enc_in_bus_format;
>  	unsigned int enc_out_bus_format;
>  	unsigned int enc_in_encoding;
> @@ -116,6 +118,7 @@ struct hdmi_data_info {
>  	unsigned int hdcp_enable;
>  	struct hdmi_vmode video_mode;
>  	bool rgb_limited_range;
> +	bool update;
>  };
>  
>  struct dw_hdmi_i2c {
> @@ -2401,6 +2404,60 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>  	return ret;
>  }
>  
> +static void
> +dw_hdmi_connector_atomic_begin(struct drm_connector *connector,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> +					    connector);
> +	unsigned int enc_in_bus_fmt = hdmi->hdmi_data.enc_in_bus_format;
> +	unsigned int enc_out_bus_fmt = hdmi->hdmi_data.enc_out_bus_format;
> +	unsigned int prev_enc_in_bus_fmt =
> +		hdmi->hdmi_data.prev_enc_in_bus_format;
> +	unsigned int prev_enc_out_bus_fmt =
> +		hdmi->hdmi_data.prev_enc_out_bus_format;
> +
> +	if (!conn_state->crtc)
> +		return;
> +
> +	if (!hdmi->hdmi_data.video_mode.mpixelclock)
> +		return;
> +
> +	if (enc_in_bus_fmt != prev_enc_in_bus_fmt ||
> +	    enc_out_bus_fmt != prev_enc_out_bus_fmt) {
> +		hdmi->hdmi_data.update = true;
> +		hdmi_writeb(hdmi, HDMI_FC_GCP_SET_AVMUTE, HDMI_FC_GCP);
> +		/* Add delay to make av mute work on sink*/
> +		msleep(50);
> +	} else {
> +		hdmi->hdmi_data.update = false;
> +	}
> +}
> +
> +static void
> +dw_hdmi_connector_atomic_flush(struct drm_connector *connector,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> +					     connector);
> +
> +	if (!conn_state->crtc)
> +		return;
> +
> +	DRM_DEBUG("%s\n", __func__);
> +
> +	if (hdmi->hdmi_data.update) {
> +		dw_hdmi_setup(hdmi, hdmi->curr_conn, &hdmi->previous_mode);
> +		/*
> +		 * Before clear AVMUTE, delay is needed to
> +		 * prevent display flash.
> +		 */
> +		msleep(50);
> +		hdmi_writeb(hdmi, HDMI_FC_GCP_CLEAR_AVMUTE, HDMI_FC_GCP);
> +		hdmi->hdmi_data.update = false;
> +	}
> +}
> +
>  static bool hdr_metadata_equal(const struct drm_connector_state *old_state,
>  			       const struct drm_connector_state *new_state)
>  {
> @@ -2465,6 +2522,8 @@ static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
>  static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
>  	.get_modes = dw_hdmi_connector_get_modes,
>  	.atomic_check = dw_hdmi_connector_atomic_check,
> +	.atomic_begin = dw_hdmi_connector_atomic_begin,
> +	.atomic_flush = dw_hdmi_connector_atomic_flush,
>  };
>  
>  static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
> @@ -2778,6 +2837,12 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
>  {
>  	struct dw_hdmi *hdmi = bridge->driver_private;
>  
> +	hdmi->hdmi_data.prev_enc_out_bus_format =
> +			hdmi->hdmi_data.enc_out_bus_format;
> +
> +	hdmi->hdmi_data.prev_enc_in_bus_format =
> +			hdmi->hdmi_data.enc_in_bus_format;
> +
>  	hdmi->hdmi_data.enc_out_bus_format =
>  			bridge_state->output_bus_cfg.format;
>  

.atomic_check() isn't allowed to change the device state, neither the
hardware state, nor the software state stored in struct dw_hdmi. You
essentially need to treat the drm_bridge and dw_hdmi as const in the
.atomic_check() operation.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 1999db05bc3b..05182418efbb 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -842,6 +842,10 @@ enum {
>  	HDMI_FC_AVICONF3_QUANT_RANGE_LIMITED = 0x00,
>  	HDMI_FC_AVICONF3_QUANT_RANGE_FULL = 0x04,
>  
> +/* HDMI_FC_GCP */
> +	HDMI_FC_GCP_SET_AVMUTE = 0x2,
> +	HDMI_FC_GCP_CLEAR_AVMUTE = 0x1,
> +
>  /* FC_DBGFORCE field values */
>  	HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
>  	HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,

-- 
Regards,

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

Re: [PATCH 1/6] drm: Add connector atomic_begin/atomic_flush

[Laurent Pinchart]

Hi Algea,

Thank you for the patch.

On Wed, Aug 12, 2020 at 04:34:07PM +0800, Algea Cao wrote:
> In some situations, connector should get some work done
> when plane is updating. Such as when change output color
> format, hdmi should send AVMUTE to make screen black before
> crtc updating color format, or screen may flash. After color
> updating, hdmi should clear AVMUTE bring screen back to normal.
> 
> The process is as follows:
> AVMUTE -> Update CRTC -> Update HDMI -> Clear AVMUTE
> 
> So we introduce connector atomic_begin/atomic_flush.

Implementing this through .atomic_begin() and .atomic_flush() seems like
a pretty big hack to me. Furthermore, I think this should be implemented
as bridge operations, not at the connector level, as the HDMI encoder
may not be the component that implements the drm_connector.

> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> 
> ---
> 
>  drivers/gpu/drm/drm_atomic_helper.c      | 46 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 19 ++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f68c69a45752..f4abd700d2c4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2471,6 +2471,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  				     struct drm_atomic_state *old_state,
>  				     uint32_t flags)
>  {
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_connector_state, *new_connector_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_plane *plane;
> @@ -2479,6 +2481,28 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  	bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
>  	bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
>  
> +	for_each_oldnew_connector_in_state(old_state, connector,
> +					   old_connector_state,
> +					   new_connector_state, i) {
> +		const struct drm_connector_helper_funcs *funcs;
> +
> +		if (!connector->state->crtc)
> +			continue;
> +
> +		if (!connector->state->crtc->state->active)
> +			continue;
> +
> +		funcs = connector->helper_private;
> +
> +		if (!funcs || !funcs->atomic_begin)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("flush beginning [CONNECTOR:%d:%s]\n",
> +				 connector->base.id, connector->name);
> +
> +		funcs->atomic_begin(connector, old_connector_state);
> +	}
> +
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
>  
> @@ -2550,6 +2574,28 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  
>  		funcs->atomic_flush(crtc, old_crtc_state);
>  	}
> +
> +	for_each_oldnew_connector_in_state(old_state, connector,
> +					   old_connector_state,
> +					   new_connector_state, i) {
> +		const struct drm_connector_helper_funcs *funcs;
> +
> +		if (!connector->state->crtc)
> +			continue;
> +
> +		if (!connector->state->crtc->state->active)
> +			continue;
> +
> +		funcs = connector->helper_private;
> +
> +		if (!funcs || !funcs->atomic_flush)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("flushing [CONNECTOR:%d:%s]\n",
> +				 connector->base.id, connector->name);
> +
> +		funcs->atomic_flush(connector, old_connector_state);
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
>  
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 421a30f08463..10f3f2e2fe28 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1075,6 +1075,25 @@ struct drm_connector_helper_funcs {
>  	void (*atomic_commit)(struct drm_connector *connector,
>  			      struct drm_connector_state *state);
>  
> +	/**
> +	 * @atomic_begin:
> +	 *
> +	 * flush atomic update
> +	 *
> +	 * This callback is used by the atomic modeset helpers but it is optional.
> +	 */
> +	void (*atomic_begin)(struct drm_connector *connector,
> +			     struct drm_connector_state *state);
> +
> +	/**
> +	 * @atomic_begin:
> +	 *
> +	 * begin atomic update
> +	 *
> +	 * This callback is used by the atomic modeset helpers but it is optional.
> +	 */
> +	void (*atomic_flush)(struct drm_connector *connector,
> +			     struct drm_connector_state *state);
>  	/**
>  	 * @prepare_writeback_job:
>  	 *

-- 
Regards,

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

Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Laurent Pinchart]

Hi Algea,

Thank you for the patch.

On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:
> Introduce struct dw_hdmi_property_ops in plat_data to support
> vendor hdmi property.
> 
> Implement hdmi vendor properties color_depth_property and
> hdmi_output_property to config hdmi output color depth and
> color format.
> 
> The property "hdmi_output_format", the possible value
> could be:
>          - RGB
>          - YCBCR 444
>          - YCBCR 422
>          - YCBCR 420
> 
> Default value of the property is set to 0 = RGB, so no changes if you
> don't set the property.
> 
> The property "hdmi_output_depth" possible value could be
>          - Automatic
>            This indicates prefer highest color depth, it is
>            30bit on rockcip platform.
>          - 24bit
>          - 30bit
> The default value of property is 24bit.
> 
> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> ---
> 
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
>  include/drm/bridge/dw_hdmi.h                |  22 +++
>  2 files changed, 196 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 23de359a1dec..8f22d9a566db 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -52,6 +52,27 @@
>  
>  #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
>  
> +/* HDMI output pixel format */
> +enum drm_hdmi_output_type {
> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> +};

Vendor-specific properties shouldn't use names starting with drm_ or
DRM_, that's for the DRM core. But this doesn't seem specific to
Rockchip at all, it should be a standard property. Additionally, new
properties need to come with a userspace implementation showing their
usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
relevant upstream project (a test tool is usually not enough).

> +
> +enum dw_hdmi_rockchip_color_depth {
> +	ROCKCHIP_HDMI_DEPTH_8,
> +	ROCKCHIP_HDMI_DEPTH_10,
> +	ROCKCHIP_HDMI_DEPTH_12,
> +	ROCKCHIP_HDMI_DEPTH_16,
> +	ROCKCHIP_HDMI_DEPTH_420_10,
> +	ROCKCHIP_HDMI_DEPTH_420_12,
> +	ROCKCHIP_HDMI_DEPTH_420_16
> +};
> +
>  /**
>   * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips
>   * @lcdsel_grf_reg: grf register offset of lcdc select
> @@ -73,6 +94,12 @@ struct rockchip_hdmi {
>  	struct clk *grf_clk;
>  	struct dw_hdmi *hdmi;
>  	struct phy *phy;
> +
> +	struct drm_property *color_depth_property;
> +	struct drm_property *hdmi_output_property;
> +
> +	unsigned int colordepth;
> +	enum drm_hdmi_output_type hdmi_output;
>  };
>  
>  #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
> @@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data)
>  	phy_power_off(hdmi->phy);
>  }
>  
> +static const struct drm_prop_enum_list color_depth_enum_list[] = {
> +	{ 0, "Automatic" }, /* Prefer highest color depth */
> +	{ 8, "24bit" },
> +	{ 10, "30bit" },
> +};
> +
> +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = {
> +	{ DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" },
> +	{ DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" },
> +	{ DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" },
> +	{ DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" },
> +	{ DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" },
> +	{ DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" },
> +	{ DRM_HDMI_OUTPUT_INVALID, "invalid_output" },
> +};
> +
> +static void
> +dw_hdmi_rockchip_attach_properties(struct drm_connector *connector,
> +				   unsigned int color, int version,
> +				   void *data)
> +{
> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> +	struct drm_property *prop;
> +
> +	switch (color) {
> +	case MEDIA_BUS_FMT_RGB101010_1X30:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
> +		hdmi->colordepth = 10;
> +		break;
> +	case MEDIA_BUS_FMT_YUV8_1X24:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
> +		hdmi->colordepth = 8;
> +		break;
> +	case MEDIA_BUS_FMT_YUV10_1X30:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
> +		hdmi->colordepth = 10;
> +		break;
> +	case MEDIA_BUS_FMT_UYVY10_1X20:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
> +		hdmi->colordepth = 10;
> +		break;
> +	case MEDIA_BUS_FMT_UYVY8_1X16:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
> +		hdmi->colordepth = 8;
> +		break;
> +	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
> +		hdmi->colordepth = 8;
> +		break;
> +	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
> +		hdmi->colordepth = 10;
> +		break;
> +	default:
> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
> +		hdmi->colordepth = 8;
> +	}
> +
> +	prop = drm_property_create_enum(connector->dev, 0,
> +					"hdmi_output_depth",
> +					color_depth_enum_list,
> +					ARRAY_SIZE(color_depth_enum_list));
> +	if (prop) {
> +		hdmi->color_depth_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +	}
> +
> +	prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format",
> +					drm_hdmi_output_enum_list,
> +					ARRAY_SIZE(drm_hdmi_output_enum_list));
> +	if (prop) {
> +		hdmi->hdmi_output_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +	}
> +}
> +
> +static void
> +dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector,
> +				    void *data)
> +{
> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> +
> +	if (hdmi->color_depth_property) {
> +		drm_property_destroy(connector->dev,
> +				     hdmi->color_depth_property);
> +		hdmi->color_depth_property = NULL;
> +	}
> +
> +	if (hdmi->hdmi_output_property) {
> +		drm_property_destroy(connector->dev,
> +				     hdmi->hdmi_output_property);
> +		hdmi->hdmi_output_property = NULL;
> +	}
> +}
> +
> +static int
> +dw_hdmi_rockchip_set_property(struct drm_connector *connector,
> +			      struct drm_connector_state *state,
> +			      struct drm_property *property,
> +			      u64 val,
> +			      void *data)
> +{
> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> +
> +	if (property == hdmi->color_depth_property) {
> +		hdmi->colordepth = val;
> +		return 0;
> +	} else if (property == hdmi->hdmi_output_property) {
> +		hdmi->hdmi_output = val;
> +		return 0;
> +	}
> +
> +	DRM_ERROR("failed to set rockchip hdmi connector property\n");
> +	return -EINVAL;
> +}
> +
> +static int
> +dw_hdmi_rockchip_get_property(struct drm_connector *connector,
> +			      const struct drm_connector_state *state,
> +			      struct drm_property *property,
> +			      u64 *val,
> +			      void *data)
> +{
> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> +
> +	if (property == hdmi->color_depth_property) {
> +		*val = hdmi->colordepth;
> +		return 0;
> +	} else if (property == hdmi->hdmi_output_property) {
> +		*val = hdmi->hdmi_output;
> +		return 0;
> +	}
> +
> +	DRM_ERROR("failed to get rockchip hdmi connector property\n");
> +	return -EINVAL;
> +}
> +
> +static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = {
> +	.attach_properties	= dw_hdmi_rockchip_attach_properties,
> +	.destroy_properties	= dw_hdmi_rockchip_destroy_properties,
> +	.set_property		= dw_hdmi_rockchip_set_property,
> +	.get_property		= dw_hdmi_rockchip_get_property,
> +};
> +
>  static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data)
>  {
>  	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> @@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	hdmi->dev = &pdev->dev;
>  	hdmi->chip_data = plat_data->phy_data;
>  	plat_data->phy_data = hdmi;
> +
> +	plat_data->property_ops = &dw_hdmi_rockchip_property_ops;
> +
>  	encoder = &hdmi->encoder;
>  
>  	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index ea34ca146b82..dc561ebe7a9b 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -6,6 +6,7 @@
>  #ifndef __DW_HDMI__
>  #define __DW_HDMI__
>  
> +#include <drm/drm_property.h>
>  #include <sound/hdmi-codec.h>
>  
>  struct drm_display_info;
> @@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops {
>  	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
>  };
>  
> +struct dw_hdmi_property_ops {
> +	void (*attach_properties)(struct drm_connector *connector,
> +				  unsigned int color, int version,
> +				  void *data);
> +	void (*destroy_properties)(struct drm_connector *connector,
> +				   void *data);
> +	int (*set_property)(struct drm_connector *connector,
> +			    struct drm_connector_state *state,
> +			    struct drm_property *property,
> +			    u64 val,
> +			    void *data);
> +	int (*get_property)(struct drm_connector *connector,
> +			    const struct drm_connector_state *state,
> +			    struct drm_property *property,
> +			    u64 *val,
> +			    void *data);
> +};
> +
>  struct dw_hdmi_plat_data {
>  	struct regmap *regm;
>  
> @@ -141,6 +160,9 @@ struct dw_hdmi_plat_data {
>  					   const struct drm_display_info *info,
>  					   const struct drm_display_mode *mode);
>  
> +	/* Vendor Property support */
> +	const struct dw_hdmi_property_ops *property_ops;
> +
>  	/* Vendor PHY support */
>  	const struct dw_hdmi_phy_ops *phy_ops;
>  	const char *phy_name;

-- 
Regards,

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

Re: [PATCH 1/6] drm: Add connector atomic_begin/atomic_flush

[kernel test robot]

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

Hi Algea,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on linus/master drm-exynos/exynos-drm-next v5.8 next-20200812]
[cannot apply to rockchip/for-next drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Algea-Cao/Support-change-dw-hdmi-output-color/20200812-164647
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: m68k-randconfig-s032-20200812 (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-168-g9554805c-dirty
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/err.h:5,
                    from include/linux/dma-fence.h:16,
                    from drivers/gpu/drm/drm_atomic_helper.c:28:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   drivers/gpu/drm/drm_atomic_helper.c: In function 'drm_atomic_helper_commit_planes':
>> drivers/gpu/drm/drm_atomic_helper.c:2475:52: warning: variable 'new_connector_state' set but not used [-Wunused-but-set-variable]
    2475 |  struct drm_connector_state *old_connector_state, *new_connector_state;
         |                                                    ^~~~~~~~~~~~~~~~~~~

vim +/new_connector_state +2475 drivers/gpu/drm/drm_atomic_helper.c

  2428	
  2429	/**
  2430	 * drm_atomic_helper_commit_planes - commit plane state
  2431	 * @dev: DRM device
  2432	 * @old_state: atomic state object with old state structures
  2433	 * @flags: flags for committing plane state
  2434	 *
  2435	 * This function commits the new plane state using the plane and atomic helper
  2436	 * functions for planes and CRTCs. It assumes that the atomic state has already
  2437	 * been pushed into the relevant object state pointers, since this step can no
  2438	 * longer fail.
  2439	 *
  2440	 * It still requires the global state object @old_state to know which planes and
  2441	 * crtcs need to be updated though.
  2442	 *
  2443	 * Note that this function does all plane updates across all CRTCs in one step.
  2444	 * If the hardware can't support this approach look at
  2445	 * drm_atomic_helper_commit_planes_on_crtc() instead.
  2446	 *
  2447	 * Plane parameters can be updated by applications while the associated CRTC is
  2448	 * disabled. The DRM/KMS core will store the parameters in the plane state,
  2449	 * which will be available to the driver when the CRTC is turned on. As a result
  2450	 * most drivers don't need to be immediately notified of plane updates for a
  2451	 * disabled CRTC.
  2452	 *
  2453	 * Unless otherwise needed, drivers are advised to set the ACTIVE_ONLY flag in
  2454	 * @flags in order not to receive plane update notifications related to a
  2455	 * disabled CRTC. This avoids the need to manually ignore plane updates in
  2456	 * driver code when the driver and/or hardware can't or just don't need to deal
  2457	 * with updates on disabled CRTCs, for example when supporting runtime PM.
  2458	 *
  2459	 * Drivers may set the NO_DISABLE_AFTER_MODESET flag in @flags if the relevant
  2460	 * display controllers require to disable a CRTC's planes when the CRTC is
  2461	 * disabled. This function would skip the &drm_plane_helper_funcs.atomic_disable
  2462	 * call for a plane if the CRTC of the old plane state needs a modesetting
  2463	 * operation. Of course, the drivers need to disable the planes in their CRTC
  2464	 * disable callbacks since no one else would do that.
  2465	 *
  2466	 * The drm_atomic_helper_commit() default implementation doesn't set the
  2467	 * ACTIVE_ONLY flag to most closely match the behaviour of the legacy helpers.
  2468	 * This should not be copied blindly by drivers.
  2469	 */
  2470	void drm_atomic_helper_commit_planes(struct drm_device *dev,
  2471					     struct drm_atomic_state *old_state,
  2472					     uint32_t flags)
  2473	{
  2474		struct drm_connector *connector;
> 2475		struct drm_connector_state *old_connector_state, *new_connector_state;
  2476		struct drm_crtc *crtc;
  2477		struct drm_crtc_state *old_crtc_state, *new_crtc_state;
  2478		struct drm_plane *plane;
  2479		struct drm_plane_state *old_plane_state, *new_plane_state;
  2480		int i;
  2481		bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
  2482		bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
  2483	
  2484		for_each_oldnew_connector_in_state(old_state, connector,
  2485						   old_connector_state,
  2486						   new_connector_state, i) {
  2487			const struct drm_connector_helper_funcs *funcs;
  2488	
  2489			if (!connector->state->crtc)
  2490				continue;
  2491	
  2492			if (!connector->state->crtc->state->active)
  2493				continue;
  2494	
  2495			funcs = connector->helper_private;
  2496	
  2497			if (!funcs || !funcs->atomic_begin)
  2498				continue;
  2499	
  2500			DRM_DEBUG_ATOMIC("flush beginning [CONNECTOR:%d:%s]\n",
  2501					 connector->base.id, connector->name);
  2502	
  2503			funcs->atomic_begin(connector, old_connector_state);
  2504		}
  2505	
  2506		for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
  2507			const struct drm_crtc_helper_funcs *funcs;
  2508	
  2509			funcs = crtc->helper_private;
  2510	
  2511			if (!funcs || !funcs->atomic_begin)
  2512				continue;
  2513	
  2514			if (active_only && !new_crtc_state->active)
  2515				continue;
  2516	
  2517			funcs->atomic_begin(crtc, old_crtc_state);
  2518		}
  2519	
  2520		for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
  2521			const struct drm_plane_helper_funcs *funcs;
  2522			bool disabling;
  2523	
  2524			funcs = plane->helper_private;
  2525	
  2526			if (!funcs)
  2527				continue;
  2528	
  2529			disabling = drm_atomic_plane_disabling(old_plane_state,
  2530							       new_plane_state);
  2531	
  2532			if (active_only) {
  2533				/*
  2534				 * Skip planes related to inactive CRTCs. If the plane
  2535				 * is enabled use the state of the current CRTC. If the
  2536				 * plane is being disabled use the state of the old
  2537				 * CRTC to avoid skipping planes being disabled on an
  2538				 * active CRTC.
  2539				 */
  2540				if (!disabling && !plane_crtc_active(new_plane_state))
  2541					continue;
  2542				if (disabling && !plane_crtc_active(old_plane_state))
  2543					continue;
  2544			}
  2545	
  2546			/*
  2547			 * Special-case disabling the plane if drivers support it.
  2548			 */
  2549			if (disabling && funcs->atomic_disable) {
  2550				struct drm_crtc_state *crtc_state;
  2551	
  2552				crtc_state = old_plane_state->crtc->state;
  2553	
  2554				if (drm_atomic_crtc_needs_modeset(crtc_state) &&
  2555				    no_disable)
  2556					continue;
  2557	
  2558				funcs->atomic_disable(plane, old_plane_state);
  2559			} else if (new_plane_state->crtc || disabling) {
  2560				funcs->atomic_update(plane, old_plane_state);
  2561			}
  2562		}
  2563	
  2564		for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
  2565			const struct drm_crtc_helper_funcs *funcs;
  2566	
  2567			funcs = crtc->helper_private;
  2568	
  2569			if (!funcs || !funcs->atomic_flush)
  2570				continue;
  2571	
  2572			if (active_only && !new_crtc_state->active)
  2573				continue;
  2574	
  2575			funcs->atomic_flush(crtc, old_crtc_state);
  2576		}
  2577	
  2578		for_each_oldnew_connector_in_state(old_state, connector,
  2579						   old_connector_state,
  2580						   new_connector_state, i) {
  2581			const struct drm_connector_helper_funcs *funcs;
  2582	
  2583			if (!connector->state->crtc)
  2584				continue;
  2585	
  2586			if (!connector->state->crtc->state->active)
  2587				continue;
  2588	
  2589			funcs = connector->helper_private;
  2590	
  2591			if (!funcs || !funcs->atomic_flush)
  2592				continue;
  2593	
  2594			DRM_DEBUG_ATOMIC("flushing [CONNECTOR:%d:%s]\n",
  2595					 connector->base.id, connector->name);
  2596	
  2597			funcs->atomic_flush(connector, old_connector_state);
  2598		}
  2599	}
  2600	EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
  2601	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22464 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

Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[crj]

[-- Attachment #1.1: Type: text/plain, Size: 10395 bytes --]

Hi Lauren,

Thank you for your review.

在 2020/8/12 17:33, Laurent Pinchart 写道:
> Hi Algea,
>
> Thank you for the patch.
>
> On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:
>> Introduce struct dw_hdmi_property_ops in plat_data to support
>> vendor hdmi property.
>>
>> Implement hdmi vendor properties color_depth_property and
>> hdmi_output_property to config hdmi output color depth and
>> color format.
>>
>> The property "hdmi_output_format", the possible value
>> could be:
>>           - RGB
>>           - YCBCR 444
>>           - YCBCR 422
>>           - YCBCR 420
>>
>> Default value of the property is set to 0 = RGB, so no changes if you
>> don't set the property.
>>
>> The property "hdmi_output_depth" possible value could be
>>           - Automatic
>>             This indicates prefer highest color depth, it is
>>             30bit on rockcip platform.
>>           - 24bit
>>           - 30bit
>> The default value of property is 24bit.
>>
>> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
>> ---
>>
>>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
>>   include/drm/bridge/dw_hdmi.h                |  22 +++
>>   2 files changed, 196 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> index 23de359a1dec..8f22d9a566db 100644
>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> @@ -52,6 +52,27 @@
>>   
>>   #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
>>   
>> +/* HDMI output pixel format */
>> +enum drm_hdmi_output_type {
>> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
>> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
>> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
>> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
>> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
>> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
>> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
>> +};
> Vendor-specific properties shouldn't use names starting with drm_ or
> DRM_, that's for the DRM core. But this doesn't seem specific to
> Rockchip at all, it should be a standard property. Additionally, new
> properties need to come with a userspace implementation showing their
> usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> relevant upstream project (a test tool is usually not enough).


We use these properties only in Android HW composer, But we can't upstream

our HW composer code right now.  Can we use this properties as private 
property

and do not upstream HW composer for the time being?


>> +
>> +enum dw_hdmi_rockchip_color_depth {
>> +	ROCKCHIP_HDMI_DEPTH_8,
>> +	ROCKCHIP_HDMI_DEPTH_10,
>> +	ROCKCHIP_HDMI_DEPTH_12,
>> +	ROCKCHIP_HDMI_DEPTH_16,
>> +	ROCKCHIP_HDMI_DEPTH_420_10,
>> +	ROCKCHIP_HDMI_DEPTH_420_12,
>> +	ROCKCHIP_HDMI_DEPTH_420_16
>> +};
>> +
>>   /**
>>    * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips
>>    * @lcdsel_grf_reg: grf register offset of lcdc select
>> @@ -73,6 +94,12 @@ struct rockchip_hdmi {
>>   	struct clk *grf_clk;
>>   	struct dw_hdmi *hdmi;
>>   	struct phy *phy;
>> +
>> +	struct drm_property *color_depth_property;
>> +	struct drm_property *hdmi_output_property;
>> +
>> +	unsigned int colordepth;
>> +	enum drm_hdmi_output_type hdmi_output;
>>   };
>>   
>>   #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
>> @@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data)
>>   	phy_power_off(hdmi->phy);
>>   }
>>   
>> +static const struct drm_prop_enum_list color_depth_enum_list[] = {
>> +	{ 0, "Automatic" }, /* Prefer highest color depth */
>> +	{ 8, "24bit" },
>> +	{ 10, "30bit" },
>> +};
>> +
>> +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = {
>> +	{ DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" },
>> +	{ DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" },
>> +	{ DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" },
>> +	{ DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" },
>> +	{ DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" },
>> +	{ DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" },
>> +	{ DRM_HDMI_OUTPUT_INVALID, "invalid_output" },
>> +};
>> +
>> +static void
>> +dw_hdmi_rockchip_attach_properties(struct drm_connector *connector,
>> +				   unsigned int color, int version,
>> +				   void *data)
>> +{
>> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
>> +	struct drm_property *prop;
>> +
>> +	switch (color) {
>> +	case MEDIA_BUS_FMT_RGB101010_1X30:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
>> +		hdmi->colordepth = 10;
>> +		break;
>> +	case MEDIA_BUS_FMT_YUV8_1X24:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
>> +		hdmi->colordepth = 8;
>> +		break;
>> +	case MEDIA_BUS_FMT_YUV10_1X30:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
>> +		hdmi->colordepth = 10;
>> +		break;
>> +	case MEDIA_BUS_FMT_UYVY10_1X20:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
>> +		hdmi->colordepth = 10;
>> +		break;
>> +	case MEDIA_BUS_FMT_UYVY8_1X16:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
>> +		hdmi->colordepth = 8;
>> +		break;
>> +	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
>> +		hdmi->colordepth = 8;
>> +		break;
>> +	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
>> +		hdmi->colordepth = 10;
>> +		break;
>> +	default:
>> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
>> +		hdmi->colordepth = 8;
>> +	}
>> +
>> +	prop = drm_property_create_enum(connector->dev, 0,
>> +					"hdmi_output_depth",
>> +					color_depth_enum_list,
>> +					ARRAY_SIZE(color_depth_enum_list));
>> +	if (prop) {
>> +		hdmi->color_depth_property = prop;
>> +		drm_object_attach_property(&connector->base, prop, 0);
>> +	}
>> +
>> +	prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format",
>> +					drm_hdmi_output_enum_list,
>> +					ARRAY_SIZE(drm_hdmi_output_enum_list));
>> +	if (prop) {
>> +		hdmi->hdmi_output_property = prop;
>> +		drm_object_attach_property(&connector->base, prop, 0);
>> +	}
>> +}
>> +
>> +static void
>> +dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector,
>> +				    void *data)
>> +{
>> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
>> +
>> +	if (hdmi->color_depth_property) {
>> +		drm_property_destroy(connector->dev,
>> +				     hdmi->color_depth_property);
>> +		hdmi->color_depth_property = NULL;
>> +	}
>> +
>> +	if (hdmi->hdmi_output_property) {
>> +		drm_property_destroy(connector->dev,
>> +				     hdmi->hdmi_output_property);
>> +		hdmi->hdmi_output_property = NULL;
>> +	}
>> +}
>> +
>> +static int
>> +dw_hdmi_rockchip_set_property(struct drm_connector *connector,
>> +			      struct drm_connector_state *state,
>> +			      struct drm_property *property,
>> +			      u64 val,
>> +			      void *data)
>> +{
>> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
>> +
>> +	if (property == hdmi->color_depth_property) {
>> +		hdmi->colordepth = val;
>> +		return 0;
>> +	} else if (property == hdmi->hdmi_output_property) {
>> +		hdmi->hdmi_output = val;
>> +		return 0;
>> +	}
>> +
>> +	DRM_ERROR("failed to set rockchip hdmi connector property\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int
>> +dw_hdmi_rockchip_get_property(struct drm_connector *connector,
>> +			      const struct drm_connector_state *state,
>> +			      struct drm_property *property,
>> +			      u64 *val,
>> +			      void *data)
>> +{
>> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
>> +
>> +	if (property == hdmi->color_depth_property) {
>> +		*val = hdmi->colordepth;
>> +		return 0;
>> +	} else if (property == hdmi->hdmi_output_property) {
>> +		*val = hdmi->hdmi_output;
>> +		return 0;
>> +	}
>> +
>> +	DRM_ERROR("failed to get rockchip hdmi connector property\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = {
>> +	.attach_properties	= dw_hdmi_rockchip_attach_properties,
>> +	.destroy_properties	= dw_hdmi_rockchip_destroy_properties,
>> +	.set_property		= dw_hdmi_rockchip_set_property,
>> +	.get_property		= dw_hdmi_rockchip_get_property,
>> +};
>> +
>>   static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data)
>>   {
>>   	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
>> @@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>>   	hdmi->dev = &pdev->dev;
>>   	hdmi->chip_data = plat_data->phy_data;
>>   	plat_data->phy_data = hdmi;
>> +
>> +	plat_data->property_ops = &dw_hdmi_rockchip_property_ops;
>> +
>>   	encoder = &hdmi->encoder;
>>   
>>   	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index ea34ca146b82..dc561ebe7a9b 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -6,6 +6,7 @@
>>   #ifndef __DW_HDMI__
>>   #define __DW_HDMI__
>>   
>> +#include <drm/drm_property.h>
>>   #include <sound/hdmi-codec.h>
>>   
>>   struct drm_display_info;
>> @@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops {
>>   	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
>>   };
>>   
>> +struct dw_hdmi_property_ops {
>> +	void (*attach_properties)(struct drm_connector *connector,
>> +				  unsigned int color, int version,
>> +				  void *data);
>> +	void (*destroy_properties)(struct drm_connector *connector,
>> +				   void *data);
>> +	int (*set_property)(struct drm_connector *connector,
>> +			    struct drm_connector_state *state,
>> +			    struct drm_property *property,
>> +			    u64 val,
>> +			    void *data);
>> +	int (*get_property)(struct drm_connector *connector,
>> +			    const struct drm_connector_state *state,
>> +			    struct drm_property *property,
>> +			    u64 *val,
>> +			    void *data);
>> +};
>> +
>>   struct dw_hdmi_plat_data {
>>   	struct regmap *regm;
>>   
>> @@ -141,6 +160,9 @@ struct dw_hdmi_plat_data {
>>   					   const struct drm_display_info *info,
>>   					   const struct drm_display_mode *mode);
>>   
>> +	/* Vendor Property support */
>> +	const struct dw_hdmi_property_ops *property_ops;
>> +
>>   	/* Vendor PHY support */
>>   	const struct dw_hdmi_phy_ops *phy_ops;
>>   	const char *phy_name;

[-- Attachment #1.2: Type: text/html, Size: 11234 bytes --]

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

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

Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Laurent Pinchart]

Hi Algea,

On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote:
> 在 2020/8/12 17:33, Laurent Pinchart 写道:
> > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:
> >> Introduce struct dw_hdmi_property_ops in plat_data to support
> >> vendor hdmi property.
> >>
> >> Implement hdmi vendor properties color_depth_property and
> >> hdmi_output_property to config hdmi output color depth and
> >> color format.
> >>
> >> The property "hdmi_output_format", the possible value
> >> could be:
> >>           - RGB
> >>           - YCBCR 444
> >>           - YCBCR 422
> >>           - YCBCR 420
> >>
> >> Default value of the property is set to 0 = RGB, so no changes if you
> >> don't set the property.
> >>
> >> The property "hdmi_output_depth" possible value could be
> >>           - Automatic
> >>             This indicates prefer highest color depth, it is
> >>             30bit on rockcip platform.
> >>           - 24bit
> >>           - 30bit
> >> The default value of property is 24bit.
> >>
> >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> >> ---
> >>
> >>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
> >>   include/drm/bridge/dw_hdmi.h                |  22 +++
> >>   2 files changed, 196 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> >> index 23de359a1dec..8f22d9a566db 100644
> >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> >> @@ -52,6 +52,27 @@
> >>   
> >>   #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
> >>   
> >> +/* HDMI output pixel format */
> >> +enum drm_hdmi_output_type {
> >> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> >> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> >> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> >> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> >> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> >> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> >> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> >> +};
> >
> > Vendor-specific properties shouldn't use names starting with drm_ or
> > DRM_, that's for the DRM core. But this doesn't seem specific to
> > Rockchip at all, it should be a standard property. Additionally, new
> > properties need to come with a userspace implementation showing their
> > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> > relevant upstream project (a test tool is usually not enough).
> 
> We use these properties only in Android HW composer, But we can't upstream
> 
> our HW composer code right now.  Can we use this properties as private 
> property
> 
> and do not upstream HW composer for the time being?

It's not my decision, it's a policy of the DRM subsystem to require an
open implementation in userspace to validate all API additions.

In any case, I think selection of the output format should be done
through a standard API (very likely a standard DRM property), possibly
related to drm_mode_create_hdmi_colorspace_property(). There's nothing
Rockchip-specific here.

> >> +
> >> +enum dw_hdmi_rockchip_color_depth {
> >> +	ROCKCHIP_HDMI_DEPTH_8,
> >> +	ROCKCHIP_HDMI_DEPTH_10,
> >> +	ROCKCHIP_HDMI_DEPTH_12,
> >> +	ROCKCHIP_HDMI_DEPTH_16,
> >> +	ROCKCHIP_HDMI_DEPTH_420_10,
> >> +	ROCKCHIP_HDMI_DEPTH_420_12,
> >> +	ROCKCHIP_HDMI_DEPTH_420_16
> >> +};
> >> +
> >>   /**
> >>    * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips
> >>    * @lcdsel_grf_reg: grf register offset of lcdc select
> >> @@ -73,6 +94,12 @@ struct rockchip_hdmi {
> >>   	struct clk *grf_clk;
> >>   	struct dw_hdmi *hdmi;
> >>   	struct phy *phy;
> >> +
> >> +	struct drm_property *color_depth_property;
> >> +	struct drm_property *hdmi_output_property;
> >> +
> >> +	unsigned int colordepth;
> >> +	enum drm_hdmi_output_type hdmi_output;
> >>   };
> >>   
> >>   #define to_rockchip_hdmi(x)	container_of(x, struct rockchip_hdmi, x)
> >> @@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data)
> >>   	phy_power_off(hdmi->phy);
> >>   }
> >>   
> >> +static const struct drm_prop_enum_list color_depth_enum_list[] = {
> >> +	{ 0, "Automatic" }, /* Prefer highest color depth */
> >> +	{ 8, "24bit" },
> >> +	{ 10, "30bit" },
> >> +};
> >> +
> >> +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = {
> >> +	{ DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" },
> >> +	{ DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" },
> >> +	{ DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" },
> >> +	{ DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" },
> >> +	{ DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" },
> >> +	{ DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" },
> >> +	{ DRM_HDMI_OUTPUT_INVALID, "invalid_output" },
> >> +};
> >> +
> >> +static void
> >> +dw_hdmi_rockchip_attach_properties(struct drm_connector *connector,
> >> +				   unsigned int color, int version,
> >> +				   void *data)
> >> +{
> >> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> >> +	struct drm_property *prop;
> >> +
> >> +	switch (color) {
> >> +	case MEDIA_BUS_FMT_RGB101010_1X30:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
> >> +		hdmi->colordepth = 10;
> >> +		break;
> >> +	case MEDIA_BUS_FMT_YUV8_1X24:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
> >> +		hdmi->colordepth = 8;
> >> +		break;
> >> +	case MEDIA_BUS_FMT_YUV10_1X30:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444;
> >> +		hdmi->colordepth = 10;
> >> +		break;
> >> +	case MEDIA_BUS_FMT_UYVY10_1X20:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
> >> +		hdmi->colordepth = 10;
> >> +		break;
> >> +	case MEDIA_BUS_FMT_UYVY8_1X16:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422;
> >> +		hdmi->colordepth = 8;
> >> +		break;
> >> +	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
> >> +		hdmi->colordepth = 8;
> >> +		break;
> >> +	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
> >> +		hdmi->colordepth = 10;
> >> +		break;
> >> +	default:
> >> +		hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB;
> >> +		hdmi->colordepth = 8;
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(connector->dev, 0,
> >> +					"hdmi_output_depth",
> >> +					color_depth_enum_list,
> >> +					ARRAY_SIZE(color_depth_enum_list));
> >> +	if (prop) {
> >> +		hdmi->color_depth_property = prop;
> >> +		drm_object_attach_property(&connector->base, prop, 0);
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format",
> >> +					drm_hdmi_output_enum_list,
> >> +					ARRAY_SIZE(drm_hdmi_output_enum_list));
> >> +	if (prop) {
> >> +		hdmi->hdmi_output_property = prop;
> >> +		drm_object_attach_property(&connector->base, prop, 0);
> >> +	}
> >> +}
> >> +
> >> +static void
> >> +dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector,
> >> +				    void *data)
> >> +{
> >> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> >> +
> >> +	if (hdmi->color_depth_property) {
> >> +		drm_property_destroy(connector->dev,
> >> +				     hdmi->color_depth_property);
> >> +		hdmi->color_depth_property = NULL;
> >> +	}
> >> +
> >> +	if (hdmi->hdmi_output_property) {
> >> +		drm_property_destroy(connector->dev,
> >> +				     hdmi->hdmi_output_property);
> >> +		hdmi->hdmi_output_property = NULL;
> >> +	}
> >> +}
> >> +
> >> +static int
> >> +dw_hdmi_rockchip_set_property(struct drm_connector *connector,
> >> +			      struct drm_connector_state *state,
> >> +			      struct drm_property *property,
> >> +			      u64 val,
> >> +			      void *data)
> >> +{
> >> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> >> +
> >> +	if (property == hdmi->color_depth_property) {
> >> +		hdmi->colordepth = val;
> >> +		return 0;
> >> +	} else if (property == hdmi->hdmi_output_property) {
> >> +		hdmi->hdmi_output = val;
> >> +		return 0;
> >> +	}
> >> +
> >> +	DRM_ERROR("failed to set rockchip hdmi connector property\n");
> >> +	return -EINVAL;
> >> +}
> >> +
> >> +static int
> >> +dw_hdmi_rockchip_get_property(struct drm_connector *connector,
> >> +			      const struct drm_connector_state *state,
> >> +			      struct drm_property *property,
> >> +			      u64 *val,
> >> +			      void *data)
> >> +{
> >> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> >> +
> >> +	if (property == hdmi->color_depth_property) {
> >> +		*val = hdmi->colordepth;
> >> +		return 0;
> >> +	} else if (property == hdmi->hdmi_output_property) {
> >> +		*val = hdmi->hdmi_output;
> >> +		return 0;
> >> +	}
> >> +
> >> +	DRM_ERROR("failed to get rockchip hdmi connector property\n");
> >> +	return -EINVAL;
> >> +}
> >> +
> >> +static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = {
> >> +	.attach_properties	= dw_hdmi_rockchip_attach_properties,
> >> +	.destroy_properties	= dw_hdmi_rockchip_destroy_properties,
> >> +	.set_property		= dw_hdmi_rockchip_set_property,
> >> +	.get_property		= dw_hdmi_rockchip_get_property,
> >> +};
> >> +
> >>   static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data)
> >>   {
> >>   	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> >> @@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> >>   	hdmi->dev = &pdev->dev;
> >>   	hdmi->chip_data = plat_data->phy_data;
> >>   	plat_data->phy_data = hdmi;
> >> +
> >> +	plat_data->property_ops = &dw_hdmi_rockchip_property_ops;
> >> +
> >>   	encoder = &hdmi->encoder;
> >>   
> >>   	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
> >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> >> index ea34ca146b82..dc561ebe7a9b 100644
> >> --- a/include/drm/bridge/dw_hdmi.h
> >> +++ b/include/drm/bridge/dw_hdmi.h
> >> @@ -6,6 +6,7 @@
> >>   #ifndef __DW_HDMI__
> >>   #define __DW_HDMI__
> >>   
> >> +#include <drm/drm_property.h>
> >>   #include <sound/hdmi-codec.h>
> >>   
> >>   struct drm_display_info;
> >> @@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops {
> >>   	void (*setup_hpd)(struct dw_hdmi *hdmi, void *data);
> >>   };
> >>   
> >> +struct dw_hdmi_property_ops {
> >> +	void (*attach_properties)(struct drm_connector *connector,
> >> +				  unsigned int color, int version,
> >> +				  void *data);
> >> +	void (*destroy_properties)(struct drm_connector *connector,
> >> +				   void *data);
> >> +	int (*set_property)(struct drm_connector *connector,
> >> +			    struct drm_connector_state *state,
> >> +			    struct drm_property *property,
> >> +			    u64 val,
> >> +			    void *data);
> >> +	int (*get_property)(struct drm_connector *connector,
> >> +			    const struct drm_connector_state *state,
> >> +			    struct drm_property *property,
> >> +			    u64 *val,
> >> +			    void *data);
> >> +};
> >> +
> >>   struct dw_hdmi_plat_data {
> >>   	struct regmap *regm;
> >>   
> >> @@ -141,6 +160,9 @@ struct dw_hdmi_plat_data {
> >>   					   const struct drm_display_info *info,
> >>   					   const struct drm_display_mode *mode);
> >>   
> >> +	/* Vendor Property support */
> >> +	const struct dw_hdmi_property_ops *property_ops;
> >> +
> >>   	/* Vendor PHY support */
> >>   	const struct dw_hdmi_phy_ops *phy_ops;
> >>   	const char *phy_name;

-- 
Regards,

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

Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 3522 bytes --]

On Wed, 12 Aug 2020 16:30:17 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Algea,
> 
> On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote:
> > 在 2020/8/12 17:33, Laurent Pinchart 写道:  
> > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:  
> > >> Introduce struct dw_hdmi_property_ops in plat_data to support
> > >> vendor hdmi property.
> > >>
> > >> Implement hdmi vendor properties color_depth_property and
> > >> hdmi_output_property to config hdmi output color depth and
> > >> color format.
> > >>
> > >> The property "hdmi_output_format", the possible value
> > >> could be:
> > >>           - RGB
> > >>           - YCBCR 444
> > >>           - YCBCR 422
> > >>           - YCBCR 420
> > >>
> > >> Default value of the property is set to 0 = RGB, so no changes if you
> > >> don't set the property.
> > >>
> > >> The property "hdmi_output_depth" possible value could be
> > >>           - Automatic
> > >>             This indicates prefer highest color depth, it is
> > >>             30bit on rockcip platform.
> > >>           - 24bit
> > >>           - 30bit
> > >> The default value of property is 24bit.
> > >>
> > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> > >> ---
> > >>
> > >>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
> > >>   include/drm/bridge/dw_hdmi.h                |  22 +++
> > >>   2 files changed, 196 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> index 23de359a1dec..8f22d9a566db 100644
> > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> @@ -52,6 +52,27 @@
> > >>   
> > >>   #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
> > >>   
> > >> +/* HDMI output pixel format */
> > >> +enum drm_hdmi_output_type {
> > >> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> > >> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> > >> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> > >> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> > >> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> > >> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> > >> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> > >> +};  
> > >
> > > Vendor-specific properties shouldn't use names starting with drm_ or
> > > DRM_, that's for the DRM core. But this doesn't seem specific to
> > > Rockchip at all, it should be a standard property. Additionally, new
> > > properties need to come with a userspace implementation showing their
> > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> > > relevant upstream project (a test tool is usually not enough).  
> > 
> > We use these properties only in Android HW composer, But we can't upstream
> > 
> > our HW composer code right now.  Can we use this properties as private 
> > property
> > 
> > and do not upstream HW composer for the time being?  
> 
> It's not my decision, it's a policy of the DRM subsystem to require an
> open implementation in userspace to validate all API additions.

Also read
https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
very carefully: it calls for a FOSS userspace project's proper upstream
to have reviewed and accepted the patches to use the new UAPI, but
those patches must NOT be MERGED at that time yet.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Laurent Pinchart]

On Thu, Aug 13, 2020 at 10:42:28AM +0300, Pekka Paalanen wrote:
> On Wed, 12 Aug 2020 16:30:17 +0300 Laurent Pinchart wrote:
> > On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote:
> > > 在 2020/8/12 17:33, Laurent Pinchart 写道:  
> > > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:  
> > > >> Introduce struct dw_hdmi_property_ops in plat_data to support
> > > >> vendor hdmi property.
> > > >>
> > > >> Implement hdmi vendor properties color_depth_property and
> > > >> hdmi_output_property to config hdmi output color depth and
> > > >> color format.
> > > >>
> > > >> The property "hdmi_output_format", the possible value
> > > >> could be:
> > > >>           - RGB
> > > >>           - YCBCR 444
> > > >>           - YCBCR 422
> > > >>           - YCBCR 420
> > > >>
> > > >> Default value of the property is set to 0 = RGB, so no changes if you
> > > >> don't set the property.
> > > >>
> > > >> The property "hdmi_output_depth" possible value could be
> > > >>           - Automatic
> > > >>             This indicates prefer highest color depth, it is
> > > >>             30bit on rockcip platform.
> > > >>           - 24bit
> > > >>           - 30bit
> > > >> The default value of property is 24bit.
> > > >>
> > > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> > > >> ---
> > > >>
> > > >>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
> > > >>   include/drm/bridge/dw_hdmi.h                |  22 +++
> > > >>   2 files changed, 196 insertions(+)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > >> index 23de359a1dec..8f22d9a566db 100644
> > > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > >> @@ -52,6 +52,27 @@
> > > >>   
> > > >>   #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
> > > >>   
> > > >> +/* HDMI output pixel format */
> > > >> +enum drm_hdmi_output_type {
> > > >> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> > > >> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> > > >> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> > > >> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> > > >> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> > > >> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> > > >> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> > > >> +};  
> > > >
> > > > Vendor-specific properties shouldn't use names starting with drm_ or
> > > > DRM_, that's for the DRM core. But this doesn't seem specific to
> > > > Rockchip at all, it should be a standard property. Additionally, new
> > > > properties need to come with a userspace implementation showing their
> > > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> > > > relevant upstream project (a test tool is usually not enough).  
> > > 
> > > We use these properties only in Android HW composer, But we can't upstream
> > > 
> > > our HW composer code right now.  Can we use this properties as private 
> > > property
> > > 
> > > and do not upstream HW composer for the time being?  
> > 
> > It's not my decision, it's a policy of the DRM subsystem to require an
> > open implementation in userspace to validate all API additions.
> 
> Also read
> https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
> very carefully: it calls for a FOSS userspace project's proper upstream
> to have reviewed and accepted the patches to use the new UAPI, but
> those patches must NOT be MERGED at that time yet.

Correct. Many userspace projects wouldn't merge a patch before the
kernel API is merged, so that would create a chicken and egg problem :-)

-- 
Regards,

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

Re: [PATCH 4/6] drm/rockchip: dw_hdmi: Add vendor hdmi properties

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4477 bytes --]

On Thu, 13 Aug 2020 13:45:22 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> On Thu, Aug 13, 2020 at 10:42:28AM +0300, Pekka Paalanen wrote:
> > On Wed, 12 Aug 2020 16:30:17 +0300 Laurent Pinchart wrote:  
> > > On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote:  
> > > > 在 2020/8/12 17:33, Laurent Pinchart 写道:    
> > > > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote:    
> > > > >> Introduce struct dw_hdmi_property_ops in plat_data to support
> > > > >> vendor hdmi property.
> > > > >>
> > > > >> Implement hdmi vendor properties color_depth_property and
> > > > >> hdmi_output_property to config hdmi output color depth and
> > > > >> color format.
> > > > >>
> > > > >> The property "hdmi_output_format", the possible value
> > > > >> could be:
> > > > >>           - RGB
> > > > >>           - YCBCR 444
> > > > >>           - YCBCR 422
> > > > >>           - YCBCR 420
> > > > >>
> > > > >> Default value of the property is set to 0 = RGB, so no changes if you
> > > > >> don't set the property.
> > > > >>
> > > > >> The property "hdmi_output_depth" possible value could be
> > > > >>           - Automatic
> > > > >>             This indicates prefer highest color depth, it is
> > > > >>             30bit on rockcip platform.
> > > > >>           - 24bit
> > > > >>           - 30bit
> > > > >> The default value of property is 24bit.
> > > > >>
> > > > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> > > > >> ---
> > > > >>
> > > > >>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++
> > > > >>   include/drm/bridge/dw_hdmi.h                |  22 +++
> > > > >>   2 files changed, 196 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > >> index 23de359a1dec..8f22d9a566db 100644
> > > > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > > > >> @@ -52,6 +52,27 @@
> > > > >>   
> > > > >>   #define HIWORD_UPDATE(val, mask)	(val | (mask) << 16)
> > > > >>   
> > > > >> +/* HDMI output pixel format */
> > > > >> +enum drm_hdmi_output_type {
> > > > >> +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> > > > >> +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> > > > >> +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> > > > >> +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> > > > >> +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> > > > >> +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> > > > >> +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> > > > >> +};    
> > > > >
> > > > > Vendor-specific properties shouldn't use names starting with drm_ or
> > > > > DRM_, that's for the DRM core. But this doesn't seem specific to
> > > > > Rockchip at all, it should be a standard property. Additionally, new
> > > > > properties need to come with a userspace implementation showing their
> > > > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another
> > > > > relevant upstream project (a test tool is usually not enough).    
> > > > 
> > > > We use these properties only in Android HW composer, But we can't upstream
> > > > 
> > > > our HW composer code right now.  Can we use this properties as private 
> > > > property
> > > > 
> > > > and do not upstream HW composer for the time being?    
> > > 
> > > It's not my decision, it's a policy of the DRM subsystem to require an
> > > open implementation in userspace to validate all API additions.  
> > 
> > Also read
> > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
> > very carefully: it calls for a FOSS userspace project's proper upstream
> > to have reviewed and accepted the patches to use the new UAPI, but
> > those patches must NOT be MERGED at that time yet.  
> 
> Correct. Many userspace projects wouldn't merge a patch before the
> kernel API is merged, so that would create a chicken and egg problem :-)

I wouldn't be so sure that absolutely everyone knows that rule. It only
takes just one userspace project to merge and release with it to
potentially require renaming everything if any change is needed after
the kernel review process.

Actually, if I remember right, I may have seen such merging happen.
After all, "accepted" is usually a synonym for "merged".


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 6/6] drm: bridge: dw-hdmi: Get output bus format when dw-hdmi is the only bridge

[Neil Armstrong]

Hi,

On 12/08/2020 10:36, Algea Cao wrote:
> If plat_data->get_output_bus_format() is exist, we can
> use it to get hdmi output bus format when dw-hdmi is the
> only bridge. The hdmi output bus format can be set by vendor
> properties.
> 
> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 1eb4736b9b59..878e9e506963 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2644,6 +2644,8 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>  					unsigned int *num_output_fmts)
>  {
>  	struct drm_connector *conn = conn_state->connector;
> +	struct dw_hdmi *hdmi = bridge->driver_private;
> +	void *data = hdmi->plat_data->phy_data;
>  	struct drm_display_info *info = &conn->display_info;
>  	struct drm_display_mode *mode = &crtc_state->mode;
>  	u8 max_bpc = conn_state->max_requested_bpc;
> @@ -2662,7 +2664,11 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>  	/* If dw-hdmi is the only bridge, avoid negociating with ourselves */
>  	if (list_is_singular(&bridge->encoder->bridge_chain)) {
>  		*num_output_fmts = 1;
> -		output_fmts[0] = MEDIA_BUS_FMT_FIXED;
> +		if (hdmi->plat_data->get_output_bus_format)
> +			output_fmts[0] =
> +				hdmi->plat_data->get_output_bus_format(data);


The whole bus format negociation was introduced to actually avoid using such get_output_bus_format()
callback, please implement proper bus format negociation.

Neil

> +		else
> +			output_fmts[0] = MEDIA_BUS_FMT_FIXED;
>  
>  		return output_fmts;
>  	}
> 

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

Re: [PATCH 3/6] drm: bridge: dw-hdmi: Introduce previous_pixelclock/previous_tmdsclock

[Neil Armstrong]

On 12/08/2020 10:34, Algea Cao wrote:
> Introduce previous_pixelclock/previous_tmdsclock to
> determine whether PHY needs initialization. If phy is power off,
> or mpixelclock/mtmdsclock is different to previous value, phy is
> neet to be reinitialized.
> 
> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 50 +++++++++++++++++++----
>  1 file changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index a1a81fc768c2..1eb4736b9b59 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -101,6 +101,8 @@ static const u16 csc_coeff_rgb_full_to_rgb_limited[3][4] = {
>  struct hdmi_vmode {
>  	bool mdataenablepolarity;
>  
> +	unsigned int previous_pixelclock;
> +	unsigned int previous_tmdsclock;
>  	unsigned int mpixelclock;
>  	unsigned int mpixelrepetitioninput;
>  	unsigned int mpixelrepetitionoutput;
> @@ -890,6 +892,32 @@ static int hdmi_bus_fmt_color_depth(unsigned int bus_format)
>  	}
>  }
>  
> +static unsigned int
> +hdmi_get_tmdsclock(struct dw_hdmi *hdmi, unsigned long mpixelclock)
> +{
> +	unsigned int tmdsclock = mpixelclock;
> +	unsigned int depth =
> +		hdmi_bus_fmt_color_depth(hdmi->hdmi_data.enc_out_bus_format);
> +
> +	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {
> +		switch (depth) {
> +		case 16:
> +			tmdsclock = mpixelclock * 2;
> +			break;
> +		case 12:
> +			tmdsclock = mpixelclock * 3 / 2;
> +			break;
> +		case 10:
> +			tmdsclock = mpixelclock * 5 / 4;
> +			break;
> +		default:
> +			break;
> +		}
> +	}


Where does this come from ? Please introduce this on another patch.

Neil

> +
> +	return tmdsclock;
> +}
> +
>  /*
>   * this submodule is responsible for the video data synchronization.
>   * for example, for RGB 4:4:4 input, the data map is defined as
> @@ -1861,11 +1889,13 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>  	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
>  	unsigned int vdisplay, hdisplay;
>  
> +	vmode->previous_pixelclock = vmode->mpixelclock;
>  	vmode->mpixelclock = mode->clock * 1000;
>  
>  	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
>  
> -	vmode->mtmdsclock = vmode->mpixelclock;
> +	vmode->previous_tmdsclock = vmode->mtmdsclock;
> +	vmode->mtmdsclock = hdmi_get_tmdsclock(hdmi, vmode->mpixelclock);
>  
>  	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) {
>  		switch (hdmi_bus_fmt_color_depth(
> @@ -2172,12 +2202,18 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
>  	hdmi_av_composer(hdmi, &connector->display_info, mode);
>  
>  	/* HDMI Initializateion Step B.2 */
> -	ret = hdmi->phy.ops->init(hdmi, hdmi->phy.data,
> -				  &connector->display_info,
> -				  &hdmi->previous_mode);
> -	if (ret)
> -		return ret;
> -	hdmi->phy.enabled = true;
> +	if (!hdmi->phy.enabled ||
> +	    hdmi->hdmi_data.video_mode.previous_pixelclock !=
> +	    hdmi->hdmi_data.video_mode.mpixelclock ||
> +	    hdmi->hdmi_data.video_mode.previous_tmdsclock !=
> +	    hdmi->hdmi_data.video_mode.mtmdsclock) {
> +		ret = hdmi->phy.ops->init(hdmi, hdmi->phy.data,
> +					  &connector->display_info,
> +					  &hdmi->previous_mode);
> +		if (ret)
> +			return ret;
> +		hdmi->phy.enabled = true;
> +	}
>  
>  	/* HDMI Initialization Step B.3 */
>  	dw_hdmi_enable_video_path(hdmi);
> 

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