Re: [PATCH v2 6/6] drm/vc4: hdmi: Enable 10/12 bpc output
Hi Maxime
On Thu, 8 Oct 2020 at 13:44, Maxime Ripard wrote:
>
> The BCM2711 supports higher bpc count than just 8, so let's support it in
> our driver.
Something appears to be slightly off with this one. Running the
monitor (Dell U2718Q 4k monitor) at 1080p60.
Use modetest to switch "max bpc" to 10bpc, and the monitor reports 1080p48.
Use modetest to switch "max bpc" to 12bpc, and the monitor reports 1080p40.
I haven't got an HDMI analyser to hand to confirm whether we're
actually putting out 10 or 12bpc.
So at a guess a clock is unaltered, but the number of bits being sent
over the link is increased, thereby dropping the overall frame rate.
Having tested with the firmware it only points to my monitor not
supporting alternate bpc values - not so helpful.
I'm not sure the handling of the GCP is correct either. It is
enabled/disabled via bit 0 of HDMI_RAM_PACKET_CONFIG in the same way
all the infoframes are.
The firmware enables/disables sending the GCP when handling avmute,
but this driver isn't handling avmute so it never gets enabled
I'll have a further experiment to see if I can make things play ball.
I'll need to collect the analyser and deep colour display to really
try it out.
Dave
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 68 +-
> drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +-
> drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 9 -
> 3 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 21d20c8494e8..3ff72fab4c40 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -76,6 +76,17 @@
> #define VC5_HDMI_VERTB_VSPO_SHIFT 16
> #define VC5_HDMI_VERTB_VSPO_MASK VC4_MASK(29, 16)
>
> +#define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_SHIFT 8
> +#define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_MASK VC4_MASK(10,
> 8)
> +
> +#define VC5_HDMI_DEEP_COLOR_CONFIG_1_COLOR_DEPTH_SHIFT 0
> +#define VC5_HDMI_DEEP_COLOR_CONFIG_1_COLOR_DEPTH_MASK VC4_MASK(3, 0)
> +
> +#define VC5_HDMI_GCP_CONFIG_GCP_ENABLE BIT(31)
> +
> +#define VC5_HDMI_GCP_WORD_1_GCP_SUBPACKET_BYTE_1_SHIFT 8
> +#define VC5_HDMI_GCP_WORD_1_GCP_SUBPACKET_BYTE_1_MASK VC4_MASK(15, 8)
> +
> # define VC4_HD_M_SW_RST BIT(2)
> # define VC4_HD_M_ENABLE BIT(0)
>
> @@ -177,6 +188,9 @@ static void vc4_hdmi_connector_reset(struct drm_connector
> *connector)
>
> kfree(connector->state);
>
> + conn_state->base.max_bpc = 8;
> + conn_state->base.max_requested_bpc = 8;
> +
> __drm_atomic_helper_connector_reset(connector, &conn_state->base);
> drm_atomic_helper_connector_tv_reset(connector);
> }
> @@ -224,12 +238,20 @@ static int vc4_hdmi_connector_init(struct drm_device
> *dev,
> vc4_hdmi->ddc);
> drm_connector_helper_add(connector, &vc4_hdmi_connector_helper_funcs);
>
> + /*
> +* Some of the properties below require access to state, like bpc.
> +* Allocate some default initial connector state with our reset
> helper.
> +*/
> + if (connector->funcs->reset)
> + connector->funcs->reset(connector);
> +
> /* Create and attach TV margin props to this connector. */
> ret = drm_mode_create_tv_margin_properties(dev);
> if (ret)
> return ret;
>
> drm_connector_attach_tv_margin_properties(connector);
> + drm_connector_attach_max_bpc_property(connector, 8, 16);
>
> connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
> DRM_CONNECTOR_POLL_DISCONNECT);
> @@ -495,6 +517,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
> bool enable)
> }
>
> static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
> +struct drm_connector_state *state,
> struct drm_display_mode *mode)
> {
> bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
> @@ -538,7 +561,9 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi
> *vc4_hdmi,
> HDMI_WRITE(HDMI_VERTB0, vertb_even);
> HDMI_WRITE(HDMI_VERTB1, vertb);
> }
> +
> static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
> +struct drm_connector_state *state,
> struct drm_display_mode *mode)
> {
> bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
> @@ -558,6 +583,9 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi
> *vc4_hdmi,
> mode->crtc_vsync_end -
> interlaced,
> VC4_HDMI_VERTB_VBP));
> + unsigned char gcp;
> + bool gcp_en;
> + u32 reg;
>
> HDMI_WRITE(HDMI_VEC_INTERFACE_XBAR, 0x354021);
> HDMI_WRIT
[PATCH v2 6/6] drm/vc4: hdmi: Enable 10/12 bpc output
The BCM2711 supports higher bpc count than just 8, so let's support it in
our driver.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 68 +-
drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +-
drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 9 -
3 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 21d20c8494e8..3ff72fab4c40 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -76,6 +76,17 @@
#define VC5_HDMI_VERTB_VSPO_SHIFT 16
#define VC5_HDMI_VERTB_VSPO_MASK VC4_MASK(29, 16)
+#define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_SHIFT 8
+#define VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_MASK VC4_MASK(10, 8)
+
+#define VC5_HDMI_DEEP_COLOR_CONFIG_1_COLOR_DEPTH_SHIFT 0
+#define VC5_HDMI_DEEP_COLOR_CONFIG_1_COLOR_DEPTH_MASK VC4_MASK(3, 0)
+
+#define VC5_HDMI_GCP_CONFIG_GCP_ENABLE BIT(31)
+
+#define VC5_HDMI_GCP_WORD_1_GCP_SUBPACKET_BYTE_1_SHIFT 8
+#define VC5_HDMI_GCP_WORD_1_GCP_SUBPACKET_BYTE_1_MASK VC4_MASK(15, 8)
+
# define VC4_HD_M_SW_RST BIT(2)
# define VC4_HD_M_ENABLE BIT(0)
@@ -177,6 +188,9 @@ static void vc4_hdmi_connector_reset(struct drm_connector
*connector)
kfree(connector->state);
+ conn_state->base.max_bpc = 8;
+ conn_state->base.max_requested_bpc = 8;
+
__drm_atomic_helper_connector_reset(connector, &conn_state->base);
drm_atomic_helper_connector_tv_reset(connector);
}
@@ -224,12 +238,20 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
vc4_hdmi->ddc);
drm_connector_helper_add(connector, &vc4_hdmi_connector_helper_funcs);
+ /*
+* Some of the properties below require access to state, like bpc.
+* Allocate some default initial connector state with our reset helper.
+*/
+ if (connector->funcs->reset)
+ connector->funcs->reset(connector);
+
/* Create and attach TV margin props to this connector. */
ret = drm_mode_create_tv_margin_properties(dev);
if (ret)
return ret;
drm_connector_attach_tv_margin_properties(connector);
+ drm_connector_attach_max_bpc_property(connector, 8, 16);
connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT);
@@ -495,6 +517,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
bool enable)
}
static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
+struct drm_connector_state *state,
struct drm_display_mode *mode)
{
bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
@@ -538,7 +561,9 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
HDMI_WRITE(HDMI_VERTB0, vertb_even);
HDMI_WRITE(HDMI_VERTB1, vertb);
}
+
static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
+struct drm_connector_state *state,
struct drm_display_mode *mode)
{
bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
@@ -558,6 +583,9 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
mode->crtc_vsync_end -
interlaced,
VC4_HDMI_VERTB_VBP));
+ unsigned char gcp;
+ bool gcp_en;
+ u32 reg;
HDMI_WRITE(HDMI_VEC_INTERFACE_XBAR, 0x354021);
HDMI_WRITE(HDMI_HORZA,
@@ -583,6 +611,39 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi,
HDMI_WRITE(HDMI_VERTB0, vertb_even);
HDMI_WRITE(HDMI_VERTB1, vertb);
+ switch (state->max_bpc) {
+ case 12:
+ gcp = 6;
+ gcp_en = true;
+ break;
+ case 10:
+ gcp = 5;
+ gcp_en = true;
+ break;
+ case 8:
+ default:
+ gcp = 4;
+ gcp_en = false;
+ break;
+ }
+
+ reg = HDMI_READ(HDMI_DEEP_COLOR_CONFIG_1);
+ reg &= ~(VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE_MASK |
+VC5_HDMI_DEEP_COLOR_CONFIG_1_COLOR_DEPTH_MASK);
+ reg |= VC4_SET_FIELD(2, VC5_HDMI_DEEP_COLOR_CONFIG_1_INIT_PACK_PHASE) |
+ VC4_SET_FIELD(gcp, VC5_HDMI_DEEP_COLOR_CONFIG_1_COLOR_DEPTH);
+ HDMI_WRITE(HDMI_DEEP_COLOR_CONFIG_1, reg);
+
+ reg = HDMI_READ(HDMI_GCP_WORD_1);
+ reg &= ~VC5_HDMI_GCP_WORD_1_GCP_SUBPACKET_BYTE_1_MASK;
+ reg |= VC4_SET_FIELD(gcp, VC5_HDMI_GCP_WORD_1_GCP_SUBPACKET_BYTE_1);
+ HDMI_WRITE(HDMI_GCP_WORD_1, reg);
+
+ reg = HDMI_READ(HDMI_GCP_CONFIG);
+ reg &= ~VC5_HDMI_GCP_CONFIG_GCP_ENABLE;
+ reg |= gcp_en ? VC5_HDMI_GCP_CONFIG_GCP_ENABLE : 0;
+
