Re: [PATCH v5 9/9] drm/vc4: hdmi: Enable 10/12 bpc output
Hi Dave, On Wed, Dec 09, 2020 at 03:27:05PM +, Dave Stevenson wrote: > Hi Maxime > > On Mon, 7 Dec 2020 at 15:57, Maxime Ripard wrote: > > > > 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 | 71 - > > drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + > > drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 9 > > 3 files changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index f4ff6b5db484..fb30ddd842b1 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) > > > > @@ -179,6 +190,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; > > + > > Having added protection from the kzalloc failing in 4/9, this adds > back in dereferencing conn_state without having checked it succeeded > first :( Yeah, you're right :/ I also noticed that we kfree the connector->state pointer, but nothing really guarantees that the base field in our structure is at the offset 0 I've fixed both issues, I'll send a new iteration Thanks! Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 9/9] drm/vc4: hdmi: Enable 10/12 bpc output
Hi Maxime
On Mon, 7 Dec 2020 at 15:57, Maxime Ripard wrote:
>
> 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 | 71 -
> drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +
> drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 9
> 3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index f4ff6b5db484..fb30ddd842b1 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)
>
> @@ -179,6 +190,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;
> +
Having added protection from the kzalloc failing in 4/9, this adds
back in dereferencing conn_state without having checked it succeeded
first :(
Otherwise this looks fine.
Dave
> __drm_atomic_helper_connector_reset(connector, _state->base);
>
> if (connector->state)
> @@ -228,12 +242,20 @@ static int vc4_hdmi_connector_init(struct drm_device
> *dev,
> vc4_hdmi->ddc);
> drm_connector_helper_add(connector, _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, 12);
>
> connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
> DRM_CONNECTOR_POLL_DISCONNECT);
> @@ -499,6 +521,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;
> @@ -542,7 +565,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;
> @@ -562,6 +587,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,
> @@ -587,6 +615,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,
[PATCH v5 9/9] 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 | 71 -
drivers/gpu/drm/vc4/vc4_hdmi.h | 1 +
drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 9
3 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index f4ff6b5db484..fb30ddd842b1 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)
@@ -179,6 +190,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, _state->base);
if (connector->state)
@@ -228,12 +242,20 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
vc4_hdmi->ddc);
drm_connector_helper_add(connector, _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, 12);
connector->polled = (DRM_CONNECTOR_POLL_CONNECT |
DRM_CONNECTOR_POLL_DISCONNECT);
@@ -499,6 +521,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;
@@ -542,7 +565,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;
@@ -562,6 +587,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,
@@ -587,6 +615,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;
+ HDMI_WRITE(HDMI_GCP_CONFIG, reg);
