Add support for HDR metadata using the hdr_output_metadata connector property,
configure Dynamic Range and Mastering InfoFrame accordingly.
A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
can use to signal when Dynamic Range and Mastering infoframes is supported.
This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
and only GXL support DRM InfoFrame.
The first patch add functionality to configure DRM InfoFrame based on the
hdr_output_metadata connector property.
The remaining patches sets the drm_infoframe flag on some SoCs supporting
Dynamic Range and Mastering InfoFrame.
Note that this was based on top of drm-misc-next and Neil Armstrong's
"drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
[1] https://patchwork.freedesktop.org/series/58725/#rev2
Jonas Karlman (4):
drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
drm/sun4i: Enable DRM InfoFrame support on H6
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
include/drm/bridge/dw_hdmi.h | 1 +
7 files changed, 157 insertions(+)
--
2.17.1
Add support for configuring Dynamic Range and Mastering InfoFrame from
the hdr_output_metadata connector property.
This patch adds a drm_infoframe flag to dw_hdmi_plat_data that platform drivers
use to signal when Dynamic Range and Mastering infoframes is supported.
This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
and only GXL support DRM InfoFrame.
These changes were based on work done by Zheng Yang <[email protected]>
to support DRM InfoFrame on the Rockchip 4.4 BSP kernel at [1] and [2]
[1] https://github.com/rockchip-linux/kernel/tree/develop-4.4
[2] https://github.com/rockchip-linux/kernel/commit/d1943fde81ff41d7cca87f4a42f03992e90bddd5
Cc: Zheng Yang <[email protected]>
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 ++++++++
include/drm/bridge/dw_hdmi.h | 1 +
3 files changed, 147 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 284ce59be8f8..801bbbd732fd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -24,6 +24,7 @@
#include <drm/drm_of.h>
#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_edid.h>
#include <drm/drm_encoder_slave.h>
@@ -1592,6 +1593,78 @@ static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
HDMI_FC_DATAUTO0_VSD_MASK);
}
+#define HDR_LSB(n) ((n) & 0xff)
+#define HDR_MSB(n) (((n) & 0xff00) >> 8)
+
+static void hdmi_config_drm_infoframe(struct dw_hdmi *hdmi)
+{
+ const struct drm_connector_state *conn_state = hdmi->connector.state;
+ struct hdmi_drm_infoframe frame;
+ int ret;
+
+ if (hdmi->version < 0x200a || !hdmi->plat_data->drm_infoframe)
+ return;
+
+ hdmi_modb(hdmi, HDMI_FC_PACKET_TX_EN_DRM_DISABLE,
+ HDMI_FC_PACKET_TX_EN_DRM_MASK, HDMI_FC_PACKET_TX_EN);
+
+ ret = drm_hdmi_infoframe_set_hdr_metadata(&frame, conn_state);
+ if (ret < 0)
+ return;
+
+ ret = hdmi_drm_infoframe_check(&frame);
+ if (WARN_ON(ret))
+ return;
+
+ hdmi_writeb(hdmi, frame.version, HDMI_FC_DRM_HB0);
+ hdmi_writeb(hdmi, frame.length, HDMI_FC_DRM_HB1);
+ hdmi_writeb(hdmi, frame.eotf, HDMI_FC_DRM_PB0);
+ hdmi_writeb(hdmi, frame.metadata_type, HDMI_FC_DRM_PB1);
+ hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[0].x),
+ HDMI_FC_DRM_PB2);
+ hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[0].x),
+ HDMI_FC_DRM_PB3);
+ hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[0].y),
+ HDMI_FC_DRM_PB4);
+ hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[0].y),
+ HDMI_FC_DRM_PB5);
+ hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[1].x),
+ HDMI_FC_DRM_PB6);
+ hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[1].x),
+ HDMI_FC_DRM_PB7);
+ hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[1].y),
+ HDMI_FC_DRM_PB8);
+ hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[1].y),
+ HDMI_FC_DRM_PB9);
+ hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[2].x),
+ HDMI_FC_DRM_PB10);
+ hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[2].x),
+ HDMI_FC_DRM_PB11);
+ hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[2].y),
+ HDMI_FC_DRM_PB12);
+ hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[2].y),
+ HDMI_FC_DRM_PB13);
+ hdmi_writeb(hdmi, HDR_LSB(frame.white_point.x), HDMI_FC_DRM_PB14);
+ hdmi_writeb(hdmi, HDR_MSB(frame.white_point.x), HDMI_FC_DRM_PB15);
+ hdmi_writeb(hdmi, HDR_LSB(frame.white_point.y), HDMI_FC_DRM_PB16);
+ hdmi_writeb(hdmi, HDR_MSB(frame.white_point.y), HDMI_FC_DRM_PB17);
+ hdmi_writeb(hdmi, HDR_LSB(frame.max_display_mastering_luminance),
+ HDMI_FC_DRM_PB18);
+ hdmi_writeb(hdmi, HDR_MSB(frame.max_display_mastering_luminance),
+ HDMI_FC_DRM_PB19);
+ hdmi_writeb(hdmi, HDR_LSB(frame.min_display_mastering_luminance),
+ HDMI_FC_DRM_PB20);
+ hdmi_writeb(hdmi, HDR_MSB(frame.min_display_mastering_luminance),
+ HDMI_FC_DRM_PB21);
+ hdmi_writeb(hdmi, HDR_LSB(frame.max_cll), HDMI_FC_DRM_PB22);
+ hdmi_writeb(hdmi, HDR_MSB(frame.max_cll), HDMI_FC_DRM_PB23);
+ hdmi_writeb(hdmi, HDR_LSB(frame.max_fall), HDMI_FC_DRM_PB24);
+ hdmi_writeb(hdmi, HDR_MSB(frame.max_fall), HDMI_FC_DRM_PB25);
+ hdmi_writeb(hdmi, 1, HDMI_FC_DRM_UP);
+ hdmi_modb(hdmi, HDMI_FC_PACKET_TX_EN_DRM_ENABLE,
+ HDMI_FC_PACKET_TX_EN_DRM_MASK, HDMI_FC_PACKET_TX_EN);
+}
+
static void hdmi_av_composer(struct dw_hdmi *hdmi,
const struct drm_display_mode *mode)
{
@@ -1963,6 +2036,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
/* HDMI Initialization Step F - Configure AVI InfoFrame */
hdmi_config_AVI(hdmi, mode);
hdmi_config_vendor_specific_infoframe(hdmi, mode);
+ hdmi_config_drm_infoframe(hdmi);
} else {
dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
}
@@ -2139,6 +2213,36 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
return ret;
}
+static bool blob_equal(const struct drm_property_blob *a,
+ const struct drm_property_blob *b)
+{
+ if (a && b)
+ return a->length == b->length &&
+ !memcmp(a->data, b->data, a->length);
+
+ return !a == !b;
+}
+
+static int dw_hdmi_connector_atomic_check(struct drm_connector *conn,
+ struct drm_connector_state *new_state)
+{
+ struct drm_connector_state *old_state =
+ drm_atomic_get_old_connector_state(new_state->state, conn);
+ struct drm_crtc_state *crtc_state;
+
+ if (!new_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
+ new_state->crtc);
+
+ if (!blob_equal(new_state->hdr_output_metadata,
+ old_state->hdr_output_metadata))
+ crtc_state->mode_changed = true;
+
+ return 0;
+}
+
static void dw_hdmi_connector_force(struct drm_connector *connector)
{
struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
@@ -2163,6 +2267,7 @@ 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,
};
static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
@@ -2179,6 +2284,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
drm_connector_init(bridge->dev, connector, &dw_hdmi_connector_funcs,
DRM_MODE_CONNECTOR_HDMIA);
+ if (hdmi->version >= 0x200a && hdmi->plat_data->drm_infoframe)
+ drm_object_attach_property(&connector->base,
+ connector->dev->mode_config.hdr_output_metadata_property, 0);
+
drm_connector_attach_encoder(connector, encoder);
return 0;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 3f3c616eba97..d4efbec14f68 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -256,6 +256,7 @@
#define HDMI_FC_POL2 0x10DB
#define HDMI_FC_PRCONF 0x10E0
#define HDMI_FC_SCRAMBLER_CTRL 0x10E1
+#define HDMI_FC_PACKET_TX_EN 0x10E3
#define HDMI_FC_GMD_STAT 0x1100
#define HDMI_FC_GMD_EN 0x1101
@@ -291,6 +292,37 @@
#define HDMI_FC_GMD_PB26 0x111F
#define HDMI_FC_GMD_PB27 0x1120
+#define HDMI_FC_DRM_UP 0x1167
+#define HDMI_FC_DRM_HB0 0x1168
+#define HDMI_FC_DRM_HB1 0x1169
+#define HDMI_FC_DRM_PB0 0x116A
+#define HDMI_FC_DRM_PB1 0x116B
+#define HDMI_FC_DRM_PB2 0x116C
+#define HDMI_FC_DRM_PB3 0x116D
+#define HDMI_FC_DRM_PB4 0x116E
+#define HDMI_FC_DRM_PB5 0x116F
+#define HDMI_FC_DRM_PB6 0x1170
+#define HDMI_FC_DRM_PB7 0x1171
+#define HDMI_FC_DRM_PB8 0x1172
+#define HDMI_FC_DRM_PB9 0x1173
+#define HDMI_FC_DRM_PB10 0x1174
+#define HDMI_FC_DRM_PB11 0x1175
+#define HDMI_FC_DRM_PB12 0x1176
+#define HDMI_FC_DRM_PB13 0x1177
+#define HDMI_FC_DRM_PB14 0x1178
+#define HDMI_FC_DRM_PB15 0x1179
+#define HDMI_FC_DRM_PB16 0x117A
+#define HDMI_FC_DRM_PB17 0x117B
+#define HDMI_FC_DRM_PB18 0x117C
+#define HDMI_FC_DRM_PB19 0x117D
+#define HDMI_FC_DRM_PB20 0x117E
+#define HDMI_FC_DRM_PB21 0x117F
+#define HDMI_FC_DRM_PB22 0x1180
+#define HDMI_FC_DRM_PB23 0x1181
+#define HDMI_FC_DRM_PB24 0x1182
+#define HDMI_FC_DRM_PB25 0x1183
+#define HDMI_FC_DRM_PB26 0x1184
+
#define HDMI_FC_DBGFORCE 0x1200
#define HDMI_FC_DBGAUD0CH0 0x1201
#define HDMI_FC_DBGAUD1CH0 0x1202
@@ -746,6 +778,11 @@ enum {
HDMI_FC_PRCONF_OUTPUT_PR_FACTOR_MASK = 0x0F,
HDMI_FC_PRCONF_OUTPUT_PR_FACTOR_OFFSET = 0,
+/* FC_PACKET_TX_EN field values */
+ HDMI_FC_PACKET_TX_EN_DRM_MASK = 0x80,
+ HDMI_FC_PACKET_TX_EN_DRM_ENABLE = 0x80,
+ HDMI_FC_PACKET_TX_EN_DRM_DISABLE = 0x00,
+
/* FC_AVICONF0-FC_AVICONF3 field values */
HDMI_FC_AVICONF0_PIX_FMT_MASK = 0x03,
HDMI_FC_AVICONF0_PIX_FMT_RGB = 0x00,
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 0f0e82638fbe..04d1ec60f218 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -131,6 +131,7 @@ struct dw_hdmi_plat_data {
unsigned long input_bus_format;
unsigned long input_bus_encoding;
bool ycbcr_420_allowed;
+ bool drm_infoframe;
/* Vendor PHY support */
const struct dw_hdmi_phy_ops *phy_ops;
--
2.17.1
This patch enables Dynamic Range and Mastering InfoFrame on RK3328 and RK3399.
Cc: Sandy Huang <[email protected]>
Cc: Heiko Stuebner <[email protected]>
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 4cdc9f86c2e5..1f31f3726f04 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -405,6 +405,7 @@ static const struct dw_hdmi_plat_data rk3328_hdmi_drv_data = {
.phy_ops = &rk3328_hdmi_phy_ops,
.phy_name = "inno_dw_hdmi_phy2",
.phy_force_vendor = true,
+ .drm_infoframe = true,
};
static struct rockchip_hdmi_chip_data rk3399_chip_data = {
@@ -419,6 +420,7 @@ static const struct dw_hdmi_plat_data rk3399_hdmi_drv_data = {
.cur_ctr = rockchip_cur_ctr,
.phy_config = rockchip_phy_config,
.phy_data = &rk3399_chip_data,
+ .drm_infoframe = true,
};
static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
--
2.17.1
This patch enables Dynamic Range and Mastering InfoFrame on GXL, GXM and G12A.
Cc: Neil Armstrong <[email protected]>
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index df3f9ddd2234..f7761e698c03 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -966,6 +966,11 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
+ if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
+ dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
+ dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
+ dw_plat_data->drm_infoframe = true;
+
platform_set_drvdata(pdev, meson_dw_hdmi);
meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
--
2.17.1
This patch enables Dynamic Range and Mastering InfoFrame on H6.
Cc: Maxime Ripard <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 39d8509d96a0..b80164dd8ad8 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
sun8i_hdmi_phy_init(hdmi->phy);
plat_data->mode_valid = hdmi->quirks->mode_valid;
+ plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
platform_set_drvdata(pdev, hdmi);
@@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = {
static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
.mode_valid = sun8i_dw_hdmi_mode_valid_h6,
+ .drm_infoframe = true,
};
static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
index 720c5aa8adc1..2a0ec08ee236 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
@@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
const struct drm_display_mode *mode);
unsigned int set_rate : 1;
+ unsigned int drm_infoframe : 1;
};
struct sun8i_dw_hdmi {
--
2.17.1
Hi!
Dne nedelja, 26. maj 2019 ob 23:18:46 CEST je Jonas Karlman napisal(a):
> Add support for HDR metadata using the hdr_output_metadata connector
> property, configure Dynamic Range and Mastering InfoFrame accordingly.
>
> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> can use to signal when Dynamic Range and Mastering infoframes is supported.
> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI
> version, and only GXL support DRM InfoFrame.
>
> The first patch add functionality to configure DRM InfoFrame based on the
> hdr_output_metadata connector property.
>
> The remaining patches sets the drm_infoframe flag on some SoCs supporting
> Dynamic Range and Mastering InfoFrame.
>
> Note that this was based on top of drm-misc-next and Neil Armstrong's
> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
>
> [1] https://patchwork.freedesktop.org/series/58725/#rev2
For Allwinner H6:
Tested-by: Jernej Skrabec <[email protected]>
Thanks for working on this!
Best regards,
Jernej
On 26/05/2019 23:20, Jonas Karlman wrote:
> This patch enables Dynamic Range and Mastering InfoFrame on GXL, GXM and G12A.
>
> Cc: Neil Armstrong <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index df3f9ddd2234..f7761e698c03 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -966,6 +966,11 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
> dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
>
> + if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> + dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
> + dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
> + dw_plat_data->drm_infoframe = true;
> +
> platform_set_drvdata(pdev, meson_dw_hdmi);
>
> meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
>
Reviewed-by: Neil Armstrong <[email protected]>
On 26/05/2019 23:19, Jonas Karlman wrote:
> Add support for configuring Dynamic Range and Mastering InfoFrame from
> the hdr_output_metadata connector property.
>
> This patch adds a drm_infoframe flag to dw_hdmi_plat_data that platform drivers
> use to signal when Dynamic Range and Mastering infoframes is supported.
> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> and only GXL support DRM InfoFrame.
>
> These changes were based on work done by Zheng Yang <[email protected]>
> to support DRM InfoFrame on the Rockchip 4.4 BSP kernel at [1] and [2]
>
> [1] https://github.com/rockchip-linux/kernel/tree/develop-4.4
> [2] https://github.com/rockchip-linux/kernel/commit/d1943fde81ff41d7cca87f4a42f03992e90bddd5
>
> Cc: Zheng Yang <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 ++++++++
> include/drm/bridge/dw_hdmi.h | 1 +
> 3 files changed, 147 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 284ce59be8f8..801bbbd732fd 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -24,6 +24,7 @@
>
> #include <drm/drm_of.h>
> #include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_encoder_slave.h>
> @@ -1592,6 +1593,78 @@ static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> HDMI_FC_DATAUTO0_VSD_MASK);
> }
>
> +#define HDR_LSB(n) ((n) & 0xff)
> +#define HDR_MSB(n) (((n) & 0xff00) >> 8)
> +
> +static void hdmi_config_drm_infoframe(struct dw_hdmi *hdmi)
> +{
> + const struct drm_connector_state *conn_state = hdmi->connector.state;
> + struct hdmi_drm_infoframe frame;
> + int ret;
> +
> + if (hdmi->version < 0x200a || !hdmi->plat_data->drm_infoframe)
> + return;
> +
> + hdmi_modb(hdmi, HDMI_FC_PACKET_TX_EN_DRM_DISABLE,
> + HDMI_FC_PACKET_TX_EN_DRM_MASK, HDMI_FC_PACKET_TX_EN);
> +
> + ret = drm_hdmi_infoframe_set_hdr_metadata(&frame, conn_state);
> + if (ret < 0)
> + return;
> +
> + ret = hdmi_drm_infoframe_check(&frame);
> + if (WARN_ON(ret))
> + return;
> +
> + hdmi_writeb(hdmi, frame.version, HDMI_FC_DRM_HB0);
> + hdmi_writeb(hdmi, frame.length, HDMI_FC_DRM_HB1);
> + hdmi_writeb(hdmi, frame.eotf, HDMI_FC_DRM_PB0);
> + hdmi_writeb(hdmi, frame.metadata_type, HDMI_FC_DRM_PB1);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[0].x),
> + HDMI_FC_DRM_PB2);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[0].x),
> + HDMI_FC_DRM_PB3);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[0].y),
> + HDMI_FC_DRM_PB4);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[0].y),
> + HDMI_FC_DRM_PB5);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[1].x),
> + HDMI_FC_DRM_PB6);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[1].x),
> + HDMI_FC_DRM_PB7);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[1].y),
> + HDMI_FC_DRM_PB8);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[1].y),
> + HDMI_FC_DRM_PB9);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[2].x),
> + HDMI_FC_DRM_PB10);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[2].x),
> + HDMI_FC_DRM_PB11);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[2].y),
> + HDMI_FC_DRM_PB12);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[2].y),
> + HDMI_FC_DRM_PB13);
> + hdmi_writeb(hdmi, HDR_LSB(frame.white_point.x), HDMI_FC_DRM_PB14);
> + hdmi_writeb(hdmi, HDR_MSB(frame.white_point.x), HDMI_FC_DRM_PB15);
> + hdmi_writeb(hdmi, HDR_LSB(frame.white_point.y), HDMI_FC_DRM_PB16);
> + hdmi_writeb(hdmi, HDR_MSB(frame.white_point.y), HDMI_FC_DRM_PB17);
> + hdmi_writeb(hdmi, HDR_LSB(frame.max_display_mastering_luminance),
> + HDMI_FC_DRM_PB18);
> + hdmi_writeb(hdmi, HDR_MSB(frame.max_display_mastering_luminance),
> + HDMI_FC_DRM_PB19);
> + hdmi_writeb(hdmi, HDR_LSB(frame.min_display_mastering_luminance),
> + HDMI_FC_DRM_PB20);
> + hdmi_writeb(hdmi, HDR_MSB(frame.min_display_mastering_luminance),
> + HDMI_FC_DRM_PB21);
> + hdmi_writeb(hdmi, HDR_LSB(frame.max_cll), HDMI_FC_DRM_PB22);
> + hdmi_writeb(hdmi, HDR_MSB(frame.max_cll), HDMI_FC_DRM_PB23);
> + hdmi_writeb(hdmi, HDR_LSB(frame.max_fall), HDMI_FC_DRM_PB24);
> + hdmi_writeb(hdmi, HDR_MSB(frame.max_fall), HDMI_FC_DRM_PB25);
> + hdmi_writeb(hdmi, 1, HDMI_FC_DRM_UP);
> + hdmi_modb(hdmi, HDMI_FC_PACKET_TX_EN_DRM_ENABLE,
> + HDMI_FC_PACKET_TX_EN_DRM_MASK, HDMI_FC_PACKET_TX_EN);
> +}
> +
> static void hdmi_av_composer(struct dw_hdmi *hdmi,
> const struct drm_display_mode *mode)
> {
> @@ -1963,6 +2036,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> /* HDMI Initialization Step F - Configure AVI InfoFrame */
> hdmi_config_AVI(hdmi, mode);
> hdmi_config_vendor_specific_infoframe(hdmi, mode);
> + hdmi_config_drm_infoframe(hdmi);
> } else {
> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
> }
> @@ -2139,6 +2213,36 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> return ret;
> }
>
> +static bool blob_equal(const struct drm_property_blob *a,
> + const struct drm_property_blob *b)
> +{
> + if (a && b)
> + return a->length == b->length &&
> + !memcmp(a->data, b->data, a->length);
> +
> + return !a == !b;
> +}
> +
> +static int dw_hdmi_connector_atomic_check(struct drm_connector *conn,
> + struct drm_connector_state *new_state)
> +{
> + struct drm_connector_state *old_state =
> + drm_atomic_get_old_connector_state(new_state->state, conn);
> + struct drm_crtc_state *crtc_state;
> +
> + if (!new_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> + new_state->crtc);
> +
> + if (!blob_equal(new_state->hdr_output_metadata,
> + old_state->hdr_output_metadata))
> + crtc_state->mode_changed = true;
> +
> + return 0;
> +}
> +
> static void dw_hdmi_connector_force(struct drm_connector *connector)
> {
> struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> @@ -2163,6 +2267,7 @@ 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,
> };
>
> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
> @@ -2179,6 +2284,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
> drm_connector_init(bridge->dev, connector, &dw_hdmi_connector_funcs,
> DRM_MODE_CONNECTOR_HDMIA);
>
> + if (hdmi->version >= 0x200a && hdmi->plat_data->drm_infoframe)
> + drm_object_attach_property(&connector->base,
> + connector->dev->mode_config.hdr_output_metadata_property, 0);
> +
> drm_connector_attach_encoder(connector, encoder);
>
> return 0;
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 3f3c616eba97..d4efbec14f68 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -256,6 +256,7 @@
> #define HDMI_FC_POL2 0x10DB
> #define HDMI_FC_PRCONF 0x10E0
> #define HDMI_FC_SCRAMBLER_CTRL 0x10E1
> +#define HDMI_FC_PACKET_TX_EN 0x10E3
>
> #define HDMI_FC_GMD_STAT 0x1100
> #define HDMI_FC_GMD_EN 0x1101
> @@ -291,6 +292,37 @@
> #define HDMI_FC_GMD_PB26 0x111F
> #define HDMI_FC_GMD_PB27 0x1120
>
> +#define HDMI_FC_DRM_UP 0x1167
> +#define HDMI_FC_DRM_HB0 0x1168
> +#define HDMI_FC_DRM_HB1 0x1169
> +#define HDMI_FC_DRM_PB0 0x116A
> +#define HDMI_FC_DRM_PB1 0x116B
> +#define HDMI_FC_DRM_PB2 0x116C
> +#define HDMI_FC_DRM_PB3 0x116D
> +#define HDMI_FC_DRM_PB4 0x116E
> +#define HDMI_FC_DRM_PB5 0x116F
> +#define HDMI_FC_DRM_PB6 0x1170
> +#define HDMI_FC_DRM_PB7 0x1171
> +#define HDMI_FC_DRM_PB8 0x1172
> +#define HDMI_FC_DRM_PB9 0x1173
> +#define HDMI_FC_DRM_PB10 0x1174
> +#define HDMI_FC_DRM_PB11 0x1175
> +#define HDMI_FC_DRM_PB12 0x1176
> +#define HDMI_FC_DRM_PB13 0x1177
> +#define HDMI_FC_DRM_PB14 0x1178
> +#define HDMI_FC_DRM_PB15 0x1179
> +#define HDMI_FC_DRM_PB16 0x117A
> +#define HDMI_FC_DRM_PB17 0x117B
> +#define HDMI_FC_DRM_PB18 0x117C
> +#define HDMI_FC_DRM_PB19 0x117D
> +#define HDMI_FC_DRM_PB20 0x117E
> +#define HDMI_FC_DRM_PB21 0x117F
> +#define HDMI_FC_DRM_PB22 0x1180
> +#define HDMI_FC_DRM_PB23 0x1181
> +#define HDMI_FC_DRM_PB24 0x1182
> +#define HDMI_FC_DRM_PB25 0x1183
> +#define HDMI_FC_DRM_PB26 0x1184
> +
> #define HDMI_FC_DBGFORCE 0x1200
> #define HDMI_FC_DBGAUD0CH0 0x1201
> #define HDMI_FC_DBGAUD1CH0 0x1202
> @@ -746,6 +778,11 @@ enum {
> HDMI_FC_PRCONF_OUTPUT_PR_FACTOR_MASK = 0x0F,
> HDMI_FC_PRCONF_OUTPUT_PR_FACTOR_OFFSET = 0,
>
> +/* FC_PACKET_TX_EN field values */
> + HDMI_FC_PACKET_TX_EN_DRM_MASK = 0x80,
> + HDMI_FC_PACKET_TX_EN_DRM_ENABLE = 0x80,
> + HDMI_FC_PACKET_TX_EN_DRM_DISABLE = 0x00,
> +
> /* FC_AVICONF0-FC_AVICONF3 field values */
> HDMI_FC_AVICONF0_PIX_FMT_MASK = 0x03,
> HDMI_FC_AVICONF0_PIX_FMT_RGB = 0x00,
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 0f0e82638fbe..04d1ec60f218 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -131,6 +131,7 @@ struct dw_hdmi_plat_data {
> unsigned long input_bus_format;
> unsigned long input_bus_encoding;
> bool ycbcr_420_allowed;
> + bool drm_infoframe;
>
> /* Vendor PHY support */
> const struct dw_hdmi_phy_ops *phy_ops;
>
Thanks for this work !
Reviewed-by: Neil Armstrong <[email protected]>
Am Sonntag, 26. Mai 2019, 23:20:05 CEST schrieb Jonas Karlman:
> This patch enables Dynamic Range and Mastering InfoFrame on RK3328 and RK3399.
>
> Cc: Sandy Huang <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
> ---
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 4cdc9f86c2e5..1f31f3726f04 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -405,6 +405,7 @@ static const struct dw_hdmi_plat_data rk3328_hdmi_drv_data = {
> .phy_ops = &rk3328_hdmi_phy_ops,
> .phy_name = "inno_dw_hdmi_phy2",
> .phy_force_vendor = true,
> + .drm_infoframe = true,
> };
>
> static struct rockchip_hdmi_chip_data rk3399_chip_data = {
> @@ -419,6 +420,7 @@ static const struct dw_hdmi_plat_data rk3399_hdmi_drv_data = {
> .cur_ctr = rockchip_cur_ctr,
> .phy_config = rockchip_phy_config,
> .phy_data = &rk3399_chip_data,
> + .drm_infoframe = true,
> };
>
> static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
> --
> 2.17.1
>
>
Hi Andrzej,
Gentle ping, could you review the dw-hdmi changes here ?
Thanks,
Neil
On 26/05/2019 23:18, Jonas Karlman wrote:
> Add support for HDR metadata using the hdr_output_metadata connector property,
> configure Dynamic Range and Mastering InfoFrame accordingly.
>
> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> can use to signal when Dynamic Range and Mastering infoframes is supported.
> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> and only GXL support DRM InfoFrame.
>
> The first patch add functionality to configure DRM InfoFrame based on the
> hdr_output_metadata connector property.
>
> The remaining patches sets the drm_infoframe flag on some SoCs supporting
> Dynamic Range and Mastering InfoFrame.
>
> Note that this was based on top of drm-misc-next and Neil Armstrong's
> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
>
> [1] https://patchwork.freedesktop.org/series/58725/#rev2
>
> Jonas Karlman (4):
> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
> drm/sun4i: Enable DRM InfoFrame support on H6
>
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> include/drm/bridge/dw_hdmi.h | 1 +
> 7 files changed, 157 insertions(+)
>
On Thu, Jun 20, 2019 at 04:40:12PM +0200, Neil Armstrong wrote:
> Hi Andrzej,
>
> Gentle ping, could you review the dw-hdmi changes here ?
btw not sure you absolutely need review from Andrzej, we're currently a
bit undersupplied with bridge reviewers I think ... Better to ramp up
more.
-Daniel
>
> Thanks,
> Neil
>
> On 26/05/2019 23:18, Jonas Karlman wrote:
> > Add support for HDR metadata using the hdr_output_metadata connector property,
> > configure Dynamic Range and Mastering InfoFrame accordingly.
> >
> > A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> > can use to signal when Dynamic Range and Mastering infoframes is supported.
> > This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> > and only GXL support DRM InfoFrame.
> >
> > The first patch add functionality to configure DRM InfoFrame based on the
> > hdr_output_metadata connector property.
> >
> > The remaining patches sets the drm_infoframe flag on some SoCs supporting
> > Dynamic Range and Mastering InfoFrame.
> >
> > Note that this was based on top of drm-misc-next and Neil Armstrong's
> > "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
> >
> > [1] https://patchwork.freedesktop.org/series/58725/#rev2
> >
> > Jonas Karlman (4):
> > drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
> > drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
> > drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
> > drm/sun4i: Enable DRM InfoFrame support on H6
> >
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
> > drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> > include/drm/bridge/dw_hdmi.h | 1 +
> > 7 files changed, 157 insertions(+)
> >
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Fri, Jun 21, 2019 at 11:01:25AM +0200, Daniel Vetter wrote:
> On Thu, Jun 20, 2019 at 04:40:12PM +0200, Neil Armstrong wrote:
> > Hi Andrzej,
> >
> > Gentle ping, could you review the dw-hdmi changes here ?
>
> btw not sure you absolutely need review from Andrzej, we're currently a
> bit undersupplied with bridge reviewers I think ... Better to ramp up
> more.
I try to review DRM bridge patches when possible, but dw-hdmi is a
special case. I was told by the supplier of an SoC datasheet that
contains the HDMI encoder IP core documentation that Synopsys required
them to route all contributions made based on that documentation through
Synopsys' internal legal review before publishing them. I thus decided
to not contribute to the driver anymore, at least for areas that require
access to documentation.
> > On 26/05/2019 23:18, Jonas Karlman wrote:
> > > Add support for HDR metadata using the hdr_output_metadata connector property,
> > > configure Dynamic Range and Mastering InfoFrame accordingly.
> > >
> > > A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> > > can use to signal when Dynamic Range and Mastering infoframes is supported.
> > > This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> > > and only GXL support DRM InfoFrame.
> > >
> > > The first patch add functionality to configure DRM InfoFrame based on the
> > > hdr_output_metadata connector property.
> > >
> > > The remaining patches sets the drm_infoframe flag on some SoCs supporting
> > > Dynamic Range and Mastering InfoFrame.
> > >
> > > Note that this was based on top of drm-misc-next and Neil Armstrong's
> > > "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
> > >
> > > [1] https://patchwork.freedesktop.org/series/58725/#rev2
> > >
> > > Jonas Karlman (4):
> > > drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
> > > drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
> > > drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
> > > drm/sun4i: Enable DRM InfoFrame support on H6
> > >
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
> > > drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> > > include/drm/bridge/dw_hdmi.h | 1 +
> > > 7 files changed, 157 insertions(+)
--
Regards,
Laurent Pinchart
Hi Daniel, Laurent, Andrzej,
On 24/06/2019 01:30, Laurent Pinchart wrote:
> On Fri, Jun 21, 2019 at 11:01:25AM +0200, Daniel Vetter wrote:
>> On Thu, Jun 20, 2019 at 04:40:12PM +0200, Neil Armstrong wrote:
>>> Hi Andrzej,
>>>
>>> Gentle ping, could you review the dw-hdmi changes here ?
>>
>> btw not sure you absolutely need review from Andrzej, we're currently a
>> bit undersupplied with bridge reviewers I think ... Better to ramp up
>> more.
>
> I try to review DRM bridge patches when possible, but dw-hdmi is a
> special case. I was told by the supplier of an SoC datasheet that
> contains the HDMI encoder IP core documentation that Synopsys required
> them to route all contributions made based on that documentation through
> Synopsys' internal legal review before publishing them. I thus decided
> to not contribute to the driver anymore, at least for areas that require
> access to documentation.
I'd like to propose myself as co-maintainer of the DRM bridge subsystem if
everybody agrees, following the excellent work Laurent and Andrzej did.
I have a very little knowledge of DSI, & other bridge drivers, but I'll do
my best.
For the dw-hdmi driver, we have a big roadmap including :
- HDR (this patchset)
- HDCP 1/2
- YUV420, YUV422, YUV44, 10bit/12bit/16bit HDMI output
- Enhanced audio support and ELD notification to ASoC
...
Having a more active maintainer/reviewer team would be needed, at least for
the dw-hdmi bridge.
I'll also propose Jonas Karlman <[email protected]> as reviewer since he is very
active for the multimedia support on RockChip, Allwinner and Amlogic SoCs.
I'll also propose Jernej [email protected] <[email protected]>, if he wants,
as reviewer since he is very active on the Allwinner SoCs side.
Neil
>
>>> On 26/05/2019 23:18, Jonas Karlman wrote:
>>>> Add support for HDR metadata using the hdr_output_metadata connector property,
>>>> configure Dynamic Range and Mastering InfoFrame accordingly.
>>>>
>>>> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
>>>> can use to signal when Dynamic Range and Mastering infoframes is supported.
>>>> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
>>>> and only GXL support DRM InfoFrame.
>>>>
>>>> The first patch add functionality to configure DRM InfoFrame based on the
>>>> hdr_output_metadata connector property.
>>>>
>>>> The remaining patches sets the drm_infoframe flag on some SoCs supporting
>>>> Dynamic Range and Mastering InfoFrame.
>>>>
>>>> Note that this was based on top of drm-misc-next and Neil Armstrong's
>>>> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
>>>>
>>>> [1] https://patchwork.freedesktop.org/series/58725/#rev2
>>>>
>>>> Jonas Karlman (4):
>>>> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
>>>> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
>>>> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
>>>> drm/sun4i: Enable DRM InfoFrame support on H6
>>>>
>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
>>>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
>>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
>>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
>>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
>>>> include/drm/bridge/dw_hdmi.h | 1 +
>>>> 7 files changed, 157 insertions(+)
>
On Mon, Jun 24, 2019 at 10:19:34AM +0200, Neil Armstrong wrote:
> Hi Daniel, Laurent, Andrzej,
>
> On 24/06/2019 01:30, Laurent Pinchart wrote:
> > On Fri, Jun 21, 2019 at 11:01:25AM +0200, Daniel Vetter wrote:
> >> On Thu, Jun 20, 2019 at 04:40:12PM +0200, Neil Armstrong wrote:
> >>> Hi Andrzej,
> >>>
> >>> Gentle ping, could you review the dw-hdmi changes here ?
> >>
> >> btw not sure you absolutely need review from Andrzej, we're currently a
> >> bit undersupplied with bridge reviewers I think ... Better to ramp up
> >> more.
> >
> > I try to review DRM bridge patches when possible, but dw-hdmi is a
> > special case. I was told by the supplier of an SoC datasheet that
> > contains the HDMI encoder IP core documentation that Synopsys required
> > them to route all contributions made based on that documentation through
> > Synopsys' internal legal review before publishing them. I thus decided
> > to not contribute to the driver anymore, at least for areas that require
> > access to documentation.
>
> I'd like to propose myself as co-maintainer of the DRM bridge subsystem if
> everybody agrees, following the excellent work Laurent and Andrzej did.
> I have a very little knowledge of DSI, & other bridge drivers, but I'll do
> my best.
Yay volunteers!
Make MAINTAINERS patch to add you, cc relevant people, get acks, merge,
tag you're it :-) Ok, co-it, the point is to have teams as much as
possible.
Cheers, Daniel
>
> For the dw-hdmi driver, we have a big roadmap including :
> - HDR (this patchset)
> - HDCP 1/2
> - YUV420, YUV422, YUV44, 10bit/12bit/16bit HDMI output
> - Enhanced audio support and ELD notification to ASoC
> ...
>
> Having a more active maintainer/reviewer team would be needed, at least for
> the dw-hdmi bridge.
>
> I'll also propose Jonas Karlman <[email protected]> as reviewer since he is very
> active for the multimedia support on RockChip, Allwinner and Amlogic SoCs.
> I'll also propose Jernej [email protected] <[email protected]>, if he wants,
> as reviewer since he is very active on the Allwinner SoCs side.
>
> Neil
>
> >
> >>> On 26/05/2019 23:18, Jonas Karlman wrote:
> >>>> Add support for HDR metadata using the hdr_output_metadata connector property,
> >>>> configure Dynamic Range and Mastering InfoFrame accordingly.
> >>>>
> >>>> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> >>>> can use to signal when Dynamic Range and Mastering infoframes is supported.
> >>>> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> >>>> and only GXL support DRM InfoFrame.
> >>>>
> >>>> The first patch add functionality to configure DRM InfoFrame based on the
> >>>> hdr_output_metadata connector property.
> >>>>
> >>>> The remaining patches sets the drm_infoframe flag on some SoCs supporting
> >>>> Dynamic Range and Mastering InfoFrame.
> >>>>
> >>>> Note that this was based on top of drm-misc-next and Neil Armstrong's
> >>>> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
> >>>>
> >>>> [1] https://patchwork.freedesktop.org/series/58725/#rev2
> >>>>
> >>>> Jonas Karlman (4):
> >>>> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
> >>>> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
> >>>> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
> >>>> drm/sun4i: Enable DRM InfoFrame support on H6
> >>>>
> >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
> >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
> >>>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> >>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> >>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> >>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> >>>> include/drm/bridge/dw_hdmi.h | 1 +
> >>>> 7 files changed, 157 insertions(+)
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Neil,
On Mon, Jun 24, 2019 at 10:19:34AM +0200, Neil Armstrong wrote:
> Hi Daniel, Laurent, Andrzej,
>
> On 24/06/2019 01:30, Laurent Pinchart wrote:
> > On Fri, Jun 21, 2019 at 11:01:25AM +0200, Daniel Vetter wrote:
> >> On Thu, Jun 20, 2019 at 04:40:12PM +0200, Neil Armstrong wrote:
> >>> Hi Andrzej,
> >>>
> >>> Gentle ping, could you review the dw-hdmi changes here ?
> >>
> >> btw not sure you absolutely need review from Andrzej, we're currently a
> >> bit undersupplied with bridge reviewers I think ... Better to ramp up
> >> more.
> >
> > I try to review DRM bridge patches when possible, but dw-hdmi is a
> > special case. I was told by the supplier of an SoC datasheet that
> > contains the HDMI encoder IP core documentation that Synopsys required
> > them to route all contributions made based on that documentation through
> > Synopsys' internal legal review before publishing them. I thus decided
> > to not contribute to the driver anymore, at least for areas that require
> > access to documentation.
>
> I'd like to propose myself as co-maintainer of the DRM bridge subsystem if
> everybody agrees, following the excellent work Laurent and Andrzej did.
> I have a very little knowledge of DSI, & other bridge drivers, but I'll do
> my best.
>
> For the dw-hdmi driver, we have a big roadmap including :
> - HDR (this patchset)
> - HDCP 1/2
> - YUV420, YUV422, YUV44, 10bit/12bit/16bit HDMI output
> - Enhanced audio support and ELD notification to ASoC
> ...
You're more than welcome as a DRM bridge maintainer, especially given
that you have just volunteered to implement bridge states and format
negotiation, right ? ;-)
> Having a more active maintainer/reviewer team would be needed, at least for
> the dw-hdmi bridge.
>
> I'll also propose Jonas Karlman <[email protected]> as reviewer since he is very
> active for the multimedia support on RockChip, Allwinner and Amlogic SoCs.
> I'll also propose Jernej [email protected] <[email protected]>, if he wants,
> as reviewer since he is very active on the Allwinner SoCs side.
>
> >>> On 26/05/2019 23:18, Jonas Karlman wrote:
> >>>> Add support for HDR metadata using the hdr_output_metadata connector property,
> >>>> configure Dynamic Range and Mastering InfoFrame accordingly.
> >>>>
> >>>> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> >>>> can use to signal when Dynamic Range and Mastering infoframes is supported.
> >>>> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> >>>> and only GXL support DRM InfoFrame.
> >>>>
> >>>> The first patch add functionality to configure DRM InfoFrame based on the
> >>>> hdr_output_metadata connector property.
> >>>>
> >>>> The remaining patches sets the drm_infoframe flag on some SoCs supporting
> >>>> Dynamic Range and Mastering InfoFrame.
> >>>>
> >>>> Note that this was based on top of drm-misc-next and Neil Armstrong's
> >>>> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
> >>>>
> >>>> [1] https://patchwork.freedesktop.org/series/58725/#rev2
> >>>>
> >>>> Jonas Karlman (4):
> >>>> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
> >>>> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
> >>>> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
> >>>> drm/sun4i: Enable DRM InfoFrame support on H6
> >>>>
> >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
> >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
> >>>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> >>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> >>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> >>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> >>>> include/drm/bridge/dw_hdmi.h | 1 +
> >>>> 7 files changed, 157 insertions(+)
--
Regards,
Laurent Pinchart
On 26.05.2019 23:19, Jonas Karlman wrote:
> Add support for configuring Dynamic Range and Mastering InfoFrame from
> the hdr_output_metadata connector property.
>
> This patch adds a drm_infoframe flag to dw_hdmi_plat_data that platform drivers
> use to signal when Dynamic Range and Mastering infoframes is supported.
> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> and only GXL support DRM InfoFrame.
>
> These changes were based on work done by Zheng Yang <[email protected]>
> to support DRM InfoFrame on the Rockchip 4.4 BSP kernel at [1] and [2]
>
> [1] https://github.com/rockchip-linux/kernel/tree/develop-4.4
> [2] https://github.com/rockchip-linux/kernel/commit/d1943fde81ff41d7cca87f4a42f03992e90bddd5
>
> Cc: Zheng Yang <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 ++++++++
> include/drm/bridge/dw_hdmi.h | 1 +
> 3 files changed, 147 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 284ce59be8f8..801bbbd732fd 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -24,6 +24,7 @@
>
> #include <drm/drm_of.h>
> #include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_encoder_slave.h>
> @@ -1592,6 +1593,78 @@ static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> HDMI_FC_DATAUTO0_VSD_MASK);
> }
>
> +#define HDR_LSB(n) ((n) & 0xff)
> +#define HDR_MSB(n) (((n) & 0xff00) >> 8)
> +
> +static void hdmi_config_drm_infoframe(struct dw_hdmi *hdmi)
> +{
> + const struct drm_connector_state *conn_state = hdmi->connector.state;
> + struct hdmi_drm_infoframe frame;
> + int ret;
> +
> + if (hdmi->version < 0x200a || !hdmi->plat_data->drm_infoframe)
Why do we need this double check? Here and in dw_hdmi_bridge_attach.
Wouldn't be enough to test drm_infoframe?
> + return;
> +
> + hdmi_modb(hdmi, HDMI_FC_PACKET_TX_EN_DRM_DISABLE,
> + HDMI_FC_PACKET_TX_EN_DRM_MASK, HDMI_FC_PACKET_TX_EN);
> +
> + ret = drm_hdmi_infoframe_set_hdr_metadata(&frame, conn_state);
> + if (ret < 0)
> + return;
> +
> + ret = hdmi_drm_infoframe_check(&frame);
> + if (WARN_ON(ret))
> + return;
> +
> + hdmi_writeb(hdmi, frame.version, HDMI_FC_DRM_HB0);
> + hdmi_writeb(hdmi, frame.length, HDMI_FC_DRM_HB1);
> + hdmi_writeb(hdmi, frame.eotf, HDMI_FC_DRM_PB0);
> + hdmi_writeb(hdmi, frame.metadata_type, HDMI_FC_DRM_PB1);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[0].x),
> + HDMI_FC_DRM_PB2);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[0].x),
> + HDMI_FC_DRM_PB3);
Adding two-byte version hdmi_writew would allow to remove multiple lines
here, but since register names and their relative location reflects
structure of raw frame you should be able to replace this
long/boring/error-prone sequence of writes with something like:
len = hdmi_infoframe_pack(frame, buf, len);
if (len < 0)
...
for (i = 0; i < len; ++i)
hdmi_writeb(hdmi, buf[i], HDMI_FC_DRM_HB0 + i);
... or something similar.
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[0].y),
> + HDMI_FC_DRM_PB4);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[0].y),
> + HDMI_FC_DRM_PB5);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[1].x),
> + HDMI_FC_DRM_PB6);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[1].x),
> + HDMI_FC_DRM_PB7);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[1].y),
> + HDMI_FC_DRM_PB8);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[1].y),
> + HDMI_FC_DRM_PB9);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[2].x),
> + HDMI_FC_DRM_PB10);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[2].x),
> + HDMI_FC_DRM_PB11);
> + hdmi_writeb(hdmi, HDR_LSB(frame.display_primaries[2].y),
> + HDMI_FC_DRM_PB12);
> + hdmi_writeb(hdmi, HDR_MSB(frame.display_primaries[2].y),
> + HDMI_FC_DRM_PB13);
> + hdmi_writeb(hdmi, HDR_LSB(frame.white_point.x), HDMI_FC_DRM_PB14);
> + hdmi_writeb(hdmi, HDR_MSB(frame.white_point.x), HDMI_FC_DRM_PB15);
> + hdmi_writeb(hdmi, HDR_LSB(frame.white_point.y), HDMI_FC_DRM_PB16);
> + hdmi_writeb(hdmi, HDR_MSB(frame.white_point.y), HDMI_FC_DRM_PB17);
> + hdmi_writeb(hdmi, HDR_LSB(frame.max_display_mastering_luminance),
> + HDMI_FC_DRM_PB18);
> + hdmi_writeb(hdmi, HDR_MSB(frame.max_display_mastering_luminance),
> + HDMI_FC_DRM_PB19);
> + hdmi_writeb(hdmi, HDR_LSB(frame.min_display_mastering_luminance),
> + HDMI_FC_DRM_PB20);
> + hdmi_writeb(hdmi, HDR_MSB(frame.min_display_mastering_luminance),
> + HDMI_FC_DRM_PB21);
> + hdmi_writeb(hdmi, HDR_LSB(frame.max_cll), HDMI_FC_DRM_PB22);
> + hdmi_writeb(hdmi, HDR_MSB(frame.max_cll), HDMI_FC_DRM_PB23);
> + hdmi_writeb(hdmi, HDR_LSB(frame.max_fall), HDMI_FC_DRM_PB24);
> + hdmi_writeb(hdmi, HDR_MSB(frame.max_fall), HDMI_FC_DRM_PB25);
> + hdmi_writeb(hdmi, 1, HDMI_FC_DRM_UP);
> + hdmi_modb(hdmi, HDMI_FC_PACKET_TX_EN_DRM_ENABLE,
> + HDMI_FC_PACKET_TX_EN_DRM_MASK, HDMI_FC_PACKET_TX_EN);
> +}
> +
> static void hdmi_av_composer(struct dw_hdmi *hdmi,
> const struct drm_display_mode *mode)
> {
> @@ -1963,6 +2036,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> /* HDMI Initialization Step F - Configure AVI InfoFrame */
> hdmi_config_AVI(hdmi, mode);
> hdmi_config_vendor_specific_infoframe(hdmi, mode);
> + hdmi_config_drm_infoframe(hdmi);
> } else {
> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
> }
> @@ -2139,6 +2213,36 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> return ret;
> }
>
> +static bool blob_equal(const struct drm_property_blob *a,
> + const struct drm_property_blob *b)
Quite generic function, but since there are no other users(?) it can
stay here.
> +{
> + if (a && b)
> + return a->length == b->length &&
> + !memcmp(a->data, b->data, a->length);
> +
> + return !a == !b;
Ugly code (but it is probably matter of taste), maybe sth like this:
if (!a || !b)
return a != b;
if (a->length != b->length)
return false;
return !memcmp(a->data, b->data, a->length);
> +}
> +
> +static int dw_hdmi_connector_atomic_check(struct drm_connector *conn,
> + struct drm_connector_state *new_state)
> +{
> + struct drm_connector_state *old_state =
> + drm_atomic_get_old_connector_state(new_state->state, conn);
> + struct drm_crtc_state *crtc_state;
> +
> + if (!new_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> + new_state->crtc);
> +
> + if (!blob_equal(new_state->hdr_output_metadata,
> + old_state->hdr_output_metadata))
> + crtc_state->mode_changed = true;
> +
> + return 0;
> +}
> +
> static void dw_hdmi_connector_force(struct drm_connector *connector)
> {
> struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> @@ -2163,6 +2267,7 @@ 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,
> };
>
> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
> @@ -2179,6 +2284,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge)
> drm_connector_init(bridge->dev, connector, &dw_hdmi_connector_funcs,
> DRM_MODE_CONNECTOR_HDMIA);
>
> + if (hdmi->version >= 0x200a && hdmi->plat_data->drm_infoframe)
> + drm_object_attach_property(&connector->base,
> + connector->dev->mode_config.hdr_output_metadata_property, 0);
> +
> drm_connector_attach_encoder(connector, encoder);
>
> return 0;
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 3f3c616eba97..d4efbec14f68 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -256,6 +256,7 @@
> #define HDMI_FC_POL2 0x10DB
> #define HDMI_FC_PRCONF 0x10E0
> #define HDMI_FC_SCRAMBLER_CTRL 0x10E1
> +#define HDMI_FC_PACKET_TX_EN 0x10E3
>
> #define HDMI_FC_GMD_STAT 0x1100
> #define HDMI_FC_GMD_EN 0x1101
> @@ -291,6 +292,37 @@
> #define HDMI_FC_GMD_PB26 0x111F
> #define HDMI_FC_GMD_PB27 0x1120
>
> +#define HDMI_FC_DRM_UP 0x1167
> +#define HDMI_FC_DRM_HB0 0x1168
> +#define HDMI_FC_DRM_HB1 0x1169
> +#define HDMI_FC_DRM_PB0 0x116A
> +#define HDMI_FC_DRM_PB1 0x116B
> +#define HDMI_FC_DRM_PB2 0x116C
> +#define HDMI_FC_DRM_PB3 0x116D
> +#define HDMI_FC_DRM_PB4 0x116E
> +#define HDMI_FC_DRM_PB5 0x116F
> +#define HDMI_FC_DRM_PB6 0x1170
> +#define HDMI_FC_DRM_PB7 0x1171
> +#define HDMI_FC_DRM_PB8 0x1172
> +#define HDMI_FC_DRM_PB9 0x1173
> +#define HDMI_FC_DRM_PB10 0x1174
> +#define HDMI_FC_DRM_PB11 0x1175
> +#define HDMI_FC_DRM_PB12 0x1176
> +#define HDMI_FC_DRM_PB13 0x1177
> +#define HDMI_FC_DRM_PB14 0x1178
> +#define HDMI_FC_DRM_PB15 0x1179
> +#define HDMI_FC_DRM_PB16 0x117A
> +#define HDMI_FC_DRM_PB17 0x117B
> +#define HDMI_FC_DRM_PB18 0x117C
> +#define HDMI_FC_DRM_PB19 0x117D
> +#define HDMI_FC_DRM_PB20 0x117E
> +#define HDMI_FC_DRM_PB21 0x117F
> +#define HDMI_FC_DRM_PB22 0x1180
> +#define HDMI_FC_DRM_PB23 0x1181
> +#define HDMI_FC_DRM_PB24 0x1182
> +#define HDMI_FC_DRM_PB25 0x1183
> +#define HDMI_FC_DRM_PB26 0x1184
> +
> #define HDMI_FC_DBGFORCE 0x1200
> #define HDMI_FC_DBGAUD0CH0 0x1201
> #define HDMI_FC_DBGAUD1CH0 0x1202
> @@ -746,6 +778,11 @@ enum {
> HDMI_FC_PRCONF_OUTPUT_PR_FACTOR_MASK = 0x0F,
> HDMI_FC_PRCONF_OUTPUT_PR_FACTOR_OFFSET = 0,
>
> +/* FC_PACKET_TX_EN field values */
> + HDMI_FC_PACKET_TX_EN_DRM_MASK = 0x80,
> + HDMI_FC_PACKET_TX_EN_DRM_ENABLE = 0x80,
> + HDMI_FC_PACKET_TX_EN_DRM_DISABLE = 0x00,
> +
> /* FC_AVICONF0-FC_AVICONF3 field values */
> HDMI_FC_AVICONF0_PIX_FMT_MASK = 0x03,
> HDMI_FC_AVICONF0_PIX_FMT_RGB = 0x00,
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 0f0e82638fbe..04d1ec60f218 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -131,6 +131,7 @@ struct dw_hdmi_plat_data {
> unsigned long input_bus_format;
> unsigned long input_bus_encoding;
> bool ycbcr_420_allowed;
> + bool drm_infoframe;
maybe sth less confusing, for example: use_drm_infoframe?
Regards
Andrzej
>
> /* Vendor PHY support */
> const struct dw_hdmi_phy_ops *phy_ops;
On 26.05.2019 23:20, Jonas Karlman wrote:
> This patch enables Dynamic Range and Mastering InfoFrame on RK3328 and RK3399.
>
> Cc: Sandy Huang <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
--
Regards
Andrzej
> ---
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 4cdc9f86c2e5..1f31f3726f04 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -405,6 +405,7 @@ static const struct dw_hdmi_plat_data rk3328_hdmi_drv_data = {
> .phy_ops = &rk3328_hdmi_phy_ops,
> .phy_name = "inno_dw_hdmi_phy2",
> .phy_force_vendor = true,
> + .drm_infoframe = true,
> };
>
> static struct rockchip_hdmi_chip_data rk3399_chip_data = {
> @@ -419,6 +420,7 @@ static const struct dw_hdmi_plat_data rk3399_hdmi_drv_data = {
> .cur_ctr = rockchip_cur_ctr,
> .phy_config = rockchip_phy_config,
> .phy_data = &rk3399_chip_data,
> + .drm_infoframe = true,
> };
>
> static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
On 26.05.2019 23:20, Jonas Karlman wrote:
> This patch enables Dynamic Range and Mastering InfoFrame on GXL, GXM and G12A.
>
> Cc: Neil Armstrong <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index df3f9ddd2234..f7761e698c03 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -966,6 +966,11 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
> dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
>
> + if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
> + dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
> + dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
> + dw_plat_data->drm_infoframe = true;
> +
I see it follows meson_dw_hdmi.c practices, but maybe it is time to drop
it and just add flag to meson_dw_hdmi_data, IMO the whole
dw_hdmi_is_compatible function should be removed
and replaced with fields/flags in meson_dw_hdmi_data - this is what
of_device_id.data field was created for.
Regards
Andrzej
> platform_set_drvdata(pdev, meson_dw_hdmi);
>
> meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
On 26.05.2019 23:20, Jonas Karlman wrote:
> This patch enables Dynamic Range and Mastering InfoFrame on H6.
>
> Cc: Maxime Ripard <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> index 39d8509d96a0..b80164dd8ad8 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> sun8i_hdmi_phy_init(hdmi->phy);
>
> plat_data->mode_valid = hdmi->quirks->mode_valid;
> + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
> sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
>
> platform_set_drvdata(pdev, hdmi);
> @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = {
>
> static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
> .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
> + .drm_infoframe = true,
> };
>
> static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> index 720c5aa8adc1..2a0ec08ee236 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
> enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
> const struct drm_display_mode *mode);
> unsigned int set_rate : 1;
> + unsigned int drm_infoframe : 1;
Again, drm_infoframe suggests it contains inforframe, but in fact it
just informs infoframe can be used, so again my suggestion
use_drm_infoframe.
Moreover bool type seems more appropriate here.
Beside this:
Reviewed-by: Andrzej Hajda <[email protected]>
--
Regards
Andrzej
> };
>
> struct sun8i_dw_hdmi {
Dne ponedeljek, 24. junij 2019 ob 17:03:31 CEST je Andrzej Hajda napisal(a):
> On 26.05.2019 23:20, Jonas Karlman wrote:
> > This patch enables Dynamic Range and Mastering InfoFrame on H6.
> >
> > Cc: Maxime Ripard <[email protected]>
> > Cc: Jernej Skrabec <[email protected]>
> > Signed-off-by: Jonas Karlman <[email protected]>
> > ---
> >
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index 39d8509d96a0..b80164dd8ad8
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev,
> > struct device *master,>
> > sun8i_hdmi_phy_init(hdmi->phy);
> >
> > plat_data->mode_valid = hdmi->quirks->mode_valid;
> >
> > + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
> >
> > sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> >
> > platform_set_drvdata(pdev, hdmi);
> >
> > @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks
> > sun8i_a83t_quirks = {>
> > static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
> >
> > .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
> >
> > + .drm_infoframe = true,
> >
> > };
> >
> > static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 720c5aa8adc1..2a0ec08ee236
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
> >
> > enum drm_mode_status (*mode_valid)(struct drm_connector
*connector,
> >
> > const struct
drm_display_mode *mode);
> >
> > unsigned int set_rate : 1;
> >
> > + unsigned int drm_infoframe : 1;
>
> Again, drm_infoframe suggests it contains inforframe, but in fact it
> just informs infoframe can be used, so again my suggestion
> use_drm_infoframe.
>
> Moreover bool type seems more appropriate here.
checkpatch will give warning if bool is used.
Best regards,
Jernej
>
> Beside this:
>
> Reviewed-by: Andrzej Hajda <[email protected]>
>
> --
> Regards
> Andrzej
>
> > };
> >
> > struct sun8i_dw_hdmi {
On 24.06.2019 13:16, Laurent Pinchart wrote:
> Hi Neil,
>
> On Mon, Jun 24, 2019 at 10:19:34AM +0200, Neil Armstrong wrote:
>> Hi Daniel, Laurent, Andrzej,
>>
>> On 24/06/2019 01:30, Laurent Pinchart wrote:
>>> On Fri, Jun 21, 2019 at 11:01:25AM +0200, Daniel Vetter wrote:
>>>> On Thu, Jun 20, 2019 at 04:40:12PM +0200, Neil Armstrong wrote:
>>>>> Hi Andrzej,
>>>>>
>>>>> Gentle ping, could you review the dw-hdmi changes here ?
>>>> btw not sure you absolutely need review from Andrzej, we're currently a
>>>> bit undersupplied with bridge reviewers I think ... Better to ramp up
>>>> more.
>>> I try to review DRM bridge patches when possible, but dw-hdmi is a
>>> special case. I was told by the supplier of an SoC datasheet that
>>> contains the HDMI encoder IP core documentation that Synopsys required
>>> them to route all contributions made based on that documentation through
>>> Synopsys' internal legal review before publishing them. I thus decided
>>> to not contribute to the driver anymore, at least for areas that require
>>> access to documentation.
>> I'd like to propose myself as co-maintainer of the DRM bridge subsystem if
>> everybody agrees, following the excellent work Laurent and Andrzej did.
>> I have a very little knowledge of DSI, & other bridge drivers, but I'll do
>> my best.
>>
>> For the dw-hdmi driver, we have a big roadmap including :
>> - HDR (this patchset)
>> - HDCP 1/2
>> - YUV420, YUV422, YUV44, 10bit/12bit/16bit HDMI output
>> - Enhanced audio support and ELD notification to ASoC
>> ...
> You're more than welcome as a DRM bridge maintainer, especially given
> that you have just volunteered to implement bridge states and format
> negotiation, right ? ;-)
Two (even three) birds with one stone :)
>
>> Having a more active maintainer/reviewer team would be needed, at least for
>> the dw-hdmi bridge.
>>
>> I'll also propose Jonas Karlman <[email protected]> as reviewer since he is very
>> active for the multimedia support on RockChip, Allwinner and Amlogic SoCs.
>> I'll also propose Jernej [email protected] <[email protected]>, if he wants,
>> as reviewer since he is very active on the Allwinner SoCs side.
Welcome on board, I will wait one/two days for possible comments, then I
will queue MAINTAINERS patch.
Regards
Andrzej
>>
>>>>> On 26/05/2019 23:18, Jonas Karlman wrote:
>>>>>> Add support for HDR metadata using the hdr_output_metadata connector property,
>>>>>> configure Dynamic Range and Mastering InfoFrame accordingly.
>>>>>>
>>>>>> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
>>>>>> can use to signal when Dynamic Range and Mastering infoframes is supported.
>>>>>> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
>>>>>> and only GXL support DRM InfoFrame.
>>>>>>
>>>>>> The first patch add functionality to configure DRM InfoFrame based on the
>>>>>> hdr_output_metadata connector property.
>>>>>>
>>>>>> The remaining patches sets the drm_infoframe flag on some SoCs supporting
>>>>>> Dynamic Range and Mastering InfoFrame.
>>>>>>
>>>>>> Note that this was based on top of drm-misc-next and Neil Armstrong's
>>>>>> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
>>>>>>
>>>>>> [1] https://patchwork.freedesktop.org/series/58725/#rev2
>>>>>>
>>>>>> Jonas Karlman (4):
>>>>>> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
>>>>>> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
>>>>>> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
>>>>>> drm/sun4i: Enable DRM InfoFrame support on H6
>>>>>>
>>>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
>>>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
>>>>>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
>>>>>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
>>>>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
>>>>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
>>>>>> include/drm/bridge/dw_hdmi.h | 1 +
>>>>>> 7 files changed, 157 insertions(+)
On 24.06.2019 17:05, Jernej Škrabec wrote:
> Dne ponedeljek, 24. junij 2019 ob 17:03:31 CEST je Andrzej Hajda napisal(a):
>> On 26.05.2019 23:20, Jonas Karlman wrote:
>>> This patch enables Dynamic Range and Mastering InfoFrame on H6.
>>>
>>> Cc: Maxime Ripard <[email protected]>
>>> Cc: Jernej Skrabec <[email protected]>
>>> Signed-off-by: Jonas Karlman <[email protected]>
>>> ---
>>>
>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
>>> 2 files changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index 39d8509d96a0..b80164dd8ad8
>>> 100644
>>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>> @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev,
>>> struct device *master,>
>>> sun8i_hdmi_phy_init(hdmi->phy);
>>>
>>> plat_data->mode_valid = hdmi->quirks->mode_valid;
>>>
>>> + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
>>>
>>> sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
>>>
>>> platform_set_drvdata(pdev, hdmi);
>>>
>>> @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks
>>> sun8i_a83t_quirks = {>
>>> static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
>>>
>>> .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
>>>
>>> + .drm_infoframe = true,
>>>
>>> };
>>>
>>> static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
>>>
>>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 720c5aa8adc1..2a0ec08ee236
>>> 100644
>>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>> @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
>>>
>>> enum drm_mode_status (*mode_valid)(struct drm_connector
> *connector,
>>>
>>> const struct
> drm_display_mode *mode);
>>>
>>> unsigned int set_rate : 1;
>>>
>>> + unsigned int drm_infoframe : 1;
>> Again, drm_infoframe suggests it contains inforframe, but in fact it
>> just informs infoframe can be used, so again my suggestion
>> use_drm_infoframe.
>>
>> Moreover bool type seems more appropriate here.
> checkpatch will give warning if bool is used.
Then I would say "fix/ignore checkpatch" :)
But maybe there is a reason.
Anyway I've tested and I do not see the warning, could you elaborate it.
Regards
Andrzej
>
> Best regards,
> Jernej
>
>> Beside this:
>>
>> Reviewed-by: Andrzej Hajda <[email protected]>
>>
>> --
>> Regards
>> Andrzej
>>
>>> };
>>>
>>> struct sun8i_dw_hdmi {
>
>
>
>
>
On Mon, Jun 24, 2019 at 11:49 PM Andrzej Hajda <[email protected]> wrote:
>
> On 24.06.2019 17:05, Jernej Škrabec wrote:
> > Dne ponedeljek, 24. junij 2019 ob 17:03:31 CEST je Andrzej Hajda napisal(a):
> >> On 26.05.2019 23:20, Jonas Karlman wrote:
> >>> This patch enables Dynamic Range and Mastering InfoFrame on H6.
> >>>
> >>> Cc: Maxime Ripard <[email protected]>
> >>> Cc: Jernej Skrabec <[email protected]>
> >>> Signed-off-by: Jonas Karlman <[email protected]>
> >>> ---
> >>>
> >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
> >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> >>> 2 files changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index 39d8509d96a0..b80164dd8ad8
> >>> 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> >>> @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev,
> >>> struct device *master,>
> >>> sun8i_hdmi_phy_init(hdmi->phy);
> >>>
> >>> plat_data->mode_valid = hdmi->quirks->mode_valid;
> >>>
> >>> + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
> >>>
> >>> sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> >>>
> >>> platform_set_drvdata(pdev, hdmi);
> >>>
> >>> @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks
> >>> sun8i_a83t_quirks = {>
> >>> static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
> >>>
> >>> .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
> >>>
> >>> + .drm_infoframe = true,
> >>>
> >>> };
> >>>
> >>> static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
> >>>
> >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 720c5aa8adc1..2a0ec08ee236
> >>> 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> >>> @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
> >>>
> >>> enum drm_mode_status (*mode_valid)(struct drm_connector
> > *connector,
> >>>
> >>> const struct
> > drm_display_mode *mode);
> >>>
> >>> unsigned int set_rate : 1;
> >>>
> >>> + unsigned int drm_infoframe : 1;
> >> Again, drm_infoframe suggests it contains inforframe, but in fact it
> >> just informs infoframe can be used, so again my suggestion
> >> use_drm_infoframe.
> >>
> >> Moreover bool type seems more appropriate here.
> > checkpatch will give warning if bool is used.
>
>
> Then I would say "fix/ignore checkpatch" :)
>
> But maybe there is a reason.
Here's an old one from Linus: https://lkml.org/lkml/2013/9/1/154
I'd say that bool in a struct is a waste of space compared to a 1 bit bitfield,
especially when there already are other bitfields in the same struct.
> Anyway I've tested and I do not see the warning, could you elaborate it.
Maybe checkpatch.pl --strict?
ChenYu
Dne ponedeljek, 24. junij 2019 ob 17:56:30 CEST je Chen-Yu Tsai napisal(a):
> On Mon, Jun 24, 2019 at 11:49 PM Andrzej Hajda <[email protected]> wrote:
> > On 24.06.2019 17:05, Jernej Škrabec wrote:
> > > Dne ponedeljek, 24. junij 2019 ob 17:03:31 CEST je Andrzej Hajda
napisal(a):
> > >> On 26.05.2019 23:20, Jonas Karlman wrote:
> > >>> This patch enables Dynamic Range and Mastering InfoFrame on H6.
> > >>>
> > >>> Cc: Maxime Ripard <[email protected]>
> > >>> Cc: Jernej Skrabec <[email protected]>
> > >>> Signed-off-by: Jonas Karlman <[email protected]>
> > >>> ---
> > >>>
> > >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
> > >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> > >>> 2 files changed, 3 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index
> > >>> 39d8509d96a0..b80164dd8ad8
> > >>> 100644
> > >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > >>> @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev,
> > >>> struct device *master,>
> > >>>
> > >>> sun8i_hdmi_phy_init(hdmi->phy);
> > >>>
> > >>> plat_data->mode_valid = hdmi->quirks->mode_valid;
> > >>>
> > >>> + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
> > >>>
> > >>> sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> > >>>
> > >>> platform_set_drvdata(pdev, hdmi);
> > >>>
> > >>> @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks
> > >>> sun8i_a83t_quirks = {>
> > >>>
> > >>> static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
> > >>>
> > >>> .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
> > >>>
> > >>> + .drm_infoframe = true,
> > >>>
> > >>> };
> > >>>
> > >>> static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
> > >>>
> > >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index
> > >>> 720c5aa8adc1..2a0ec08ee236
> > >>> 100644
> > >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > >>> @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
> > >>>
> > >>> enum drm_mode_status (*mode_valid)(struct drm_connector
> > >
> > > *connector,
> > >
> > >>> const struct
> > >
> > > drm_display_mode *mode);
> > >
> > >>> unsigned int set_rate : 1;
> > >>>
> > >>> + unsigned int drm_infoframe : 1;
> > >>
> > >> Again, drm_infoframe suggests it contains inforframe, but in fact it
> > >> just informs infoframe can be used, so again my suggestion
> > >> use_drm_infoframe.
> > >>
> > >> Moreover bool type seems more appropriate here.
> > >
> > > checkpatch will give warning if bool is used.
> >
> > Then I would say "fix/ignore checkpatch" :)
> >
> > But maybe there is a reason.
>
> Here's an old one from Linus: https://lkml.org/lkml/2013/9/1/154
>
> I'd say that bool in a struct is a waste of space compared to a 1 bit
> bitfield, especially when there already are other bitfields in the same
> struct.
> > Anyway I've tested and I do not see the warning, could you elaborate it.
>
> Maybe checkpatch.pl --strict?
It seems they removed that check:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?
id=7967656ffbfa493f5546c0f1
After reading that block of text, I'm not sure what would be prefered way for
this case.
Best regards,
Jernej
On Tue, Jun 25, 2019 at 12:03 AM Jernej Škrabec <[email protected]> wrote:
>
> Dne ponedeljek, 24. junij 2019 ob 17:56:30 CEST je Chen-Yu Tsai napisal(a):
> > On Mon, Jun 24, 2019 at 11:49 PM Andrzej Hajda <[email protected]> wrote:
> > > On 24.06.2019 17:05, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 24. junij 2019 ob 17:03:31 CEST je Andrzej Hajda
> napisal(a):
> > > >> On 26.05.2019 23:20, Jonas Karlman wrote:
> > > >>> This patch enables Dynamic Range and Mastering InfoFrame on H6.
> > > >>>
> > > >>> Cc: Maxime Ripard <[email protected]>
> > > >>> Cc: Jernej Skrabec <[email protected]>
> > > >>> Signed-off-by: Jonas Karlman <[email protected]>
> > > >>> ---
> > > >>>
> > > >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
> > > >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> > > >>> 2 files changed, 3 insertions(+)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > > >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index
> > > >>> 39d8509d96a0..b80164dd8ad8
> > > >>> 100644
> > > >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > > >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > > >>> @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev,
> > > >>> struct device *master,>
> > > >>>
> > > >>> sun8i_hdmi_phy_init(hdmi->phy);
> > > >>>
> > > >>> plat_data->mode_valid = hdmi->quirks->mode_valid;
> > > >>>
> > > >>> + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
> > > >>>
> > > >>> sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> > > >>>
> > > >>> platform_set_drvdata(pdev, hdmi);
> > > >>>
> > > >>> @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks
> > > >>> sun8i_a83t_quirks = {>
> > > >>>
> > > >>> static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
> > > >>>
> > > >>> .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
> > > >>>
> > > >>> + .drm_infoframe = true,
> > > >>>
> > > >>> };
> > > >>>
> > > >>> static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > > >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index
> > > >>> 720c5aa8adc1..2a0ec08ee236
> > > >>> 100644
> > > >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > > >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > > >>> @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
> > > >>>
> > > >>> enum drm_mode_status (*mode_valid)(struct drm_connector
> > > >
> > > > *connector,
> > > >
> > > >>> const struct
> > > >
> > > > drm_display_mode *mode);
> > > >
> > > >>> unsigned int set_rate : 1;
> > > >>>
> > > >>> + unsigned int drm_infoframe : 1;
> > > >>
> > > >> Again, drm_infoframe suggests it contains inforframe, but in fact it
> > > >> just informs infoframe can be used, so again my suggestion
> > > >> use_drm_infoframe.
> > > >>
> > > >> Moreover bool type seems more appropriate here.
> > > >
> > > > checkpatch will give warning if bool is used.
> > >
> > > Then I would say "fix/ignore checkpatch" :)
> > >
> > > But maybe there is a reason.
> >
> > Here's an old one from Linus: https://lkml.org/lkml/2013/9/1/154
> >
> > I'd say that bool in a struct is a waste of space compared to a 1 bit
> > bitfield, especially when there already are other bitfields in the same
> > struct.
> > > Anyway I've tested and I do not see the warning, could you elaborate it.
> >
> > Maybe checkpatch.pl --strict?
>
> It seems they removed that check:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?
> id=7967656ffbfa493f5546c0f1
>
> After reading that block of text, I'm not sure what would be prefered way for
> this case.
This:
+If a structure has many true/false values, consider consolidating them into a
+bitfield with 1 bit members, or using an appropriate fixed width type, such as
+u8.
would suggest using a bitfield, or flags within a fixed width type?
ChenYu
On 24.06.2019 18:07, Chen-Yu Tsai wrote:
> On Tue, Jun 25, 2019 at 12:03 AM Jernej Škrabec <[email protected]> wrote:
>> Dne ponedeljek, 24. junij 2019 ob 17:56:30 CEST je Chen-Yu Tsai napisal(a):
>>> On Mon, Jun 24, 2019 at 11:49 PM Andrzej Hajda <[email protected]> wrote:
>>>> On 24.06.2019 17:05, Jernej Škrabec wrote:
>>>>> Dne ponedeljek, 24. junij 2019 ob 17:03:31 CEST je Andrzej Hajda
>> napisal(a):
>>>>>> On 26.05.2019 23:20, Jonas Karlman wrote:
>>>>>>> This patch enables Dynamic Range and Mastering InfoFrame on H6.
>>>>>>>
>>>>>>> Cc: Maxime Ripard <[email protected]>
>>>>>>> Cc: Jernej Skrabec <[email protected]>
>>>>>>> Signed-off-by: Jonas Karlman <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 ++
>>>>>>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
>>>>>>> 2 files changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>>>>>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index
>>>>>>> 39d8509d96a0..b80164dd8ad8
>>>>>>> 100644
>>>>>>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>>>>>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>>>>>> @@ -189,6 +189,7 @@ static int sun8i_dw_hdmi_bind(struct device *dev,
>>>>>>> struct device *master,>
>>>>>>>
>>>>>>> sun8i_hdmi_phy_init(hdmi->phy);
>>>>>>>
>>>>>>> plat_data->mode_valid = hdmi->quirks->mode_valid;
>>>>>>>
>>>>>>> + plat_data->drm_infoframe = hdmi->quirks->drm_infoframe;
>>>>>>>
>>>>>>> sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
>>>>>>>
>>>>>>> platform_set_drvdata(pdev, hdmi);
>>>>>>>
>>>>>>> @@ -255,6 +256,7 @@ static const struct sun8i_dw_hdmi_quirks
>>>>>>> sun8i_a83t_quirks = {>
>>>>>>>
>>>>>>> static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = {
>>>>>>>
>>>>>>> .mode_valid = sun8i_dw_hdmi_mode_valid_h6,
>>>>>>>
>>>>>>> + .drm_infoframe = true,
>>>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>>>>>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index
>>>>>>> 720c5aa8adc1..2a0ec08ee236
>>>>>>> 100644
>>>>>>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>>>>>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>>>>>> @@ -178,6 +178,7 @@ struct sun8i_dw_hdmi_quirks {
>>>>>>>
>>>>>>> enum drm_mode_status (*mode_valid)(struct drm_connector
>>>>> *connector,
>>>>>
>>>>>>> const struct
>>>>> drm_display_mode *mode);
>>>>>
>>>>>>> unsigned int set_rate : 1;
>>>>>>>
>>>>>>> + unsigned int drm_infoframe : 1;
>>>>>> Again, drm_infoframe suggests it contains inforframe, but in fact it
>>>>>> just informs infoframe can be used, so again my suggestion
>>>>>> use_drm_infoframe.
>>>>>>
>>>>>> Moreover bool type seems more appropriate here.
>>>>> checkpatch will give warning if bool is used.
>>>> Then I would say "fix/ignore checkpatch" :)
>>>>
>>>> But maybe there is a reason.
>>> Here's an old one from Linus: https://lkml.org/lkml/2013/9/1/154
>>>
>>> I'd say that bool in a struct is a waste of space compared to a 1 bit
>>> bitfield, especially when there already are other bitfields in the same
>>> struct.
>>>> Anyway I've tested and I do not see the warning, could you elaborate it.
>>> Maybe checkpatch.pl --strict?
>> It seems they removed that check:
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?
>> id=7967656ffbfa493f5546c0f1
>>
>> After reading that block of text, I'm not sure what would be prefered way for
>> this case.
> This:
>
> +If a structure has many true/false values, consider consolidating them into a
> +bitfield with 1 bit members, or using an appropriate fixed width type, such as
> +u8.
>
> would suggest using a bitfield, or flags within a fixed width type?
OK, I have also guessed what kind of warning Jernej was writing about.
And IMO it rather does not fit here:
- no concurrent writes,
- no need for size/cache optimizations.
But since there are some controversies about bool in struct and file has
already convention of bitfield I do not insist on it, you can keep it as is.
Regards
Andrzej
>
> ChenYu
>
>
On 24/06/2019 16:59, Andrzej Hajda wrote:
> On 26.05.2019 23:20, Jonas Karlman wrote:
>> This patch enables Dynamic Range and Mastering InfoFrame on GXL, GXM and G12A.
>>
>> Cc: Neil Armstrong <[email protected]>
>> Signed-off-by: Jonas Karlman <[email protected]>
>> ---
>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index df3f9ddd2234..f7761e698c03 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -966,6 +966,11 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>> dw_plat_data->input_bus_format = MEDIA_BUS_FMT_YUV8_1X24;
>> dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
>>
>> + if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
>> + dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
>> + dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
>> + dw_plat_data->drm_infoframe = true;
>> +
>
>
> I see it follows meson_dw_hdmi.c practices, but maybe it is time to drop
> it and just add flag to meson_dw_hdmi_data, IMO the whole
> dw_hdmi_is_compatible function should be removed
>
> and replaced with fields/flags in meson_dw_hdmi_data - this is what
> of_device_id.data field was created for.
It's in our cleanup process, this will pushed shortly, but in the meantime it
can be left as-is until cleanup is pushed, I'll sort it manually if both
happens in the same time.
Neil
>
>
> Regards
>
> Andrzej
>
>
>> platform_set_drvdata(pdev, meson_dw_hdmi);
>>
>> meson_dw_hdmi->hdmi = dw_hdmi_bind(pdev, encoder,
>
>
Hi Jonas,
On 26/05/2019 23:18, Jonas Karlman wrote:
> Add support for HDR metadata using the hdr_output_metadata connector property,
> configure Dynamic Range and Mastering InfoFrame accordingly.
>
> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
> can use to signal when Dynamic Range and Mastering infoframes is supported.
> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
> and only GXL support DRM InfoFrame.
>
> The first patch add functionality to configure DRM InfoFrame based on the
> hdr_output_metadata connector property.
>
> The remaining patches sets the drm_infoframe flag on some SoCs supporting
> Dynamic Range and Mastering InfoFrame.
>
> Note that this was based on top of drm-misc-next and Neil Armstrong's
> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
Do you plan to resend this serie ?
The "drm/meson: Add support for HDMI2.0 YUV420 4k60" serie is no more
valid with the format negociation work from boris, so you should rebase
on drm-misc-next !
Thanks,
Neil
>
> [1] https://patchwork.freedesktop.org/series/58725/#rev2
>
> Jonas Karlman (4):
> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
> drm/sun4i: Enable DRM InfoFrame support on H6
>
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> include/drm/bridge/dw_hdmi.h | 1 +
> 7 files changed, 157 insertions(+)
>
Hi Neil,
On 2019-09-18 10:05, Neil Armstrong wrote:
> Hi Jonas,
>
> On 26/05/2019 23:18, Jonas Karlman wrote:
>> Add support for HDR metadata using the hdr_output_metadata connector property,
>> configure Dynamic Range and Mastering InfoFrame accordingly.
>>
>> A drm_infoframe flag is added to dw_hdmi_plat_data that platform drivers
>> can use to signal when Dynamic Range and Mastering infoframes is supported.
>> This flag is needed because Amlogic GXBB and GXL report same DW-HDMI version,
>> and only GXL support DRM InfoFrame.
>>
>> The first patch add functionality to configure DRM InfoFrame based on the
>> hdr_output_metadata connector property.
>>
>> The remaining patches sets the drm_infoframe flag on some SoCs supporting
>> Dynamic Range and Mastering InfoFrame.
>>
>> Note that this was based on top of drm-misc-next and Neil Armstrong's
>> "drm/meson: Add support for HDMI2.0 YUV420 4k60" series at [1]
> Do you plan to resend this serie ?
Thanks for reminding me, I will send a v2 rebased on drm-misc-next as soon as I have some free time (next week).
Regards,
Jonas
>
> The "drm/meson: Add support for HDMI2.0 YUV420 4k60" serie is no more
> valid with the format negociation work from boris, so you should rebase
> on drm-misc-next !
>
> Thanks,
>
> Neil
>
>> [1] https://patchwork.freedesktop.org/series/58725/#rev2
>>
>> Jonas Karlman (4):
>> drm/bridge: dw-hdmi: Add Dynamic Range and Mastering InfoFrame support
>> drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
>> drm/meson: Enable DRM InfoFrame support on GXL, GXM and G12A
>> drm/sun4i: Enable DRM InfoFrame support on H6
>>
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109 ++++++++++++++++++++
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++++++
>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
>> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
>> include/drm/bridge/dw_hdmi.h | 1 +
>> 7 files changed, 157 insertions(+)
>>