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
To: Emma Anholt <[email protected]>
To: Maxime Ripard <[email protected]>
To: David Airlie <[email protected]>
To: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Dave Stevenson <[email protected]>
Signed-off-by: Maxime Ripard <[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_bo.c | 2 +-
drivers/gpu/drm/vc4/vc4_crtc.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 | 19 +-
drivers/gpu/drm/vc4/vc4_gem.c | 7 +-
drivers/gpu/drm/vc4/vc4_hdmi.c | 343 +++++++++++++++++++++++-----
drivers/gpu/drm/vc4/vc4_hdmi.h | 25 +-
drivers/gpu/drm/vc4/vc4_irq.c | 2 +-
drivers/gpu/drm/vc4/vc4_kms.c | 16 +-
drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
drivers/gpu/drm/vc4/vc4_txp.c | 12 +-
drivers/gpu/drm/vc4/vc4_v3d.c | 2 +-
drivers/gpu/drm/vc4/vc4_vec.c | 14 +-
16 files changed, 355 insertions(+), 172 deletions(-)
---
base-commit: 9fbee811e479aca2f3523787cae1f46553141b40
change-id: 20221207-rpi-hdmi-improvements-3de1c0dba2dc
Best regards,
--
Maxime Ripard <[email protected]>
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_bo.c | 2 +-
drivers/gpu/drm/vc4/vc4_crtc.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 | 19 ++++-----
drivers/gpu/drm/vc4/vc4_gem.c | 7 ++--
drivers/gpu/drm/vc4/vc4_hdmi.c | 7 ++--
drivers/gpu/drm/vc4/vc4_hdmi.h | 16 +++----
drivers/gpu/drm/vc4/vc4_irq.c | 2 +-
drivers/gpu/drm/vc4/vc4_kms.c | 16 +++----
drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
drivers/gpu/drm/vc4/vc4_txp.c | 12 ++----
drivers/gpu/drm/vc4/vc4_v3d.c | 2 +-
drivers/gpu/drm/vc4/vc4_vec.c | 14 ++-----
16 files changed, 65 insertions(+), 117 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_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 86d629e45307..d0a00ed42cb0 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object *gem_bo)
static void vc4_bo_cache_time_work(struct work_struct *work)
{
struct vc4_dev *vc4 =
- container_of(work, struct vc4_dev, bo_cache.time_work);
+ container_of_const(work, struct vc4_dev, bo_cache.time_work);
struct drm_device *dev = &vc4->base;
mutex_lock(&vc4->bo_lock);
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index cdc0559221f0..4425dc15308d 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -869,7 +869,7 @@ vc4_async_page_flip_complete(struct vc4_async_flip_state *flip_state)
static void vc4_async_page_flip_seqno_complete(struct vc4_seqno_cb *cb)
{
struct vc4_async_flip_state *flip_state =
- container_of(cb, struct vc4_async_flip_state, cb.seqno);
+ container_of_const(cb, struct vc4_async_flip_state, cb.seqno);
struct vc4_bo *bo = NULL;
if (flip_state->old_fb) {
@@ -897,7 +897,7 @@ static void vc4_async_page_flip_fence_complete(struct dma_fence *fence,
struct dma_fence_cb *cb)
{
struct vc4_async_flip_state *flip_state =
- container_of(cb, struct vc4_async_flip_state, cb.fence);
+ container_of_const(cb, struct vc4_async_flip_state, cb.fence);
vc4_async_page_flip_complete(flip_state);
dma_fence_put(fence);
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..0923680f2b2b 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)
@@ -1625,7 +1620,7 @@ static void vc4_dsi_dma_chan_release(void *ptr)
static void vc4_dsi_release(struct kref *kref)
{
struct vc4_dsi *dsi =
- container_of(kref, struct vc4_dsi, kref);
+ container_of_const(kref, struct vc4_dsi, kref);
kfree(dsi);
}
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 628d40ff3aa1..0cb2e86edbf3 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -315,7 +315,7 @@ static void
vc4_reset_work(struct work_struct *work)
{
struct vc4_dev *vc4 =
- container_of(work, struct vc4_dev, hangcheck.reset_work);
+ container_of_const(work, struct vc4_dev, hangcheck.reset_work);
vc4_save_hang_state(&vc4->base);
@@ -1039,7 +1039,8 @@ vc4_job_handle_completed(struct vc4_dev *vc4)
static void vc4_seqno_cb_work(struct work_struct *work)
{
- struct vc4_seqno_cb *cb = container_of(work, struct vc4_seqno_cb, work);
+ struct vc4_seqno_cb *cb =
+ container_of_const(work, struct vc4_seqno_cb, work);
cb->func(cb);
}
@@ -1077,7 +1078,7 @@ static void
vc4_job_done_work(struct work_struct *work)
{
struct vc4_dev *vc4 =
- container_of(work, struct vc4_dev, job_done_work);
+ container_of_const(work, struct vc4_dev, job_done_work);
vc4_job_handle_completed(vc4);
}
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 14628864487a..b3e7030305ea 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -948,9 +948,10 @@ static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder)
static void vc4_hdmi_scrambling_wq(struct work_struct *work)
{
- struct vc4_hdmi *vc4_hdmi = container_of(to_delayed_work(work),
- struct vc4_hdmi,
- scrambling_work);
+ struct vc4_hdmi *vc4_hdmi =
+ container_of_const(to_delayed_work(work),
+ struct vc4_hdmi,
+ scrambling_work);
if (drm_scdc_get_scrambling_status(vc4_hdmi->ddc))
return;
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_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 1e6db0121ccd..6408fbd1684e 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -63,7 +63,7 @@ static void
vc4_overflow_mem_work(struct work_struct *work)
{
struct vc4_dev *vc4 =
- container_of(work, struct vc4_dev, overflow_mem_work);
+ container_of_const(work, struct vc4_dev, overflow_mem_work);
struct vc4_bo *bo;
int bin_bo_slot;
struct vc4_exec_info *exec;
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 dee525bacd4b..f3cb28657973 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -1333,7 +1333,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);
+ container_of_const(state, typeof(*vc4_state), base);
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_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 29a664c8bf44..f48cf1ea48bb 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -349,7 +349,7 @@ int vc4_v3d_bin_bo_get(struct vc4_dev *vc4, bool *used)
static void bin_bo_release(struct kref *ref)
{
- struct vc4_dev *vc4 = container_of(ref, struct vc4_dev, bin_bo_kref);
+ struct vc4_dev *vc4 = container_of_const(ref, struct vc4_dev, bin_bo_kref);
if (WARN_ON_ONCE(!vc4->bin_bo))
return;
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.1
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 b3e7030305ea..4b3bf77bb5cd 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -531,6 +531,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.1
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 4b3bf77bb5cd..78749c6fa837 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -150,10 +150,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;
}
@@ -524,8 +530,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)
@@ -558,6 +568,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;
@@ -571,6 +582,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 =
@@ -590,6 +644,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);
}
@@ -607,6 +662,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;
@@ -617,6 +673,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 = {
@@ -625,6 +683,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)
{
@@ -671,6 +756,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;
@@ -825,7 +912,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);
@@ -1066,6 +1153,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;
@@ -1079,7 +1168,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
@@ -1232,7 +1321,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.1
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 78749c6fa837..e5e7fe4c6a63 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -149,8 +149,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;
@@ -912,7 +912,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);
@@ -1168,7 +1168,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
@@ -1321,7 +1321,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.1
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 e5e7fe4c6a63..78c17166f296 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1233,7 +1233,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
*
*
@@ -1243,28 +1243,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])
{
@@ -1278,6 +1262,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)
@@ -1300,7 +1298,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:
@@ -1315,7 +1314,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.1
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 78c17166f296..53e4afcacc6f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1201,52 +1201,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,
@@ -1283,6 +1303,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;
@@ -1299,7 +1320,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:
@@ -1314,16 +1335,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.1
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 53e4afcacc6f..c74a01f97de5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1296,6 +1296,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)
@@ -1305,6 +1319,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;
@@ -1319,11 +1334,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 |
@@ -1335,7 +1353,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.1
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 c74a01f97de5..5d37952d620b 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1237,6 +1237,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
*
@@ -1300,6 +1331,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.1
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 5d37952d620b..523b55a18409 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1300,6 +1300,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])
{
@@ -1345,6 +1376,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.1
Hi Maxime,
On 26/01/2023 14:46, Maxime Ripard wrote:
> 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_bo.c | 2 +-
> drivers/gpu/drm/vc4/vc4_crtc.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 | 19 ++++-----
> drivers/gpu/drm/vc4/vc4_gem.c | 7 ++--
> drivers/gpu/drm/vc4/vc4_hdmi.c | 7 ++--
> drivers/gpu/drm/vc4/vc4_hdmi.h | 16 +++----
> drivers/gpu/drm/vc4/vc4_irq.c | 2 +-
> drivers/gpu/drm/vc4/vc4_kms.c | 16 +++----
> drivers/gpu/drm/vc4/vc4_plane.c | 2 +-
> drivers/gpu/drm/vc4/vc4_txp.c | 12 ++----
> drivers/gpu/drm/vc4/vc4_v3d.c | 2 +-
> drivers/gpu/drm/vc4/vc4_vec.c | 14 ++-----
> 16 files changed, 65 insertions(+), 117 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)
I think it is OKish to do this in a define like this, but...
> +
> 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_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 86d629e45307..d0a00ed42cb0 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object *gem_bo)
> static void vc4_bo_cache_time_work(struct work_struct *work)
> {
> struct vc4_dev *vc4 =
> - container_of(work, struct vc4_dev, bo_cache.time_work);
> + container_of_const(work, struct vc4_dev, bo_cache.time_work);
...I think this is misleading. It's definitely not const, so switching to
container_of_const suggests that there is some 'constness' involved, which
isn't the case. I'd leave this just as 'container_of'. This also reduces the
size of the patch, since this is done in quite a few places.
Regards,
Hans
> struct drm_device *dev = &vc4->base;
>
> mutex_lock(&vc4->bo_lock);
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index cdc0559221f0..4425dc15308d 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -869,7 +869,7 @@ vc4_async_page_flip_complete(struct vc4_async_flip_state *flip_state)
> static void vc4_async_page_flip_seqno_complete(struct vc4_seqno_cb *cb)
> {
> struct vc4_async_flip_state *flip_state =
> - container_of(cb, struct vc4_async_flip_state, cb.seqno);
> + container_of_const(cb, struct vc4_async_flip_state, cb.seqno);
> struct vc4_bo *bo = NULL;
>
> if (flip_state->old_fb) {
> @@ -897,7 +897,7 @@ static void vc4_async_page_flip_fence_complete(struct dma_fence *fence,
> struct dma_fence_cb *cb)
> {
> struct vc4_async_flip_state *flip_state =
> - container_of(cb, struct vc4_async_flip_state, cb.fence);
> + container_of_const(cb, struct vc4_async_flip_state, cb.fence);
>
> vc4_async_page_flip_complete(flip_state);
> dma_fence_put(fence);
> 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..0923680f2b2b 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)
> @@ -1625,7 +1620,7 @@ static void vc4_dsi_dma_chan_release(void *ptr)
> static void vc4_dsi_release(struct kref *kref)
> {
> struct vc4_dsi *dsi =
> - container_of(kref, struct vc4_dsi, kref);
> + container_of_const(kref, struct vc4_dsi, kref);
>
> kfree(dsi);
> }
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index 628d40ff3aa1..0cb2e86edbf3 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -315,7 +315,7 @@ static void
> vc4_reset_work(struct work_struct *work)
> {
> struct vc4_dev *vc4 =
> - container_of(work, struct vc4_dev, hangcheck.reset_work);
> + container_of_const(work, struct vc4_dev, hangcheck.reset_work);
>
> vc4_save_hang_state(&vc4->base);
>
> @@ -1039,7 +1039,8 @@ vc4_job_handle_completed(struct vc4_dev *vc4)
>
> static void vc4_seqno_cb_work(struct work_struct *work)
> {
> - struct vc4_seqno_cb *cb = container_of(work, struct vc4_seqno_cb, work);
> + struct vc4_seqno_cb *cb =
> + container_of_const(work, struct vc4_seqno_cb, work);
>
> cb->func(cb);
> }
> @@ -1077,7 +1078,7 @@ static void
> vc4_job_done_work(struct work_struct *work)
> {
> struct vc4_dev *vc4 =
> - container_of(work, struct vc4_dev, job_done_work);
> + container_of_const(work, struct vc4_dev, job_done_work);
>
> vc4_job_handle_completed(vc4);
> }
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 14628864487a..b3e7030305ea 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -948,9 +948,10 @@ static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder)
>
> static void vc4_hdmi_scrambling_wq(struct work_struct *work)
> {
> - struct vc4_hdmi *vc4_hdmi = container_of(to_delayed_work(work),
> - struct vc4_hdmi,
> - scrambling_work);
> + struct vc4_hdmi *vc4_hdmi =
> + container_of_const(to_delayed_work(work),
> + struct vc4_hdmi,
> + scrambling_work);
>
> if (drm_scdc_get_scrambling_status(vc4_hdmi->ddc))
> return;
> 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_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> index 1e6db0121ccd..6408fbd1684e 100644
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -63,7 +63,7 @@ static void
> vc4_overflow_mem_work(struct work_struct *work)
> {
> struct vc4_dev *vc4 =
> - container_of(work, struct vc4_dev, overflow_mem_work);
> + container_of_const(work, struct vc4_dev, overflow_mem_work);
> struct vc4_bo *bo;
> int bin_bo_slot;
> struct vc4_exec_info *exec;
> 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 dee525bacd4b..f3cb28657973 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -1333,7 +1333,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);
> + container_of_const(state, typeof(*vc4_state), base);
>
> 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_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index 29a664c8bf44..f48cf1ea48bb 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -349,7 +349,7 @@ int vc4_v3d_bin_bo_get(struct vc4_dev *vc4, bool *used)
>
> static void bin_bo_release(struct kref *ref)
> {
> - struct vc4_dev *vc4 = container_of(ref, struct vc4_dev, bin_bo_kref);
> + struct vc4_dev *vc4 = container_of_const(ref, struct vc4_dev, bin_bo_kref);
>
> if (WARN_ON_ONCE(!vc4->bin_bo))
> return;
> 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,
>
Hi Maxime, Dave,
On 26/01/2023 14:46, Maxime Ripard 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.
Do we have to copy that property as-is?
First of all, I think this should really be a drm-level property, rather than
a driver property: RGB Quantization Range mismatches are the bane of my life,
and I think a way to override this would help everyone.
Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty meaningless.
Can't we stick to something closer to what the CTA-861/HDMI specs use, which is
'RGB Quantization Range'? So either use that, or just 'RGB Range'.
In addition, 'Limited 16:235' should just be 'Limited' since the actual range
depends on the bits-per-color-component.
>
> 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 4b3bf77bb5cd..78749c6fa837 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -150,10 +150,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;
> }
> @@ -524,8 +530,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)
> @@ -558,6 +568,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 ||
The problem with this is that this will cause a mode change, even though all
that is necessary is to update the csc matrix and AVI InfoFrame.
I used this code (added just before the 'return 0;' at the end of this function):
if (old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb) {
const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
old_vc4_state->broadcast_rgb = new_vc4_state->broadcast_rgb;
vc4_hdmi_set_avi_infoframe(encoder);
if (vc4_hdmi->variant->csc_setup)
vc4_hdmi->variant->csc_setup(vc4_hdmi, old_state, mode);
}
I'm certain this is in the wrong place, but I'm not familiar enough with the drm API
to determine where this should go.
This approach probably applies to the hdr_metadata metadata as well, that too
doesn't need a mode change.
I see that the i915 driver has a 'fastset' mechanism for changes like this, but
it is not clear to me how that interacts with the drm API.
I've been playing around with this vc4 driver and it is proving to be very useful
for debugging all sorts of quantization range bugs in other equipment.
Regards,
Hans
> !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
> struct drm_crtc_state *crtc_state;
>
> @@ -571,6 +582,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 =
> @@ -590,6 +644,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);
> }
>
> @@ -607,6 +662,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;
> @@ -617,6 +673,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 = {
> @@ -625,6 +683,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)
> {
> @@ -671,6 +756,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;
> @@ -825,7 +912,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);
> @@ -1066,6 +1153,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;
> @@ -1079,7 +1168,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
> @@ -1232,7 +1321,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) \
>
Hi Hans
On Sat, 18 Feb 2023 at 11:33, Hans Verkuil <[email protected]> wrote:
>
> Hi Maxime, Dave,
>
> On 26/01/2023 14:46, Maxime Ripard 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.
>
> Do we have to copy that property as-is?
Firstly I'll agree with your later comment that having this control
allows testing of a range of output modes, and working around HDMI
sinks that have dodgy implementations.
(In our vendor kernel we now also have a property to override the
kernel chosen output format to enable testing of YCbCr4:4:4 and 4:2:2
output).
The DRM subsystem has the requirement for an open-source userspace
project to be using any new property before it will be merged [1].
This property already exists for i915 and gma-500, therefore avoids
that requirement.
There are objections to the UAPI for Broadcast RGB [2], but if it's
good enough for the existing implementations then there can be no
objection to it being implemented in the same way for other drivers.
Otherwise it is a missing feature of the DRM API, and the linked
discussion is realistically at least a year away from being resolved.
Why bury our heads in the sand for that period?
[1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
[2] https://lists.freedesktop.org/archives/dri-devel/2023-February/391061.html
> First of all, I think this should really be a drm-level property, rather than
> a driver property: RGB Quantization Range mismatches are the bane of my life,
> and I think a way to override this would help everyone.
Linked to above, if it were the preferred method for controlling this
then I would expect it to become a drm-level property.
> Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty meaningless.
> Can't we stick to something closer to what the CTA-861/HDMI specs use, which is
> 'RGB Quantization Range'? So either use that, or just 'RGB Range'.
>
> In addition, 'Limited 16:235' should just be 'Limited' since the actual range
> depends on the bits-per-color-component.
It's documented UAPI with those names[3], therefore any change would
be a change to user-space's expectation and a regression. At least by
sticking with the same names then any user space implementation that
exists for i915 or gma-500 will also work in the same way on vc4.
[3] https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#existing-kms-properties
> >
> > 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 4b3bf77bb5cd..78749c6fa837 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -150,10 +150,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;
> > }
> > @@ -524,8 +530,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)
> > @@ -558,6 +568,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 ||
>
> The problem with this is that this will cause a mode change, even though all
> that is necessary is to update the csc matrix and AVI InfoFrame.
>
> I used this code (added just before the 'return 0;' at the end of this function):
>
> if (old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb) {
> const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
>
> old_vc4_state->broadcast_rgb = new_vc4_state->broadcast_rgb;
> vc4_hdmi_set_avi_infoframe(encoder);
> if (vc4_hdmi->variant->csc_setup)
> vc4_hdmi->variant->csc_setup(vc4_hdmi, old_state, mode);
> }
>
> I'm certain this is in the wrong place, but I'm not familiar enough with the drm API
> to determine where this should go.
atomic_check is meant to validate the state, but must never touch the
hardware [4].
atomic_commit updates the hardware, although for struct
drm_connector_helper_funcs atomic_commit is documented as only being
applicable for writeback connectors [5]. Hence our only sensible
options are to trigger a mode change which reprograms everything, or
ignore the helpers.
There may be scope for improving this, but it'll need significant
driver (and possibly framework) rework in order to be done properly.
[4] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1083
[5] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1101
> This approach probably applies to the hdr_metadata metadata as well, that too
> doesn't need a mode change.
>
> I see that the i915 driver has a 'fastset' mechanism for changes like this, but
> it is not clear to me how that interacts with the drm API.
>
> I've been playing around with this vc4 driver and it is proving to be very useful
> for debugging all sorts of quantization range bugs in other equipment.
Glad it's proving useful.
We're also needing it as users have misbehaving monitors, and telling
them to replace the monitor as it breaks the HDMI spec generally
doesn't go down well! Likewise I've had users needing YCbCr444 output
because of some limitation in their receiving device, hence overriding
the kernel choice of output mode. It'd be nice if there were consensus
on the correct way to achieve that.
Cheers.
Dave
> Regards,
>
> Hans
>
> > !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
> > struct drm_crtc_state *crtc_state;
> >
> > @@ -571,6 +582,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 =
> > @@ -590,6 +644,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);
> > }
> >
> > @@ -607,6 +662,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;
> > @@ -617,6 +673,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 = {
> > @@ -625,6 +683,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)
> > {
> > @@ -671,6 +756,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;
> > @@ -825,7 +912,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);
> > @@ -1066,6 +1153,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;
> > @@ -1079,7 +1168,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
> > @@ -1232,7 +1321,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) \
> >
>
Hi Hans,
On Sat, Feb 18, 2023 at 11:45:04AM +0100, Hans Verkuil wrote:
> > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> > index 86d629e45307..d0a00ed42cb0 100644
> > --- a/drivers/gpu/drm/vc4/vc4_bo.c
> > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> > @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object *gem_bo)
> > static void vc4_bo_cache_time_work(struct work_struct *work)
> > {
> > struct vc4_dev *vc4 =
> > - container_of(work, struct vc4_dev, bo_cache.time_work);
> > + container_of_const(work, struct vc4_dev, bo_cache.time_work);
>
> ...I think this is misleading. It's definitely not const, so switching to
> container_of_const suggests that there is some 'constness' involved, which
> isn't the case. I'd leave this just as 'container_of'. This also reduces the
> size of the patch, since this is done in quite a few places.
The name threw me off too, but it's supposed to keep the argument
pointer constness, not always take and return a const pointer. I still
believe that it's beneficial since, if the work pointer was ever to
change constness, we would have that additional check.
Maxime
On Mon, Feb 20, 2023 at 4:24 PM Dave Stevenson
<[email protected]> wrote:
>
> Hi Hans
>
> On Sat, 18 Feb 2023 at 11:33, Hans Verkuil <[email protected]> wrote:
> >
> > Hi Maxime, Dave,
> >
> > On 26/01/2023 14:46, Maxime Ripard 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.
> >
> > Do we have to copy that property as-is?
>
> Firstly I'll agree with your later comment that having this control
> allows testing of a range of output modes, and working around HDMI
> sinks that have dodgy implementations.
> (In our vendor kernel we now also have a property to override the
> kernel chosen output format to enable testing of YCbCr4:4:4 and 4:2:2
> output).
>
> The DRM subsystem has the requirement for an open-source userspace
> project to be using any new property before it will be merged [1].
> This property already exists for i915 and gma-500, therefore avoids
> that requirement.
>
> There are objections to the UAPI for Broadcast RGB [2], but if it's
> good enough for the existing implementations then there can be no
> objection to it being implemented in the same way for other drivers.
> Otherwise it is a missing feature of the DRM API, and the linked
> discussion is realistically at least a year away from being resolved.
> Why bury our heads in the sand for that period?
>
> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> [2] https://lists.freedesktop.org/archives/dri-devel/2023-February/391061.html
FWIW, from a user space perspective I'm still very much in favor of
adding the `Broadcast RGB` property to more drivers and even moving it
to drm core. Splitting things into color pipeline control properties
and infoframe/connector properties is much harder than it seems at
first and IMO needs a more holistic approach that you can't get from
just converting the one property.
>
> > First of all, I think this should really be a drm-level property, rather than
> > a driver property: RGB Quantization Range mismatches are the bane of my life,
> > and I think a way to override this would help everyone.
>
> Linked to above, if it were the preferred method for controlling this
> then I would expect it to become a drm-level property.
>
> > Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty meaningless.
> > Can't we stick to something closer to what the CTA-861/HDMI specs use, which is
> > 'RGB Quantization Range'? So either use that, or just 'RGB Range'.
> >
> > In addition, 'Limited 16:235' should just be 'Limited' since the actual range
> > depends on the bits-per-color-component.
>
> It's documented UAPI with those names[3], therefore any change would
> be a change to user-space's expectation and a regression. At least by
> sticking with the same names then any user space implementation that
> exists for i915 or gma-500 will also work in the same way on vc4.
>
> [3] https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#existing-kms-properties
>
> > >
> > > 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 4b3bf77bb5cd..78749c6fa837 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -150,10 +150,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;
> > > }
> > > @@ -524,8 +530,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)
> > > @@ -558,6 +568,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 ||
> >
> > The problem with this is that this will cause a mode change, even though all
> > that is necessary is to update the csc matrix and AVI InfoFrame.
> >
> > I used this code (added just before the 'return 0;' at the end of this function):
> >
> > if (old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb) {
> > const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
> >
> > old_vc4_state->broadcast_rgb = new_vc4_state->broadcast_rgb;
> > vc4_hdmi_set_avi_infoframe(encoder);
> > if (vc4_hdmi->variant->csc_setup)
> > vc4_hdmi->variant->csc_setup(vc4_hdmi, old_state, mode);
> > }
> >
> > I'm certain this is in the wrong place, but I'm not familiar enough with the drm API
> > to determine where this should go.
>
> atomic_check is meant to validate the state, but must never touch the
> hardware [4].
> atomic_commit updates the hardware, although for struct
> drm_connector_helper_funcs atomic_commit is documented as only being
> applicable for writeback connectors [5]. Hence our only sensible
> options are to trigger a mode change which reprograms everything, or
> ignore the helpers.
> There may be scope for improving this, but it'll need significant
> driver (and possibly framework) rework in order to be done properly.
>
> [4] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1083
> [5] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1101
>
> > This approach probably applies to the hdr_metadata metadata as well, that too
> > doesn't need a mode change.
> >
> > I see that the i915 driver has a 'fastset' mechanism for changes like this, but
> > it is not clear to me how that interacts with the drm API.
> >
> > I've been playing around with this vc4 driver and it is proving to be very useful
> > for debugging all sorts of quantization range bugs in other equipment.
>
> Glad it's proving useful.
>
> We're also needing it as users have misbehaving monitors, and telling
> them to replace the monitor as it breaks the HDMI spec generally
> doesn't go down well! Likewise I've had users needing YCbCr444 output
> because of some limitation in their receiving device, hence overriding
> the kernel choice of output mode. It'd be nice if there were consensus
> on the correct way to achieve that.
>
> Cheers.
> Dave
>
> > Regards,
> >
> > Hans
> >
> > > !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
> > > struct drm_crtc_state *crtc_state;
> > >
> > > @@ -571,6 +582,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 =
> > > @@ -590,6 +644,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);
> > > }
> > >
> > > @@ -607,6 +662,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;
> > > @@ -617,6 +673,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 = {
> > > @@ -625,6 +683,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)
> > > {
> > > @@ -671,6 +756,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;
> > > @@ -825,7 +912,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);
> > > @@ -1066,6 +1153,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;
> > > @@ -1079,7 +1168,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
> > > @@ -1232,7 +1321,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) \
> > >
> >
>
On 2/20/23 16:23, Dave Stevenson wrote:
> Hi Hans
>
> On Sat, 18 Feb 2023 at 11:33, Hans Verkuil <[email protected]> wrote:
>>
>> Hi Maxime, Dave,
>>
>> On 26/01/2023 14:46, Maxime Ripard 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.
>>
>> Do we have to copy that property as-is?
>
> Firstly I'll agree with your later comment that having this control
> allows testing of a range of output modes, and working around HDMI
> sinks that have dodgy implementations.
> (In our vendor kernel we now also have a property to override the
> kernel chosen output format to enable testing of YCbCr4:4:4 and 4:2:2
> output).
>
> The DRM subsystem has the requirement for an open-source userspace
> project to be using any new property before it will be merged [1].
> This property already exists for i915 and gma-500, therefore avoids
> that requirement.
>
> There are objections to the UAPI for Broadcast RGB [2], but if it's
> good enough for the existing implementations then there can be no
> objection to it being implemented in the same way for other drivers.
> Otherwise it is a missing feature of the DRM API, and the linked
> discussion is realistically at least a year away from being resolved.
> Why bury our heads in the sand for that period?
>
> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> [2] https://lists.freedesktop.org/archives/dri-devel/2023-February/391061.html
>
>> First of all, I think this should really be a drm-level property, rather than
>> a driver property: RGB Quantization Range mismatches are the bane of my life,
>> and I think a way to override this would help everyone.
>
> Linked to above, if it were the preferred method for controlling this
> then I would expect it to become a drm-level property.
>
>> Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty meaningless.
>> Can't we stick to something closer to what the CTA-861/HDMI specs use, which is
>> 'RGB Quantization Range'? So either use that, or just 'RGB Range'.
>>
>> In addition, 'Limited 16:235' should just be 'Limited' since the actual range
>> depends on the bits-per-color-component.
>
> It's documented UAPI with those names[3], therefore any change would
> be a change to user-space's expectation and a regression. At least by
> sticking with the same names then any user space implementation that
> exists for i915 or gma-500 will also work in the same way on vc4.
>
> [3] https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#existing-kms-properties
Thank you for linking to these threads. It's a terrible name, but it is still
better to have this property than to not have it :-)
>
>>>
>>> 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 4b3bf77bb5cd..78749c6fa837 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> @@ -150,10 +150,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;
>>> }
>>> @@ -524,8 +530,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)
>>> @@ -558,6 +568,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 ||
>>
>> The problem with this is that this will cause a mode change, even though all
>> that is necessary is to update the csc matrix and AVI InfoFrame.
>>
>> I used this code (added just before the 'return 0;' at the end of this function):
>>
>> if (old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb) {
>> const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
>>
>> old_vc4_state->broadcast_rgb = new_vc4_state->broadcast_rgb;
>> vc4_hdmi_set_avi_infoframe(encoder);
>> if (vc4_hdmi->variant->csc_setup)
>> vc4_hdmi->variant->csc_setup(vc4_hdmi, old_state, mode);
>> }
>>
>> I'm certain this is in the wrong place, but I'm not familiar enough with the drm API
>> to determine where this should go.
>
> atomic_check is meant to validate the state, but must never touch the
> hardware [4].
> atomic_commit updates the hardware, although for struct
> drm_connector_helper_funcs atomic_commit is documented as only being
> applicable for writeback connectors [5]. Hence our only sensible
> options are to trigger a mode change which reprograms everything, or
> ignore the helpers.
> There may be scope for improving this, but it'll need significant
> driver (and possibly framework) rework in order to be done properly.
>
> [4] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1083
> [5] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1101
>
>> This approach probably applies to the hdr_metadata metadata as well, that too
>> doesn't need a mode change.
>>
>> I see that the i915 driver has a 'fastset' mechanism for changes like this, but
>> it is not clear to me how that interacts with the drm API.
>>
>> I've been playing around with this vc4 driver and it is proving to be very useful
>> for debugging all sorts of quantization range bugs in other equipment.
>
> Glad it's proving useful.
>
> We're also needing it as users have misbehaving monitors, and telling
> them to replace the monitor as it breaks the HDMI spec generally
> doesn't go down well! Likewise I've had users needing YCbCr444 output
> because of some limitation in their receiving device, hence overriding
> the kernel choice of output mode. It'd be nice if there were consensus
> on the correct way to achieve that.
In any case, given this background you can add my:
Reviewed-by: Hans Verkuil <[email protected]>
Regards,
Hans
>
> Cheers.
> Dave
>
>> Regards,
>>
>> Hans
>>
>>> !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
>>> struct drm_crtc_state *crtc_state;
>>>
>>> @@ -571,6 +582,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 =
>>> @@ -590,6 +644,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);
>>> }
>>>
>>> @@ -607,6 +662,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;
>>> @@ -617,6 +673,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 = {
>>> @@ -625,6 +683,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)
>>> {
>>> @@ -671,6 +756,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;
>>> @@ -825,7 +912,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);
>>> @@ -1066,6 +1153,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;
>>> @@ -1079,7 +1168,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
>>> @@ -1232,7 +1321,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) \
>>>
>>
On 2/21/23 12:38, Maxime Ripard wrote:
> Hi Hans,
>
> On Sat, Feb 18, 2023 at 11:45:04AM +0100, Hans Verkuil wrote:
>>> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>>> index 86d629e45307..d0a00ed42cb0 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_bo.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>>> @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object *gem_bo)
>>> static void vc4_bo_cache_time_work(struct work_struct *work)
>>> {
>>> struct vc4_dev *vc4 =
>>> - container_of(work, struct vc4_dev, bo_cache.time_work);
>>> + container_of_const(work, struct vc4_dev, bo_cache.time_work);
>>
>> ...I think this is misleading. It's definitely not const, so switching to
>> container_of_const suggests that there is some 'constness' involved, which
>> isn't the case. I'd leave this just as 'container_of'. This also reduces the
>> size of the patch, since this is done in quite a few places.
>
> The name threw me off too, but it's supposed to keep the argument
> pointer constness, not always take and return a const pointer. I still
> believe that it's beneficial since, if the work pointer was ever to
> change constness, we would have that additional check.
If both inner (work) and outer (vc4) pointers are non-const, then there is no sense
in switching to container_of_const. I don't see it used like that elsewhere
in the kernel.
It only makes sense to use it if the inner pointer might be const.
If the work pointer (in this example) would ever become const, then the
regular container_of macro would report a warning, that's something that
was added in commit 7376e561fd2e. So preemptively switching to container_of_const
appears unnecessary to me.
Regards,
Hans