2023-03-06 10:49:47

by Maxime Ripard

Subject: [PATCH v3 0/9] drm/vc4: hdmi: Broadcast RGB, BT601, BT2020

Hi,

Here's a collection of patches that have been in the downstream tree for a
while to add a bunch of new features to the HDMI controller.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <[email protected]>
---
Changes in v3:
- Convert only helper functions to container_of_const but drop the
direct calls in the driver
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Added a new patch to convert every state accessor to container_of_const
- Added a comment to mention why planes don't need to be checked
- Removed vc4_hdmi.broadcast_rgb field
- Reordered the CSC swap and CSC matrices organization patches to make it clearer
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Dave Stevenson (7):
drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range
drm/vc4: hdmi: Rename full range helper
drm/vc4: hdmi: Swap CSC matrix channels for YUV444
drm/vc4: hdmi: Rework the CSC matrices organization
drm/vc4: hdmi: Add a function to retrieve the CSC matrix
drm/vc4: hdmi: Add BT.601 Support
drm/vc4: hdmi: Add BT.2020 Support

Maxime Ripard (2):
drm/vc4: Switch to container_of_const
drm/vc4: hdmi: Update all the planes if the TV margins are changed

drivers/gpu/drm/vc4/tests/vc4_mock.h | 3 +
drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 4 +-
drivers/gpu/drm/vc4/vc4_dpi.c | 7 +-
drivers/gpu/drm/vc4/vc4_drv.h | 65 ++----
drivers/gpu/drm/vc4/vc4_dsi.c | 17 +-
drivers/gpu/drm/vc4/vc4_hdmi.c | 336 +++++++++++++++++++++++-----
drivers/gpu/drm/vc4/vc4_hdmi.h | 25 ++-
drivers/gpu/drm/vc4/vc4_kms.c | 16 +-
drivers/gpu/drm/vc4/vc4_plane.c | 3 +-
drivers/gpu/drm/vc4/vc4_txp.c | 12 +-
drivers/gpu/drm/vc4/vc4_vec.c | 14 +-
11 files changed, 341 insertions(+), 161 deletions(-)
---
base-commit: dc837c1a5137a8cf2e9432c1891392b6a66f4d8d
change-id: 20221207-rpi-hdmi-improvements-3de1c0dba2dc

Best regards,
--
Maxime Ripard <[email protected]>

2023-03-06 10:49:55

by Maxime Ripard

Subject: [PATCH v3 1/9] drm/vc4: Switch to container_of_const

container_of_const() allows to preserve the pointer constness and is
thus more flexible than inline functions.

Let's switch all our instances of container_of() to
container_of_const().

Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/tests/vc4_mock.h | 3 ++
drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 4 +-
drivers/gpu/drm/vc4/vc4_dpi.c | 7 +---
drivers/gpu/drm/vc4/vc4_drv.h | 65 +++++++++--------------------
drivers/gpu/drm/vc4/vc4_dsi.c | 17 +++-----
drivers/gpu/drm/vc4/vc4_hdmi.h | 16 +++----
drivers/gpu/drm/vc4/vc4_kms.c | 16 +++----
drivers/gpu/drm/vc4/vc4_plane.c | 3 +-
drivers/gpu/drm/vc4/vc4_txp.c | 12 ++----
drivers/gpu/drm/vc4/vc4_vec.c | 14 ++-----
10 files changed, 51 insertions(+), 106 deletions(-)

diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.h b/drivers/gpu/drm/vc4/tests/vc4_mock.h
index db8e9a141ef8..2d0b339bd9f3 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.h
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.h
@@ -43,6 +43,9 @@ struct vc4_dummy_output {
struct drm_connector connector;
};

+#define encoder_to_vc4_dummy_output(_enc) \
+ container_of_const(_enc, struct vc4_dummy_output, encoder.base)
+
struct vc4_dummy_output *vc4_dummy_output(struct kunit *test,
struct drm_device *drm,
struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
index 8d33be828d9a..6e11fcc9ef45 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock_output.c
@@ -80,7 +80,7 @@ int vc4_mock_atomic_add_output(struct kunit *test,
crtc = vc4_find_crtc_for_encoder(test, drm, encoder);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);

- output = container_of(encoder, struct vc4_dummy_output, encoder.base);
+ output = encoder_to_vc4_dummy_output(encoder);
conn = &output->connector;
conn_state = drm_atomic_get_connector_state(state, conn);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
@@ -126,7 +126,7 @@ int vc4_mock_atomic_del_output(struct kunit *test,
ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
KUNIT_ASSERT_EQ(test, ret, 0);

- output = container_of(encoder, struct vc4_dummy_output, encoder.base);
+ output = encoder_to_vc4_dummy_output(encoder);
conn = &output->connector;
conn_state = drm_atomic_get_connector_state(state, conn);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index f518d6e59ed6..e68c07d86040 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -97,11 +97,8 @@ struct vc4_dpi {
struct debugfs_regset32 regset;
};

-static inline struct vc4_dpi *
-to_vc4_dpi(struct drm_encoder *encoder)
-{
- return container_of(encoder, struct vc4_dpi, encoder.base);
-}
+#define to_vc4_dpi(_encoder) \
+ container_of_const(_encoder, struct vc4_dpi, encoder.base)

#define DPI_READ(offset) \
({ \
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 95069bb16821..e23084f3d6c2 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -232,11 +232,8 @@ struct vc4_dev {
struct kref bin_bo_kref;
};

-static inline struct vc4_dev *
-to_vc4_dev(const struct drm_device *dev)
-{
- return container_of(dev, struct vc4_dev, base);
-}
+#define to_vc4_dev(_dev) \
+ container_of_const(_dev, struct vc4_dev, base)

struct vc4_bo {
struct drm_gem_dma_object base;
@@ -285,11 +282,8 @@ struct vc4_bo {
struct mutex madv_lock;
};

-static inline struct vc4_bo *
-to_vc4_bo(const struct drm_gem_object *bo)
-{
- return container_of(to_drm_gem_dma_obj(bo), struct vc4_bo, base);
-}
+#define to_vc4_bo(_bo) \
+ container_of_const(to_drm_gem_dma_obj(_bo), struct vc4_bo, base)

struct vc4_fence {
struct dma_fence base;
@@ -298,11 +292,8 @@ struct vc4_fence {
uint64_t seqno;
};

-static inline struct vc4_fence *
-to_vc4_fence(const struct dma_fence *fence)
-{
- return container_of(fence, struct vc4_fence, base);
-}
+#define to_vc4_fence(_fence) \
+ container_of_const(_fence, struct vc4_fence, base)

struct vc4_seqno_cb {
struct work_struct work;
@@ -368,11 +359,8 @@ struct vc4_hvs_state {
} fifo_state[HVS_NUM_CHANNELS];
};

-static inline struct vc4_hvs_state *
-to_vc4_hvs_state(const struct drm_private_state *priv)
-{
- return container_of(priv, struct vc4_hvs_state, base);
-}
+#define to_vc4_hvs_state(_state) \
+ container_of_const(_state, struct vc4_hvs_state, base)

struct vc4_hvs_state *vc4_hvs_get_global_state(struct drm_atomic_state *state);
struct vc4_hvs_state *vc4_hvs_get_old_global_state(const struct drm_atomic_state *state);
@@ -382,11 +370,8 @@ struct vc4_plane {
struct drm_plane base;
};

-static inline struct vc4_plane *
-to_vc4_plane(const struct drm_plane *plane)
-{
- return container_of(plane, struct vc4_plane, base);
-}
+#define to_vc4_plane(_plane) \
+ container_of_const(_plane, struct vc4_plane, base)

enum vc4_scaling_mode {
VC4_SCALING_NONE,
@@ -458,11 +443,8 @@ struct vc4_plane_state {
u64 membus_load;
};

-static inline struct vc4_plane_state *
-to_vc4_plane_state(const struct drm_plane_state *state)
-{
- return container_of(state, struct vc4_plane_state, base);
-}
+#define to_vc4_plane_state(_state) \
+ container_of_const(_state, struct vc4_plane_state, base)

enum vc4_encoder_type {
VC4_ENCODER_TYPE_NONE,
@@ -489,11 +471,8 @@ struct vc4_encoder {
void (*post_crtc_powerdown)(struct drm_encoder *encoder, struct drm_atomic_state *state);
};

-static inline struct vc4_encoder *
-to_vc4_encoder(const struct drm_encoder *encoder)
-{
- return container_of(encoder, struct vc4_encoder, base);
-}
+#define to_vc4_encoder(_encoder) \
+ container_of_const(_encoder, struct vc4_encoder, base)

static inline
struct drm_encoder *vc4_find_encoder_by_type(struct drm_device *drm,
@@ -591,11 +570,8 @@ struct vc4_crtc {
unsigned int current_hvs_channel;
};

-static inline struct vc4_crtc *
-to_vc4_crtc(const struct drm_crtc *crtc)
-{
- return container_of(crtc, struct vc4_crtc, base);
-}
+#define to_vc4_crtc(_crtc) \
+ container_of_const(_crtc, struct vc4_crtc, base)

static inline const struct vc4_crtc_data *
vc4_crtc_to_vc4_crtc_data(const struct vc4_crtc *crtc)
@@ -608,7 +584,7 @@ vc4_crtc_to_vc4_pv_data(const struct vc4_crtc *crtc)
{
const struct vc4_crtc_data *data = vc4_crtc_to_vc4_crtc_data(crtc);

- return container_of(data, struct vc4_pv_data, base);
+ return container_of_const(data, struct vc4_pv_data, base);
}

struct drm_encoder *vc4_get_crtc_encoder(struct drm_crtc *crtc,
@@ -636,11 +612,8 @@ struct vc4_crtc_state {

#define VC4_HVS_CHANNEL_DISABLED ((unsigned int)-1)

-static inline struct vc4_crtc_state *
-to_vc4_crtc_state(const struct drm_crtc_state *crtc_state)
-{
- return container_of(crtc_state, struct vc4_crtc_state, base);
-}
+#define to_vc4_crtc_state(_state) \
+ container_of_const(_state, struct vc4_crtc_state, base)

#define V3D_READ(offset) \
({ \
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index a5c075f802e4..9e0c355b236f 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -600,19 +600,14 @@ struct vc4_dsi {
struct debugfs_regset32 regset;
};

-#define host_to_dsi(host) container_of(host, struct vc4_dsi, dsi_host)
+#define host_to_dsi(host) \
+ container_of_const(host, struct vc4_dsi, dsi_host)

-static inline struct vc4_dsi *
-to_vc4_dsi(struct drm_encoder *encoder)
-{
- return container_of(encoder, struct vc4_dsi, encoder.base);
-}
+#define to_vc4_dsi(_encoder) \
+ container_of_const(_encoder, struct vc4_dsi, encoder.base)

-static inline struct vc4_dsi *
-bridge_to_vc4_dsi(struct drm_bridge *bridge)
-{
- return container_of(bridge, struct vc4_dsi, bridge);
-}
+#define bridge_to_vc4_dsi(_bridge) \
+ container_of_const(_bridge, struct vc4_dsi, bridge)

static inline void
dsi_dma_workaround_write(struct vc4_dsi *dsi, u32 offset, u32 val)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index dc3ccd8002a0..5d249ac54cd1 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -223,17 +223,14 @@ struct vc4_hdmi {
enum vc4_hdmi_output_format output_format;
};

-static inline struct vc4_hdmi *
-connector_to_vc4_hdmi(struct drm_connector *connector)
-{
- return container_of(connector, struct vc4_hdmi, connector);
-}
+#define connector_to_vc4_hdmi(_connector) \
+ container_of_const(_connector, struct vc4_hdmi, connector)

static inline struct vc4_hdmi *
encoder_to_vc4_hdmi(struct drm_encoder *encoder)
{
struct vc4_encoder *_encoder = to_vc4_encoder(encoder);
- return container_of(_encoder, struct vc4_hdmi, encoder);
+ return container_of_const(_encoder, struct vc4_hdmi, encoder);
}

struct vc4_hdmi_connector_state {
@@ -243,11 +240,8 @@ struct vc4_hdmi_connector_state {
enum vc4_hdmi_output_format output_format;
};

-static inline struct vc4_hdmi_connector_state *
-conn_state_to_vc4_hdmi_conn_state(struct drm_connector_state *conn_state)
-{
- return container_of(conn_state, struct vc4_hdmi_connector_state, base);
-}
+#define conn_state_to_vc4_hdmi_conn_state(_state) \
+ container_of_const(_state, struct vc4_hdmi_connector_state, base)

void vc4_hdmi_phy_init(struct vc4_hdmi *vc4_hdmi,
struct vc4_hdmi_connector_state *vc4_conn_state);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index a7e3d47c50f4..5495f2a94fa9 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -31,11 +31,8 @@ struct vc4_ctm_state {
int fifo;
};

-static struct vc4_ctm_state *
-to_vc4_ctm_state(const struct drm_private_state *priv)
-{
- return container_of(priv, struct vc4_ctm_state, base);
-}
+#define to_vc4_ctm_state(_state) \
+ container_of_const(_state, struct vc4_ctm_state, base)

struct vc4_load_tracker_state {
struct drm_private_state base;
@@ -43,11 +40,8 @@ struct vc4_load_tracker_state {
u64 membus_load;
};

-static struct vc4_load_tracker_state *
-to_vc4_load_tracker_state(const struct drm_private_state *priv)
-{
- return container_of(priv, struct vc4_load_tracker_state, base);
-}
+#define to_vc4_load_tracker_state(_state) \
+ container_of_const(_state, struct vc4_load_tracker_state, base)

static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state *state,
struct drm_private_obj *manager)
@@ -717,7 +711,7 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
static void vc4_hvs_channels_print_state(struct drm_printer *p,
const struct drm_private_state *state)
{
- struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
+ const struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
unsigned int i;

drm_printf(p, "HVS State\n");
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 97c84a3f5a46..00e713faecd5 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1334,8 +1334,7 @@ u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist)

u32 vc4_plane_dlist_size(const struct drm_plane_state *state)
{
- const struct vc4_plane_state *vc4_state =
- container_of(state, typeof(*vc4_state), base);
+ const struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);

return vc4_state->dlist_count;
}
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index ef5cab2a3aa9..c5abdec03103 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -168,15 +168,11 @@ struct vc4_txp {
void __iomem *regs;
};

-static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
-{
- return container_of(encoder, struct vc4_txp, encoder.base);
-}
+#define encoder_to_vc4_txp(_encoder) \
+ container_of_const(_encoder, struct vc4_txp, encoder.base)

-static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
-{
- return container_of(conn, struct vc4_txp, connector.base);
-}
+#define connector_to_vc4_txp(_connector) \
+ container_of_const(_connector, struct vc4_txp, connector.base)

static const struct debugfs_reg32 txp_regs[] = {
VC4_REG32(TXP_DST_PTR),
diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index a3782d05cd66..d6e6a1a22eba 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -219,17 +219,11 @@ struct vc4_vec {
writel(val, vec->regs + (offset)); \
} while (0)

-static inline struct vc4_vec *
-encoder_to_vc4_vec(struct drm_encoder *encoder)
-{
- return container_of(encoder, struct vc4_vec, encoder.base);
-}
+#define encoder_to_vc4_vec(_encoder) \
+ container_of_const(_encoder, struct vc4_vec, encoder.base)

-static inline struct vc4_vec *
-connector_to_vc4_vec(struct drm_connector *connector)
-{
- return container_of(connector, struct vc4_vec, connector);
-}
+#define connector_to_vc4_vec(_connector) \
+ container_of_const(_connector, struct vc4_vec, connector)

enum vc4_vec_tv_mode_id {
VC4_VEC_TV_MODE_NTSC,

--
2.39.2

2023-03-06 10:49:59

by Maxime Ripard

Subject: [PATCH v3 2/9] drm/vc4: hdmi: Update all the planes if the TV margins are changed

On VC4, the TV margins on the HDMI connector are implemented by scaling
the planes.

However, if only the TV margins or the connector are changed by a new
state, the planes ending up on that connector won't be. Thus, they won't
be updated properly and we'll effectively ignore that change until the
next commit affecting these planes.

Let's make sure to add all the planes attached to the connector so that
we can update them properly.

Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ea22c9bf223a..522cfbc83fe4 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -535,6 +535,32 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
if (!crtc)
return 0;

+ if (old_state->tv.margins.left != new_state->tv.margins.left ||
+ old_state->tv.margins.right != new_state->tv.margins.right ||
+ old_state->tv.margins.top != new_state->tv.margins.top ||
+ old_state->tv.margins.bottom != new_state->tv.margins.bottom) {
+ struct drm_crtc_state *crtc_state;
+ int ret;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ /*
+ * Strictly speaking, we should be calling
+ * drm_atomic_helper_check_planes() after our call to
+ * drm_atomic_add_affected_planes(). However, the
+ * connector atomic_check is called as part of
+ * drm_atomic_helper_check_modeset() that already
+ * happens before a call to
+ * drm_atomic_helper_check_planes() in
+ * drm_atomic_helper_check().
+ */
+ ret = drm_atomic_add_affected_planes(state, crtc);
+ if (ret)
+ return ret;
+ }
+
if (old_state->colorspace != new_state->colorspace ||
!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
struct drm_crtc_state *crtc_state;

--
2.39.2

2023-03-06 10:50:03

by Maxime Ripard

Subject: [PATCH v3 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

From: Dave Stevenson <[email protected]>

Copy Intel's "Broadcast RGB" property semantics to add manual override
of the HDMI pixel range for monitors that don't abide by the content
of the AVI Infoframe.

Signed-off-by: Dave Stevenson <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 97 ++++++++++++++++++++++++++++++++++++++++--
drivers/gpu/drm/vc4/vc4_hdmi.h | 9 ++++
2 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 522cfbc83fe4..d23c0c3df2ee 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -154,10 +154,16 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
}

static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
- const struct drm_display_mode *mode)
+ struct vc4_hdmi_connector_state *vc4_state)
{
+ const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
struct drm_display_info *display = &vc4_hdmi->connector.display_info;

+ if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
+ return false;
+ else if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
+ return true;
+
return !display->is_hdmi ||
drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;
}
@@ -528,8 +534,12 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
{
struct drm_connector_state *old_state =
drm_atomic_get_old_connector_state(state, connector);
+ struct vc4_hdmi_connector_state *old_vc4_state =
+ conn_state_to_vc4_hdmi_conn_state(old_state);
struct drm_connector_state *new_state =
drm_atomic_get_new_connector_state(state, connector);
+ struct vc4_hdmi_connector_state *new_vc4_state =
+ conn_state_to_vc4_hdmi_conn_state(new_state);
struct drm_crtc *crtc = new_state->crtc;

if (!crtc)
@@ -562,6 +572,7 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
}

if (old_state->colorspace != new_state->colorspace ||
+ old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb ||
!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
struct drm_crtc_state *crtc_state;

@@ -575,6 +586,49 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
return 0;
}

+static int vc4_hdmi_connector_get_property(struct drm_connector *connector,
+ const struct drm_connector_state *state,
+ struct drm_property *property,
+ uint64_t *val)
+{
+ struct drm_device *drm = connector->dev;
+ struct vc4_hdmi *vc4_hdmi =
+ connector_to_vc4_hdmi(connector);
+ const struct vc4_hdmi_connector_state *vc4_conn_state =
+ conn_state_to_vc4_hdmi_conn_state(state);
+
+ if (property == vc4_hdmi->broadcast_rgb_property) {
+ *val = vc4_conn_state->broadcast_rgb;
+ } else {
+ drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
+ property->base.id, property->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int vc4_hdmi_connector_set_property(struct drm_connector *connector,
+ struct drm_connector_state *state,
+ struct drm_property *property,
+ uint64_t val)
+{
+ struct drm_device *drm = connector->dev;
+ struct vc4_hdmi *vc4_hdmi =
+ connector_to_vc4_hdmi(connector);
+ struct vc4_hdmi_connector_state *vc4_conn_state =
+ conn_state_to_vc4_hdmi_conn_state(state);
+
+ if (property == vc4_hdmi->broadcast_rgb_property) {
+ vc4_conn_state->broadcast_rgb = val;
+ return 0;
+ }
+
+ drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
+ property->base.id, property->name);
+ return -EINVAL;
+}
+
static void vc4_hdmi_connector_reset(struct drm_connector *connector)
{
struct vc4_hdmi_connector_state *old_state =
@@ -594,6 +648,7 @@ static void vc4_hdmi_connector_reset(struct drm_connector *connector)
new_state->base.max_bpc = 8;
new_state->base.max_requested_bpc = 8;
new_state->output_format = VC4_HDMI_OUTPUT_RGB;
+ new_state->broadcast_rgb = VC4_HDMI_BROADCAST_RGB_AUTO;
drm_atomic_helper_connector_tv_margins_reset(connector);
}

@@ -611,6 +666,7 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
new_state->tmds_char_rate = vc4_state->tmds_char_rate;
new_state->output_bpc = vc4_state->output_bpc;
new_state->output_format = vc4_state->output_format;
+ new_state->broadcast_rgb = vc4_state->broadcast_rgb;
__drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);

return &new_state->base;
@@ -621,6 +677,8 @@ static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
.reset = vc4_hdmi_connector_reset,
.atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+ .atomic_get_property = vc4_hdmi_connector_get_property,
+ .atomic_set_property = vc4_hdmi_connector_set_property,
};

static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
@@ -629,6 +687,33 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs =
.atomic_check = vc4_hdmi_connector_atomic_check,
};

+static const struct drm_prop_enum_list broadcast_rgb_names[] = {
+ { VC4_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
+ { VC4_HDMI_BROADCAST_RGB_FULL, "Full" },
+ { VC4_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
+};
+
+static void
+vc4_hdmi_attach_broadcast_rgb_property(struct drm_device *dev,
+ struct vc4_hdmi *vc4_hdmi)
+{
+ struct drm_property *prop = vc4_hdmi->broadcast_rgb_property;
+
+ if (!prop) {
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+ "Broadcast RGB",
+ broadcast_rgb_names,
+ ARRAY_SIZE(broadcast_rgb_names));
+ if (!prop)
+ return;
+
+ vc4_hdmi->broadcast_rgb_property = prop;
+ }
+
+ drm_object_attach_property(&vc4_hdmi->connector.base, prop,
+ VC4_HDMI_BROADCAST_RGB_AUTO);
+}
+
static int vc4_hdmi_connector_init(struct drm_device *dev,
struct vc4_hdmi *vc4_hdmi)
{
@@ -675,6 +760,8 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
if (vc4_hdmi->variant->supports_hdr)
drm_connector_attach_hdr_output_metadata_property(connector);

+ vc4_hdmi_attach_broadcast_rgb_property(dev, vc4_hdmi);
+
drm_connector_attach_encoder(connector, encoder);

return 0;
@@ -829,7 +916,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)

drm_hdmi_avi_infoframe_quant_range(&frame.avi,
connector, mode,
- vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode) ?
+ vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state) ?
HDMI_QUANTIZATION_RANGE_FULL :
HDMI_QUANTIZATION_RANGE_LIMITED);
drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
@@ -1069,6 +1156,8 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
struct drm_connector_state *state,
const struct drm_display_mode *mode)
{
+ struct vc4_hdmi_connector_state *vc4_state =
+ conn_state_to_vc4_hdmi_conn_state(state);
struct drm_device *drm = vc4_hdmi->connector.dev;
unsigned long flags;
u32 csc_ctl;
@@ -1082,7 +1171,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
VC4_HD_CSC_CTL_ORDER);

- if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode)) {
+ if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state)) {
/* CEA VICs other than #1 requre limited range RGB
* output unless overridden by an AVI infoframe.
* Apply a colorspace conversion to squash 0-255 down
@@ -1235,7 +1324,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
case VC4_HDMI_OUTPUT_RGB:
if_xbar = 0x354021;

- if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode))
+ if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state))
vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_rgb);
else
vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_unity);
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 5d249ac54cd1..89800c48aa24 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -117,6 +117,12 @@ enum vc4_hdmi_output_format {
VC4_HDMI_OUTPUT_YUV420,
};

+enum vc4_hdmi_broadcast_rgb {
+ VC4_HDMI_BROADCAST_RGB_AUTO,
+ VC4_HDMI_BROADCAST_RGB_FULL,
+ VC4_HDMI_BROADCAST_RGB_LIMITED,
+};
+
/* General HDMI hardware state. */
struct vc4_hdmi {
struct vc4_hdmi_audio audio;
@@ -129,6 +135,8 @@ struct vc4_hdmi {

struct delayed_work scrambling_work;

+ struct drm_property *broadcast_rgb_property;
+
struct i2c_adapter *ddc;
void __iomem *hdmicore_regs;
void __iomem *hd_regs;
@@ -238,6 +246,7 @@ struct vc4_hdmi_connector_state {
unsigned long long tmds_char_rate;
unsigned int output_bpc;
enum vc4_hdmi_output_format output_format;
+ enum vc4_hdmi_broadcast_rgb broadcast_rgb;
};

#define conn_state_to_vc4_hdmi_conn_state(_state) \

--
2.39.2

2023-03-06 10:50:08

by Maxime Ripard

Subject: [PATCH v3 4/9] drm/vc4: hdmi: Rename full range helper

From: Dave Stevenson <[email protected]>

The VC4 HDMI driver has a helper function to figure out whether full
range or limited range RGB is being used called
vc4_hdmi_is_full_range_rgb().

We'll need it to support other colorspaces, so let's rename it to
vc4_hdmi_is_full_range().

Signed-off-by: Dave Stevenson <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d23c0c3df2ee..f0d8da241f24 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -153,8 +153,8 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
return clock > HDMI_14_MAX_TMDS_CLK;
}

-static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
- struct vc4_hdmi_connector_state *vc4_state)
+static bool vc4_hdmi_is_full_range(struct vc4_hdmi *vc4_hdmi,
+ struct vc4_hdmi_connector_state *vc4_state)
{
const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
struct drm_display_info *display = &vc4_hdmi->connector.display_info;
@@ -916,7 +916,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)

drm_hdmi_avi_infoframe_quant_range(&frame.avi,
connector, mode,
- vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state) ?
+ vc4_hdmi_is_full_range(vc4_hdmi, vc4_state) ?
HDMI_QUANTIZATION_RANGE_FULL :
HDMI_QUANTIZATION_RANGE_LIMITED);
drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
@@ -1171,7 +1171,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
VC4_HD_CSC_CTL_ORDER);

- if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state)) {
+ if (!vc4_hdmi_is_full_range(vc4_hdmi, vc4_state)) {
/* CEA VICs other than #1 requre limited range RGB
* output unless overridden by an AVI infoframe.
* Apply a colorspace conversion to squash 0-255 down
@@ -1324,7 +1324,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
case VC4_HDMI_OUTPUT_RGB:
if_xbar = 0x354021;

- if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state))
+ if (!vc4_hdmi_is_full_range(vc4_hdmi, vc4_state))
vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_rgb);
else
vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_unity);

--
2.39.2

2023-03-06 10:50:13

by Maxime Ripard

Subject: [PATCH v3 5/9] drm/vc4: hdmi: Swap CSC matrix channels for YUV444

From: Dave Stevenson <[email protected]>

YUV444 and YUV422 actually require the same matrix, but programmed
differently.

We've dealt with it in the past by having two matrices, with the one
for YUV444 reordered to accomodate the hardware.

This gets in the way of subsequent reworks so let's define a function
that will take the coefficients swap into account, and remove the now
redundant YUV444 matrix.

Signed-off-by: Dave Stevenson <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index f0d8da241f24..f051e501efe6 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1236,7 +1236,7 @@ static const u16 vc5_hdmi_csc_full_rgb_to_limited_rgb[3][4] = {
};

/*
- * Conversion between Full Range RGB and Full Range YUV422 using the
+ * Conversion between Full Range RGB and Limited Range YUV using the
* BT.709 Colorspace
*
*
@@ -1246,28 +1246,12 @@ static const u16 vc5_hdmi_csc_full_rgb_to_limited_rgb[3][4] = {
*
* Matrix is signed 2p13 fixed point, with signed 9p6 offsets
*/
-static const u16 vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709[3][4] = {
+static const u16 vc5_hdmi_csc_full_rgb_to_limited_bt709[3][4] = {
{ 0x05d2, 0x1394, 0x01fa, 0x0400 },
{ 0xfccc, 0xf536, 0x0e00, 0x2000 },
{ 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
};

-/*
- * Conversion between Full Range RGB and Full Range YUV444 using the
- * BT.709 Colorspace
- *
- * [ -0.100268 -0.337232 0.437500 128 ]
- * [ 0.437500 -0.397386 -0.040114 128 ]
- * [ 0.181906 0.611804 0.061758 16 ]
- *
- * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
- */
-static const u16 vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709[3][4] = {
- { 0xfccc, 0xf536, 0x0e00, 0x2000 },
- { 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
- { 0x05d2, 0x1394, 0x01fa, 0x0400 },
-};
-
static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
const u16 coeffs[3][4])
{
@@ -1281,6 +1265,20 @@ static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
HDMI_WRITE(HDMI_CSC_34_33, (coeffs[2][3] << 16) | coeffs[2][2]);
}

+static void vc5_hdmi_set_csc_coeffs_swap(struct vc4_hdmi *vc4_hdmi,
+ const u16 coeffs[3][4])
+{
+ lockdep_assert_held(&vc4_hdmi->hw_lock);
+
+ /* YUV444 needs the CSC matrices using the channels in a different order */
+ HDMI_WRITE(HDMI_CSC_12_11, (coeffs[1][1] << 16) | coeffs[1][0]);
+ HDMI_WRITE(HDMI_CSC_14_13, (coeffs[1][3] << 16) | coeffs[1][2]);
+ HDMI_WRITE(HDMI_CSC_22_21, (coeffs[2][1] << 16) | coeffs[2][0]);
+ HDMI_WRITE(HDMI_CSC_24_23, (coeffs[2][3] << 16) | coeffs[2][2]);
+ HDMI_WRITE(HDMI_CSC_32_31, (coeffs[0][1] << 16) | coeffs[0][0]);
+ HDMI_WRITE(HDMI_CSC_34_33, (coeffs[0][3] << 16) | coeffs[0][2]);
+}
+
static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
struct drm_connector_state *state,
const struct drm_display_mode *mode)
@@ -1303,7 +1301,8 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,

switch (vc4_state->output_format) {
case VC4_HDMI_OUTPUT_YUV444:
- vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709);
+ vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi,
+ vc5_hdmi_csc_full_rgb_to_limited_bt709);
break;

case VC4_HDMI_OUTPUT_YUV422:
@@ -1318,7 +1317,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
if_cfg |= VC4_SET_FIELD(VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422_FORMAT_422_LEGACY,
VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422);

- vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709);
+ vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_bt709);
break;

case VC4_HDMI_OUTPUT_RGB:

--
2.39.2

2023-03-06 10:50:17

by Maxime Ripard

Subject: [PATCH v3 6/9] drm/vc4: hdmi: Rework the CSC matrices organization

From: Dave Stevenson <[email protected]>

The CSC matrices were stored as separate matrix for each colorspace, and
if we wanted a limited or full RGB output.

This created some gaps in our support and we would not always pick the
relevant matrix.

Let's rework our data structure to store one per colorspace, and then a
matrix for limited range and one for full range. This makes us add a new
matrix to support full range BT709 YUV output.

Signed-off-by: Dave Stevenson <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 108 ++++++++++++++++++++++++-----------------
1 file changed, 63 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index f051e501efe6..a3e0bf00e4c6 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1204,52 +1204,72 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
}

/*
- * If we need to output Full Range RGB, then use the unity matrix
+ * Matrices for (internal) RGB to RGB output.
*
- * [ 1 0 0 0]
- * [ 0 1 0 0]
- * [ 0 0 1 0]
- *
- * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
+ * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
*/
-static const u16 vc5_hdmi_csc_full_rgb_unity[3][4] = {
- { 0x2000, 0x0000, 0x0000, 0x0000 },
- { 0x0000, 0x2000, 0x0000, 0x0000 },
- { 0x0000, 0x0000, 0x2000, 0x0000 },
+static const u16 vc5_hdmi_csc_full_rgb_to_rgb[2][3][4] = {
+ {
+ /*
+ * Full range - unity
+ *
+ * [ 1 0 0 0]
+ * [ 0 1 0 0]
+ * [ 0 0 1 0]
+ */
+ { 0x2000, 0x0000, 0x0000, 0x0000 },
+ { 0x0000, 0x2000, 0x0000, 0x0000 },
+ { 0x0000, 0x0000, 0x2000, 0x0000 },
+ },
+ {
+ /*
+ * Limited range
+ *
+ * CEA VICs other than #1 require limited range RGB
+ * output unless overridden by an AVI infoframe. Apply a
+ * colorspace conversion to squash 0-255 down to 16-235.
+ * The matrix here is:
+ *
+ * [ 0.8594 0 0 16]
+ * [ 0 0.8594 0 16]
+ * [ 0 0 0.8594 16]
+ */
+ { 0x1b80, 0x0000, 0x0000, 0x0400 },
+ { 0x0000, 0x1b80, 0x0000, 0x0400 },
+ { 0x0000, 0x0000, 0x1b80, 0x0400 },
+ },
};

/*
- * CEA VICs other than #1 require limited range RGB output unless
- * overridden by an AVI infoframe. Apply a colorspace conversion to
- * squash 0-255 down to 16-235. The matrix here is:
- *
- * [ 0.8594 0 0 16]
- * [ 0 0.8594 0 16]
- * [ 0 0 0.8594 16]
- *
- * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
- */
-static const u16 vc5_hdmi_csc_full_rgb_to_limited_rgb[3][4] = {
- { 0x1b80, 0x0000, 0x0000, 0x0400 },
- { 0x0000, 0x1b80, 0x0000, 0x0400 },
- { 0x0000, 0x0000, 0x1b80, 0x0400 },
-};
-
-/*
- * Conversion between Full Range RGB and Limited Range YUV using the
- * BT.709 Colorspace
- *
- *
- * [ 0.181906 0.611804 0.061758 16 ]
- * [ -0.100268 -0.337232 0.437500 128 ]
- * [ 0.437500 -0.397386 -0.040114 128 ]
+ * Conversion between Full Range RGB and YUV using the BT.709 Colorspace
*
- * Matrix is signed 2p13 fixed point, with signed 9p6 offsets
+ * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
*/
-static const u16 vc5_hdmi_csc_full_rgb_to_limited_bt709[3][4] = {
- { 0x05d2, 0x1394, 0x01fa, 0x0400 },
- { 0xfccc, 0xf536, 0x0e00, 0x2000 },
- { 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
+static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt709[2][3][4] = {
+ {
+ /*
+ * Full Range
+ *
+ * [ 0.212600 0.715200 0.072200 0 ]
+ * [ -0.114572 -0.385428 0.500000 128 ]
+ * [ 0.500000 -0.454153 -0.045847 128 ]
+ */
+ { 0x06ce, 0x16e3, 0x024f, 0x0000 },
+ { 0xfc56, 0xf3ac, 0x1000, 0x2000 },
+ { 0x1000, 0xf179, 0xfe89, 0x2000 },
+ },
+ {
+ /*
+ * Limited Range
+ *
+ * [ 0.181906 0.611804 0.061758 16 ]
+ * [ -0.100268 -0.337232 0.437500 128 ]
+ * [ 0.437500 -0.397386 -0.040114 128 ]
+ */
+ { 0x05d2, 0x1394, 0x01fa, 0x0400 },
+ { 0xfccc, 0xf536, 0x0e00, 0x2000 },
+ { 0x0e00, 0xf34a, 0xfeb8, 0x2000 },
+ },
};

static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
@@ -1286,6 +1306,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
struct drm_device *drm = vc4_hdmi->connector.dev;
struct vc4_hdmi_connector_state *vc4_state =
conn_state_to_vc4_hdmi_conn_state(state);
+ unsigned int lim_range = vc4_hdmi_is_full_range(vc4_hdmi, vc4_state) ? 0 : 1;
unsigned long flags;
u32 if_cfg = 0;
u32 if_xbar = 0x543210;
@@ -1302,7 +1323,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
switch (vc4_state->output_format) {
case VC4_HDMI_OUTPUT_YUV444:
vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi,
- vc5_hdmi_csc_full_rgb_to_limited_bt709);
+ vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
break;

case VC4_HDMI_OUTPUT_YUV422:
@@ -1317,16 +1338,13 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
if_cfg |= VC4_SET_FIELD(VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422_FORMAT_422_LEGACY,
VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422);

- vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_bt709);
+ vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
break;

case VC4_HDMI_OUTPUT_RGB:
if_xbar = 0x354021;

- if (!vc4_hdmi_is_full_range(vc4_hdmi, vc4_state))
- vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_rgb);
- else
- vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_unity);
+ vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_rgb[lim_range]);
break;

default:

--
2.39.2

2023-03-06 10:50:28

by Maxime Ripard

Subject: [PATCH v3 7/9] drm/vc4: hdmi: Add a function to retrieve the CSC matrix

From: Dave Stevenson <[email protected]>

The CSC matrix to use depends on the output format, its range and the
colorspace.

Since we're going to add more colorspaces, let's move the CSC matrix
retrieval to a function.

Signed-off-by: Dave Stevenson <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index a3e0bf00e4c6..1424835bd83e 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1299,6 +1299,20 @@ static void vc5_hdmi_set_csc_coeffs_swap(struct vc4_hdmi *vc4_hdmi,
HDMI_WRITE(HDMI_CSC_34_33, (coeffs[0][3] << 16) | coeffs[0][2]);
}

+static const u16
+(*vc5_hdmi_find_yuv_csc_coeffs(struct vc4_hdmi *vc4_hdmi, u32 colorspace, bool limited))[4]
+{
+ switch (colorspace) {
+ default:
+ case DRM_MODE_COLORIMETRY_NO_DATA:
+ case DRM_MODE_COLORIMETRY_BT709_YCC:
+ case DRM_MODE_COLORIMETRY_XVYCC_709:
+ case DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
+ case DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
+ return vc5_hdmi_csc_full_rgb_to_yuv_bt709[limited];
+ }
+}
+
static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
struct drm_connector_state *state,
const struct drm_display_mode *mode)
@@ -1308,6 +1322,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
conn_state_to_vc4_hdmi_conn_state(state);
unsigned int lim_range = vc4_hdmi_is_full_range(vc4_hdmi, vc4_state) ? 0 : 1;
unsigned long flags;
+ const u16 (*csc)[4];
u32 if_cfg = 0;
u32 if_xbar = 0x543210;
u32 csc_chan_ctl = 0;
@@ -1322,11 +1337,14 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,

switch (vc4_state->output_format) {
case VC4_HDMI_OUTPUT_YUV444:
- vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi,
- vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
+ csc = vc5_hdmi_find_yuv_csc_coeffs(vc4_hdmi, state->colorspace, !!lim_range);
+
+ vc5_hdmi_set_csc_coeffs_swap(vc4_hdmi, csc);
break;

case VC4_HDMI_OUTPUT_YUV422:
+ csc = vc5_hdmi_find_yuv_csc_coeffs(vc4_hdmi, state->colorspace, !!lim_range);
+
csc_ctl |= VC4_SET_FIELD(VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422_STANDARD,
VC5_MT_CP_CSC_CTL_FILTER_MODE_444_TO_422) |
VC5_MT_CP_CSC_CTL_USE_444_TO_422 |
@@ -1338,7 +1356,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
if_cfg |= VC4_SET_FIELD(VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422_FORMAT_422_LEGACY,
VC5_DVP_HT_VEC_INTERFACE_CFG_SEL_422);

- vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_yuv_bt709[lim_range]);
+ vc5_hdmi_set_csc_coeffs(vc4_hdmi, csc);
break;

case VC4_HDMI_OUTPUT_RGB:

--
2.39.2

2023-03-06 10:50:33

by Maxime Ripard

Subject: [PATCH v3 8/9] drm/vc4: hdmi: Add BT.601 Support

From: Dave Stevenson <[email protected]>

Even though we report that we support the BT601 Colorspace, we were
always using the BT.709 conversion matrices. Let's add the BT601 ones.

Signed-off-by: Dave Stevenson <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 1424835bd83e..ad38cac3d1b9 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1240,6 +1240,37 @@ static const u16 vc5_hdmi_csc_full_rgb_to_rgb[2][3][4] = {
},
};

+/*
+ * Conversion between Full Range RGB and YUV using the BT.601 Colorspace
+ *
+ * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
+ */
+static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt601[2][3][4] = {
+ {
+ /*
+ * Full Range
+ *
+ * [ 0.299000 0.587000 0.114000 0 ]
+ * [ -0.168736 -0.331264 0.500000 128 ]
+ * [ 0.500000 -0.418688 -0.081312 128 ]
+ */
+ { 0x0991, 0x12c9, 0x03a6, 0x0000 },
+ { 0xfa9b, 0xf567, 0x1000, 0x2000 },
+ { 0x1000, 0xf29b, 0xfd67, 0x2000 },
+ },
+ {
+ /* Limited Range
+ *
+ * [ 0.255785 0.502160 0.097523 16 ]
+ * [ -0.147644 -0.289856 0.437500 128 ]
+ * [ 0.437500 -0.366352 -0.071148 128 ]
+ */
+ { 0x082f, 0x1012, 0x031f, 0x0400 },
+ { 0xfb48, 0xf6ba, 0x0e00, 0x2000 },
+ { 0x0e00, 0xf448, 0xfdba, 0x2000 },
+ },
+};
+
/*
* Conversion between Full Range RGB and YUV using the BT.709 Colorspace
*
@@ -1303,6 +1334,13 @@ static const u16
(*vc5_hdmi_find_yuv_csc_coeffs(struct vc4_hdmi *vc4_hdmi, u32 colorspace, bool limited))[4]
{
switch (colorspace) {
+ case DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
+ case DRM_MODE_COLORIMETRY_XVYCC_601:
+ case DRM_MODE_COLORIMETRY_SYCC_601:
+ case DRM_MODE_COLORIMETRY_OPYCC_601:
+ case DRM_MODE_COLORIMETRY_BT601_YCC:
+ return vc5_hdmi_csc_full_rgb_to_yuv_bt601[limited];
+
default:
case DRM_MODE_COLORIMETRY_NO_DATA:
case DRM_MODE_COLORIMETRY_BT709_YCC:

--
2.39.2

2023-03-06 10:50:37

by Maxime Ripard

Subject: [PATCH v3 9/9] drm/vc4: hdmi: Add BT.2020 Support

From: Dave Stevenson <[email protected]>

Even though we report that we support the BT.2020 Colorspace, we were
always using the BT.709 conversion matrices. Let's add the BT.2020 ones.

Signed-off-by: Dave Stevenson <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ad38cac3d1b9..2787284e60ea 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1303,6 +1303,37 @@ static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt709[2][3][4] = {
},
};

+/*
+ * Conversion between Full Range RGB and YUV using the BT.2020 Colorspace
+ *
+ * Matrices are signed 2p13 fixed point, with signed 9p6 offsets
+ */
+static const u16 vc5_hdmi_csc_full_rgb_to_yuv_bt2020[2][3][4] = {
+ {
+ /*
+ * Full Range
+ *
+ * [ 0.262700 0.678000 0.059300 0 ]
+ * [ -0.139630 -0.360370 0.500000 128 ]
+ * [ 0.500000 -0.459786 -0.040214 128 ]
+ */
+ { 0x0868, 0x15b2, 0x01e6, 0x0000 },
+ { 0xfb89, 0xf479, 0x1000, 0x2000 },
+ { 0x1000, 0xf14a, 0xfeb8, 0x2000 },
+ },
+ {
+ /* Limited Range
+ *
+ * [ 0.224732 0.580008 0.050729 16 ]
+ * [ -0.122176 -0.315324 0.437500 128 ]
+ * [ 0.437500 -0.402312 -0.035188 128 ]
+ */
+ { 0x082f, 0x1012, 0x031f, 0x0400 },
+ { 0xfb48, 0xf6ba, 0x0e00, 0x2000 },
+ { 0x0e00, 0xf448, 0xfdba, 0x2000 },
+ },
+};
+
static void vc5_hdmi_set_csc_coeffs(struct vc4_hdmi *vc4_hdmi,
const u16 coeffs[3][4])
{
@@ -1348,6 +1379,13 @@ static const u16
case DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED:
case DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT:
return vc5_hdmi_csc_full_rgb_to_yuv_bt709[limited];
+
+ case DRM_MODE_COLORIMETRY_BT2020_CYCC:
+ case DRM_MODE_COLORIMETRY_BT2020_YCC:
+ case DRM_MODE_COLORIMETRY_BT2020_RGB:
+ case DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65:
+ case DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER:
+ return vc5_hdmi_csc_full_rgb_to_yuv_bt2020[limited];
}
}


--
2.39.2

2023-04-22 05:39:32

by Javier Martinez Canillas

Subject: Re: [PATCH v3 1/9] drm/vc4: Switch to container_of_const

Maxime Ripard <[email protected]> writes:

Hello Maxime,

> container_of_const() allows to preserve the pointer constness and is
> thus more flexible than inline functions.
>
> Let's switch all our instances of container_of() to
> container_of_const().
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---

[...]

> -static inline struct vc4_dpi *
> -to_vc4_dpi(struct drm_encoder *encoder)
> -{
> - return container_of(encoder, struct vc4_dpi, encoder.base);
> -}
> +#define to_vc4_dpi(_encoder) \
> + container_of_const(_encoder, struct vc4_dpi, encoder.base)
>

A disadvantage of this approach though is that the type checking is lost.
Since you already had these, I would probably had changed them to return
a const pointer and just replace container_of() for container_of_const().

But I see that there are a lot of patches from Greg all over the kernel
that do exactly this, dropping static inline functions in favor of using
container_of_const() directly. So it seems the convention is what you do.

Reviewed-by: Javier Martinez Canillas <[email protected]>

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-04-25 07:40:05

by Maxime Ripard

Subject: Re: [PATCH v3 0/9] drm/vc4: hdmi: Broadcast RGB, BT601, BT2020

On Mon, 06 Mar 2023 11:46:41 +0100, Maxime Ripard wrote:
> Here's a collection of patches that have been in the downstream tree for a
> while to add a bunch of new features to the HDMI controller.
>
> Let me know what you think,
> Maxime
>
>
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

2023-04-25 07:41:56

by Maxime Ripard

Subject: Re: [PATCH v3 1/9] drm/vc4: Switch to container_of_const

Hi Javier,

On Sat, Apr 22, 2023 at 07:26:13AM +0200, Javier Martinez Canillas wrote:
> Maxime Ripard <[email protected]> writes:
> > container_of_const() allows to preserve the pointer constness and is
> > thus more flexible than inline functions.
> >
> > Let's switch all our instances of container_of() to
> > container_of_const().
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
>
> [...]
>
> > -static inline struct vc4_dpi *
> > -to_vc4_dpi(struct drm_encoder *encoder)
> > -{
> > - return container_of(encoder, struct vc4_dpi, encoder.base);
> > -}
> > +#define to_vc4_dpi(_encoder) \
> > + container_of_const(_encoder, struct vc4_dpi, encoder.base)
> >
>
> A disadvantage of this approach though is that the type checking is lost.

Not entirely, the argument is still type-checked, but yeah, it's true
for the returned value.

> Since you already had these, I would probably had changed them to return
> a const pointer and just replace container_of() for container_of_const().
>
> But I see that there are a lot of patches from Greg all over the kernel
> that do exactly this, dropping static inline functions in favor of using
> container_of_const() directly. So it seems the convention is what you do.

More importantly, container_of_const() isn't always returning a const
pointer or always taking a const argument, it's returning the pointer
with the same const-ness than the argument.

This is why it makes sense to remove the inline function entirely,
because it removes the main benefit it brings.

> Reviewed-by: Javier Martinez Canillas <[email protected]>

Thanks, I've applied this series

Maxime

Attachments:

2023-04-25 08:26:31

by Javier Martinez Canillas

Subject: Re: [PATCH v3 1/9] drm/vc4: Switch to container_of_const

Maxime Ripard <[email protected]> writes:

> Hi Javier,
>
> On Sat, Apr 22, 2023 at 07:26:13AM +0200, Javier Martinez Canillas wrote:
>> Maxime Ripard <[email protected]> writes:
>> > container_of_const() allows to preserve the pointer constness and is
>> > thus more flexible than inline functions.
>> >
>> > Let's switch all our instances of container_of() to
>> > container_of_const().
>> >
>> > Signed-off-by: Maxime Ripard <[email protected]>
>> > ---
>>
>> [...]
>>
>> > -static inline struct vc4_dpi *
>> > -to_vc4_dpi(struct drm_encoder *encoder)
>> > -{
>> > - return container_of(encoder, struct vc4_dpi, encoder.base);
>> > -}
>> > +#define to_vc4_dpi(_encoder) \
>> > + container_of_const(_encoder, struct vc4_dpi, encoder.base)
>> >
>>
>> A disadvantage of this approach though is that the type checking is lost.
>
> Not entirely, the argument is still type-checked, but yeah, it's true
> for the returned value.
>
>> Since you already had these, I would probably had changed them to return
>> a const pointer and just replace container_of() for container_of_const().
>>
>> But I see that there are a lot of patches from Greg all over the kernel
>> that do exactly this, dropping static inline functions in favor of using
>> container_of_const() directly. So it seems the convention is what you do.
>
> More importantly, container_of_const() isn't always returning a const
> pointer or always taking a const argument, it's returning the pointer
> with the same const-ness than the argument.
>
> This is why it makes sense to remove the inline function entirely,
> because it removes the main benefit it brings.
>

Got it. Thanks for the explanations.

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

2023-10-11 13:23:47

by Daniel Vetter

Subject: Re: [PATCH v3 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

On Mon, 6 Mar 2023 at 11:49, Maxime Ripard <[email protected]> wrote:
>
> From: Dave Stevenson <[email protected]>
>
> Copy Intel's "Broadcast RGB" property semantics to add manual override
> of the HDMI pixel range for monitors that don't abide by the content
> of the AVI Infoframe.
>
> Signed-off-by: Dave Stevenson <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>

Stumbled over this grepping around, but would have been nice to lift
this into drm code and document the property. It's one of the legacy
ones from the table of horrors after all ...

Shouldn't be an uapi problem because it's copypasted to much, just not great.
-Sima
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 97 ++++++++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/vc4/vc4_hdmi.h | 9 ++++
> 2 files changed, 102 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 522cfbc83fe4..d23c0c3df2ee 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -154,10 +154,16 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
> }
>
> static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
> - const struct drm_display_mode *mode)
> + struct vc4_hdmi_connector_state *vc4_state)
> {
> + const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
> struct drm_display_info *display = &vc4_hdmi->connector.display_info;
>
> + if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
> + return false;
> + else if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
> + return true;
> +
> return !display->is_hdmi ||
> drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;
> }
> @@ -528,8 +534,12 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
> {
> struct drm_connector_state *old_state =
> drm_atomic_get_old_connector_state(state, connector);
> + struct vc4_hdmi_connector_state *old_vc4_state =
> + conn_state_to_vc4_hdmi_conn_state(old_state);
> struct drm_connector_state *new_state =
> drm_atomic_get_new_connector_state(state, connector);
> + struct vc4_hdmi_connector_state *new_vc4_state =
> + conn_state_to_vc4_hdmi_conn_state(new_state);
> struct drm_crtc *crtc = new_state->crtc;
>
> if (!crtc)
> @@ -562,6 +572,7 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
> }
>
> if (old_state->colorspace != new_state->colorspace ||
> + old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb ||
> !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
> struct drm_crtc_state *crtc_state;
>
> @@ -575,6 +586,49 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
> return 0;
> }
>
> +static int vc4_hdmi_connector_get_property(struct drm_connector *connector,
> + const struct drm_connector_state *state,
> + struct drm_property *property,
> + uint64_t *val)
> +{
> + struct drm_device *drm = connector->dev;
> + struct vc4_hdmi *vc4_hdmi =
> + connector_to_vc4_hdmi(connector);
> + const struct vc4_hdmi_connector_state *vc4_conn_state =
> + conn_state_to_vc4_hdmi_conn_state(state);
> +
> + if (property == vc4_hdmi->broadcast_rgb_property) {
> + *val = vc4_conn_state->broadcast_rgb;
> + } else {
> + drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
> + property->base.id, property->name);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int vc4_hdmi_connector_set_property(struct drm_connector *connector,
> + struct drm_connector_state *state,
> + struct drm_property *property,
> + uint64_t val)
> +{
> + struct drm_device *drm = connector->dev;
> + struct vc4_hdmi *vc4_hdmi =
> + connector_to_vc4_hdmi(connector);
> + struct vc4_hdmi_connector_state *vc4_conn_state =
> + conn_state_to_vc4_hdmi_conn_state(state);
> +
> + if (property == vc4_hdmi->broadcast_rgb_property) {
> + vc4_conn_state->broadcast_rgb = val;
> + return 0;
> + }
> +
> + drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
> + property->base.id, property->name);
> + return -EINVAL;
> +}
> +
> static void vc4_hdmi_connector_reset(struct drm_connector *connector)
> {
> struct vc4_hdmi_connector_state *old_state =
> @@ -594,6 +648,7 @@ static void vc4_hdmi_connector_reset(struct drm_connector *connector)
> new_state->base.max_bpc = 8;
> new_state->base.max_requested_bpc = 8;
> new_state->output_format = VC4_HDMI_OUTPUT_RGB;
> + new_state->broadcast_rgb = VC4_HDMI_BROADCAST_RGB_AUTO;
> drm_atomic_helper_connector_tv_margins_reset(connector);
> }
>
> @@ -611,6 +666,7 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
> new_state->tmds_char_rate = vc4_state->tmds_char_rate;
> new_state->output_bpc = vc4_state->output_bpc;
> new_state->output_format = vc4_state->output_format;
> + new_state->broadcast_rgb = vc4_state->broadcast_rgb;
> __drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
>
> return &new_state->base;
> @@ -621,6 +677,8 @@ static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
> .reset = vc4_hdmi_connector_reset,
> .atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> + .atomic_get_property = vc4_hdmi_connector_get_property,
> + .atomic_set_property = vc4_hdmi_connector_set_property,
> };
>
> static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
> @@ -629,6 +687,33 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs =
> .atomic_check = vc4_hdmi_connector_atomic_check,
> };
>
> +static const struct drm_prop_enum_list broadcast_rgb_names[] = {
> + { VC4_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
> + { VC4_HDMI_BROADCAST_RGB_FULL, "Full" },
> + { VC4_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
> +};
> +
> +static void
> +vc4_hdmi_attach_broadcast_rgb_property(struct drm_device *dev,
> + struct vc4_hdmi *vc4_hdmi)
> +{
> + struct drm_property *prop = vc4_hdmi->broadcast_rgb_property;
> +
> + if (!prop) {
> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> + "Broadcast RGB",
> + broadcast_rgb_names,
> + ARRAY_SIZE(broadcast_rgb_names));
> + if (!prop)
> + return;
> +
> + vc4_hdmi->broadcast_rgb_property = prop;
> + }
> +
> + drm_object_attach_property(&vc4_hdmi->connector.base, prop,
> + VC4_HDMI_BROADCAST_RGB_AUTO);
> +}
> +
> static int vc4_hdmi_connector_init(struct drm_device *dev,
> struct vc4_hdmi *vc4_hdmi)
> {
> @@ -675,6 +760,8 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
> if (vc4_hdmi->variant->supports_hdr)
> drm_connector_attach_hdr_output_metadata_property(connector);
>
> + vc4_hdmi_attach_broadcast_rgb_property(dev, vc4_hdmi);
> +
> drm_connector_attach_encoder(connector, encoder);
>
> return 0;
> @@ -829,7 +916,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
>
> drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> connector, mode,
> - vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode) ?
> + vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state) ?
> HDMI_QUANTIZATION_RANGE_FULL :
> HDMI_QUANTIZATION_RANGE_LIMITED);
> drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
> @@ -1069,6 +1156,8 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> struct drm_connector_state *state,
> const struct drm_display_mode *mode)
> {
> + struct vc4_hdmi_connector_state *vc4_state =
> + conn_state_to_vc4_hdmi_conn_state(state);
> struct drm_device *drm = vc4_hdmi->connector.dev;
> unsigned long flags;
> u32 csc_ctl;
> @@ -1082,7 +1171,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
> VC4_HD_CSC_CTL_ORDER);
>
> - if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode)) {
> + if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state)) {
> /* CEA VICs other than #1 requre limited range RGB
> * output unless overridden by an AVI infoframe.
> * Apply a colorspace conversion to squash 0-255 down
> @@ -1235,7 +1324,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> case VC4_HDMI_OUTPUT_RGB:
> if_xbar = 0x354021;
>
> - if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode))
> + if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state))
> vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_rgb);
> else
> vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_unity);
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index 5d249ac54cd1..89800c48aa24 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -117,6 +117,12 @@ enum vc4_hdmi_output_format {
> VC4_HDMI_OUTPUT_YUV420,
> };
>
> +enum vc4_hdmi_broadcast_rgb {
> + VC4_HDMI_BROADCAST_RGB_AUTO,
> + VC4_HDMI_BROADCAST_RGB_FULL,
> + VC4_HDMI_BROADCAST_RGB_LIMITED,
> +};
> +
> /* General HDMI hardware state. */
> struct vc4_hdmi {
> struct vc4_hdmi_audio audio;
> @@ -129,6 +135,8 @@ struct vc4_hdmi {
>
> struct delayed_work scrambling_work;
>
> + struct drm_property *broadcast_rgb_property;
> +
> struct i2c_adapter *ddc;
> void __iomem *hdmicore_regs;
> void __iomem *hd_regs;
> @@ -238,6 +246,7 @@ struct vc4_hdmi_connector_state {
> unsigned long long tmds_char_rate;
> unsigned int output_bpc;
> enum vc4_hdmi_output_format output_format;
> + enum vc4_hdmi_broadcast_rgb broadcast_rgb;
> };
>
> #define conn_state_to_vc4_hdmi_conn_state(_state) \
>
> --
> 2.39.2
>


--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2023-10-19 08:03:11

by Maxime Ripard

Subject: Re: [PATCH v3 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

Hi,

On Wed, Oct 11, 2023 at 03:23:18PM +0200, Daniel Vetter wrote:
> On Mon, 6 Mar 2023 at 11:49, Maxime Ripard <[email protected]> wrote:
> >
> > From: Dave Stevenson <[email protected]>
> >
> > Copy Intel's "Broadcast RGB" property semantics to add manual override
> > of the HDMI pixel range for monitors that don't abide by the content
> > of the AVI Infoframe.
> >
> > Signed-off-by: Dave Stevenson <[email protected]>
> > Signed-off-by: Maxime Ripard <[email protected]>
>
> Stumbled over this grepping around, but would have been nice to lift
> this into drm code and document the property. It's one of the legacy
> ones from the table of horrors after all ...
>
> Shouldn't be an uapi problem because it's copypasted to much, just not great.

We already discussed it on IRC, but just for the record I have a current
series that should address exactly that:

https://lore.kernel.org/dri-devel/[email protected]/

Maxime

Attachments:

2023-10-19 08:26:58

by Hans Verkuil

Subject: Re: [PATCH v3 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

Hi Maxime,

On 19/10/2023 10:02, Maxime Ripard wrote:
> Hi,
>
> On Wed, Oct 11, 2023 at 03:23:18PM +0200, Daniel Vetter wrote:
>> On Mon, 6 Mar 2023 at 11:49, Maxime Ripard <[email protected]> wrote:
>>>
>>> From: Dave Stevenson <[email protected]>
>>>
>>> Copy Intel's "Broadcast RGB" property semantics to add manual override
>>> of the HDMI pixel range for monitors that don't abide by the content
>>> of the AVI Infoframe.
>>>
>>> Signed-off-by: Dave Stevenson <[email protected]>
>>> Signed-off-by: Maxime Ripard <[email protected]>
>>
>> Stumbled over this grepping around, but would have been nice to lift
>> this into drm code and document the property. It's one of the legacy
>> ones from the table of horrors after all ...
>>
>> Shouldn't be an uapi problem because it's copypasted to much, just not great.
>
> We already discussed it on IRC, but just for the record I have a current
> series that should address exactly that:
>
> https://lore.kernel.org/dri-devel/[email protected]/
>
> Maxime

I've pasted a snippet from that patch below for a quick review:

> /**
> * DOC: HDMI connector properties
> *
> + * Broadcast RGB (HDMI Specific):

Full vs Limited is actually not HDMI specific, DisplayPort can signal this as
well for whatever it is worth.

> + * Indicates the RGB Range (Full vs Limited) used.

RGB Range -> RGB Quantization Range

> + *
> + * The value of this property can be one of the following:
> + *
> + * Automatic:
> + * RGB Range is selected automatically based on the mode
> + * according to the HDMI specifications.
> + *
> + * Full:
> + * Full RGB Range is forced.
> + *
> + * Limited 16:235:

It is very unfortunate that this is called "Limited 16:235" instead of just "Limited"
since for color component bit depths > 8 these values are different.

I have no idea if it is possible to add an alias "Limited" that you can use instead.
In any case, this should document that it works just as well for higher bit depths,
but with different limits.

Regards,

Hans

> + * Limited RGB Range is forced.
> + *
> + * Drivers can set up this property by calling
> + * drm_connector_attach_broadcast_rgb_property().
> + *
> * content type (HDMI specific):
> * Indicates content type setting to be used in HDMI infoframes to indicate
> * content type for the external device, so that it adjusts its display

2023-10-23 14:59:13

by Maxime Ripard

Subject: Re: [PATCH v3 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

Hi Hans,

On Thu, Oct 19, 2023 at 10:26:40AM +0200, Hans Verkuil wrote:
> Hi Maxime,
>
> On 19/10/2023 10:02, Maxime Ripard wrote:
> > Hi,
> >
> > On Wed, Oct 11, 2023 at 03:23:18PM +0200, Daniel Vetter wrote:
> >> On Mon, 6 Mar 2023 at 11:49, Maxime Ripard <[email protected]> wrote:
> >>>
> >>> From: Dave Stevenson <[email protected]>
> >>>
> >>> Copy Intel's "Broadcast RGB" property semantics to add manual override
> >>> of the HDMI pixel range for monitors that don't abide by the content
> >>> of the AVI Infoframe.
> >>>
> >>> Signed-off-by: Dave Stevenson <[email protected]>
> >>> Signed-off-by: Maxime Ripard <[email protected]>
> >>
> >> Stumbled over this grepping around, but would have been nice to lift
> >> this into drm code and document the property. It's one of the legacy
> >> ones from the table of horrors after all ...
> >>
> >> Shouldn't be an uapi problem because it's copypasted to much, just not great.
> >
> > We already discussed it on IRC, but just for the record I have a current
> > series that should address exactly that:
> >
> > https://lore.kernel.org/dri-devel/[email protected]/
> >
> > Maxime
>
> I've pasted a snippet from that patch below for a quick review:
>
> > /**
> > * DOC: HDMI connector properties
> > *
> > + * Broadcast RGB (HDMI Specific):
>
> Full vs Limited is actually not HDMI specific, DisplayPort can signal this as
> well for whatever it is worth.

Sure, what I (and the original patch I guess) meant is that it's only
ever used on HDMI connectors these days. If that ever changes, then we
can update the doc.

> > + * Indicates the RGB Range (Full vs Limited) used.
>
> RGB Range -> RGB Quantization Range
>
> > + *
> > + * The value of this property can be one of the following:
> > + *
> > + * Automatic:
> > + * RGB Range is selected automatically based on the mode
> > + * according to the HDMI specifications.
> > + *
> > + * Full:
> > + * Full RGB Range is forced.
> > + *
> > + * Limited 16:235:
>
> It is very unfortunate that this is called "Limited 16:235" instead of just "Limited"
> since for color component bit depths > 8 these values are different.
>
> I have no idea if it is possible to add an alias "Limited" that you can use instead.
> In any case, this should document that it works just as well for higher bit depths,
> but with different limits.

I had a look and it doesn't look like the property infrastructure can
deal with aliases. I'll add something in the doc

Thanks!
Maxime

Attachments:

2023-10-23 16:24:20

by Sebastian Wick

Subject: Re: [PATCH v3 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

Honestly, the less time people spend on this property the better. Lift
the Intel one into core and be done with it. We'll hopefully be able
to remove it in the not-to-distant future with the new color pipeline
API and adding a new property which only sets the connector metadata
instead of influencing the color pipeline as well.

On Mon, Oct 23, 2023 at 4:58 PM Maxime Ripard <[email protected]> wrote:
>
> Hi Hans,
>
> On Thu, Oct 19, 2023 at 10:26:40AM +0200, Hans Verkuil wrote:
> > Hi Maxime,
> >
> > On 19/10/2023 10:02, Maxime Ripard wrote:
> > > Hi,
> > >
> > > On Wed, Oct 11, 2023 at 03:23:18PM +0200, Daniel Vetter wrote:
> > >> On Mon, 6 Mar 2023 at 11:49, Maxime Ripard <[email protected]> wrote:
> > >>>
> > >>> From: Dave Stevenson <[email protected]>
> > >>>
> > >>> Copy Intel's "Broadcast RGB" property semantics to add manual override
> > >>> of the HDMI pixel range for monitors that don't abide by the content
> > >>> of the AVI Infoframe.
> > >>>
> > >>> Signed-off-by: Dave Stevenson <[email protected]>
> > >>> Signed-off-by: Maxime Ripard <[email protected]>
> > >>
> > >> Stumbled over this grepping around, but would have been nice to lift
> > >> this into drm code and document the property. It's one of the legacy
> > >> ones from the table of horrors after all ...
> > >>
> > >> Shouldn't be an uapi problem because it's copypasted to much, just not great.
> > >
> > > We already discussed it on IRC, but just for the record I have a current
> > > series that should address exactly that:
> > >
> > > https://lore.kernel.org/dri-devel/[email protected]/
> > >
> > > Maxime
> >
> > I've pasted a snippet from that patch below for a quick review:
> >
> > > /**
> > > * DOC: HDMI connector properties
> > > *
> > > + * Broadcast RGB (HDMI Specific):
> >
> > Full vs Limited is actually not HDMI specific, DisplayPort can signal this as
> > well for whatever it is worth.
>
> Sure, what I (and the original patch I guess) meant is that it's only
> ever used on HDMI connectors these days. If that ever changes, then we
> can update the doc.
>
> > > + * Indicates the RGB Range (Full vs Limited) used.
> >
> > RGB Range -> RGB Quantization Range
> >
> > > + *
> > > + * The value of this property can be one of the following:
> > > + *
> > > + * Automatic:
> > > + * RGB Range is selected automatically based on the mode
> > > + * according to the HDMI specifications.
> > > + *
> > > + * Full:
> > > + * Full RGB Range is forced.
> > > + *
> > > + * Limited 16:235:
> >
> > It is very unfortunate that this is called "Limited 16:235" instead of just "Limited"
> > since for color component bit depths > 8 these values are different.
> >
> > I have no idea if it is possible to add an alias "Limited" that you can use instead.
> > In any case, this should document that it works just as well for higher bit depths,
> > but with different limits.
>
> I had a look and it doesn't look like the property infrastructure can
> deal with aliases. I'll add something in the doc
>
> Thanks!
> Maxime