Hi,
Here's a series introducing the CEC support for the BCM2711 found on the RaspberryPi4.
The BCM2711 HDMI controller uses a similar layout for the CEC registers, the main difference being that the interrupt handling part is now shared between both HDMI controllers.
This series is mainly about fixing a couple of bugs, reworking the driver to support having two different interrupts, one for each direction, provided by an external irqchip, and enables the irqchip driver for the controller we have.
This has been tested on an RPi3 and RPi4, but requires the latest firmware. It's is based on the 10 and 12 bpc series.
Here is the cec-compliance output:
$ cec-ctl --tuner -p 1.0.0.0 The CEC adapter doesn't allow setting the physical address manually, ignore this option.
Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : 1.0.0.0 Logical Address Mask : 0x0008 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Tuner Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 3 (Tuner 1) Primary Device Type : Tuner Logical Address Type : Tuner All Device Types : Tuner RC TV Profile : None Device Features : None
$ cec-compliance cec-compliance SHA : not available Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : 1.0.0.0 Logical Address Mask : 0x0008 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Tuner Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 3 (Tuner 1) Primary Device Type : Tuner Logical Address Type : Tuner All Device Types : Tuner RC TV Profile : None Device Features : None
Compliance test for vc4_hdmi device /dev/cec0:
The test results mean the following: OK Supported correctly by the device. OK (Not Supported) Not supported and not mandatory for the device. OK (Presumed) Presumably supported. Manually check to confirm. OK (Unexpected) Supported correctly but is not expected to be supported for this device. OK (Refused) Supported by the device, but was refused. FAIL Failed and was expected to be supported by this device.
Find remote devices: Polling: OK
Network topology: System Information for device 0 (TV) from device 3 (Tuner 1): CEC Version : 2.0 Physical Address : 0.0.0.0 Primary Device Type : TV Vendor ID : 0x000c03 (HDMI) OSD Name : 'test-124' Power Status : Tx, OK, Rx, OK, Feature Abort
Total for vc4_hdmi device /dev/cec0: 1, Succeeded: 1, Failed: 0, Warnings: 0
Let me know what you think, Maxime
Dom Cobley (5): drm/vc4: hdmi: Move hdmi reset to bind drm/vc4: hdmi: Fix register offset with longer CEC messages drm/vc4: hdmi: Fix up CEC registers drm/vc4: hdmi: Restore cec physical address on reconnect drm/vc4: hdmi: Remove cec_available flag
Maxime Ripard (10): irqchip: Allow to compile bcmstb on other platforms drm/vc4: hdmi: Compute the CEC clock divider from the clock rate drm/vc4: hdmi: Update the CEC clock divider on HSM rate change drm/vc4: hdmi: Introduce a CEC clock drm/vc4: hdmi: Split the interrupt handlers drm/vc4: hdmi: Support BCM2711 CEC interrupt setup drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts ARM: dts: bcm2711: Add the BSC interrupt controller ARM: dts: bcm2711: Add the CEC interrupt controller
.../bindings/display/brcm,bcm2711-hdmi.yaml | 20 +- arch/arm/boot/dts/bcm2711.dtsi | 30 +++ drivers/gpu/drm/vc4/vc4_hdmi.c | 224 +++++++++++++----- drivers/gpu/drm/vc4/vc4_hdmi.h | 11 +- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +- drivers/irqchip/Kconfig | 2 +- 6 files changed, 232 insertions(+), 59 deletions(-)
The BCM2711 uses a number of instances of the bcmstb-l2 controller in its display engine. Let's allow the driver to be enabled through KConfig.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/irqchip/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index c6098eee0c7c..f1e58de117dc 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -131,7 +131,7 @@ config BCM7120_L2_IRQ select IRQ_DOMAIN
config BRCMSTB_L2_IRQ - bool + bool "Broadcom STB L2 Interrupt Controller" select GENERIC_IRQ_CHIP select IRQ_DOMAIN
On 12/10/2020 5:46 AM, Maxime Ripard wrote:
The BCM2711 uses a number of instances of the bcmstb-l2 controller in its display engine. Let's allow the driver to be enabled through KConfig.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Acked-by: Florian Fainelli f.fainelli@gmail.com
Hi Maxime,
On 2020-12-10 13:46, Maxime Ripard wrote:
The BCM2711 uses a number of instances of the bcmstb-l2 controller in its display engine. Let's allow the driver to be enabled through KConfig.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/irqchip/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index c6098eee0c7c..f1e58de117dc 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -131,7 +131,7 @@ config BCM7120_L2_IRQ select IRQ_DOMAIN
config BRCMSTB_L2_IRQ
- bool
- bool "Broadcom STB L2 Interrupt Controller" select GENERIC_IRQ_CHIP select IRQ_DOMAIN
I'm always sceptical of making interrupt controllers user-selectable. Who is going to know that they need to pick that one?
I'd be much more in favour of directly selecting this symbol from DRM_VC4_HDMI_CEC, since there is an obvious dependency.
Thanks,
M.
Hi Marc,
On Thu, Dec 10, 2020 at 05:59:09PM +0000, Marc Zyngier wrote:
On 2020-12-10 13:46, Maxime Ripard wrote:
The BCM2711 uses a number of instances of the bcmstb-l2 controller in its display engine. Let's allow the driver to be enabled through KConfig.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/irqchip/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index c6098eee0c7c..f1e58de117dc 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -131,7 +131,7 @@ config BCM7120_L2_IRQ select IRQ_DOMAIN
config BRCMSTB_L2_IRQ
- bool
- bool "Broadcom STB L2 Interrupt Controller" select GENERIC_IRQ_CHIP select IRQ_DOMAIN
I'm always sceptical of making interrupt controllers user-selectable. Who is going to know that they need to pick that one?
I'd be much more in favour of directly selecting this symbol from DRM_VC4_HDMI_CEC, since there is an obvious dependency.
It's a bit weird to me that the HDMI CEC support selects it, since that interrupt controller is external and here no matter what. Would selecting it from the ARCH_* Kconfig option work for you?
Thanks! Maxime
Hi Maxime,
On 2020-12-14 15:27, Maxime Ripard wrote:
Hi Marc,
On Thu, Dec 10, 2020 at 05:59:09PM +0000, Marc Zyngier wrote:
[...]
I'm always sceptical of making interrupt controllers user-selectable. Who is going to know that they need to pick that one?
I'd be much more in favour of directly selecting this symbol from DRM_VC4_HDMI_CEC, since there is an obvious dependency.
It's a bit weird to me that the HDMI CEC support selects it, since that interrupt controller is external and here no matter what.
From glancing at the series, I was under the impression that these controllers were there for the sole benefit of the HDMI controllers. Is there anything else connected to them?
Would selecting it from the ARCH_* Kconfig option work for you?
Sure. My only ask is that the low level plumbing is selected without requiring any user guesswork.
Thanks,
M.
From: Dom Cobley popcornmix@gmail.com
The hdmi reset got moved to a later point in the commit 9045e91a476b ("drm/vc4: hdmi: Add reset callback").
However, the reset now occurs after vc4_hdmi_cec_init and so tramples the setup of registers like HDMI_CEC_CNTRL_1
This only affects pi0-3 as on pi4 the cec registers are in a separate block
Fixes: 9045e91a476b ("drm/vc4: hdmi: Add reset callback") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8006bddc8fbb..3df1747dd917 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -773,9 +773,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, return; }
- if (vc4_hdmi->variant->reset) - vc4_hdmi->variant->reset(vc4_hdmi); - if (vc4_hdmi->variant->phy_init) vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state);
@@ -1865,6 +1862,9 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi->disable_wifi_frequencies = of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
+ if (vc4_hdmi->variant->reset) + vc4_hdmi->variant->reset(vc4_hdmi); + pm_runtime_enable(dev);
drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
Hi Maxime & Dom
On Thu, 10 Dec 2020 at 13:46, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
The hdmi reset got moved to a later point in the commit 9045e91a476b ("drm/vc4: hdmi: Add reset callback").
However, the reset now occurs after vc4_hdmi_cec_init and so tramples the setup of registers like HDMI_CEC_CNTRL_1
This only affects pi0-3 as on pi4 the cec registers are in a separate block
It does mean that this reset only happens once on bind rather than on every pre_crtc_configure, but as this really is the big reset the entire block I don't see it needing to be triggered on every configure.
Fixes: 9045e91a476b ("drm/vc4: hdmi: Add reset callback") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8006bddc8fbb..3df1747dd917 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -773,9 +773,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, return; }
if (vc4_hdmi->variant->reset)vc4_hdmi->variant->reset(vc4_hdmi);if (vc4_hdmi->variant->phy_init) vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state);@@ -1865,6 +1862,9 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi->disable_wifi_frequencies = of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence");
if (vc4_hdmi->variant->reset)vc4_hdmi->variant->reset(vc4_hdmi);pm_runtime_enable(dev); drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);-- 2.28.0
From: Dom Cobley popcornmix@gmail.com
The code prior to 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") was relying on the fact that the register offset was incremented by 4 for each readl call. That worked since the register width is 4 bytes.
However, since that commit the HDMI_READ macro is now taking an enum, and the offset doesn't increment by 4 but 1 now. Divide the index by 4 to fix this.
Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 3df1747dd917..28b78ea885ea 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1434,13 +1434,20 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) { + struct drm_device *dev = vc4_hdmi->connector.dev; struct cec_msg *msg = &vc4_hdmi->cec_rx_msg; unsigned int i;
msg->len = 1 + ((cntrl1 & VC4_HDMI_CEC_REC_WRD_CNT_MASK) >> VC4_HDMI_CEC_REC_WRD_CNT_SHIFT); + + if (msg->len > 16) { + drm_err(dev, "Attempting to read too much data (%d)\n", msg->len); + return; + } + for (i = 0; i < msg->len; i += 4) { - u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + i); + u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + (i >> 2));
msg->msg[i] = val & 0xff; msg->msg[i + 1] = (val >> 8) & 0xff; @@ -1533,11 +1540,17 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, u32 signal_free_time, struct cec_msg *msg) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + struct drm_device *dev = vc4_hdmi->connector.dev; u32 val; unsigned int i;
+ if (msg->len > 16) { + drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len); + return -ENOMEM; + } + for (i = 0; i < msg->len; i += 4) - HDMI_WRITE(HDMI_CEC_TX_DATA_1 + i, + HDMI_WRITE(HDMI_CEC_TX_DATA_1 + (i >> 2), (msg->msg[i]) | (msg->msg[i + 1] << 8) | (msg->msg[i + 2] << 16) |
Hi Dom & Maxime
On Thu, 10 Dec 2020 at 13:46, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
The code prior to 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") was relying on the fact that the register offset was incremented by 4 for each readl call. That worked since the register width is 4 bytes.
However, since that commit the HDMI_READ macro is now taking an enum, and the offset doesn't increment by 4 but 1 now. Divide the index by 4 to fix this.
Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 3df1747dd917..28b78ea885ea 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1434,13 +1434,20 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv)
static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) {
struct drm_device *dev = vc4_hdmi->connector.dev; struct cec_msg *msg = &vc4_hdmi->cec_rx_msg; unsigned int i; msg->len = 1 + ((cntrl1 & VC4_HDMI_CEC_REC_WRD_CNT_MASK) >> VC4_HDMI_CEC_REC_WRD_CNT_SHIFT);if (msg->len > 16) {drm_err(dev, "Attempting to read too much data (%d)\n", msg->len);return;}for (i = 0; i < msg->len; i += 4) {
u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + i);
u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + (i >> 2)); msg->msg[i] = val & 0xff; msg->msg[i + 1] = (val >> 8) & 0xff;@@ -1533,11 +1540,17 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, u32 signal_free_time, struct cec_msg *msg) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
struct drm_device *dev = vc4_hdmi->connector.dev; u32 val; unsigned int i;if (msg->len > 16) {drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len);return -ENOMEM;}for (i = 0; i < msg->len; i += 4)
HDMI_WRITE(HDMI_CEC_TX_DATA_1 + i,
HDMI_WRITE(HDMI_CEC_TX_DATA_1 + (i >> 2), (msg->msg[i]) | (msg->msg[i + 1] << 8) | (msg->msg[i + 2] << 16) |-- 2.28.0
From: Dom Cobley popcornmix@gmail.com
The commit 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") forgot one CEC register, and made a copy and paste mistake for another one. Fix those mistakes.
Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h index 013fd57febd8..20a1438a72cb 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h @@ -29,6 +29,7 @@ enum vc4_hdmi_field { HDMI_CEC_CPU_MASK_SET, HDMI_CEC_CPU_MASK_STATUS, HDMI_CEC_CPU_STATUS, + HDMI_CEC_CPU_SET,
/* * Transmit data, first byte is low byte of the 32-bit reg. @@ -199,9 +200,10 @@ static const struct vc4_hdmi_register vc4_hdmi_fields[] = { VC4_HDMI_REG(HDMI_TX_PHY_RESET_CTL, 0x02c0), VC4_HDMI_REG(HDMI_TX_PHY_CTL_0, 0x02c4), VC4_HDMI_REG(HDMI_CEC_CPU_STATUS, 0x0340), + VC4_HDMI_REG(HDMI_CEC_CPU_SET, 0x0344), VC4_HDMI_REG(HDMI_CEC_CPU_CLEAR, 0x0348), VC4_HDMI_REG(HDMI_CEC_CPU_MASK_STATUS, 0x034c), - VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x034c), + VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x0350), VC4_HDMI_REG(HDMI_CEC_CPU_MASK_CLEAR, 0x0354), VC4_HDMI_REG(HDMI_RAM_PACKET_START, 0x0400), };
Hi Maxime & Dom
On Thu, 10 Dec 2020 at 13:46, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
The commit 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") forgot one CEC register, and made a copy and paste mistake for another one. Fix those mistakes.
Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h index 013fd57febd8..20a1438a72cb 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h @@ -29,6 +29,7 @@ enum vc4_hdmi_field { HDMI_CEC_CPU_MASK_SET, HDMI_CEC_CPU_MASK_STATUS, HDMI_CEC_CPU_STATUS,
HDMI_CEC_CPU_SET, /* * Transmit data, first byte is low byte of the 32-bit reg.@@ -199,9 +200,10 @@ static const struct vc4_hdmi_register vc4_hdmi_fields[] = { VC4_HDMI_REG(HDMI_TX_PHY_RESET_CTL, 0x02c0), VC4_HDMI_REG(HDMI_TX_PHY_CTL_0, 0x02c4), VC4_HDMI_REG(HDMI_CEC_CPU_STATUS, 0x0340),
VC4_HDMI_REG(HDMI_CEC_CPU_SET, 0x0344), VC4_HDMI_REG(HDMI_CEC_CPU_CLEAR, 0x0348), VC4_HDMI_REG(HDMI_CEC_CPU_MASK_STATUS, 0x034c),
VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x034c),
VC4_HDMI_REG(HDMI_CEC_CPU_MASK_SET, 0x0350), VC4_HDMI_REG(HDMI_CEC_CPU_MASK_CLEAR, 0x0354), VC4_HDMI_REG(HDMI_RAM_PACKET_START, 0x0400),};
2.28.0
From: Dom Cobley popcornmix@gmail.com
Currently we call cec_phys_addr_invalidate on a hotplug deassert. That may be due to a TV power cycling, or an AVR being switched on (and switching edid).
This makes CEC unusable since our controller wouldn't have a physical address anymore.
Set it back up again on the hotplug assert.
Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 28b78ea885ea..eff3bac562c6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -136,20 +136,29 @@ static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); + bool connected = false;
if (vc4_hdmi->hpd_gpio) { if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ vc4_hdmi->hpd_active_low) - return connector_status_connected; - cec_phys_addr_invalidate(vc4_hdmi->cec_adap); - return connector_status_disconnected; - } - - if (drm_probe_ddc(vc4_hdmi->ddc)) - return connector_status_connected; - + connected = true; + } else if (drm_probe_ddc(vc4_hdmi->ddc)) + connected = true; if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) + connected = true; + if (connected) { + if (connector->status != connector_status_connected) { + struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc); + + if (edid) { + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); + vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); + drm_connector_update_edid_property(connector, edid); + kfree(edid); + } + } return connector_status_connected; + } cec_phys_addr_invalidate(vc4_hdmi->cec_adap); return connector_status_disconnected; }
Hi Maxime & Dom
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
Currently we call cec_phys_addr_invalidate on a hotplug deassert. That may be due to a TV power cycling, or an AVR being switched on (and switching edid).
This makes CEC unusable since our controller wouldn't have a physical address anymore.
Set it back up again on the hotplug assert.
Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 28b78ea885ea..eff3bac562c6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -136,20 +136,29 @@ static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
bool connected = false; if (vc4_hdmi->hpd_gpio) { if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ vc4_hdmi->hpd_active_low)
return connector_status_connected;cec_phys_addr_invalidate(vc4_hdmi->cec_adap);return connector_status_disconnected;}if (drm_probe_ddc(vc4_hdmi->ddc))return connector_status_connected;
connected = true;} else if (drm_probe_ddc(vc4_hdmi->ddc))connected = true; if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)
This needs to become an "else if(...". It used to be that all the other paths would return, so were mutually exclusive to this. Now they set a thing and keep going we need to avoid reading the register should there be a HPD gpio or the ddc probe succeeds. Memory says that otherwise Pi3 always reports connected.
I fixed this in a downstream patch already - https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453...
Otherwise fine.
Dave
connected = true;if (connected) {if (connector->status != connector_status_connected) {struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);if (edid) {cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);drm_connector_update_edid_property(connector, edid);kfree(edid);}} return connector_status_connected;} cec_phys_addr_invalidate(vc4_hdmi->cec_adap); return connector_status_disconnected;}
2.28.0
On Fri, 18 Dec 2020 at 14:21, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi Maxime & Dom
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
Currently we call cec_phys_addr_invalidate on a hotplug deassert. That may be due to a TV power cycling, or an AVR being switched on (and switching edid).
This makes CEC unusable since our controller wouldn't have a physical address anymore.
Set it back up again on the hotplug assert.
Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 28b78ea885ea..eff3bac562c6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -136,20 +136,29 @@ static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
bool connected = false; if (vc4_hdmi->hpd_gpio) { if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ vc4_hdmi->hpd_active_low)
return connector_status_connected;cec_phys_addr_invalidate(vc4_hdmi->cec_adap);return connector_status_disconnected;}if (drm_probe_ddc(vc4_hdmi->ddc))return connector_status_connected;
connected = true;} else if (drm_probe_ddc(vc4_hdmi->ddc))connected = true; if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)This needs to become an "else if(...". It used to be that all the other paths would return, so were mutually exclusive to this. Now they set a thing and keep going we need to avoid reading the register should there be a HPD gpio or the ddc probe succeeds. Memory says that otherwise Pi3 always reports connected.
I fixed this in a downstream patch already - https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453...
Otherwise fine.
Dave
connected = true;if (connected) {if (connector->status != connector_status_connected) {struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);if (edid) {cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);drm_connector_update_edid_property(connector, edid);
Actually looking at this again in the context of the other changes, do we need to call drm_connector_update_edid_property() here?
We've just called drm_get_edid() to get the edid, and that calls drm_connector_update_edid_property() as well [1] Updating vc4_hdmi->encoder.hdmi_monitor may be necessary. It's otherwise done in vc4_hdmi_connector_get_modes, which I sort of expect to be called almost immediately by the framework when connector_detect returns "connected". I haven't checked if that is guaranteed though.
vc4_hdmi_connector_get_modes also includes a manual call to drm_connector_update_edid_property after having just called drm_get_edid, so that one feels redundant too.
Dave
[1] https://elixir.bootlin.com/linux/v5.10/source/drivers/gpu/drm/drm_edid.c#L20...
kfree(edid);}} return connector_status_connected;} cec_phys_addr_invalidate(vc4_hdmi->cec_adap); return connector_status_disconnected;}
2.28.0
Hi Dave,
Thanks for your review
On Fri, Dec 18, 2020 at 02:45:54PM +0000, Dave Stevenson wrote:
On Fri, 18 Dec 2020 at 14:21, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi Maxime & Dom
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
Currently we call cec_phys_addr_invalidate on a hotplug deassert. That may be due to a TV power cycling, or an AVR being switched on (and switching edid).
This makes CEC unusable since our controller wouldn't have a physical address anymore.
Set it back up again on the hotplug assert.
Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 28b78ea885ea..eff3bac562c6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -136,20 +136,29 @@ static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
bool connected = false; if (vc4_hdmi->hpd_gpio) { if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ vc4_hdmi->hpd_active_low)
return connector_status_connected;cec_phys_addr_invalidate(vc4_hdmi->cec_adap);return connector_status_disconnected;}if (drm_probe_ddc(vc4_hdmi->ddc))return connector_status_connected;
connected = true;} else if (drm_probe_ddc(vc4_hdmi->ddc))connected = true; if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED)This needs to become an "else if(...". It used to be that all the other paths would return, so were mutually exclusive to this. Now they set a thing and keep going we need to avoid reading the register should there be a HPD gpio or the ddc probe succeeds. Memory says that otherwise Pi3 always reports connected.
I fixed this in a downstream patch already - https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453...
Otherwise fine.
Dave
connected = true;if (connected) {if (connector->status != connector_status_connected) {struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);if (edid) {cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid);drm_connector_update_edid_property(connector, edid);Actually looking at this again in the context of the other changes, do we need to call drm_connector_update_edid_property() here?
We've just called drm_get_edid() to get the edid, and that calls drm_connector_update_edid_property() as well [1]
Yeah, you're right I'll drop it
Updating vc4_hdmi->encoder.hdmi_monitor may be necessary. It's otherwise done in vc4_hdmi_connector_get_modes, which I sort of expect to be called almost immediately by the framework when connector_detect returns "connected". I haven't checked if that is guaranteed though.
vc4_hdmi_connector_get_modes also includes a manual call to drm_connector_update_edid_property after having just called drm_get_edid, so that one feels redundant too.
.get_modes is called in drm_helper_probe_single_connector_modes, which is usually the helper set in .fill_modes. .fill_modes seems to only be called when either DRM_IOCTL_MODE_GETCONNECTOR is called, or when the connector status is forced through sysfs, so it doesn't look like it's done automatically.
I'm not sure we need to set hdmi_monitor though, it's only used to configure the display related side, and that can't happen without get_modes being called.
Maxime
The CEC clock divider needs to output a frequency of 40kHz from the HSM rate on the BCM2835. The driver used to have a fixed frequency for it, but that changed and we now need to compute it dynamically to maintain the proper rate.
Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index eff3bac562c6..0c53d7427d15 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1586,6 +1586,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev; + u16 clk_cnt; u32 value; int ret;
@@ -1611,8 +1612,9 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) * divider: the hsm_clock rate and this divider setting will * give a 40 kHz CEC clock. */ + clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; value |= VC4_HDMI_CEC_ADDR_MASK | - (4091 << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); + (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); HDMI_WRITE(HDMI_CEC_CNTRL_1, value); ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), vc4_cec_irq_handler,
Hi Maxime
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
The CEC clock divider needs to output a frequency of 40kHz from the HSM rate on the BCM2835. The driver used to have a fixed frequency for it, but that changed and we now need to compute it dynamically to maintain the proper rate.
Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate") Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
(To be a total pedant it's still a fixed frequency on vc4, but it's configurable via the variant entry).
drivers/gpu/drm/vc4/vc4_hdmi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index eff3bac562c6..0c53d7427d15 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1586,6 +1586,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev;
u16 clk_cnt; u32 value; int ret;@@ -1611,8 +1612,9 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) * divider: the hsm_clock rate and this divider setting will * give a 40 kHz CEC clock. */
clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; value |= VC4_HDMI_CEC_ADDR_MASK |
(4091 << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
(clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); HDMI_WRITE(HDMI_CEC_CNTRL_1, value); ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), vc4_cec_irq_handler,-- 2.28.0
As part of the enable sequence we might change the HSM clock rate if the pixel rate is different than the one we were already dealing with.
On the BCM2835 however, the CEC clock derives from the HSM clock so any rate change will need to be reflected in the CEC clock divider to output 40kHz.
Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate") Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 39 +++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 0c53d7427d15..b93ee3e26e2b 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -132,6 +132,27 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi) HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL); }
+#ifdef CONFIG_DRM_VC4_HDMI_CEC +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) +{ + u16 clk_cnt; + u32 value; + + value = HDMI_READ(HDMI_CEC_CNTRL_1); + value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK; + + /* + * Set the clock divider: the hsm_clock rate and this divider + * setting will give a 40 kHz CEC clock. + */ + clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; + value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; + HDMI_WRITE(HDMI_CEC_CNTRL_1, value); +} +#else +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {} +#endif + static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -761,6 +782,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, return; }
+ vc4_hdmi_cec_update_clk_div(vc4_hdmi); + /* * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup * at 300MHz. @@ -1586,7 +1609,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev; - u16 clk_cnt; u32 value; int ret;
@@ -1605,17 +1627,14 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); + value = HDMI_READ(HDMI_CEC_CNTRL_1); - value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK; - /* - * Set the logical address to Unregistered and set the clock - * divider: the hsm_clock rate and this divider setting will - * give a 40 kHz CEC clock. - */ - clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; - value |= VC4_HDMI_CEC_ADDR_MASK | - (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); + /* Set the logical address to Unregistered */ + value |= VC4_HDMI_CEC_ADDR_MASK; HDMI_WRITE(HDMI_CEC_CNTRL_1, value); + + vc4_hdmi_cec_update_clk_div(vc4_hdmi); + ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), vc4_cec_irq_handler, vc4_cec_irq_handler_thread, 0,
Hi Maxime
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
As part of the enable sequence we might change the HSM clock rate if the pixel rate is different than the one we were already dealing with.
On the BCM2835 however, the CEC clock derives from the HSM clock so any rate change will need to be reflected in the CEC clock divider to output 40kHz.
Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate") Signed-off-by: Maxime Ripard maxime@cerno.tech
I thought we'd got a duplicate patch here, but it's moving code that was changed in patch 6/15 so it can be called from vc4_hdmi_encoder_pre_crtc_configure too. Good for confusing me!
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_hdmi.c | 39 +++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 0c53d7427d15..b93ee3e26e2b 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -132,6 +132,27 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi) HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL); }
+#ifdef CONFIG_DRM_VC4_HDMI_CEC +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) +{
u16 clk_cnt;u32 value;value = HDMI_READ(HDMI_CEC_CNTRL_1);value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;/** Set the clock divider: the hsm_clock rate and this divider* setting will give a 40 kHz CEC clock.*/clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT;HDMI_WRITE(HDMI_CEC_CNTRL_1, value);+} +#else +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {} +#endif
static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -761,6 +782,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, return; }
vc4_hdmi_cec_update_clk_div(vc4_hdmi);/* * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup * at 300MHz.@@ -1586,7 +1609,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev;
u16 clk_cnt; u32 value; int ret;@@ -1605,17 +1627,14 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff);
value = HDMI_READ(HDMI_CEC_CNTRL_1);
value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK;/** Set the logical address to Unregistered and set the clock* divider: the hsm_clock rate and this divider setting will* give a 40 kHz CEC clock.*/clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;value |= VC4_HDMI_CEC_ADDR_MASK |(clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT);
/* Set the logical address to Unregistered */value |= VC4_HDMI_CEC_ADDR_MASK; HDMI_WRITE(HDMI_CEC_CNTRL_1, value);vc4_hdmi_cec_update_clk_div(vc4_hdmi);ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), vc4_cec_irq_handler, vc4_cec_irq_handler_thread, 0,-- 2.28.0
While the BCM2835 had the CEC clock derived from the HSM clock, the BCM2711 has a dedicated parent clock for it.
Let's introduce a separate clock for it so that we can handle both cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b93ee3e26e2b..0debd22bc992 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) * Set the clock divider: the hsm_clock rate and this divider * setting will give a 40 kHz CEC clock. */ - clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; + clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; HDMI_WRITE(HDMI_CEC_CNTRL_1, value); } @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->hsm_clock); } vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock; + vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock;
return 0; } @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->audio_clock); }
+ vc4_hdmi->cec_clock = devm_clk_get(dev, "cec"); + if (IS_ERR(vc4_hdmi->cec_clock)) { + DRM_ERROR("Failed to get CEC clock\n"); + return PTR_ERR(vc4_hdmi->cec_clock); + } + vc4_hdmi->reset = devm_reset_control_get(dev, NULL); if (IS_ERR(vc4_hdmi->reset)) { DRM_ERROR("Failed to get HDMI reset line\n"); diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 720914761261..adc4bf33ff15 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -155,6 +155,7 @@ struct vc4_hdmi { bool cec_tx_ok; bool cec_irq_was_rx;
+ struct clk *cec_clock; struct clk *pixel_clock; struct clk *hsm_clock; struct clk *audio_clock;
Hi Maxime
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
While the BCM2835 had the CEC clock derived from the HSM clock, the BCM2711 has a dedicated parent clock for it.
Let's introduce a separate clock for it so that we can handle both cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b93ee3e26e2b..0debd22bc992 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) * Set the clock divider: the hsm_clock rate and this divider * setting will give a 40 kHz CEC clock. */
clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; HDMI_WRITE(HDMI_CEC_CNTRL_1, value);} @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->hsm_clock); } vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock; return 0;} @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->audio_clock); }
vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");if (IS_ERR(vc4_hdmi->cec_clock)) {DRM_ERROR("Failed to get CEC clock\n");return PTR_ERR(vc4_hdmi->cec_clock);}
Aren't we adding to the DT binding here and breaking backwards compatibility? Admittedly CEC didn't work before (and was masked out) for vc5, but do we need to worry about those with existing DT files that currently work happily?
Otherwise I'm happy with the patch.
Dave
vc4_hdmi->reset = devm_reset_control_get(dev, NULL); if (IS_ERR(vc4_hdmi->reset)) { DRM_ERROR("Failed to get HDMI reset line\n");diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 720914761261..adc4bf33ff15 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -155,6 +155,7 @@ struct vc4_hdmi { bool cec_tx_ok; bool cec_irq_was_rx;
struct clk *cec_clock; struct clk *pixel_clock; struct clk *hsm_clock; struct clk *audio_clock;-- 2.28.0
Hi Dave,
On Fri, Dec 18, 2020 at 11:37:50AM +0000, Dave Stevenson wrote:
Hi Maxime
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
While the BCM2835 had the CEC clock derived from the HSM clock, the BCM2711 has a dedicated parent clock for it.
Let's introduce a separate clock for it so that we can handle both cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b93ee3e26e2b..0debd22bc992 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) * Set the clock divider: the hsm_clock rate and this divider * setting will give a 40 kHz CEC clock. */
clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; HDMI_WRITE(HDMI_CEC_CNTRL_1, value);} @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->hsm_clock); } vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock; return 0;} @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->audio_clock); }
vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");if (IS_ERR(vc4_hdmi->cec_clock)) {DRM_ERROR("Failed to get CEC clock\n");return PTR_ERR(vc4_hdmi->cec_clock);}Aren't we adding to the DT binding here and breaking backwards compatibility? Admittedly CEC didn't work before (and was masked out) for vc5, but do we need to worry about those with existing DT files that currently work happily?
The DT compatibility is not a worry here: I made sure the CEC clock and range were part of the binding since it's been introduced:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
So we were not using it so far, but it was in the DT all along
Maxime
On Fri, 18 Dec 2020 at 12:23, Maxime Ripard maxime@cerno.tech wrote:
Hi Dave,
On Fri, Dec 18, 2020 at 11:37:50AM +0000, Dave Stevenson wrote:
Hi Maxime
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
While the BCM2835 had the CEC clock derived from the HSM clock, the BCM2711 has a dedicated parent clock for it.
Let's introduce a separate clock for it so that we can handle both cases.
Signed-off-by: Maxime Ripard maxime@cerno.tech
drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index b93ee3e26e2b..0debd22bc992 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) * Set the clock divider: the hsm_clock rate and this divider * setting will give a 40 kHz CEC clock. */
clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ;
clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; HDMI_WRITE(HDMI_CEC_CNTRL_1, value);} @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->hsm_clock); } vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock; return 0;} @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) return PTR_ERR(vc4_hdmi->audio_clock); }
vc4_hdmi->cec_clock = devm_clk_get(dev, "cec");if (IS_ERR(vc4_hdmi->cec_clock)) {DRM_ERROR("Failed to get CEC clock\n");return PTR_ERR(vc4_hdmi->cec_clock);}Aren't we adding to the DT binding here and breaking backwards compatibility? Admittedly CEC didn't work before (and was masked out) for vc5, but do we need to worry about those with existing DT files that currently work happily?
The DT compatibility is not a worry here: I made sure the CEC clock and range were part of the binding since it's been introduced:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
So we were not using it so far, but it was in the DT all along
I guess I should have read it then :-) In which case Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
The BCM2711 has two different interrupt sources to transmit and receive CEC messages, provided through an external interrupt chip shared between the two HDMI interrupt controllers.
The rest of the CEC controller is identical though so we need to change a bit the code organisation to share the code as much as possible, yet still allowing to register independant handlers.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 86 +++++++++++++++++++++++++--------- 1 file changed, 65 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 0debd22bc992..80a81fcea315 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1442,15 +1442,22 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) }
#ifdef CONFIG_DRM_VC4_HDMI_CEC -static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv) +static irqreturn_t vc4_cec_irq_handler_rx_thread(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv;
- if (vc4_hdmi->cec_irq_was_rx) { - if (vc4_hdmi->cec_rx_msg.len) - cec_received_msg(vc4_hdmi->cec_adap, - &vc4_hdmi->cec_rx_msg); - } else if (vc4_hdmi->cec_tx_ok) { + if (vc4_hdmi->cec_rx_msg.len) + cec_received_msg(vc4_hdmi->cec_adap, + &vc4_hdmi->cec_rx_msg); + + return IRQ_HANDLED; +} + +static irqreturn_t vc4_cec_irq_handler_tx_thread(int irq, void *priv) +{ + struct vc4_hdmi *vc4_hdmi = priv; + + if (vc4_hdmi->cec_tx_ok) { cec_transmit_done(vc4_hdmi->cec_adap, CEC_TX_STATUS_OK, 0, 0, 0, 0); } else { @@ -1464,6 +1471,19 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv) return IRQ_HANDLED; }
+static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv) +{ + struct vc4_hdmi *vc4_hdmi = priv; + irqreturn_t ret; + + if (vc4_hdmi->cec_irq_was_rx) + ret = vc4_cec_irq_handler_rx_thread(irq, priv); + else + ret = vc4_cec_irq_handler_tx_thread(irq, priv); + + return ret; +} + static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) { struct drm_device *dev = vc4_hdmi->connector.dev; @@ -1488,31 +1508,55 @@ static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) } }
+static irqreturn_t vc4_cec_irq_handler_tx_bare(int irq, void *priv) +{ + struct vc4_hdmi *vc4_hdmi = priv; + u32 cntrl1; + + cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); + vc4_hdmi->cec_tx_ok = cntrl1 & VC4_HDMI_CEC_TX_STATUS_GOOD; + cntrl1 &= ~VC4_HDMI_CEC_START_XMIT_BEGIN; + HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1); + + return IRQ_WAKE_THREAD; +} + +static irqreturn_t vc4_cec_irq_handler_rx_bare(int irq, void *priv) +{ + struct vc4_hdmi *vc4_hdmi = priv; + u32 cntrl1; + + vc4_hdmi->cec_rx_msg.len = 0; + cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); + vc4_cec_read_msg(vc4_hdmi, cntrl1); + cntrl1 |= VC4_HDMI_CEC_CLEAR_RECEIVE_OFF; + HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1); + cntrl1 &= ~VC4_HDMI_CEC_CLEAR_RECEIVE_OFF; + + HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1); + + return IRQ_WAKE_THREAD; +} + static irqreturn_t vc4_cec_irq_handler(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv; u32 stat = HDMI_READ(HDMI_CEC_CPU_STATUS); - u32 cntrl1, cntrl5; + irqreturn_t ret; + u32 cntrl5;
if (!(stat & VC4_HDMI_CPU_CEC)) return IRQ_NONE; - vc4_hdmi->cec_rx_msg.len = 0; - cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); + cntrl5 = HDMI_READ(HDMI_CEC_CNTRL_5); vc4_hdmi->cec_irq_was_rx = cntrl5 & VC4_HDMI_CEC_RX_CEC_INT; - if (vc4_hdmi->cec_irq_was_rx) { - vc4_cec_read_msg(vc4_hdmi, cntrl1); - cntrl1 |= VC4_HDMI_CEC_CLEAR_RECEIVE_OFF; - HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1); - cntrl1 &= ~VC4_HDMI_CEC_CLEAR_RECEIVE_OFF; - } else { - vc4_hdmi->cec_tx_ok = cntrl1 & VC4_HDMI_CEC_TX_STATUS_GOOD; - cntrl1 &= ~VC4_HDMI_CEC_START_XMIT_BEGIN; - } - HDMI_WRITE(HDMI_CEC_CNTRL_1, cntrl1); + if (vc4_hdmi->cec_irq_was_rx) + ret = vc4_cec_irq_handler_rx_bare(irq, priv); + else + ret = vc4_cec_irq_handler_tx_bare(irq, priv); + HDMI_WRITE(HDMI_CEC_CPU_CLEAR, VC4_HDMI_CPU_CEC); - - return IRQ_WAKE_THREAD; + return ret; }
static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
The HDMI controller found in the BCM2711 has an external interrupt controller for the CEC and hotplug interrupt shared between the two instances.
Let's add a variant flag to register a single interrupt handler and deals with the interrupt handler setup, or two interrupt handlers relying on an external irqchip.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 42 ++++++++++++++++++++++++++-------- drivers/gpu/drm/vc4/vc4_hdmi.h | 7 ++++++ 2 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 80a81fcea315..d208b7d1d937 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1593,9 +1593,11 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) | ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
- HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC); + if (!vc4_hdmi->variant->external_irq_controller) + HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC); } else { - HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); + if (!vc4_hdmi->variant->external_irq_controller) + HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); HDMI_WRITE(HDMI_CEC_CNTRL_5, val | VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); } @@ -1670,8 +1672,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector); cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info);
- HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); - value = HDMI_READ(HDMI_CEC_CNTRL_1); /* Set the logical address to Unregistered */ value |= VC4_HDMI_CEC_ADDR_MASK; @@ -1679,12 +1679,32 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi)
vc4_hdmi_cec_update_clk_div(vc4_hdmi);
- ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), - vc4_cec_irq_handler, - vc4_cec_irq_handler_thread, 0, - "vc4 hdmi cec", vc4_hdmi); - if (ret) - goto err_delete_cec_adap; + if (vc4_hdmi->variant->external_irq_controller) { + ret = devm_request_threaded_irq(&pdev->dev, + platform_get_irq_byname(pdev, "cec-rx"), + vc4_cec_irq_handler_rx_bare, + vc4_cec_irq_handler_rx_thread, 0, + "vc4 hdmi cec rx", vc4_hdmi); + if (ret) + goto err_delete_cec_adap; + + ret = devm_request_threaded_irq(&pdev->dev, + platform_get_irq_byname(pdev, "cec-tx"), + vc4_cec_irq_handler_tx_bare, + vc4_cec_irq_handler_tx_thread, 0, + "vc4 hdmi cec tx", vc4_hdmi); + if (ret) + goto err_delete_cec_adap; + } else { + HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); + + ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), + vc4_cec_irq_handler, + vc4_cec_irq_handler_thread, 0, + "vc4 hdmi cec", vc4_hdmi); + if (ret) + goto err_delete_cec_adap; + }
ret = cec_register_adapter(vc4_hdmi->cec_adap, &pdev->dev); if (ret < 0) @@ -2083,6 +2103,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = { PHY_LANE_CK, }, .unsupported_odd_h_timings = true, + .external_irq_controller = true,
.init_resources = vc5_hdmi_init_resources, .csc_setup = vc5_hdmi_csc_setup, @@ -2109,6 +2130,7 @@ static const struct vc4_hdmi_variant bcm2711_hdmi1_variant = { PHY_LANE_2, }, .unsupported_odd_h_timings = true, + .external_irq_controller = true,
.init_resources = vc5_hdmi_init_resources, .csc_setup = vc5_hdmi_csc_setup, diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index adc4bf33ff15..27352827f70c 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -64,6 +64,13 @@ struct vc4_hdmi_variant { /* The BCM2711 cannot deal with odd horizontal pixel timings */ bool unsupported_odd_h_timings;
+ /* + * The BCM2711 CEC/hotplug IRQ controller is shared between the + * two HDMI controllers, and we have a proper irqchip driver for + * it. + */ + bool external_irq_controller; + /* Callback to get the resources (memory region, interrupts, * clocks, etc) for that variant. */
From: Dom Cobley popcornmix@gmail.com
Now that our HDMI controller supports CEC for the BCM2711, let's remove that flag.
Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ---- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 --- 2 files changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index d208b7d1d937..327638d93032 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1658,9 +1658,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) u32 value; int ret;
- if (!vc4_hdmi->variant->cec_available) - return 0; - vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops, vc4_hdmi, "vc4", CEC_CAP_DEFAULTS | @@ -2074,7 +2071,6 @@ static const struct vc4_hdmi_variant bcm2835_variant = { .debugfs_name = "hdmi_regs", .card_name = "vc4-hdmi", .max_pixel_clock = 162000000, - .cec_available = true, .registers = vc4_hdmi_fields, .num_registers = ARRAY_SIZE(vc4_hdmi_fields),
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 27352827f70c..c93ada62f429 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -42,9 +42,6 @@ struct vc4_hdmi_variant { /* Filename to expose the registers in debugfs */ const char *debugfs_name;
- /* Set to true when the CEC support is available */ - bool cec_available; - /* Maximum pixel clock supported by the controller (in Hz) */ unsigned long long max_pixel_clock;
Hi Dom & Maxime
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
From: Dom Cobley popcornmix@gmail.com
Now that our HDMI controller supports CEC for the BCM2711, let's remove that flag.
Signed-off-by: Dom Cobley popcornmix@gmail.com Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ---- drivers/gpu/drm/vc4/vc4_hdmi.h | 3 --- 2 files changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index d208b7d1d937..327638d93032 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1658,9 +1658,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) u32 value; int ret;
if (!vc4_hdmi->variant->cec_available)return 0;vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops, vc4_hdmi, "vc4", CEC_CAP_DEFAULTS |@@ -2074,7 +2071,6 @@ static const struct vc4_hdmi_variant bcm2835_variant = { .debugfs_name = "hdmi_regs", .card_name = "vc4-hdmi", .max_pixel_clock = 162000000,
.cec_available = true, .registers = vc4_hdmi_fields, .num_registers = ARRAY_SIZE(vc4_hdmi_fields),diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h index 27352827f70c..c93ada62f429 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.h +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h @@ -42,9 +42,6 @@ struct vc4_hdmi_variant { /* Filename to expose the registers in debugfs */ const char *debugfs_name;
/* Set to true when the CEC support is available */bool cec_available;/* Maximum pixel clock supported by the controller (in Hz) */ unsigned long long max_pixel_clock;-- 2.28.0
We introduced the BCM2711 support to the vc4 HDMI controller with 5.10, but this was lacking any of the interrupts of the CEC controller so we have to deal with the backward compatibility.
Do so by simply ignoring the CEC setup if the DT doesn't have the interrupts property.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 327638d93032..69217c68d3a4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1655,9 +1655,15 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev; + struct device *dev = &pdev->dev; u32 value; int ret;
+ if (!of_find_property(dev->of_node, "interrupts", NULL)) { + dev_warn(dev, "'interrupts' DT property is missing, no CEC\n"); + return 0; + } + vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops, vc4_hdmi, "vc4", CEC_CAP_DEFAULTS |
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard maxime@cerno.tech wrote:
We introduced the BCM2711 support to the vc4 HDMI controller with 5.10, but this was lacking any of the interrupts of the CEC controller so we have to deal with the backward compatibility.
Do so by simply ignoring the CEC setup if the DT doesn't have the interrupts property.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com
drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 327638d93032..69217c68d3a4 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1655,9 +1655,15 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev;
struct device *dev = &pdev->dev; u32 value; int ret;if (!of_find_property(dev->of_node, "interrupts", NULL)) {dev_warn(dev, "'interrupts' DT property is missing, no CEC\n");return 0;}vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops, vc4_hdmi, "vc4", CEC_CAP_DEFAULTS |-- 2.28.0
The CEC and hotplug interrupts were missing when that binding was introduced, let's add them in now that we've figured out how it works.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- .../bindings/display/brcm,bcm2711-hdmi.yaml | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml index 7ce06f9f9f8e..6e8ac910bdd8 100644 --- a/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml +++ b/Documentation/devicetree/bindings/display/brcm,bcm2711-hdmi.yaml @@ -53,6 +53,24 @@ properties: - const: audio - const: cec
+ interrupts: + items: + - description: CEC TX interrupt + - description: CEC RX interrupt + - description: CEC stuck at low interrupt + - description: Wake-up interrupt + - description: Hotplug connected interrupt + - description: Hotplug removed interrupt + + interrupt-names: + items: + - const: cec-tx + - const: cec-rx + - const: cec-low + - const: wakeup + - const: hpd-connected + - const: hpd-removed + ddc: allOf: - $ref: /schemas/types.yaml#/definitions/phandle @@ -90,7 +108,7 @@ required: - resets - ddc
-additionalProperties: false +unevaluatedProperties: false
examples: - |
The BSC controllers used for the HDMI DDC have an interrupt controller shared between both instances. Let's add it to avoid polling.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- arch/arm/boot/dts/bcm2711.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi index 4847dd305317..8bb46ae76a92 100644 --- a/arch/arm/boot/dts/bcm2711.dtsi +++ b/arch/arm/boot/dts/bcm2711.dtsi @@ -308,6 +308,14 @@ dvp: clock@7ef00000 { #reset-cells = <1>; };
+ bsc_intr: interrupt-controller@7ef00040 { + compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc"; + reg = <0x7ef00040 0x30>; + interrupts = <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <1>; + }; + hdmi0: hdmi@7ef00700 { compatible = "brcm,bcm2711-hdmi0"; reg = <0x7ef00700 0x300>, @@ -341,6 +349,8 @@ ddc0: i2c@7ef04500 { reg = <0x7ef04500 0x100>, <0x7ef00b00 0x300>; reg-names = "bsc", "auto-i2c"; clock-frequency = <97500>; + interrupt-parent = <&bsc_intr>; + interrupts = <0>; status = "disabled"; };
@@ -377,6 +387,8 @@ ddc1: i2c@7ef09500 { reg = <0x7ef09500 0x100>, <0x7ef05b00 0x300>; reg-names = "bsc", "auto-i2c"; clock-frequency = <97500>; + interrupt-parent = <&bsc_intr>; + interrupts = <1>; status = "disabled"; }; };
On 12/10/2020 5:46 AM, Maxime Ripard wrote:
The BSC controllers used for the HDMI DDC have an interrupt controller shared between both instances. Let's add it to avoid polling.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Florian Fainelli f.fainelli@gmail.com
The CEC and hotplug interrupts go through an interrupt controller shared between the two HDMI controllers.
Let's add that interrupt controller and the interrupts for both HDMI controllers
Signed-off-by: Maxime Ripard maxime@cerno.tech --- arch/arm/boot/dts/bcm2711.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi index 8bb46ae76a92..154cf6d35333 100644 --- a/arch/arm/boot/dts/bcm2711.dtsi +++ b/arch/arm/boot/dts/bcm2711.dtsi @@ -316,6 +316,14 @@ bsc_intr: interrupt-controller@7ef00040 { #interrupt-cells = <1>; };
+ aon_intr: interrupt-controller@7ef00100 { + compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc"; + reg = <0x7ef00100 0x30>; + interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <1>; + }; + hdmi0: hdmi@7ef00700 { compatible = "brcm,bcm2711-hdmi0"; reg = <0x7ef00700 0x300>, @@ -338,6 +346,11 @@ hdmi0: hdmi@7ef00700 { "hd"; clock-names = "hdmi", "bvb", "audio", "cec"; resets = <&dvp 0>; + interrupt-parent = <&aon_intr>; + interrupts = <0>, <1>, <2>, + <3>, <4>, <5>; + interrupt-names = "cec-tx", "cec-rx", "cec-low", + "wakeup", "hpd-connected", "hpd-removed"; ddc = <&ddc0>; dmas = <&dma 10>; dma-names = "audio-rx"; @@ -377,6 +390,11 @@ hdmi1: hdmi@7ef05700 { ddc = <&ddc1>; clock-names = "hdmi", "bvb", "audio", "cec"; resets = <&dvp 1>; + interrupt-parent = <&aon_intr>; + interrupts = <6>, <7>, <8>, + <9>, <10>, <11>; + interrupt-names = "cec-tx", "cec-rx", "cec-low", + "wakeup", "hpd-connected", "hpd-removed"; dmas = <&dma 17>; dma-names = "audio-rx"; status = "disabled";
On 12/10/2020 5:46 AM, Maxime Ripard wrote:
The CEC and hotplug interrupts go through an interrupt controller shared between the two HDMI controllers.
Let's add that interrupt controller and the interrupts for both HDMI controllers
Signed-off-by: Maxime Ripard maxime@cerno.tech
Reviewed-by: Florian Fainelli f.fainelli@gmail.com
Hi Maxime,
On 10/12/2020 14:46, Maxime Ripard wrote:
Hi,
Here's a series introducing the CEC support for the BCM2711 found on the RaspberryPi4.
The BCM2711 HDMI controller uses a similar layout for the CEC registers, the main difference being that the interrupt handling part is now shared between both HDMI controllers.
This series is mainly about fixing a couple of bugs, reworking the driver to support having two different interrupts, one for each direction, provided by an external irqchip, and enables the irqchip driver for the controller we have.
This has been tested on an RPi3 and RPi4, but requires the latest firmware. It's is based on the 10 and 12 bpc series.
This series looks good to me. Before I give my Acked-by for this series, can you confirm that it is possible to transmit the Image View On message on both outputs of the RPi4 when the HPD is low?
See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt on how to test this with a Pulse-Eight device.
This should work.
Regards,
Hans
Here is the cec-compliance output:
$ cec-ctl --tuner -p 1.0.0.0 The CEC adapter doesn't allow setting the physical address manually, ignore this option.
Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : 1.0.0.0 Logical Address Mask : 0x0008 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Tuner Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 3 (Tuner 1) Primary Device Type : Tuner Logical Address Type : Tuner All Device Types : Tuner RC TV Profile : None Device Features : None$ cec-compliance cec-compliance SHA : not available Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : 1.0.0.0 Logical Address Mask : 0x0008 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Tuner Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 3 (Tuner 1) Primary Device Type : Tuner Logical Address Type : Tuner All Device Types : Tuner RC TV Profile : None Device Features : NoneCompliance test for vc4_hdmi device /dev/cec0:
The test results mean the following: OK Supported correctly by the device. OK (Not Supported) Not supported and not mandatory for the device. OK (Presumed) Presumably supported. Manually check to confirm. OK (Unexpected) Supported correctly but is not expected to be supported for this device. OK (Refused) Supported by the device, but was refused. FAIL Failed and was expected to be supported by this device.Find remote devices: Polling: OK
Network topology: System Information for device 0 (TV) from device 3 (Tuner 1): CEC Version : 2.0 Physical Address : 0.0.0.0 Primary Device Type : TV Vendor ID : 0x000c03 (HDMI) OSD Name : 'test-124' Power Status : Tx, OK, Rx, OK, Feature Abort
Total for vc4_hdmi device /dev/cec0: 1, Succeeded: 1, Failed: 0, Warnings: 0
Let me know what you think, Maxime
Dom Cobley (5): drm/vc4: hdmi: Move hdmi reset to bind drm/vc4: hdmi: Fix register offset with longer CEC messages drm/vc4: hdmi: Fix up CEC registers drm/vc4: hdmi: Restore cec physical address on reconnect drm/vc4: hdmi: Remove cec_available flag
Maxime Ripard (10): irqchip: Allow to compile bcmstb on other platforms drm/vc4: hdmi: Compute the CEC clock divider from the clock rate drm/vc4: hdmi: Update the CEC clock divider on HSM rate change drm/vc4: hdmi: Introduce a CEC clock drm/vc4: hdmi: Split the interrupt handlers drm/vc4: hdmi: Support BCM2711 CEC interrupt setup drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts ARM: dts: bcm2711: Add the BSC interrupt controller ARM: dts: bcm2711: Add the CEC interrupt controller
.../bindings/display/brcm,bcm2711-hdmi.yaml | 20 +- arch/arm/boot/dts/bcm2711.dtsi | 30 +++ drivers/gpu/drm/vc4/vc4_hdmi.c | 224 +++++++++++++----- drivers/gpu/drm/vc4/vc4_hdmi.h | 11 +- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +- drivers/irqchip/Kconfig | 2 +- 6 files changed, 232 insertions(+), 59 deletions(-)
Hi Hans,
On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote:
Hi Maxime,
On 10/12/2020 14:46, Maxime Ripard wrote:
Hi,
Here's a series introducing the CEC support for the BCM2711 found on the RaspberryPi4.
The BCM2711 HDMI controller uses a similar layout for the CEC registers, the main difference being that the interrupt handling part is now shared between both HDMI controllers.
This series is mainly about fixing a couple of bugs, reworking the driver to support having two different interrupts, one for each direction, provided by an external irqchip, and enables the irqchip driver for the controller we have.
This has been tested on an RPi3 and RPi4, but requires the latest firmware. It's is based on the 10 and 12 bpc series.
This series looks good to me. Before I give my Acked-by for this series, can you confirm that it is possible to transmit the Image View On message on both outputs of the RPi4 when the HPD is low?
See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt on how to test this with a Pulse-Eight device.
This should work.
This is the output on the RPi4:
# cec-ctl --playback Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None
# cec-ctl -t0 --image-view-on Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None
Transmit from Unregistered to TV (15 to 0): CEC_MSG_IMAGE_VIEW_ON (0x04) Sequence: 1 Tx Timestamp: 77.631s
And this is the output on my desktop with the Pulse-Eight: $ sudo cec-ctl -p0.0.0.0 --tv Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : None
$ sudo cec-ctl -M Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : None
Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04)
So it looks like it's working as expected?
Maxime
On 17/12/2020 11:49, Maxime Ripard wrote:
Hi Hans,
On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote:
Hi Maxime,
On 10/12/2020 14:46, Maxime Ripard wrote:
Hi,
Here's a series introducing the CEC support for the BCM2711 found on the RaspberryPi4.
The BCM2711 HDMI controller uses a similar layout for the CEC registers, the main difference being that the interrupt handling part is now shared between both HDMI controllers.
This series is mainly about fixing a couple of bugs, reworking the driver to support having two different interrupts, one for each direction, provided by an external irqchip, and enables the irqchip driver for the controller we have.
This has been tested on an RPi3 and RPi4, but requires the latest firmware. It's is based on the 10 and 12 bpc series.
This series looks good to me. Before I give my Acked-by for this series, can you confirm that it is possible to transmit the Image View On message on both outputs of the RPi4 when the HPD is low?
See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt on how to test this with a Pulse-Eight device.
This should work.
This is the output on the RPi4:
# cec-ctl --playback Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None# cec-ctl -t0 --image-view-on Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : NoneTransmit from Unregistered to TV (15 to 0): CEC_MSG_IMAGE_VIEW_ON (0x04) Sequence: 1 Tx Timestamp: 77.631s
And this is the output on my desktop with the Pulse-Eight: $ sudo cec-ctl -p0.0.0.0 --tv Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : None$ sudo cec-ctl -M Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : NoneInitial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04)
So it looks like it's working as expected?
Yes, it looks good. Make sure you test this for both outputs of the RPi4. If it works for both, then you can add my
Acked-by: Hans Verkuil hverkuil-cisco@xs4all.nl
for this series.
Very nice work, thank you for doing this!
Regards,
Hans
Maxime
Hi Hans,
On Thu, Dec 17, 2020 at 11:53:42AM +0100, Hans Verkuil wrote:
On 17/12/2020 11:49, Maxime Ripard wrote:
Hi Hans,
On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote:
Hi Maxime,
On 10/12/2020 14:46, Maxime Ripard wrote:
Hi,
Here's a series introducing the CEC support for the BCM2711 found on the RaspberryPi4.
The BCM2711 HDMI controller uses a similar layout for the CEC registers, the main difference being that the interrupt handling part is now shared between both HDMI controllers.
This series is mainly about fixing a couple of bugs, reworking the driver to support having two different interrupts, one for each direction, provided by an external irqchip, and enables the irqchip driver for the controller we have.
This has been tested on an RPi3 and RPi4, but requires the latest firmware. It's is based on the 10 and 12 bpc series.
This series looks good to me. Before I give my Acked-by for this series, can you confirm that it is possible to transmit the Image View On message on both outputs of the RPi4 when the HPD is low?
See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt on how to test this with a Pulse-Eight device.
This should work.
This is the output on the RPi4:
# cec-ctl --playback Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None# cec-ctl -t0 --image-view-on Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : NoneTransmit from Unregistered to TV (15 to 0): CEC_MSG_IMAGE_VIEW_ON (0x04) Sequence: 1 Tx Timestamp: 77.631s
And this is the output on my desktop with the Pulse-Eight: $ sudo cec-ctl -p0.0.0.0 --tv Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : None$ sudo cec-ctl -M Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : NoneInitial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04)
So it looks like it's working as expected?
Yes, it looks good. Make sure you test this for both outputs of the RPi4.
It's a good thing you asked, I don't appear to get CEC interrupts from HDMI1. I'll fix it and send another version (probably not before the end of december though).
If it works for both, then you can add my
Acked-by: Hans Verkuil hverkuil-cisco@xs4all.nl
for this series.
Very nice work, thank you for doing this!
Thanks!
I'll hold your a-b until the next version though, fixing hdmi1 might change a few things.
Maxime
dri-devel@lists.freedesktop.org
-
Dave Stevenson -
Florian Fainelli -
Hans Verkuil -
Marc Zyngier -
Maxime Ripard