* [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
@ 2017-03-20 6:10 ` Nickey Yang
2017-03-20 11:08 ` Jose Abreu
2017-03-21 7:47 ` Andrzej Hajda
0 siblings, 2 replies; 7+ messages in thread
From: Nickey Yang @ 2017-03-20 6:10 UTC (permalink / raw
To: architt, airlied
Cc: linux-kernel, dri-devel, linux-rockchip, dianders,
laurent.pinchart+renesas, joabreu, andy.yan, nickey.yang,
zhengyang, rmk+kernel, vladimir_zapolskiy
Vendor specific infoframe is mandatory for 4K2K resolution.
Without this, the HDMI protocol compliance fails.
Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
2 files changed, 54 insertions(+)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 9a9ec27..79e2e48 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
}
+static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
+ struct drm_display_mode *mode)
+{
+ struct hdmi_vendor_infoframe frame;
+ u8 buffer[10];
+ ssize_t err;
+
+ err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
+ if (err) {
+ dev_err(hdmi->dev,
+ "Failed to get vendor infoframe from mode: %zd\n", err);
+ return;
+ }
+
+ err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (!err) {
+ dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
+ err);
+ return;
+ }
+ hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
+ HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
+
+ /* Set the length of HDMI vendor specific InfoFrame payload */
+ hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
+
+ /* Set 24bit IEEE Registration Identifier */
+ hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
+ hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
+ hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
+
+ /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
+ hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
+ hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
+
+ if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
+ hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
+
+ /* Packet frame interpolation */
+ hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
+
+ /* Auto packets per frame and line spacing */
+ hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
+
+ /* Configures the Frame Composer On RDRB mode */
+ hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
+ HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
+}
+
static void hdmi_av_composer(struct dw_hdmi *hdmi,
const struct drm_display_mode *mode)
{
@@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
/* HDMI Initialization Step F - Configure AVI InfoFrame */
hdmi_config_AVI(hdmi, mode);
+ hdmi_config_vendor_specific_infoframe(hdmi, mode);
} else {
dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
}
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
index 325b0b8..c59f87e 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/dw-hdmi.h
@@ -854,6 +854,10 @@ enum {
HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
+/* FC_DATAUTO0 field values */
+ HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
+ HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
+
/* PHY_CONF0 field values */
HDMI_PHY_CONF0_PDZ_MASK = 0x80,
HDMI_PHY_CONF0_PDZ_OFFSET = 7,
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
@ 2017-03-20 11:08 ` Jose Abreu
0 siblings, 0 replies; 7+ messages in thread
From: Jose Abreu @ 2017-03-20 11:08 UTC (permalink / raw
To: Nickey Yang, architt, airlied
Cc: linux-kernel, dri-devel, linux-rockchip, dianders,
laurent.pinchart+renesas, Jose.Abreu, andy.yan, zhengyang,
rmk+kernel, vladimir_zapolskiy
Hi Nickey,
On 20-03-2017 06:10, Nickey Yang wrote:
> Vendor specific infoframe is mandatory for 4K2K resolution.
> Without this, the HDMI protocol compliance fails.
>
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9a9ec27..79e2e48 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
> }
>
> +static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> + struct drm_display_mode *mode)
> +{
> + struct hdmi_vendor_infoframe frame;
> + u8 buffer[10];
> + ssize_t err;
> +
> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
> + if (err) {
> + dev_err(hdmi->dev,
> + "Failed to get vendor infoframe from mode: %zd\n", err);
I think you should not throw a error here. As you said in commit
message this is only for 4K2K modes and
drm_hdmi_vendor_infoframe_from_display_mode() only checks for
these modes. So just return gracefully and it will be fine.
> + return;
> + }
> +
> + err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
> + if (!err) {
> + dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
> + err);
> + return;
> + }
> + hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
Maybe use hdmi_mask_writeb() here?
> +
> + /* Set the length of HDMI vendor specific InfoFrame payload */
> + hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
> +
> + /* Set 24bit IEEE Registration Identifier */
> + hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
> + hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
> + hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
> +
> + /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
> + hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
> + hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
> +
> + if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> + hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
> +
> + /* Packet frame interpolation */
> + hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
> +
> + /* Auto packets per frame and line spacing */
> + hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
> +
> + /* Configures the Frame Composer On RDRB mode */
> + hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
Same here.
Best regards,
Jose Miguel Abreu
> +}
> +
> static void hdmi_av_composer(struct dw_hdmi *hdmi,
> const struct drm_display_mode *mode)
> {
> @@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>
> /* HDMI Initialization Step F - Configure AVI InfoFrame */
> hdmi_config_AVI(hdmi, mode);
> + hdmi_config_vendor_specific_infoframe(hdmi, mode);
> } else {
> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
> }
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
> index 325b0b8..c59f87e 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
> @@ -854,6 +854,10 @@ enum {
> HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
> HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
>
> +/* FC_DATAUTO0 field values */
> + HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
> + HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
> +
> /* PHY_CONF0 field values */
> HDMI_PHY_CONF0_PDZ_MASK = 0x80,
> HDMI_PHY_CONF0_PDZ_OFFSET = 7,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
@ 2017-03-20 11:08 ` Jose Abreu
0 siblings, 0 replies; 7+ messages in thread
From: Jose Abreu @ 2017-03-20 11:08 UTC (permalink / raw
To: Nickey Yang, architt-sgV2jX0FEOL9JmXXK+q4OQ, airlied-cv59FeDIM0c
Cc: Jose.Abreu-HKixBCOQz3hWk0Htik3J/w,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dianders-F7+t8E8rja9g9hUCZPvPmw,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ,
andy.yan-TNX95d0MmH7DzftRWevZcw,
vladimir_zapolskiy-nmGgyN9QBj3QT0dZR+AlfA,
zhengyang-TNX95d0MmH7DzftRWevZcw
Hi Nickey,
On 20-03-2017 06:10, Nickey Yang wrote:
> Vendor specific infoframe is mandatory for 4K2K resolution.
> Without this, the HDMI protocol compliance fails.
>
> Signed-off-by: Nickey Yang <nickey.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9a9ec27..79e2e48 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
> }
>
> +static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> + struct drm_display_mode *mode)
> +{
> + struct hdmi_vendor_infoframe frame;
> + u8 buffer[10];
> + ssize_t err;
> +
> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
> + if (err) {
> + dev_err(hdmi->dev,
> + "Failed to get vendor infoframe from mode: %zd\n", err);
I think you should not throw a error here. As you said in commit
message this is only for 4K2K modes and
drm_hdmi_vendor_infoframe_from_display_mode() only checks for
these modes. So just return gracefully and it will be fine.
> + return;
> + }
> +
> + err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
> + if (!err) {
> + dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
> + err);
> + return;
> + }
> + hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
Maybe use hdmi_mask_writeb() here?
> +
> + /* Set the length of HDMI vendor specific InfoFrame payload */
> + hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
> +
> + /* Set 24bit IEEE Registration Identifier */
> + hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
> + hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
> + hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
> +
> + /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
> + hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
> + hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
> +
> + if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> + hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
> +
> + /* Packet frame interpolation */
> + hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
> +
> + /* Auto packets per frame and line spacing */
> + hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
> +
> + /* Configures the Frame Composer On RDRB mode */
> + hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
Same here.
Best regards,
Jose Miguel Abreu
> +}
> +
> static void hdmi_av_composer(struct dw_hdmi *hdmi,
> const struct drm_display_mode *mode)
> {
> @@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>
> /* HDMI Initialization Step F - Configure AVI InfoFrame */
> hdmi_config_AVI(hdmi, mode);
> + hdmi_config_vendor_specific_infoframe(hdmi, mode);
> } else {
> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
> }
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
> index 325b0b8..c59f87e 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
> @@ -854,6 +854,10 @@ enum {
> HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
> HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
>
> +/* FC_DATAUTO0 field values */
> + HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
> + HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
> +
> /* PHY_CONF0 field values */
> HDMI_PHY_CONF0_PDZ_MASK = 0x80,
> HDMI_PHY_CONF0_PDZ_OFFSET = 7,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
2017-03-20 6:10 ` [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config Nickey Yang
@ 2017-03-21 7:47 ` Andrzej Hajda
2017-03-21 7:47 ` Andrzej Hajda
1 sibling, 0 replies; 7+ messages in thread
From: Andrzej Hajda @ 2017-03-21 7:47 UTC (permalink / raw
To: Nickey Yang, architt, airlied
Cc: laurent.pinchart+renesas, linux-kernel, dri-devel, dianders,
linux-rockchip, joabreu, rmk+kernel, andy.yan, vladimir_zapolskiy,
zhengyang
On 20.03.2017 07:10, Nickey Yang wrote:
> Vendor specific infoframe is mandatory for 4K2K resolution.
> Without this, the HDMI protocol compliance fails.
>
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9a9ec27..79e2e48 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
> }
>
> +static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> + struct drm_display_mode *mode)
> +{
> + struct hdmi_vendor_infoframe frame;
> + u8 buffer[10];
> + ssize_t err;
> +
> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
> + if (err) {
> + dev_err(hdmi->dev,
> + "Failed to get vendor infoframe from mode: %zd\n", err);
> + return;
> + }
> +
> + err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
> + if (!err) {
> + dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
> + err);
> + return;
> + }
AFAIK function returns number of packed bytes or -EINVAL so the check is
incorrect, and -EINVAL means that there is no need to generate vendor
infoframe, so definitely not dev_err.
> + hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
I see no gain in constructs (0 << HDMI_FC_DATAUTO0_VSD_OFFSET),
additionally mask and offset are redundant (mask == 1 << offset), but it
seems you follow existing practice, so I do not oppose :)
Regards
Andrzej
> +
> + /* Set the length of HDMI vendor specific InfoFrame payload */
> + hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
> +
> + /* Set 24bit IEEE Registration Identifier */
> + hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
> + hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
> + hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
> +
> + /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
> + hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
> + hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
> +
> + if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> + hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
> +
> + /* Packet frame interpolation */
> + hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
> +
> + /* Auto packets per frame and line spacing */
> + hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
> +
> + /* Configures the Frame Composer On RDRB mode */
> + hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
> +}
> +
> static void hdmi_av_composer(struct dw_hdmi *hdmi,
> const struct drm_display_mode *mode)
> {
> @@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>
> /* HDMI Initialization Step F - Configure AVI InfoFrame */
> hdmi_config_AVI(hdmi, mode);
> + hdmi_config_vendor_specific_infoframe(hdmi, mode);
> } else {
> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
> }
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
> index 325b0b8..c59f87e 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
> @@ -854,6 +854,10 @@ enum {
> HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
> HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
>
> +/* FC_DATAUTO0 field values */
> + HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
> + HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
> +
> /* PHY_CONF0 field values */
> HDMI_PHY_CONF0_PDZ_MASK = 0x80,
> HDMI_PHY_CONF0_PDZ_OFFSET = 7,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
@ 2017-03-21 7:47 ` Andrzej Hajda
0 siblings, 0 replies; 7+ messages in thread
From: Andrzej Hajda @ 2017-03-21 7:47 UTC (permalink / raw
To: Nickey Yang, architt, airlied
Cc: laurent.pinchart+renesas, linux-kernel, dri-devel, dianders,
linux-rockchip, joabreu, andy.yan, rmk+kernel, vladimir_zapolskiy,
zhengyang
On 20.03.2017 07:10, Nickey Yang wrote:
> Vendor specific infoframe is mandatory for 4K2K resolution.
> Without this, the HDMI protocol compliance fails.
>
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9a9ec27..79e2e48 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
> }
>
> +static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
> + struct drm_display_mode *mode)
> +{
> + struct hdmi_vendor_infoframe frame;
> + u8 buffer[10];
> + ssize_t err;
> +
> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
> + if (err) {
> + dev_err(hdmi->dev,
> + "Failed to get vendor infoframe from mode: %zd\n", err);
> + return;
> + }
> +
> + err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
> + if (!err) {
> + dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
> + err);
> + return;
> + }
AFAIK function returns number of packed bytes or -EINVAL so the check is
incorrect, and -EINVAL means that there is no need to generate vendor
infoframe, so definitely not dev_err.
> + hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
I see no gain in constructs (0 << HDMI_FC_DATAUTO0_VSD_OFFSET),
additionally mask and offset are redundant (mask == 1 << offset), but it
seems you follow existing practice, so I do not oppose :)
Regards
Andrzej
> +
> + /* Set the length of HDMI vendor specific InfoFrame payload */
> + hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
> +
> + /* Set 24bit IEEE Registration Identifier */
> + hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
> + hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
> + hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
> +
> + /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
> + hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
> + hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
> +
> + if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> + hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
> +
> + /* Packet frame interpolation */
> + hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
> +
> + /* Auto packets per frame and line spacing */
> + hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
> +
> + /* Configures the Frame Composer On RDRB mode */
> + hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
> +}
> +
> static void hdmi_av_composer(struct dw_hdmi *hdmi,
> const struct drm_display_mode *mode)
> {
> @@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>
> /* HDMI Initialization Step F - Configure AVI InfoFrame */
> hdmi_config_AVI(hdmi, mode);
> + hdmi_config_vendor_specific_infoframe(hdmi, mode);
> } else {
> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
> }
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
> index 325b0b8..c59f87e 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
> @@ -854,6 +854,10 @@ enum {
> HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
> HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
>
> +/* FC_DATAUTO0 field values */
> + HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
> + HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
> +
> /* PHY_CONF0 field values */
> HDMI_PHY_CONF0_PDZ_MASK = 0x80,
> HDMI_PHY_CONF0_PDZ_OFFSET = 7,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
2017-03-21 7:47 ` Andrzej Hajda
@ 2017-03-21 7:51 ` Archit Taneja
-1 siblings, 0 replies; 7+ messages in thread
From: Archit Taneja @ 2017-03-21 7:51 UTC (permalink / raw
To: Andrzej Hajda, Nickey Yang
Cc: airlied, laurent.pinchart+renesas, linux-kernel, dri-devel,
dianders, linux-rockchip, joabreu, rmk+kernel, andy.yan,
vladimir_zapolskiy, zhengyang
On 03/21/2017 01:17 PM, Andrzej Hajda wrote:
> On 20.03.2017 07:10, Nickey Yang wrote:
>> Vendor specific infoframe is mandatory for 4K2K resolution.
>> Without this, the HDMI protocol compliance fails.
>>
>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>> ---
>> drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 9a9ec27..79e2e48 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>> hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
>> }
>>
>> +static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>> + struct drm_display_mode *mode)
>> +{
>> + struct hdmi_vendor_infoframe frame;
>> + u8 buffer[10];
>> + ssize_t err;
>> +
>> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
>> + if (err) {
>> + dev_err(hdmi->dev,
>> + "Failed to get vendor infoframe from mode: %zd\n", err);
>> + return;
>> + }
>> +
>> + err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
>> + if (!err) {
>> + dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
>> + err);
>> + return;
>> + }
>
> AFAIK function returns number of packed bytes or -EINVAL so the check is
> incorrect, and -EINVAL means that there is no need to generate vendor
> infoframe, so definitely not dev_err.
I queued this to drm-misc-next before you posted this. We can have a follow
up patch to fix this (since we don't rebase drm-misc).
Thanks,
Archit
>
>> + hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
>> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
>
> I see no gain in constructs (0 << HDMI_FC_DATAUTO0_VSD_OFFSET),
> additionally mask and offset are redundant (mask == 1 << offset), but it
> seems you follow existing practice, so I do not oppose :)
>
> Regards
> Andrzej
>
>> +
>> + /* Set the length of HDMI vendor specific InfoFrame payload */
>> + hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
>> +
>> + /* Set 24bit IEEE Registration Identifier */
>> + hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
>> + hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
>> + hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
>> +
>> + /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
>> + hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
>> + hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
>> +
>> + if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
>> + hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
>> +
>> + /* Packet frame interpolation */
>> + hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
>> +
>> + /* Auto packets per frame and line spacing */
>> + hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
>> +
>> + /* Configures the Frame Composer On RDRB mode */
>> + hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
>> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
>> +}
>> +
>> static void hdmi_av_composer(struct dw_hdmi *hdmi,
>> const struct drm_display_mode *mode)
>> {
>> @@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>>
>> /* HDMI Initialization Step F - Configure AVI InfoFrame */
>> hdmi_config_AVI(hdmi, mode);
>> + hdmi_config_vendor_specific_infoframe(hdmi, mode);
>> } else {
>> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
>> }
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
>> index 325b0b8..c59f87e 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
>> @@ -854,6 +854,10 @@ enum {
>> HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
>> HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
>>
>> +/* FC_DATAUTO0 field values */
>> + HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
>> + HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
>> +
>> /* PHY_CONF0 field values */
>> HDMI_PHY_CONF0_PDZ_MASK = 0x80,
>> HDMI_PHY_CONF0_PDZ_OFFSET = 7,
>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config
@ 2017-03-21 7:51 ` Archit Taneja
0 siblings, 0 replies; 7+ messages in thread
From: Archit Taneja @ 2017-03-21 7:51 UTC (permalink / raw
To: Andrzej Hajda, Nickey Yang
Cc: laurent.pinchart+renesas, linux-kernel, dri-devel, dianders,
linux-rockchip, joabreu, andy.yan, rmk+kernel, vladimir_zapolskiy,
zhengyang
On 03/21/2017 01:17 PM, Andrzej Hajda wrote:
> On 20.03.2017 07:10, Nickey Yang wrote:
>> Vendor specific infoframe is mandatory for 4K2K resolution.
>> Without this, the HDMI protocol compliance fails.
>>
>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>> ---
>> drivers/gpu/drm/bridge/dw-hdmi.c | 50 ++++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/bridge/dw-hdmi.h | 4 ++++
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 9a9ec27..79e2e48 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1195,6 +1195,55 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>> hdmi_writeb(hdmi, (frame.right_bar >> 8) & 0xff, HDMI_FC_AVISRB1);
>> }
>>
>> +static void hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi,
>> + struct drm_display_mode *mode)
>> +{
>> + struct hdmi_vendor_infoframe frame;
>> + u8 buffer[10];
>> + ssize_t err;
>> +
>> + err = drm_hdmi_vendor_infoframe_from_display_mode(&frame, mode);
>> + if (err) {
>> + dev_err(hdmi->dev,
>> + "Failed to get vendor infoframe from mode: %zd\n", err);
>> + return;
>> + }
>> +
>> + err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
>> + if (!err) {
>> + dev_err(hdmi->dev, "Failed to pack vendor infoframe: %zd\n",
>> + err);
>> + return;
>> + }
>
> AFAIK function returns number of packed bytes or -EINVAL so the check is
> incorrect, and -EINVAL means that there is no need to generate vendor
> infoframe, so definitely not dev_err.
I queued this to drm-misc-next before you posted this. We can have a follow
up patch to fix this (since we don't rebase drm-misc).
Thanks,
Archit
>
>> + hdmi_modb(hdmi, 0 << HDMI_FC_DATAUTO0_VSD_OFFSET,
>> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
>
> I see no gain in constructs (0 << HDMI_FC_DATAUTO0_VSD_OFFSET),
> additionally mask and offset are redundant (mask == 1 << offset), but it
> seems you follow existing practice, so I do not oppose :)
>
> Regards
> Andrzej
>
>> +
>> + /* Set the length of HDMI vendor specific InfoFrame payload */
>> + hdmi_writeb(hdmi, buffer[2], HDMI_FC_VSDSIZE);
>> +
>> + /* Set 24bit IEEE Registration Identifier */
>> + hdmi_writeb(hdmi, buffer[4], HDMI_FC_VSDIEEEID0);
>> + hdmi_writeb(hdmi, buffer[5], HDMI_FC_VSDIEEEID1);
>> + hdmi_writeb(hdmi, buffer[6], HDMI_FC_VSDIEEEID2);
>> +
>> + /* Set HDMI_Video_Format and HDMI_VIC/3D_Structure */
>> + hdmi_writeb(hdmi, buffer[7], HDMI_FC_VSDPAYLOAD0);
>> + hdmi_writeb(hdmi, buffer[8], HDMI_FC_VSDPAYLOAD1);
>> +
>> + if (frame.s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
>> + hdmi_writeb(hdmi, buffer[9], HDMI_FC_VSDPAYLOAD2);
>> +
>> + /* Packet frame interpolation */
>> + hdmi_writeb(hdmi, 1, HDMI_FC_DATAUTO1);
>> +
>> + /* Auto packets per frame and line spacing */
>> + hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
>> +
>> + /* Configures the Frame Composer On RDRB mode */
>> + hdmi_modb(hdmi, 1 << HDMI_FC_DATAUTO0_VSD_OFFSET,
>> + HDMI_FC_DATAUTO0_VSD_MASK, HDMI_FC_DATAUTO0);
>> +}
>> +
>> static void hdmi_av_composer(struct dw_hdmi *hdmi,
>> const struct drm_display_mode *mode)
>> {
>> @@ -1446,6 +1495,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>>
>> /* HDMI Initialization Step F - Configure AVI InfoFrame */
>> hdmi_config_AVI(hdmi, mode);
>> + hdmi_config_vendor_specific_infoframe(hdmi, mode);
>> } else {
>> dev_dbg(hdmi->dev, "%s DVI mode\n", __func__);
>> }
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
>> index 325b0b8..c59f87e 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
>> @@ -854,6 +854,10 @@ enum {
>> HDMI_FC_DBGFORCE_FORCEAUDIO = 0x10,
>> HDMI_FC_DBGFORCE_FORCEVIDEO = 0x1,
>>
>> +/* FC_DATAUTO0 field values */
>> + HDMI_FC_DATAUTO0_VSD_MASK = 0x08,
>> + HDMI_FC_DATAUTO0_VSD_OFFSET = 3,
>> +
>> /* PHY_CONF0 field values */
>> HDMI_PHY_CONF0_PDZ_MASK = 0x80,
>> HDMI_PHY_CONF0_PDZ_OFFSET = 7,
>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-21 7:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20170321003517epcas5p1ac97c55740345a7845e39b2133d925df@epcas5p1.samsung.com>
2017-03-20 6:10 ` [PATCH] drm: bridge: dw-hdmi: add HDMI vendor specific infoframe config Nickey Yang
2017-03-20 11:08 ` Jose Abreu
2017-03-20 11:08 ` Jose Abreu
2017-03-21 7:47 ` Andrzej Hajda
2017-03-21 7:47 ` Andrzej Hajda
2017-03-21 7:51 ` Archit Taneja
2017-03-21 7:51 ` Archit Taneja
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.