[PATCH 0/4] Support HDMI CEC on Qualcomm SoCs

[PATCH 0/4] Support HDMI CEC on Qualcomm SoCs

[Arnaud Vrac]

Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
and MSM8998. The hardware block can handle a single CEC logical address
and broadcast messages.

Port the CEC driver from downstream msm-4.4 kernel. It has been tested
on MSM8998 and passes the cec-compliance tool tests. The equivalent
downstream driver also passed CEC CTS tests using a Quantum Data QD882E
analyzer.

Some registers bitfield definitions were added to make the code clearer,
and those will also be proposed for upstream in the original xml file
from which the header was generated, in the mesa project.

Note HDMI support is not yet included upstream for MSM8998, I would
appreciate if someone can verify this driver at least works on MSM8996,
for which adding the pinctrl nodes for CEC should be sufficient.

Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
---
Arnaud Vrac (4):
      drm/msm: add some cec register bitfield details
      drm/msm: add hdmi cec support
      drm/msm: expose edid to hdmi cec adapter
      arm64: dts: qcom: msm8998: add hdmi cec pinctrl nodes

 arch/arm64/boot/dts/qcom/msm8998.dtsi  |  14 ++
 drivers/gpu/drm/msm/Kconfig            |   8 +
 drivers/gpu/drm/msm/Makefile           |   1 +
 drivers/gpu/drm/msm/hdmi/hdmi.c        |  15 ++
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  18 +++
 drivers/gpu/drm/msm/hdmi/hdmi.xml.h    |  62 +++++++-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |   2 +
 drivers/gpu/drm/msm/hdmi/hdmi_cec.c    | 280 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  17 +-
 9 files changed, 412 insertions(+), 5 deletions(-)
---
base-commit: e3342532ecd39bbd9c2ab5b9001cec1589bc37e9
change-id: 20230418-msm8998-hdmi-cec-08b5890bf41e

Best regards,
-- 
Arnaud Vrac <avrac@freebox.fr>

[PATCH 1/4] drm/msm: add some cec register bitfield details

[Arnaud Vrac]

The register names and bitfields were determined from the downstream
msm-4.4 driver.

Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
---
 drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
index 973b460486a5a..b4dd6e8cba6b7 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
@@ -76,6 +76,13 @@ enum hdmi_acr_cts {
 	ACR_48 = 3,
 };
 
+enum hdmi_cec_tx_status {
+	CEC_TX_OK = 0,
+	CEC_TX_NACK = 1,
+	CEC_TX_ARB_LOSS = 2,
+	CEC_TX_MAX_RETRIES = 3,
+};
+
 #define REG_HDMI_CTRL						0x00000000
 #define HDMI_CTRL_ENABLE					0x00000001
 #define HDMI_CTRL_HDMI						0x00000002
@@ -476,20 +483,73 @@ static inline uint32_t HDMI_DDC_REF_REFTIMER(uint32_t val)
 #define REG_HDMI_HDCP_SW_LOWER_AKSV				0x00000288
 
 #define REG_HDMI_CEC_CTRL					0x0000028c
+#define HDMI_CEC_CTRL_ENABLE					0x00000001
+#define HDMI_CEC_CTRL_SEND_TRIGGER				0x00000002
+#define HDMI_CEC_CTRL_FRAME_SIZE__MASK				0x000001f0
+#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT				4
+static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
+{
+	return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & HDMI_CEC_CTRL_FRAME_SIZE__MASK;
+}
+#define HDMI_CEC_CTRL_LINE_OE					0x00000200
 
 #define REG_HDMI_CEC_WR_DATA					0x00000290
+#define HDMI_CEC_WR_DATA_BROADCAST				0x00000001
+#define HDMI_CEC_WR_DATA_DATA__MASK				0x0000ff00
+#define HDMI_CEC_WR_DATA_DATA__SHIFT				8
+static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
+{
+	return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & HDMI_CEC_WR_DATA_DATA__MASK;
+}
 
-#define REG_HDMI_CEC_CEC_RETRANSMIT				0x00000294
+#define REG_HDMI_CEC_RETRANSMIT					0x00000294
+#define HDMI_CEC_RETRANSMIT_ENABLE				0x00000001
+#define HDMI_CEC_RETRANSMIT_COUNT__MASK				0x000000fe
+#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT			1
+static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
+{
+	return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & HDMI_CEC_RETRANSMIT_COUNT__MASK;
+}
 
 #define REG_HDMI_CEC_STATUS					0x00000298
+#define HDMI_CEC_STATUS_BUSY					0x00000001
+#define HDMI_CEC_STATUS_TX_FRAME_DONE				0x00000008
+#define HDMI_CEC_STATUS_TX_STATUS__MASK				0x000000f0
+#define HDMI_CEC_STATUS_TX_STATUS__SHIFT			4
+static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum hdmi_cec_tx_status val)
+{
+	return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & HDMI_CEC_STATUS_TX_STATUS__MASK;
+}
 
 #define REG_HDMI_CEC_INT					0x0000029c
+#define HDMI_CEC_INT_TX_DONE					0x00000001
+#define HDMI_CEC_INT_TX_DONE_MASK				0x00000002
+#define HDMI_CEC_INT_TX_ERROR					0x00000004
+#define HDMI_CEC_INT_TX_ERROR_MASK				0x00000008
+#define HDMI_CEC_INT_MONITOR					0x00000010
+#define HDMI_CEC_INT_MONITOR_MASK				0x00000020
+#define HDMI_CEC_INT_RX_DONE					0x00000040
+#define HDMI_CEC_INT_RX_DONE_MASK				0x00000080
 
 #define REG_HDMI_CEC_ADDR					0x000002a0
 
 #define REG_HDMI_CEC_TIME					0x000002a4
+#define HDMI_CEC_TIME_ENABLE					0x00000001
+#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK			0x0000ff80
+#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT			7
+static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
+{
+	return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
+}
 
 #define REG_HDMI_CEC_REFTIMER					0x000002a8
+#define HDMI_CEC_REFTIMER_ENABLE				0x00010000
+#define HDMI_CEC_REFTIMER_REFTIMER__MASK			0x0000ffff
+#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT			0
+static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
+{
+	return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & HDMI_CEC_REFTIMER_REFTIMER__MASK;
+}
 
 #define REG_HDMI_CEC_RD_DATA					0x000002ac
 

-- 
2.40.0

Re: [PATCH 1/4] drm/msm: add some cec register bitfield details

[Dmitry Baryshkov]

On 18/04/2023 21:10, Arnaud Vrac wrote:
> The register names and bitfields were determined from the downstream
> msm-4.4 driver.
> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 61 insertions(+), 1 deletion(-)

I have opened MR against Mesa at 
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.

The patch is:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Minor nit below

> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> index 973b460486a5a..b4dd6e8cba6b7 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>   	ACR_48 = 3,
>   };
>   
> +enum hdmi_cec_tx_status {
> +	CEC_TX_OK = 0,
> +	CEC_TX_NACK = 1,
> +	CEC_TX_ARB_LOSS = 2,
> +	CEC_TX_MAX_RETRIES = 3,
> +};
> +
>   #define REG_HDMI_CTRL						0x00000000
>   #define HDMI_CTRL_ENABLE					0x00000001
>   #define HDMI_CTRL_HDMI						0x00000002
> @@ -476,20 +483,73 @@ static inline uint32_t HDMI_DDC_REF_REFTIMER(uint32_t val)
>   #define REG_HDMI_HDCP_SW_LOWER_AKSV				0x00000288
>   
>   #define REG_HDMI_CEC_CTRL					0x0000028c
> +#define HDMI_CEC_CTRL_ENABLE					0x00000001
> +#define HDMI_CEC_CTRL_SEND_TRIGGER				0x00000002
> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK				0x000001f0
> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT				4
> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
> +{
> +	return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & HDMI_CEC_CTRL_FRAME_SIZE__MASK;
> +}
> +#define HDMI_CEC_CTRL_LINE_OE					0x00000200
>   
>   #define REG_HDMI_CEC_WR_DATA					0x00000290
> +#define HDMI_CEC_WR_DATA_BROADCAST				0x00000001
> +#define HDMI_CEC_WR_DATA_DATA__MASK				0x0000ff00
> +#define HDMI_CEC_WR_DATA_DATA__SHIFT				8
> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
> +{
> +	return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & HDMI_CEC_WR_DATA_DATA__MASK;
> +}
>   
> -#define REG_HDMI_CEC_CEC_RETRANSMIT				0x00000294
> +#define REG_HDMI_CEC_RETRANSMIT					0x00000294
> +#define HDMI_CEC_RETRANSMIT_ENABLE				0x00000001
> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK				0x000000fe
> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT			1
> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
> +{
> +	return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & HDMI_CEC_RETRANSMIT_COUNT__MASK;
> +}
>   
>   #define REG_HDMI_CEC_STATUS					0x00000298
> +#define HDMI_CEC_STATUS_BUSY					0x00000001
> +#define HDMI_CEC_STATUS_TX_FRAME_DONE				0x00000008
> +#define HDMI_CEC_STATUS_TX_STATUS__MASK				0x000000f0
> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT			4
> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum hdmi_cec_tx_status val)
> +{
> +	return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & HDMI_CEC_STATUS_TX_STATUS__MASK;
> +}
>   
>   #define REG_HDMI_CEC_INT					0x0000029c
> +#define HDMI_CEC_INT_TX_DONE					0x00000001
> +#define HDMI_CEC_INT_TX_DONE_MASK				0x00000002
> +#define HDMI_CEC_INT_TX_ERROR					0x00000004
> +#define HDMI_CEC_INT_TX_ERROR_MASK				0x00000008
> +#define HDMI_CEC_INT_MONITOR					0x00000010
> +#define HDMI_CEC_INT_MONITOR_MASK				0x00000020
> +#define HDMI_CEC_INT_RX_DONE					0x00000040
> +#define HDMI_CEC_INT_RX_DONE_MASK				0x00000080
>   
>   #define REG_HDMI_CEC_ADDR					0x000002a0
>   
>   #define REG_HDMI_CEC_TIME					0x000002a4
> +#define HDMI_CEC_TIME_ENABLE					0x00000001
> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK			0x0000ff80
> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT			7
> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
> +{
> +	return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
> +}
>   
>   #define REG_HDMI_CEC_REFTIMER					0x000002a8
> +#define HDMI_CEC_REFTIMER_ENABLE				0x00010000

I think this should come after the REFTIMER field.

> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK			0x0000ffff
> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT			0
> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
> +{
> +	return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & HDMI_CEC_REFTIMER_REFTIMER__MASK;
> +}
>   
>   #define REG_HDMI_CEC_RD_DATA					0x000002ac
>   
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm/msm: add some cec register bitfield details

[Abhinav Kumar]

On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
> On 18/04/2023 21:10, Arnaud Vrac wrote:
>> The register names and bitfields were determined from the downstream
>> msm-4.4 driver.
>>
>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>> ---
>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 
>> ++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 61 insertions(+), 1 deletion(-)
> 
> I have opened MR against Mesa at 
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
> 
> The patch is:
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Minor nit below
> 

Also, shouldnt the register updates be done using rnn tool instead of 
manual edits?

>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h 
>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>> index 973b460486a5a..b4dd6e8cba6b7 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>>       ACR_48 = 3,
>>   };
>> +enum hdmi_cec_tx_status {
>> +    CEC_TX_OK = 0,
>> +    CEC_TX_NACK = 1,
>> +    CEC_TX_ARB_LOSS = 2,
>> +    CEC_TX_MAX_RETRIES = 3,
>> +};
>> +
>>   #define REG_HDMI_CTRL                        0x00000000
>>   #define HDMI_CTRL_ENABLE                    0x00000001
>>   #define HDMI_CTRL_HDMI                        0x00000002
>> @@ -476,20 +483,73 @@ static inline uint32_t 
>> HDMI_DDC_REF_REFTIMER(uint32_t val)
>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
>> +{
>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & 
>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
>> +}
>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
>> +{
>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & 
>> HDMI_CEC_WR_DATA_DATA__MASK;
>> +}
>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
>> +{
>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & 
>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
>> +}
>>   #define REG_HDMI_CEC_STATUS                    0x00000298
>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum 
>> hdmi_cec_tx_status val)
>> +{
>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & 
>> HDMI_CEC_STATUS_TX_STATUS__MASK;
>> +}
>>   #define REG_HDMI_CEC_INT                    0x0000029c
>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
>>   #define REG_HDMI_CEC_TIME                    0x000002a4
>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
>> +{
>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & 
>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
>> +}
>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
> 
> I think this should come after the REFTIMER field.
> 
>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
>> +{
>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & 
>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
>> +}
>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
>>
> 

Re: [PATCH 1/4] drm/msm: add some cec register bitfield details

[Dmitry Baryshkov]

On 20/04/2023 03:10, Abhinav Kumar wrote:
> 
> 
> On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
>> On 18/04/2023 21:10, Arnaud Vrac wrote:
>>> The register names and bitfields were determined from the downstream
>>> msm-4.4 driver.
>>>
>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>>> ---
>>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 
>>> ++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 61 insertions(+), 1 deletion(-)
>>
>> I have opened MR against Mesa at 
>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
>>
>> The patch is:
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Minor nit below
>>
> 
> Also, shouldnt the register updates be done using rnn tool instead of 
> manual edits?

We usually update the rnn and ask Rob to pull it at the beginning of the 
cycle.

> 
>>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>> index 973b460486a5a..b4dd6e8cba6b7 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>>>       ACR_48 = 3,
>>>   };
>>> +enum hdmi_cec_tx_status {
>>> +    CEC_TX_OK = 0,
>>> +    CEC_TX_NACK = 1,
>>> +    CEC_TX_ARB_LOSS = 2,
>>> +    CEC_TX_MAX_RETRIES = 3,
>>> +};
>>> +
>>>   #define REG_HDMI_CTRL                        0x00000000
>>>   #define HDMI_CTRL_ENABLE                    0x00000001
>>>   #define HDMI_CTRL_HDMI                        0x00000002
>>> @@ -476,20 +483,73 @@ static inline uint32_t 
>>> HDMI_DDC_REF_REFTIMER(uint32_t val)
>>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
>>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
>>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
>>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
>>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
>>> +{
>>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & 
>>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
>>> +}
>>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
>>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
>>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
>>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
>>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
>>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
>>> +{
>>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & 
>>> HDMI_CEC_WR_DATA_DATA__MASK;
>>> +}
>>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
>>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
>>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
>>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
>>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
>>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
>>> +{
>>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & 
>>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
>>> +}
>>>   #define REG_HDMI_CEC_STATUS                    0x00000298
>>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
>>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
>>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
>>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
>>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum 
>>> hdmi_cec_tx_status val)
>>> +{
>>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & 
>>> HDMI_CEC_STATUS_TX_STATUS__MASK;
>>> +}
>>>   #define REG_HDMI_CEC_INT                    0x0000029c
>>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
>>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
>>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
>>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
>>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
>>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
>>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
>>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
>>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
>>>   #define REG_HDMI_CEC_TIME                    0x000002a4
>>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
>>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
>>> +{
>>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & 
>>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
>>> +}
>>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
>>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
>>
>> I think this should come after the REFTIMER field.
>>
>>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
>>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
>>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
>>> +{
>>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & 
>>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
>>> +}
>>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm/msm: add some cec register bitfield details

[Abhinav Kumar]

On 4/19/2023 5:11 PM, Dmitry Baryshkov wrote:
> On 20/04/2023 03:10, Abhinav Kumar wrote:
>>
>>
>> On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
>>> On 18/04/2023 21:10, Arnaud Vrac wrote:
>>>> The register names and bitfields were determined from the downstream
>>>> msm-4.4 driver.
>>>>
>>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>>>> ---
>>>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 
>>>> ++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 61 insertions(+), 1 deletion(-)
>>>
>>> I have opened MR against Mesa at 
>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
>>>
>>> The patch is:
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> Minor nit below
>>>
>>
>> Also, shouldnt the register updates be done using rnn tool instead of 
>> manual edits?
> 
> We usually update the rnn and ask Rob to pull it at the beginning of the 
> cycle.
> 

Sorry, I didnt get this. So you are saying, we will accept manual edits 
and then replace it with the tool generated xml later? I was not aware 
of that, because previously I was always asked by Rob to use the tool to 
generate the xml and push that.

>>
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h 
>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>> index 973b460486a5a..b4dd6e8cba6b7 100644
>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>>>>       ACR_48 = 3,
>>>>   };
>>>> +enum hdmi_cec_tx_status {
>>>> +    CEC_TX_OK = 0,
>>>> +    CEC_TX_NACK = 1,
>>>> +    CEC_TX_ARB_LOSS = 2,
>>>> +    CEC_TX_MAX_RETRIES = 3,
>>>> +};
>>>> +
>>>>   #define REG_HDMI_CTRL                        0x00000000
>>>>   #define HDMI_CTRL_ENABLE                    0x00000001
>>>>   #define HDMI_CTRL_HDMI                        0x00000002
>>>> @@ -476,20 +483,73 @@ static inline uint32_t 
>>>> HDMI_DDC_REF_REFTIMER(uint32_t val)
>>>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
>>>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
>>>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
>>>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
>>>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
>>>> +{
>>>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & 
>>>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
>>>> +}
>>>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
>>>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
>>>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
>>>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
>>>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
>>>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
>>>> +{
>>>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & 
>>>> HDMI_CEC_WR_DATA_DATA__MASK;
>>>> +}
>>>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
>>>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
>>>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
>>>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
>>>> +{
>>>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & 
>>>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
>>>> +}
>>>>   #define REG_HDMI_CEC_STATUS                    0x00000298
>>>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
>>>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
>>>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
>>>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
>>>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum 
>>>> hdmi_cec_tx_status val)
>>>> +{
>>>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & 
>>>> HDMI_CEC_STATUS_TX_STATUS__MASK;
>>>> +}
>>>>   #define REG_HDMI_CEC_INT                    0x0000029c
>>>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
>>>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
>>>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
>>>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
>>>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
>>>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
>>>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
>>>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
>>>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
>>>>   #define REG_HDMI_CEC_TIME                    0x000002a4
>>>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
>>>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
>>>> +{
>>>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & 
>>>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
>>>> +}
>>>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
>>>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
>>>
>>> I think this should come after the REFTIMER field.
>>>
>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
>>>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
>>>> +{
>>>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & 
>>>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
>>>> +}
>>>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
>>>>
>>>
> 

Re: [PATCH 1/4] drm/msm: add some cec register bitfield details

[Dmitry Baryshkov]

On 20/04/2023 03:17, Abhinav Kumar wrote:
> 
> 
> On 4/19/2023 5:11 PM, Dmitry Baryshkov wrote:
>> On 20/04/2023 03:10, Abhinav Kumar wrote:
>>>
>>>
>>> On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
>>>> On 18/04/2023 21:10, Arnaud Vrac wrote:
>>>>> The register names and bitfields were determined from the downstream
>>>>> msm-4.4 driver.
>>>>>
>>>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 
>>>>> ++++++++++++++++++++++++++++++++++++-
>>>>>   1 file changed, 61 insertions(+), 1 deletion(-)
>>>>
>>>> I have opened MR against Mesa at 
>>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
>>>>
>>>> The patch is:
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>
>>>> Minor nit below
>>>>
>>>
>>> Also, shouldnt the register updates be done using rnn tool instead of 
>>> manual edits?
>>
>> We usually update the rnn and ask Rob to pull it at the beginning of 
>> the cycle.
>>
> 
> Sorry, I didnt get this. So you are saying, we will accept manual edits 
> and then replace it with the tool generated xml later? I was not aware 
> of that, because previously I was always asked by Rob to use the tool to 
> generate the xml and push that.

We accept manual edits for the patchset (so that one can test it), but 
before merging the patchset we ask Rob to pull the xml.

> 
>>>
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h 
>>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>> index 973b460486a5a..b4dd6e8cba6b7 100644
>>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>>>>>       ACR_48 = 3,
>>>>>   };
>>>>> +enum hdmi_cec_tx_status {
>>>>> +    CEC_TX_OK = 0,
>>>>> +    CEC_TX_NACK = 1,
>>>>> +    CEC_TX_ARB_LOSS = 2,
>>>>> +    CEC_TX_MAX_RETRIES = 3,
>>>>> +};
>>>>> +
>>>>>   #define REG_HDMI_CTRL                        0x00000000
>>>>>   #define HDMI_CTRL_ENABLE                    0x00000001
>>>>>   #define HDMI_CTRL_HDMI                        0x00000002
>>>>> @@ -476,20 +483,73 @@ static inline uint32_t 
>>>>> HDMI_DDC_REF_REFTIMER(uint32_t val)
>>>>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
>>>>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
>>>>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
>>>>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
>>>>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
>>>>> +{
>>>>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & 
>>>>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
>>>>> +}
>>>>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
>>>>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
>>>>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
>>>>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
>>>>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
>>>>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
>>>>> +{
>>>>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & 
>>>>> HDMI_CEC_WR_DATA_DATA__MASK;
>>>>> +}
>>>>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
>>>>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
>>>>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
>>>>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
>>>>> +{
>>>>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & 
>>>>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
>>>>> +}
>>>>>   #define REG_HDMI_CEC_STATUS                    0x00000298
>>>>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
>>>>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
>>>>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum 
>>>>> hdmi_cec_tx_status val)
>>>>> +{
>>>>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & 
>>>>> HDMI_CEC_STATUS_TX_STATUS__MASK;
>>>>> +}
>>>>>   #define REG_HDMI_CEC_INT                    0x0000029c
>>>>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
>>>>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
>>>>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
>>>>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
>>>>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
>>>>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
>>>>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
>>>>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
>>>>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
>>>>>   #define REG_HDMI_CEC_TIME                    0x000002a4
>>>>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
>>>>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
>>>>> +{
>>>>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & 
>>>>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
>>>>> +}
>>>>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
>>>>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
>>>>
>>>> I think this should come after the REFTIMER field.
>>>>
>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
>>>>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
>>>>> +{
>>>>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & 
>>>>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
>>>>> +}
>>>>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
>>>>>
>>>>
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 1/4] drm/msm: add some cec register bitfield details

[Abhinav Kumar]

On 4/19/2023 5:21 PM, Dmitry Baryshkov wrote:
> On 20/04/2023 03:17, Abhinav Kumar wrote:
>>
>>
>> On 4/19/2023 5:11 PM, Dmitry Baryshkov wrote:
>>> On 20/04/2023 03:10, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
>>>>> On 18/04/2023 21:10, Arnaud Vrac wrote:
>>>>>> The register names and bitfields were determined from the downstream
>>>>>> msm-4.4 driver.
>>>>>>
>>>>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 
>>>>>> ++++++++++++++++++++++++++++++++++++-
>>>>>>   1 file changed, 61 insertions(+), 1 deletion(-)
>>>>>
>>>>> I have opened MR against Mesa at 
>>>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
>>>>>
>>>>> The patch is:
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>
>>>>> Minor nit below
>>>>>
>>>>
>>>> Also, shouldnt the register updates be done using rnn tool instead 
>>>> of manual edits?
>>>
>>> We usually update the rnn and ask Rob to pull it at the beginning of 
>>> the cycle.
>>>
>>
>> Sorry, I didnt get this. So you are saying, we will accept manual 
>> edits and then replace it with the tool generated xml later? I was not 
>> aware of that, because previously I was always asked by Rob to use the 
>> tool to generate the xml and push that.
> 
> We accept manual edits for the patchset (so that one can test it), but 
> before merging the patchset we ask Rob to pull the xml.
> 

Interesting, and Rob generates the xml that time or who does that?

The MR you have created updates the freedreno/registers which is just to 
keep the XML in the driver and mesa in sync.

But I am trying to understand who generates the updated xml to merge it 
with the patchset if its not the developer who does that anymore.

>>
>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h 
>>>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>>> index 973b460486a5a..b4dd6e8cba6b7 100644
>>>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>>>>>>       ACR_48 = 3,
>>>>>>   };
>>>>>> +enum hdmi_cec_tx_status {
>>>>>> +    CEC_TX_OK = 0,
>>>>>> +    CEC_TX_NACK = 1,
>>>>>> +    CEC_TX_ARB_LOSS = 2,
>>>>>> +    CEC_TX_MAX_RETRIES = 3,
>>>>>> +};
>>>>>> +
>>>>>>   #define REG_HDMI_CTRL                        0x00000000
>>>>>>   #define HDMI_CTRL_ENABLE                    0x00000001
>>>>>>   #define HDMI_CTRL_HDMI                        0x00000002
>>>>>> @@ -476,20 +483,73 @@ static inline uint32_t 
>>>>>> HDMI_DDC_REF_REFTIMER(uint32_t val)
>>>>>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
>>>>>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
>>>>>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
>>>>>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
>>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
>>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
>>>>>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
>>>>>> +{
>>>>>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & 
>>>>>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
>>>>>> +}
>>>>>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
>>>>>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
>>>>>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
>>>>>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
>>>>>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
>>>>>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
>>>>>> +{
>>>>>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & 
>>>>>> HDMI_CEC_WR_DATA_DATA__MASK;
>>>>>> +}
>>>>>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
>>>>>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
>>>>>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
>>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
>>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
>>>>>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
>>>>>> +{
>>>>>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & 
>>>>>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
>>>>>> +}
>>>>>>   #define REG_HDMI_CEC_STATUS                    0x00000298
>>>>>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
>>>>>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
>>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
>>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
>>>>>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum 
>>>>>> hdmi_cec_tx_status val)
>>>>>> +{
>>>>>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & 
>>>>>> HDMI_CEC_STATUS_TX_STATUS__MASK;
>>>>>> +}
>>>>>>   #define REG_HDMI_CEC_INT                    0x0000029c
>>>>>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
>>>>>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
>>>>>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
>>>>>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
>>>>>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
>>>>>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
>>>>>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
>>>>>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
>>>>>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
>>>>>>   #define REG_HDMI_CEC_TIME                    0x000002a4
>>>>>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
>>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
>>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
>>>>>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
>>>>>> +{
>>>>>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & 
>>>>>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
>>>>>> +}
>>>>>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
>>>>>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
>>>>>
>>>>> I think this should come after the REFTIMER field.
>>>>>
>>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
>>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
>>>>>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
>>>>>> +{
>>>>>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & 
>>>>>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
>>>>>> +}
>>>>>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
>>>>>>
>>>>>
>>>
> 

Re: [Freedreno] [PATCH 1/4] drm/msm: add some cec register bitfield details

[Dmitry Baryshkov]

On 20/04/2023 03:27, Abhinav Kumar wrote:
> 
> 
> On 4/19/2023 5:21 PM, Dmitry Baryshkov wrote:
>> On 20/04/2023 03:17, Abhinav Kumar wrote:
>>>
>>>
>>> On 4/19/2023 5:11 PM, Dmitry Baryshkov wrote:
>>>> On 20/04/2023 03:10, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
>>>>>> On 18/04/2023 21:10, Arnaud Vrac wrote:
>>>>>>> The register names and bitfields were determined from the downstream
>>>>>>> msm-4.4 driver.
>>>>>>>
>>>>>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62 
>>>>>>> ++++++++++++++++++++++++++++++++++++-
>>>>>>>   1 file changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> I have opened MR against Mesa at 
>>>>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
>>>>>>
>>>>>> The patch is:
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>
>>>>>> Minor nit below
>>>>>>
>>>>>
>>>>> Also, shouldnt the register updates be done using rnn tool instead 
>>>>> of manual edits?
>>>>
>>>> We usually update the rnn and ask Rob to pull it at the beginning of 
>>>> the cycle.
>>>>
>>>
>>> Sorry, I didnt get this. So you are saying, we will accept manual 
>>> edits and then replace it with the tool generated xml later? I was 
>>> not aware of that, because previously I was always asked by Rob to 
>>> use the tool to generate the xml and push that.
>>
>> We accept manual edits for the patchset (so that one can test it), but 
>> before merging the patchset we ask Rob to pull the xml.
>>
> 
> Interesting, and Rob generates the xml that time or who does that?
> 
> The MR you have created updates the freedreno/registers which is just to 
> keep the XML in the driver and mesa in sync.
> 
> But I am trying to understand who generates the updated xml to merge it 
> with the patchset if its not the developer who does that anymore.

In this case I went on and created the MR as Arnaud didn't create one. 
Yes, usually we do this on our own when updating the register file (in 
other words: I usually edit the xml, then regen the xml.h, then add it 
to the patchset).

> 
>>>
>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h 
>>>>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>>>> index 973b460486a5a..b4dd6e8cba6b7 100644
>>>>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
>>>>>>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
>>>>>>>       ACR_48 = 3,
>>>>>>>   };
>>>>>>> +enum hdmi_cec_tx_status {
>>>>>>> +    CEC_TX_OK = 0,
>>>>>>> +    CEC_TX_NACK = 1,
>>>>>>> +    CEC_TX_ARB_LOSS = 2,
>>>>>>> +    CEC_TX_MAX_RETRIES = 3,
>>>>>>> +};
>>>>>>> +
>>>>>>>   #define REG_HDMI_CTRL                        0x00000000
>>>>>>>   #define HDMI_CTRL_ENABLE                    0x00000001
>>>>>>>   #define HDMI_CTRL_HDMI                        0x00000002
>>>>>>> @@ -476,20 +483,73 @@ static inline uint32_t 
>>>>>>> HDMI_DDC_REF_REFTIMER(uint32_t val)
>>>>>>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
>>>>>>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
>>>>>>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
>>>>>>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
>>>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
>>>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
>>>>>>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
>>>>>>> +{
>>>>>>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) & 
>>>>>>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
>>>>>>> +}
>>>>>>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
>>>>>>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
>>>>>>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
>>>>>>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
>>>>>>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
>>>>>>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
>>>>>>> +{
>>>>>>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) & 
>>>>>>> HDMI_CEC_WR_DATA_DATA__MASK;
>>>>>>> +}
>>>>>>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
>>>>>>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
>>>>>>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
>>>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
>>>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
>>>>>>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
>>>>>>> +{
>>>>>>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) & 
>>>>>>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
>>>>>>> +}
>>>>>>>   #define REG_HDMI_CEC_STATUS                    0x00000298
>>>>>>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
>>>>>>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
>>>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
>>>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
>>>>>>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum 
>>>>>>> hdmi_cec_tx_status val)
>>>>>>> +{
>>>>>>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) & 
>>>>>>> HDMI_CEC_STATUS_TX_STATUS__MASK;
>>>>>>> +}
>>>>>>>   #define REG_HDMI_CEC_INT                    0x0000029c
>>>>>>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
>>>>>>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
>>>>>>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
>>>>>>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
>>>>>>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
>>>>>>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
>>>>>>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
>>>>>>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
>>>>>>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
>>>>>>>   #define REG_HDMI_CEC_TIME                    0x000002a4
>>>>>>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
>>>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
>>>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
>>>>>>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
>>>>>>> +{
>>>>>>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) & 
>>>>>>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
>>>>>>> +}
>>>>>>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
>>>>>>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
>>>>>>
>>>>>> I think this should come after the REFTIMER field.
>>>>>>
>>>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
>>>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
>>>>>>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
>>>>>>> +{
>>>>>>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) & 
>>>>>>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
>>>>>>> +}
>>>>>>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
>>>>>>>
>>>>>>
>>>>
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 1/4] drm/msm: add some cec register bitfield details

[Arnaud Vrac]

Le jeu. 20 avr. 2023 à 02:30, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> a écrit :
>
> On 20/04/2023 03:27, Abhinav Kumar wrote:
> >
> >
> > On 4/19/2023 5:21 PM, Dmitry Baryshkov wrote:
> >> On 20/04/2023 03:17, Abhinav Kumar wrote:
> >>>
> >>>
> >>> On 4/19/2023 5:11 PM, Dmitry Baryshkov wrote:
> >>>> On 20/04/2023 03:10, Abhinav Kumar wrote:
> >>>>>
> >>>>>
> >>>>> On 4/19/2023 4:53 PM, Dmitry Baryshkov wrote:
> >>>>>> On 18/04/2023 21:10, Arnaud Vrac wrote:
> >>>>>>> The register names and bitfields were determined from the downstream
> >>>>>>> msm-4.4 driver.
> >>>>>>>
> >>>>>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> >>>>>>> ---
> >>>>>>>   drivers/gpu/drm/msm/hdmi/hdmi.xml.h | 62
> >>>>>>> ++++++++++++++++++++++++++++++++++++-
> >>>>>>>   1 file changed, 61 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> I have opened MR against Mesa at
> >>>>>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22588.
> >>>>>>
> >>>>>> The patch is:
> >>>>>>
> >>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>>>
> >>>>>> Minor nit below
> >>>>>>
> >>>>>
> >>>>> Also, shouldnt the register updates be done using rnn tool instead
> >>>>> of manual edits?
> >>>>
> >>>> We usually update the rnn and ask Rob to pull it at the beginning of
> >>>> the cycle.
> >>>>
> >>>
> >>> Sorry, I didnt get this. So you are saying, we will accept manual
> >>> edits and then replace it with the tool generated xml later? I was
> >>> not aware of that, because previously I was always asked by Rob to
> >>> use the tool to generate the xml and push that.
> >>
> >> We accept manual edits for the patchset (so that one can test it), but
> >> before merging the patchset we ask Rob to pull the xml.
> >>
> >
> > Interesting, and Rob generates the xml that time or who does that?
> >
> > The MR you have created updates the freedreno/registers which is just to
> > keep the XML in the driver and mesa in sync.
> >
> > But I am trying to understand who generates the updated xml to merge it
> > with the patchset if its not the developer who does that anymore.
>
> In this case I went on and created the MR as Arnaud didn't create one.
> Yes, usually we do this on our own when updating the register file (in
> other words: I usually edit the xml, then regen the xml.h, then add it
> to the patchset).

Ok thanks, I wasn't sure in which order to do this, thanks for posting
the MR on mesa. The changes in hdmi.xml.h I posted are not manually
edited, they were generated using the gen_header.py script in mesa (I
omitted the top comments changes about envytools which are not present
anymore though).

>
> >
> >>>
> >>>>>
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> >>>>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> >>>>>>> index 973b460486a5a..b4dd6e8cba6b7 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> >>>>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.xml.h
> >>>>>>> @@ -76,6 +76,13 @@ enum hdmi_acr_cts {
> >>>>>>>       ACR_48 = 3,
> >>>>>>>   };
> >>>>>>> +enum hdmi_cec_tx_status {
> >>>>>>> +    CEC_TX_OK = 0,
> >>>>>>> +    CEC_TX_NACK = 1,
> >>>>>>> +    CEC_TX_ARB_LOSS = 2,
> >>>>>>> +    CEC_TX_MAX_RETRIES = 3,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>>   #define REG_HDMI_CTRL                        0x00000000
> >>>>>>>   #define HDMI_CTRL_ENABLE                    0x00000001
> >>>>>>>   #define HDMI_CTRL_HDMI                        0x00000002
> >>>>>>> @@ -476,20 +483,73 @@ static inline uint32_t
> >>>>>>> HDMI_DDC_REF_REFTIMER(uint32_t val)
> >>>>>>>   #define REG_HDMI_HDCP_SW_LOWER_AKSV                0x00000288
> >>>>>>>   #define REG_HDMI_CEC_CTRL                    0x0000028c
> >>>>>>> +#define HDMI_CEC_CTRL_ENABLE                    0x00000001
> >>>>>>> +#define HDMI_CEC_CTRL_SEND_TRIGGER                0x00000002
> >>>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__MASK                0x000001f0
> >>>>>>> +#define HDMI_CEC_CTRL_FRAME_SIZE__SHIFT                4
> >>>>>>> +static inline uint32_t HDMI_CEC_CTRL_FRAME_SIZE(uint32_t val)
> >>>>>>> +{
> >>>>>>> +    return ((val) << HDMI_CEC_CTRL_FRAME_SIZE__SHIFT) &
> >>>>>>> HDMI_CEC_CTRL_FRAME_SIZE__MASK;
> >>>>>>> +}
> >>>>>>> +#define HDMI_CEC_CTRL_LINE_OE                    0x00000200
> >>>>>>>   #define REG_HDMI_CEC_WR_DATA                    0x00000290
> >>>>>>> +#define HDMI_CEC_WR_DATA_BROADCAST                0x00000001
> >>>>>>> +#define HDMI_CEC_WR_DATA_DATA__MASK                0x0000ff00
> >>>>>>> +#define HDMI_CEC_WR_DATA_DATA__SHIFT                8
> >>>>>>> +static inline uint32_t HDMI_CEC_WR_DATA_DATA(uint32_t val)
> >>>>>>> +{
> >>>>>>> +    return ((val) << HDMI_CEC_WR_DATA_DATA__SHIFT) &
> >>>>>>> HDMI_CEC_WR_DATA_DATA__MASK;
> >>>>>>> +}
> >>>>>>> -#define REG_HDMI_CEC_CEC_RETRANSMIT                0x00000294
> >>>>>>> +#define REG_HDMI_CEC_RETRANSMIT                    0x00000294
> >>>>>>> +#define HDMI_CEC_RETRANSMIT_ENABLE                0x00000001
> >>>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__MASK                0x000000fe
> >>>>>>> +#define HDMI_CEC_RETRANSMIT_COUNT__SHIFT            1
> >>>>>>> +static inline uint32_t HDMI_CEC_RETRANSMIT_COUNT(uint32_t val)
> >>>>>>> +{
> >>>>>>> +    return ((val) << HDMI_CEC_RETRANSMIT_COUNT__SHIFT) &
> >>>>>>> HDMI_CEC_RETRANSMIT_COUNT__MASK;
> >>>>>>> +}
> >>>>>>>   #define REG_HDMI_CEC_STATUS                    0x00000298
> >>>>>>> +#define HDMI_CEC_STATUS_BUSY                    0x00000001
> >>>>>>> +#define HDMI_CEC_STATUS_TX_FRAME_DONE                0x00000008
> >>>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__MASK                0x000000f0
> >>>>>>> +#define HDMI_CEC_STATUS_TX_STATUS__SHIFT            4
> >>>>>>> +static inline uint32_t HDMI_CEC_STATUS_TX_STATUS(enum
> >>>>>>> hdmi_cec_tx_status val)
> >>>>>>> +{
> >>>>>>> +    return ((val) << HDMI_CEC_STATUS_TX_STATUS__SHIFT) &
> >>>>>>> HDMI_CEC_STATUS_TX_STATUS__MASK;
> >>>>>>> +}
> >>>>>>>   #define REG_HDMI_CEC_INT                    0x0000029c
> >>>>>>> +#define HDMI_CEC_INT_TX_DONE                    0x00000001
> >>>>>>> +#define HDMI_CEC_INT_TX_DONE_MASK                0x00000002
> >>>>>>> +#define HDMI_CEC_INT_TX_ERROR                    0x00000004
> >>>>>>> +#define HDMI_CEC_INT_TX_ERROR_MASK                0x00000008
> >>>>>>> +#define HDMI_CEC_INT_MONITOR                    0x00000010
> >>>>>>> +#define HDMI_CEC_INT_MONITOR_MASK                0x00000020
> >>>>>>> +#define HDMI_CEC_INT_RX_DONE                    0x00000040
> >>>>>>> +#define HDMI_CEC_INT_RX_DONE_MASK                0x00000080
> >>>>>>>   #define REG_HDMI_CEC_ADDR                    0x000002a0
> >>>>>>>   #define REG_HDMI_CEC_TIME                    0x000002a4
> >>>>>>> +#define HDMI_CEC_TIME_ENABLE                    0x00000001
> >>>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK            0x0000ff80
> >>>>>>> +#define HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT            7
> >>>>>>> +static inline uint32_t HDMI_CEC_TIME_SIGNAL_FREE_TIME(uint32_t val)
> >>>>>>> +{
> >>>>>>> +    return ((val) << HDMI_CEC_TIME_SIGNAL_FREE_TIME__SHIFT) &
> >>>>>>> HDMI_CEC_TIME_SIGNAL_FREE_TIME__MASK;
> >>>>>>> +}
> >>>>>>>   #define REG_HDMI_CEC_REFTIMER                    0x000002a8
> >>>>>>> +#define HDMI_CEC_REFTIMER_ENABLE                0x00010000
> >>>>>>
> >>>>>> I think this should come after the REFTIMER field.
> >>>>>>
> >>>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__MASK            0x0000ffff
> >>>>>>> +#define HDMI_CEC_REFTIMER_REFTIMER__SHIFT            0
> >>>>>>> +static inline uint32_t HDMI_CEC_REFTIMER_REFTIMER(uint32_t val)
> >>>>>>> +{
> >>>>>>> +    return ((val) << HDMI_CEC_REFTIMER_REFTIMER__SHIFT) &
> >>>>>>> HDMI_CEC_REFTIMER_REFTIMER__MASK;
> >>>>>>> +}
> >>>>>>>   #define REG_HDMI_CEC_RD_DATA                    0x000002ac
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
> --
> With best wishes
> Dmitry
>

[PATCH 2/4] drm/msm: add hdmi cec support

[Arnaud Vrac]

Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
and MSM8998. The hardware block can handle a single CEC logical address
and broadcast messages.

Port the CEC driver from downstream msm-4.4 kernel. It has been tested
on MSM8998 and passes the cec-compliance tool tests.

Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
---
 drivers/gpu/drm/msm/Kconfig         |   8 ++
 drivers/gpu/drm/msm/Makefile        |   1 +
 drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
 drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
 drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
 5 files changed, 322 insertions(+)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 85f5ab1d552c4..2a02c74207935 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
 	default y
 	help
 	  Choose this option to enable HDCP state machine
+
+config DRM_MSM_HDMI_CEC
+	bool "Enable HDMI CEC support in MSM DRM driver"
+	depends on DRM_MSM && DRM_MSM_HDMI
+	select CEC_CORE
+	default y
+	help
+	  Choose this option to enable CEC support
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 7274c41228ed9..0237a2f219ac2 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
 
 msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
 
+msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
 msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
 
 msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 3132105a2a433..1dde3890e25c0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -11,6 +11,8 @@
 #include <drm/drm_bridge_connector.h>
 #include <drm/drm_of.h>
 
+#include <media/cec.h>
+
 #include <sound/hdmi-codec.h>
 #include "hdmi.h"
 
@@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
 	if (hdmi->hdcp_ctrl)
 		msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
 
+	/* Process CEC: */
+	msm_hdmi_cec_irq(hdmi);
+
 	/* TODO audio.. */
 
 	return IRQ_HANDLED;
@@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
 	 */
 	if (hdmi->workq)
 		destroy_workqueue(hdmi->workq);
+
+	msm_hdmi_cec_exit(hdmi);
 	msm_hdmi_hdcp_destroy(hdmi);
 
 	if (hdmi->i2c)
@@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
 		hdmi->hdcp_ctrl = NULL;
 	}
 
+	msm_hdmi_cec_init(hdmi);
+
 	return 0;
 
 fail:
@@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 
 	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
 
+	if (hdmi->cec_adap) {
+		struct cec_connector_info conn_info;
+		cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
+		cec_s_conn_info(hdmi->cec_adap, &conn_info);
+	}
+
 	ret = devm_request_irq(dev->dev, hdmi->irq,
 			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
 			"hdmi_isr", hdmi);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index e8dbee50637fa..c639bd87f4b8f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -29,6 +29,7 @@ struct hdmi_audio {
 };
 
 struct hdmi_hdcp_ctrl;
+struct cec_adapter;
 
 struct hdmi {
 	struct drm_device *dev;
@@ -73,6 +74,7 @@ struct hdmi {
 	struct workqueue_struct *workq;
 
 	struct hdmi_hdcp_ctrl *hdcp_ctrl;
+	struct cec_adapter *cec_adap;
 
 	/*
 	* spinlock to protect registers shared by different execution
@@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
 static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
 #endif
 
+/*
+ * cec
+ */
+#ifdef CONFIG_DRM_MSM_HDMI_CEC
+int msm_hdmi_cec_init(struct hdmi *hdmi);
+void msm_hdmi_cec_exit(struct hdmi *hdmi);
+void msm_hdmi_cec_irq(struct hdmi *hdmi);
+#else
+static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
+{
+	return -ENXIO;
+}
+static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
+static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
+#endif
+
 #endif /* __HDMI_CONNECTOR_H__ */
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
new file mode 100644
index 0000000000000..51326e493e5da
--- /dev/null
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
@@ -0,0 +1,280 @@
+#include <linux/iopoll.h>
+#include <media/cec.h>
+
+#include "hdmi.h"
+
+#define HDMI_CEC_INT_MASK ( \
+	HDMI_CEC_INT_TX_DONE_MASK | \
+	HDMI_CEC_INT_TX_ERROR_MASK | \
+	HDMI_CEC_INT_RX_DONE_MASK)
+
+struct hdmi_cec_ctrl {
+	struct hdmi *hdmi;
+	struct work_struct work;
+	spinlock_t lock;
+	u32 irq_status;
+	u32 tx_status;
+	u32 tx_retransmits;
+};
+
+static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
+	struct hdmi *hdmi = cec_ctrl->hdmi;
+
+	if (enable) {
+		/* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
+		hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
+			   HDMI_CEC_REFTIMER_REFTIMER(960) |
+			   HDMI_CEC_REFTIMER_ENABLE);
+
+		/* read and write timings */
+		hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);
+		hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
+		hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
+		hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
+
+		/* start bit low pulse duration, 3.7ms */
+		hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
+
+		/* signal free time, 7 * 2.4ms */
+		hdmi_write(hdmi, REG_HDMI_CEC_TIME,
+			   HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
+			   HDMI_CEC_TIME_ENABLE);
+
+		hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
+		hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
+		hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
+
+		hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
+		hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
+	} else {
+		hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
+		hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
+		cancel_work_sync(&cec_ctrl->work);
+	}
+
+	return 0;
+}
+
+static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
+{
+	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
+	struct hdmi *hdmi = cec_ctrl->hdmi;
+
+	hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);
+
+	return 0;
+}
+
+static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+				      u32 signal_free_time, struct cec_msg *msg)
+{
+	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
+	struct hdmi *hdmi = cec_ctrl->hdmi;
+	u8 retransmits;
+	u32 broadcast;
+	u32 status;
+	int i;
+
+	/* toggle cec in order to flush out bad hw state, if any */
+	hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
+	hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
+
+	/* flush register writes */
+	wmb();
+
+	retransmits = attempts ? (attempts - 1) : 0;
+	hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
+		   HDMI_CEC_RETRANSMIT_ENABLE |
+		   HDMI_CEC_RETRANSMIT_COUNT(retransmits));
+
+	broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
+	for (i = 0; i < msg->len; i++) {
+		hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
+			   HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
+	}
+
+	/* check line status */
+	if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
+			      5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
+		pr_err("CEC line is busy. Retry failed\n");
+		return -EBUSY;
+	}
+
+	cec_ctrl->tx_retransmits = retransmits;
+
+	/* start transmission */
+	hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
+		   HDMI_CEC_CTRL_ENABLE |
+		   HDMI_CEC_CTRL_SEND_TRIGGER |
+		   HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
+		   HDMI_CEC_CTRL_LINE_OE);
+
+	return 0;
+}
+
+static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
+{
+	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
+
+	cec_ctrl->hdmi->cec_adap = NULL;
+	kfree(cec_ctrl);
+}
+
+static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
+	.adap_enable = msm_hdmi_cec_adap_enable,
+	.adap_log_addr = msm_hdmi_cec_adap_log_addr,
+	.adap_transmit = msm_hdmi_cec_adap_transmit,
+	.adap_free = msm_hdmi_cec_adap_free,
+};
+
+#define CEC_IRQ_FRAME_WR_DONE 0x01
+#define CEC_IRQ_FRAME_RD_DONE 0x02
+
+static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
+{
+	struct hdmi *hdmi = cec_ctrl->hdmi;
+	struct cec_msg msg = {};
+	u32 data;
+	int i;
+
+	data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
+	msg.len = (data & 0x1f00) >> 8;
+	if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
+		return;
+
+	msg.msg[0] = data & 0xff;
+	for (i = 1; i < msg.len; i++)
+		msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
+
+	cec_received_msg(hdmi->cec_adap, &msg);
+}
+
+static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
+{
+	struct hdmi *hdmi = cec_ctrl->hdmi;
+	u32 tx_status;
+
+	tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
+		HDMI_CEC_STATUS_TX_STATUS__SHIFT;
+
+	switch (tx_status) {
+	case 0:
+		cec_transmit_done(hdmi->cec_adap,
+				  CEC_TX_STATUS_OK, 0, 0, 0, 0);
+		break;
+	case 1:
+		cec_transmit_done(hdmi->cec_adap,
+				  CEC_TX_STATUS_NACK, 0, 1, 0, 0);
+		break;
+	case 2:
+		cec_transmit_done(hdmi->cec_adap,
+				  CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);
+		break;
+	case 3:
+		cec_transmit_done(hdmi->cec_adap,
+				  CEC_TX_STATUS_MAX_RETRIES |
+				  CEC_TX_STATUS_NACK,
+				  0, cec_ctrl->tx_retransmits + 1, 0, 0);
+		break;
+	default:
+		cec_transmit_done(hdmi->cec_adap,
+				  CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
+		break;
+	}
+}
+
+static void msm_hdmi_cec_work(struct work_struct *work)
+{
+	struct hdmi_cec_ctrl *cec_ctrl =
+		container_of(work, struct hdmi_cec_ctrl, work);
+	unsigned long flags;
+
+	spin_lock_irqsave(&cec_ctrl->lock, flags);
+
+	if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
+		msm_hdmi_cec_handle_tx_done(cec_ctrl);
+
+	if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
+		msm_hdmi_cec_handle_rx_done(cec_ctrl);
+
+	cec_ctrl->irq_status = 0;
+	cec_ctrl->tx_status = 0;
+
+	spin_unlock_irqrestore(&cec_ctrl->lock, flags);
+}
+
+void msm_hdmi_cec_irq(struct hdmi *hdmi)
+{
+	struct hdmi_cec_ctrl *cec_ctrl;
+	unsigned long flags;
+	u32 int_status;
+
+	if (!hdmi->cec_adap)
+		return;
+
+	cec_ctrl = hdmi->cec_adap->priv;
+
+	int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
+	if (!(int_status & HDMI_CEC_INT_MASK))
+		return;
+
+	spin_lock_irqsave(&cec_ctrl->lock, flags);
+
+	if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
+		cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
+		cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
+	}
+
+	if (int_status & HDMI_CEC_INT_RX_DONE)
+		cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
+
+	spin_unlock_irqrestore(&cec_ctrl->lock, flags);
+
+	hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
+	queue_work(hdmi->workq, &cec_ctrl->work);
+}
+
+int msm_hdmi_cec_init(struct hdmi *hdmi)
+{
+	struct platform_device *pdev = hdmi->pdev;
+	struct hdmi_cec_ctrl *cec_ctrl;
+	struct cec_adapter *cec_adap;
+	int ret;
+
+	cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
+	if (!cec_ctrl)
+		return -ENOMEM;
+
+	cec_ctrl->hdmi = hdmi;
+	INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
+
+	cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
+					cec_ctrl, "msm",
+					CEC_CAP_DEFAULTS |
+					CEC_CAP_CONNECTOR_INFO, 1);
+	ret = PTR_ERR_OR_ZERO(cec_adap);
+	if (ret < 0) {
+		kfree(cec_ctrl);
+		return ret;
+	}
+
+	/* Set the logical address to Unregistered */
+	hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
+
+	ret = cec_register_adapter(cec_adap, &pdev->dev);
+	if (ret < 0) {
+		cec_delete_adapter(cec_adap);
+		return ret;
+	}
+
+	hdmi->cec_adap = cec_adap;
+
+	return 0;
+}
+
+void msm_hdmi_cec_exit(struct hdmi *hdmi)
+{
+	cec_unregister_adapter(hdmi->cec_adap);
+}

-- 
2.40.0

Re: [PATCH 2/4] drm/msm: add hdmi cec support

[Dmitry Baryshkov]

On 18/04/2023 21:10, Arnaud Vrac wrote:
> Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
> and MSM8998. The hardware block can handle a single CEC logical address
> and broadcast messages.
> 
> Port the CEC driver from downstream msm-4.4 kernel. It has been tested
> on MSM8998 and passes the cec-compliance tool tests.
> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> ---
>   drivers/gpu/drm/msm/Kconfig         |   8 ++
>   drivers/gpu/drm/msm/Makefile        |   1 +
>   drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
>   drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
>   drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
>   5 files changed, 322 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index 85f5ab1d552c4..2a02c74207935 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
>   	default y
>   	help
>   	  Choose this option to enable HDCP state machine
> +
> +config DRM_MSM_HDMI_CEC
> +	bool "Enable HDMI CEC support in MSM DRM driver"
> +	depends on DRM_MSM && DRM_MSM_HDMI
> +	select CEC_CORE
> +	default y
> +	help
> +	  Choose this option to enable CEC support
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 7274c41228ed9..0237a2f219ac2 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>   
>   msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
>   
> +msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
>   msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
>   
>   msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 3132105a2a433..1dde3890e25c0 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -11,6 +11,8 @@
>   #include <drm/drm_bridge_connector.h>
>   #include <drm/drm_of.h>
>   
> +#include <media/cec.h>
> +
>   #include <sound/hdmi-codec.h>
>   #include "hdmi.h"
>   
> @@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
>   	if (hdmi->hdcp_ctrl)
>   		msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
>   
> +	/* Process CEC: */
> +	msm_hdmi_cec_irq(hdmi);
> +
>   	/* TODO audio.. */
>   
>   	return IRQ_HANDLED;
> @@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>   	 */
>   	if (hdmi->workq)
>   		destroy_workqueue(hdmi->workq);
> +
> +	msm_hdmi_cec_exit(hdmi);
>   	msm_hdmi_hdcp_destroy(hdmi);
>   
>   	if (hdmi->i2c)
> @@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>   		hdmi->hdcp_ctrl = NULL;
>   	}
>   
> +	msm_hdmi_cec_init(hdmi);
> +
>   	return 0;
>   
>   fail:
> @@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>   
> +	if (hdmi->cec_adap) {
> +		struct cec_connector_info conn_info;
> +		cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
> +		cec_s_conn_info(hdmi->cec_adap, &conn_info);
> +	}
> +
>   	ret = devm_request_irq(dev->dev, hdmi->irq,
>   			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>   			"hdmi_isr", hdmi);
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index e8dbee50637fa..c639bd87f4b8f 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -29,6 +29,7 @@ struct hdmi_audio {
>   };
>   
>   struct hdmi_hdcp_ctrl;
> +struct cec_adapter;
>   
>   struct hdmi {
>   	struct drm_device *dev;
> @@ -73,6 +74,7 @@ struct hdmi {
>   	struct workqueue_struct *workq;
>   
>   	struct hdmi_hdcp_ctrl *hdcp_ctrl;
> +	struct cec_adapter *cec_adap;
>   
>   	/*
>   	* spinlock to protect registers shared by different execution
> @@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
>   static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
>   #endif
>   
> +/*
> + * cec
> + */
> +#ifdef CONFIG_DRM_MSM_HDMI_CEC
> +int msm_hdmi_cec_init(struct hdmi *hdmi);
> +void msm_hdmi_cec_exit(struct hdmi *hdmi);
> +void msm_hdmi_cec_irq(struct hdmi *hdmi);
> +#else
> +static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
> +{
> +	return -ENXIO;
> +}
> +static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
> +static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
> +#endif
> +
>   #endif /* __HDMI_CONNECTOR_H__ */
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> new file mode 100644
> index 0000000000000..51326e493e5da
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> @@ -0,0 +1,280 @@
> +#include <linux/iopoll.h>
> +#include <media/cec.h>
> +
> +#include "hdmi.h"
> +
> +#define HDMI_CEC_INT_MASK ( \
> +	HDMI_CEC_INT_TX_DONE_MASK | \
> +	HDMI_CEC_INT_TX_ERROR_MASK | \
> +	HDMI_CEC_INT_RX_DONE_MASK)
> +
> +struct hdmi_cec_ctrl {
> +	struct hdmi *hdmi;
> +	struct work_struct work;
> +	spinlock_t lock;
> +	u32 irq_status;
> +	u32 tx_status;
> +	u32 tx_retransmits;
> +};
> +
> +static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +
> +	if (enable) {
> +		/* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
> +		hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
> +			   HDMI_CEC_REFTIMER_REFTIMER(960) |
> +			   HDMI_CEC_REFTIMER_ENABLE);
> +
> +		/* read and write timings */
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);

lowercase hex please. We are trying to switch to it.

> +		hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
> +
> +		/* start bit low pulse duration, 3.7ms */
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
> +
> +		/* signal free time, 7 * 2.4ms */
> +		hdmi_write(hdmi, REG_HDMI_CEC_TIME,
> +			   HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
> +			   HDMI_CEC_TIME_ENABLE);
> +
> +		hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
> +		hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
> +
> +		hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
> +		hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> +	} else {
> +		hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
> +		hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> +		cancel_work_sync(&cec_ctrl->work);
> +	}
> +
> +	return 0;
> +}
> +
> +static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +
> +	hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);
> +
> +	return 0;
> +}
> +
> +static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				      u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +	u8 retransmits;
> +	u32 broadcast;
> +	u32 status;
> +	int i;
> +
> +	/* toggle cec in order to flush out bad hw state, if any */
> +	hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> +	hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> +
> +	/* flush register writes */
> +	wmb();
> +
> +	retransmits = attempts ? (attempts - 1) : 0;
> +	hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
> +		   HDMI_CEC_RETRANSMIT_ENABLE |
> +		   HDMI_CEC_RETRANSMIT_COUNT(retransmits));
> +
> +	broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
> +	for (i = 0; i < msg->len; i++) {
> +		hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
> +			   HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
> +	}
> +
> +	/* check line status */
> +	if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
> +			      5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
> +		pr_err("CEC line is busy. Retry failed\n");
> +		return -EBUSY;
> +	}
> +
> +	cec_ctrl->tx_retransmits = retransmits;
> +
> +	/* start transmission */
> +	hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
> +		   HDMI_CEC_CTRL_ENABLE |
> +		   HDMI_CEC_CTRL_SEND_TRIGGER |
> +		   HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
> +		   HDMI_CEC_CTRL_LINE_OE);
> +
> +	return 0;
> +}
> +
> +static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +
> +	cec_ctrl->hdmi->cec_adap = NULL;
> +	kfree(cec_ctrl);
> +}
> +
> +static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
> +	.adap_enable = msm_hdmi_cec_adap_enable,
> +	.adap_log_addr = msm_hdmi_cec_adap_log_addr,
> +	.adap_transmit = msm_hdmi_cec_adap_transmit,
> +	.adap_free = msm_hdmi_cec_adap_free,
> +};
> +
> +#define CEC_IRQ_FRAME_WR_DONE 0x01
> +#define CEC_IRQ_FRAME_RD_DONE 0x02

Please move these to top. Also you might consider replacing this mask 
with two boolean flags.

> +
> +static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
> +{
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +	struct cec_msg msg = {};
> +	u32 data;
> +	int i;
> +
> +	data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
> +	msg.len = (data & 0x1f00) >> 8;

We can also use FIELD_GET here. I'll add defines to the mesa merge request.

> +	if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
> +		return;
> +
> +	msg.msg[0] = data & 0xff;
> +	for (i = 1; i < msg.len; i++)
> +		msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
> +
> +	cec_received_msg(hdmi->cec_adap, &msg);
> +}
> +
> +static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
> +{
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +	u32 tx_status;
> +
> +	tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
> +		HDMI_CEC_STATUS_TX_STATUS__SHIFT;

FIELD_GET(HDMI_CEC_STATUS_TX_STATUS__MASK, cec_ctrl->tx_status).

> +
> +	switch (tx_status) {
> +	case 0:

Please use valus from enum hdmi_cec_tx_status

> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_OK, 0, 0, 0, 0);
> +		break;
> +	case 1:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_NACK, 0, 1, 0, 0);
> +		break;
> +	case 2:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);
> +		break;
> +	case 3:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_MAX_RETRIES |
> +				  CEC_TX_STATUS_NACK,
> +				  0, cec_ctrl->tx_retransmits + 1, 0, 0);
> +		break;
> +	default:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
> +		break;
> +	}
> +}
> +
> +static void msm_hdmi_cec_work(struct work_struct *work)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl =
> +		container_of(work, struct hdmi_cec_ctrl, work);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cec_ctrl->lock, flags);
> +
> +	if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
> +		msm_hdmi_cec_handle_tx_done(cec_ctrl);
> +
> +	if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
> +		msm_hdmi_cec_handle_rx_done(cec_ctrl);
> +
> +	cec_ctrl->irq_status = 0;
> +	cec_ctrl->tx_status = 0;
> +
> +	spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> +}
> +
> +void msm_hdmi_cec_irq(struct hdmi *hdmi)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl;
> +	unsigned long flags;
> +	u32 int_status;
> +
> +	if (!hdmi->cec_adap)
> +		return;
> +
> +	cec_ctrl = hdmi->cec_adap->priv;
> +
> +	int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
> +	if (!(int_status & HDMI_CEC_INT_MASK))
> +		return;
> +
> +	spin_lock_irqsave(&cec_ctrl->lock, flags);
> +
> +	if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
> +		cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
> +		cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
> +	}
> +
> +	if (int_status & HDMI_CEC_INT_RX_DONE)
> +		cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
> +
> +	spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> +
> +	hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
> +	queue_work(hdmi->workq, &cec_ctrl->work);
> +}
> +
> +int msm_hdmi_cec_init(struct hdmi *hdmi)
> +{
> +	struct platform_device *pdev = hdmi->pdev;
> +	struct hdmi_cec_ctrl *cec_ctrl;
> +	struct cec_adapter *cec_adap;
> +	int ret;

hdmi_cec_enable from msm-4.4 tells that CEC is not supported if 
REG_HDMI_VERSION reads < 0x30000001. Could you please add this check?




> +
> +	cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
> +	if (!cec_ctrl)
> +		return -ENOMEM;
> +
> +	cec_ctrl->hdmi = hdmi;
> +	INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
> +
> +	cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
> +					cec_ctrl, "msm",
> +					CEC_CAP_DEFAULTS |
> +					CEC_CAP_CONNECTOR_INFO, 1);
> +	ret = PTR_ERR_OR_ZERO(cec_adap);
> +	if (ret < 0) {
> +		kfree(cec_ctrl);
> +		return ret;
> +	}
> +
> +	/* Set the logical address to Unregistered */
> +	hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
> +
> +	ret = cec_register_adapter(cec_adap, &pdev->dev);
> +	if (ret < 0) {
> +		cec_delete_adapter(cec_adap);
> +		return ret;
> +	}
> +
> +	hdmi->cec_adap = cec_adap;
> +
> +	return 0;
> +}
> +
> +void msm_hdmi_cec_exit(struct hdmi *hdmi)
> +{
> +	cec_unregister_adapter(hdmi->cec_adap);
> +}
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/msm: add hdmi cec support

[Arnaud Vrac]

Le jeu. 20 avr. 2023 à 02:20, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> a écrit :
>
> On 18/04/2023 21:10, Arnaud Vrac wrote:
> > Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
> > and MSM8998. The hardware block can handle a single CEC logical address
> > and broadcast messages.
> >
> > Port the CEC driver from downstream msm-4.4 kernel. It has been tested
> > on MSM8998 and passes the cec-compliance tool tests.
> >
> > Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> > ---
> >   drivers/gpu/drm/msm/Kconfig         |   8 ++
> >   drivers/gpu/drm/msm/Makefile        |   1 +
> >   drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
> >   drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
> >   drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
> >   5 files changed, 322 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> > index 85f5ab1d552c4..2a02c74207935 100644
> > --- a/drivers/gpu/drm/msm/Kconfig
> > +++ b/drivers/gpu/drm/msm/Kconfig
> > @@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
> >       default y
> >       help
> >         Choose this option to enable HDCP state machine
> > +
> > +config DRM_MSM_HDMI_CEC
> > +     bool "Enable HDMI CEC support in MSM DRM driver"
> > +     depends on DRM_MSM && DRM_MSM_HDMI
> > +     select CEC_CORE
> > +     default y
> > +     help
> > +       Choose this option to enable CEC support
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 7274c41228ed9..0237a2f219ac2 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
> >
> >   msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
> >
> > +msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
> >   msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
> >
> >   msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > index 3132105a2a433..1dde3890e25c0 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > @@ -11,6 +11,8 @@
> >   #include <drm/drm_bridge_connector.h>
> >   #include <drm/drm_of.h>
> >
> > +#include <media/cec.h>
> > +
> >   #include <sound/hdmi-codec.h>
> >   #include "hdmi.h"
> >
> > @@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
> >       if (hdmi->hdcp_ctrl)
> >               msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
> >
> > +     /* Process CEC: */
> > +     msm_hdmi_cec_irq(hdmi);
> > +
> >       /* TODO audio.. */
> >
> >       return IRQ_HANDLED;
> > @@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
> >        */
> >       if (hdmi->workq)
> >               destroy_workqueue(hdmi->workq);
> > +
> > +     msm_hdmi_cec_exit(hdmi);
> >       msm_hdmi_hdcp_destroy(hdmi);
> >
> >       if (hdmi->i2c)
> > @@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
> >               hdmi->hdcp_ctrl = NULL;
> >       }
> >
> > +     msm_hdmi_cec_init(hdmi);
> > +
> >       return 0;
> >
> >   fail:
> > @@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
> >
> >       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
> >
> > +     if (hdmi->cec_adap) {
> > +             struct cec_connector_info conn_info;
> > +             cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
> > +             cec_s_conn_info(hdmi->cec_adap, &conn_info);
> > +     }
> > +
> >       ret = devm_request_irq(dev->dev, hdmi->irq,
> >                       msm_hdmi_irq, IRQF_TRIGGER_HIGH,
> >                       "hdmi_isr", hdmi);
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > index e8dbee50637fa..c639bd87f4b8f 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > @@ -29,6 +29,7 @@ struct hdmi_audio {
> >   };
> >
> >   struct hdmi_hdcp_ctrl;
> > +struct cec_adapter;
> >
> >   struct hdmi {
> >       struct drm_device *dev;
> > @@ -73,6 +74,7 @@ struct hdmi {
> >       struct workqueue_struct *workq;
> >
> >       struct hdmi_hdcp_ctrl *hdcp_ctrl;
> > +     struct cec_adapter *cec_adap;
> >
> >       /*
> >       * spinlock to protect registers shared by different execution
> > @@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
> >   static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
> >   #endif
> >
> > +/*
> > + * cec
> > + */
> > +#ifdef CONFIG_DRM_MSM_HDMI_CEC
> > +int msm_hdmi_cec_init(struct hdmi *hdmi);
> > +void msm_hdmi_cec_exit(struct hdmi *hdmi);
> > +void msm_hdmi_cec_irq(struct hdmi *hdmi);
> > +#else
> > +static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
> > +{
> > +     return -ENXIO;
> > +}
> > +static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
> > +static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
> > +#endif
> > +
> >   #endif /* __HDMI_CONNECTOR_H__ */
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> > new file mode 100644
> > index 0000000000000..51326e493e5da
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> > @@ -0,0 +1,280 @@
> > +#include <linux/iopoll.h>
> > +#include <media/cec.h>
> > +
> > +#include "hdmi.h"
> > +
> > +#define HDMI_CEC_INT_MASK ( \
> > +     HDMI_CEC_INT_TX_DONE_MASK | \
> > +     HDMI_CEC_INT_TX_ERROR_MASK | \
> > +     HDMI_CEC_INT_RX_DONE_MASK)
> > +
> > +struct hdmi_cec_ctrl {
> > +     struct hdmi *hdmi;
> > +     struct work_struct work;
> > +     spinlock_t lock;
> > +     u32 irq_status;
> > +     u32 tx_status;
> > +     u32 tx_retransmits;
> > +};
> > +
> > +static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +
> > +     if (enable) {
> > +             /* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
> > +                        HDMI_CEC_REFTIMER_REFTIMER(960) |
> > +                        HDMI_CEC_REFTIMER_ENABLE);
> > +
> > +             /* read and write timings */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);
>
> lowercase hex please. We are trying to switch to it.
>
> > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
> > +
> > +             /* start bit low pulse duration, 3.7ms */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
> > +
> > +             /* signal free time, 7 * 2.4ms */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_TIME,
> > +                        HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
> > +                        HDMI_CEC_TIME_ENABLE);
> > +
> > +             hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
> > +
> > +             hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> > +     } else {
> > +             hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> > +             cancel_work_sync(&cec_ctrl->work);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +
> > +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);
> > +
> > +     return 0;
> > +}
> > +
> > +static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > +                                   u32 signal_free_time, struct cec_msg *msg)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +     u8 retransmits;
> > +     u32 broadcast;
> > +     u32 status;
> > +     int i;
> > +
> > +     /* toggle cec in order to flush out bad hw state, if any */
> > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> > +
> > +     /* flush register writes */
> > +     wmb();
> > +
> > +     retransmits = attempts ? (attempts - 1) : 0;
> > +     hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
> > +                HDMI_CEC_RETRANSMIT_ENABLE |
> > +                HDMI_CEC_RETRANSMIT_COUNT(retransmits));
> > +
> > +     broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
> > +     for (i = 0; i < msg->len; i++) {
> > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
> > +                        HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
> > +     }
> > +
> > +     /* check line status */
> > +     if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
> > +                           5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
> > +             pr_err("CEC line is busy. Retry failed\n");
> > +             return -EBUSY;
> > +     }
> > +
> > +     cec_ctrl->tx_retransmits = retransmits;
> > +
> > +     /* start transmission */
> > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
> > +                HDMI_CEC_CTRL_ENABLE |
> > +                HDMI_CEC_CTRL_SEND_TRIGGER |
> > +                HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
> > +                HDMI_CEC_CTRL_LINE_OE);
> > +
> > +     return 0;
> > +}
> > +
> > +static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +
> > +     cec_ctrl->hdmi->cec_adap = NULL;
> > +     kfree(cec_ctrl);
> > +}
> > +
> > +static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
> > +     .adap_enable = msm_hdmi_cec_adap_enable,
> > +     .adap_log_addr = msm_hdmi_cec_adap_log_addr,
> > +     .adap_transmit = msm_hdmi_cec_adap_transmit,
> > +     .adap_free = msm_hdmi_cec_adap_free,
> > +};
> > +
> > +#define CEC_IRQ_FRAME_WR_DONE 0x01
> > +#define CEC_IRQ_FRAME_RD_DONE 0x02
>
> Please move these to top. Also you might consider replacing this mask
> with two boolean flags.
>
> > +
> > +static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
> > +{
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +     struct cec_msg msg = {};
> > +     u32 data;
> > +     int i;
> > +
> > +     data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
> > +     msg.len = (data & 0x1f00) >> 8;
>
> We can also use FIELD_GET here. I'll add defines to the mesa merge request.

Ok, I wasn't sure if it made sense to add the bitfield definition for
this since it's only valid for the first byte of the message. I'll add
the change since this has been integrated into mesa.

>
> > +     if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
> > +             return;
> > +
> > +     msg.msg[0] = data & 0xff;
> > +     for (i = 1; i < msg.len; i++)
> > +             msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
> > +
> > +     cec_received_msg(hdmi->cec_adap, &msg);
> > +}
> > +
> > +static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
> > +{
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +     u32 tx_status;
> > +
> > +     tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
> > +             HDMI_CEC_STATUS_TX_STATUS__SHIFT;
>
> FIELD_GET(HDMI_CEC_STATUS_TX_STATUS__MASK, cec_ctrl->tx_status).
>
> > +
> > +     switch (tx_status) {
> > +     case 0:
>
> Please use valus from enum hdmi_cec_tx_status
>
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_OK, 0, 0, 0, 0);
> > +             break;
> > +     case 1:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_NACK, 0, 1, 0, 0);
> > +             break;
> > +     case 2:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);
> > +             break;
> > +     case 3:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_MAX_RETRIES |
> > +                               CEC_TX_STATUS_NACK,
> > +                               0, cec_ctrl->tx_retransmits + 1, 0, 0);
> > +             break;
> > +     default:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
> > +             break;
> > +     }
> > +}
> > +
> > +static void msm_hdmi_cec_work(struct work_struct *work)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl =
> > +             container_of(work, struct hdmi_cec_ctrl, work);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&cec_ctrl->lock, flags);
> > +
> > +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
> > +             msm_hdmi_cec_handle_tx_done(cec_ctrl);
> > +
> > +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
> > +             msm_hdmi_cec_handle_rx_done(cec_ctrl);
> > +
> > +     cec_ctrl->irq_status = 0;
> > +     cec_ctrl->tx_status = 0;
> > +
> > +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> > +}
> > +
> > +void msm_hdmi_cec_irq(struct hdmi *hdmi)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl;
> > +     unsigned long flags;
> > +     u32 int_status;
> > +
> > +     if (!hdmi->cec_adap)
> > +             return;
> > +
> > +     cec_ctrl = hdmi->cec_adap->priv;
> > +
> > +     int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
> > +     if (!(int_status & HDMI_CEC_INT_MASK))
> > +             return;
> > +
> > +     spin_lock_irqsave(&cec_ctrl->lock, flags);
> > +
> > +     if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
> > +             cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
> > +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
> > +     }
> > +
> > +     if (int_status & HDMI_CEC_INT_RX_DONE)
> > +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
> > +
> > +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> > +
> > +     hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
> > +     queue_work(hdmi->workq, &cec_ctrl->work);
> > +}
> > +
> > +int msm_hdmi_cec_init(struct hdmi *hdmi)
> > +{
> > +     struct platform_device *pdev = hdmi->pdev;
> > +     struct hdmi_cec_ctrl *cec_ctrl;
> > +     struct cec_adapter *cec_adap;
> > +     int ret;
>
> hdmi_cec_enable from msm-4.4 tells that CEC is not supported if
> REG_HDMI_VERSION reads < 0x30000001. Could you please add this check?

Ack. I wonder which SoCs before MSM8996 can actually support CEC.

>
>
>
>
> > +
> > +     cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
> > +     if (!cec_ctrl)
> > +             return -ENOMEM;
> > +
> > +     cec_ctrl->hdmi = hdmi;
> > +     INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
> > +
> > +     cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
> > +                                     cec_ctrl, "msm",
> > +                                     CEC_CAP_DEFAULTS |
> > +                                     CEC_CAP_CONNECTOR_INFO, 1);
> > +     ret = PTR_ERR_OR_ZERO(cec_adap);
> > +     if (ret < 0) {
> > +             kfree(cec_ctrl);
> > +             return ret;
> > +     }
> > +
> > +     /* Set the logical address to Unregistered */
> > +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
> > +
> > +     ret = cec_register_adapter(cec_adap, &pdev->dev);
> > +     if (ret < 0) {
> > +             cec_delete_adapter(cec_adap);
> > +             return ret;
> > +     }
> > +
> > +     hdmi->cec_adap = cec_adap;
> > +
> > +     return 0;
> > +}
> > +
> > +void msm_hdmi_cec_exit(struct hdmi *hdmi)
> > +{
> > +     cec_unregister_adapter(hdmi->cec_adap);
> > +}
> >
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH 2/4] drm/msm: add hdmi cec support

[Dmitry Baryshkov]

On Thu, 20 Apr 2023 at 10:24, Arnaud Vrac <avrac@freebox.fr> wrote:
>
> Le jeu. 20 avr. 2023 à 02:20, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> a écrit :
> >
> > On 18/04/2023 21:10, Arnaud Vrac wrote:
> > > Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
> > > and MSM8998. The hardware block can handle a single CEC logical address
> > > and broadcast messages.
> > >
> > > Port the CEC driver from downstream msm-4.4 kernel. It has been tested
> > > on MSM8998 and passes the cec-compliance tool tests.
> > >
> > > Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> > > ---
> > >   drivers/gpu/drm/msm/Kconfig         |   8 ++
> > >   drivers/gpu/drm/msm/Makefile        |   1 +
> > >   drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
> > >   drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
> > >   drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
> > >   5 files changed, 322 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> > > index 85f5ab1d552c4..2a02c74207935 100644
> > > --- a/drivers/gpu/drm/msm/Kconfig
> > > +++ b/drivers/gpu/drm/msm/Kconfig
> > > @@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
> > >       default y
> > >       help
> > >         Choose this option to enable HDCP state machine
> > > +
> > > +config DRM_MSM_HDMI_CEC
> > > +     bool "Enable HDMI CEC support in MSM DRM driver"
> > > +     depends on DRM_MSM && DRM_MSM_HDMI
> > > +     select CEC_CORE
> > > +     default y
> > > +     help
> > > +       Choose this option to enable CEC support
> > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > > index 7274c41228ed9..0237a2f219ac2 100644
> > > --- a/drivers/gpu/drm/msm/Makefile
> > > +++ b/drivers/gpu/drm/msm/Makefile
> > > @@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
> > >
> > >   msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
> > >
> > > +msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
> > >   msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
> > >
> > >   msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
> > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > > index 3132105a2a433..1dde3890e25c0 100644
> > > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> > > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > > @@ -11,6 +11,8 @@
> > >   #include <drm/drm_bridge_connector.h>
> > >   #include <drm/drm_of.h>
> > >
> > > +#include <media/cec.h>
> > > +
> > >   #include <sound/hdmi-codec.h>
> > >   #include "hdmi.h"
> > >
> > > @@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
> > >       if (hdmi->hdcp_ctrl)
> > >               msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
> > >
> > > +     /* Process CEC: */
> > > +     msm_hdmi_cec_irq(hdmi);
> > > +
> > >       /* TODO audio.. */
> > >
> > >       return IRQ_HANDLED;
> > > @@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
> > >        */
> > >       if (hdmi->workq)
> > >               destroy_workqueue(hdmi->workq);
> > > +
> > > +     msm_hdmi_cec_exit(hdmi);
> > >       msm_hdmi_hdcp_destroy(hdmi);
> > >
> > >       if (hdmi->i2c)
> > > @@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
> > >               hdmi->hdcp_ctrl = NULL;
> > >       }
> > >
> > > +     msm_hdmi_cec_init(hdmi);
> > > +
> > >       return 0;
> > >
> > >   fail:
> > > @@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
> > >
> > >       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
> > >
> > > +     if (hdmi->cec_adap) {
> > > +             struct cec_connector_info conn_info;
> > > +             cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
> > > +             cec_s_conn_info(hdmi->cec_adap, &conn_info);
> > > +     }
> > > +
> > >       ret = devm_request_irq(dev->dev, hdmi->irq,
> > >                       msm_hdmi_irq, IRQF_TRIGGER_HIGH,
> > >                       "hdmi_isr", hdmi);
> > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > > index e8dbee50637fa..c639bd87f4b8f 100644
> > > --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> > > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > > @@ -29,6 +29,7 @@ struct hdmi_audio {
> > >   };
> > >
> > >   struct hdmi_hdcp_ctrl;
> > > +struct cec_adapter;
> > >
> > >   struct hdmi {
> > >       struct drm_device *dev;
> > > @@ -73,6 +74,7 @@ struct hdmi {
> > >       struct workqueue_struct *workq;
> > >
> > >       struct hdmi_hdcp_ctrl *hdcp_ctrl;
> > > +     struct cec_adapter *cec_adap;
> > >
> > >       /*
> > >       * spinlock to protect registers shared by different execution
> > > @@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
> > >   static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
> > >   #endif
> > >
> > > +/*
> > > + * cec
> > > + */
> > > +#ifdef CONFIG_DRM_MSM_HDMI_CEC
> > > +int msm_hdmi_cec_init(struct hdmi *hdmi);
> > > +void msm_hdmi_cec_exit(struct hdmi *hdmi);
> > > +void msm_hdmi_cec_irq(struct hdmi *hdmi);
> > > +#else
> > > +static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
> > > +{
> > > +     return -ENXIO;
> > > +}
> > > +static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
> > > +static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
> > > +#endif
> > > +
> > >   #endif /* __HDMI_CONNECTOR_H__ */
> > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> > > new file mode 100644
> > > index 0000000000000..51326e493e5da
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> > > @@ -0,0 +1,280 @@
> > > +#include <linux/iopoll.h>
> > > +#include <media/cec.h>
> > > +
> > > +#include "hdmi.h"
> > > +
> > > +#define HDMI_CEC_INT_MASK ( \
> > > +     HDMI_CEC_INT_TX_DONE_MASK | \
> > > +     HDMI_CEC_INT_TX_ERROR_MASK | \
> > > +     HDMI_CEC_INT_RX_DONE_MASK)
> > > +
> > > +struct hdmi_cec_ctrl {
> > > +     struct hdmi *hdmi;
> > > +     struct work_struct work;
> > > +     spinlock_t lock;
> > > +     u32 irq_status;
> > > +     u32 tx_status;
> > > +     u32 tx_retransmits;
> > > +};
> > > +
> > > +static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > > +{
> > > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > > +
> > > +     if (enable) {
> > > +             /* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
> > > +                        HDMI_CEC_REFTIMER_REFTIMER(960) |
> > > +                        HDMI_CEC_REFTIMER_ENABLE);
> > > +
> > > +             /* read and write timings */
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);
> >
> > lowercase hex please. We are trying to switch to it.
> >
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
> > > +
> > > +             /* start bit low pulse duration, 3.7ms */
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
> > > +
> > > +             /* signal free time, 7 * 2.4ms */
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_TIME,
> > > +                        HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
> > > +                        HDMI_CEC_TIME_ENABLE);
> > > +
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
> > > +
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> > > +     } else {
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> > > +             cancel_work_sync(&cec_ctrl->work);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > > +{
> > > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > > +
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > > +                                   u32 signal_free_time, struct cec_msg *msg)
> > > +{
> > > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > > +     u8 retransmits;
> > > +     u32 broadcast;
> > > +     u32 status;
> > > +     int i;
> > > +
> > > +     /* toggle cec in order to flush out bad hw state, if any */
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> > > +
> > > +     /* flush register writes */
> > > +     wmb();
> > > +
> > > +     retransmits = attempts ? (attempts - 1) : 0;
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
> > > +                HDMI_CEC_RETRANSMIT_ENABLE |
> > > +                HDMI_CEC_RETRANSMIT_COUNT(retransmits));
> > > +
> > > +     broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
> > > +     for (i = 0; i < msg->len; i++) {
> > > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
> > > +                        HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
> > > +     }
> > > +
> > > +     /* check line status */
> > > +     if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
> > > +                           5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
> > > +             pr_err("CEC line is busy. Retry failed\n");
> > > +             return -EBUSY;
> > > +     }
> > > +
> > > +     cec_ctrl->tx_retransmits = retransmits;
> > > +
> > > +     /* start transmission */
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
> > > +                HDMI_CEC_CTRL_ENABLE |
> > > +                HDMI_CEC_CTRL_SEND_TRIGGER |
> > > +                HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
> > > +                HDMI_CEC_CTRL_LINE_OE);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
> > > +{
> > > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > > +
> > > +     cec_ctrl->hdmi->cec_adap = NULL;
> > > +     kfree(cec_ctrl);
> > > +}
> > > +
> > > +static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
> > > +     .adap_enable = msm_hdmi_cec_adap_enable,
> > > +     .adap_log_addr = msm_hdmi_cec_adap_log_addr,
> > > +     .adap_transmit = msm_hdmi_cec_adap_transmit,
> > > +     .adap_free = msm_hdmi_cec_adap_free,
> > > +};
> > > +
> > > +#define CEC_IRQ_FRAME_WR_DONE 0x01
> > > +#define CEC_IRQ_FRAME_RD_DONE 0x02
> >
> > Please move these to top. Also you might consider replacing this mask
> > with two boolean flags.
> >
> > > +
> > > +static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
> > > +{
> > > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > > +     struct cec_msg msg = {};
> > > +     u32 data;
> > > +     int i;
> > > +
> > > +     data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
> > > +     msg.len = (data & 0x1f00) >> 8;
> >
> > We can also use FIELD_GET here. I'll add defines to the mesa merge request.
>
> Ok, I wasn't sure if it made sense to add the bitfield definition for
> this since it's only valid for the first byte of the message. I'll add
> the change since this has been integrated into mesa.
>
> >
> > > +     if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
> > > +             return;
> > > +
> > > +     msg.msg[0] = data & 0xff;
> > > +     for (i = 1; i < msg.len; i++)
> > > +             msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
> > > +
> > > +     cec_received_msg(hdmi->cec_adap, &msg);
> > > +}
> > > +
> > > +static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
> > > +{
> > > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > > +     u32 tx_status;
> > > +
> > > +     tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
> > > +             HDMI_CEC_STATUS_TX_STATUS__SHIFT;
> >
> > FIELD_GET(HDMI_CEC_STATUS_TX_STATUS__MASK, cec_ctrl->tx_status).
> >
> > > +
> > > +     switch (tx_status) {
> > > +     case 0:
> >
> > Please use valus from enum hdmi_cec_tx_status
> >
> > > +             cec_transmit_done(hdmi->cec_adap,
> > > +                               CEC_TX_STATUS_OK, 0, 0, 0, 0);
> > > +             break;
> > > +     case 1:
> > > +             cec_transmit_done(hdmi->cec_adap,
> > > +                               CEC_TX_STATUS_NACK, 0, 1, 0, 0);
> > > +             break;
> > > +     case 2:
> > > +             cec_transmit_done(hdmi->cec_adap,
> > > +                               CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);
> > > +             break;
> > > +     case 3:
> > > +             cec_transmit_done(hdmi->cec_adap,
> > > +                               CEC_TX_STATUS_MAX_RETRIES |
> > > +                               CEC_TX_STATUS_NACK,
> > > +                               0, cec_ctrl->tx_retransmits + 1, 0, 0);
> > > +             break;
> > > +     default:
> > > +             cec_transmit_done(hdmi->cec_adap,
> > > +                               CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
> > > +             break;
> > > +     }
> > > +}
> > > +
> > > +static void msm_hdmi_cec_work(struct work_struct *work)
> > > +{
> > > +     struct hdmi_cec_ctrl *cec_ctrl =
> > > +             container_of(work, struct hdmi_cec_ctrl, work);
> > > +     unsigned long flags;
> > > +
> > > +     spin_lock_irqsave(&cec_ctrl->lock, flags);
> > > +
> > > +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
> > > +             msm_hdmi_cec_handle_tx_done(cec_ctrl);
> > > +
> > > +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
> > > +             msm_hdmi_cec_handle_rx_done(cec_ctrl);
> > > +
> > > +     cec_ctrl->irq_status = 0;
> > > +     cec_ctrl->tx_status = 0;
> > > +
> > > +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> > > +}
> > > +
> > > +void msm_hdmi_cec_irq(struct hdmi *hdmi)
> > > +{
> > > +     struct hdmi_cec_ctrl *cec_ctrl;
> > > +     unsigned long flags;
> > > +     u32 int_status;
> > > +
> > > +     if (!hdmi->cec_adap)
> > > +             return;
> > > +
> > > +     cec_ctrl = hdmi->cec_adap->priv;
> > > +
> > > +     int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
> > > +     if (!(int_status & HDMI_CEC_INT_MASK))
> > > +             return;
> > > +
> > > +     spin_lock_irqsave(&cec_ctrl->lock, flags);
> > > +
> > > +     if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
> > > +             cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
> > > +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
> > > +     }
> > > +
> > > +     if (int_status & HDMI_CEC_INT_RX_DONE)
> > > +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
> > > +
> > > +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> > > +
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
> > > +     queue_work(hdmi->workq, &cec_ctrl->work);
> > > +}
> > > +
> > > +int msm_hdmi_cec_init(struct hdmi *hdmi)
> > > +{
> > > +     struct platform_device *pdev = hdmi->pdev;
> > > +     struct hdmi_cec_ctrl *cec_ctrl;
> > > +     struct cec_adapter *cec_adap;
> > > +     int ret;
> >
> > hdmi_cec_enable from msm-4.4 tells that CEC is not supported if
> > REG_HDMI_VERSION reads < 0x30000001. Could you please add this check?
>
> Ack. I wonder which SoCs before MSM8996 can actually support CEC.

Granted the commit at [1], one can assume that apq8084 supported CEC
and it even wasn't the first one.

[1] https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/8d8640d18491660f8c6bd082fd2030e2d7cbc1f8

I checked, on apq8064 this register reads as  0x2010000. I'd guess the
first chips to support CEC were msm8974/apq8074

>
> >
> >
> >
> >
> > > +
> > > +     cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
> > > +     if (!cec_ctrl)
> > > +             return -ENOMEM;
> > > +
> > > +     cec_ctrl->hdmi = hdmi;
> > > +     INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
> > > +
> > > +     cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
> > > +                                     cec_ctrl, "msm",
> > > +                                     CEC_CAP_DEFAULTS |
> > > +                                     CEC_CAP_CONNECTOR_INFO, 1);
> > > +     ret = PTR_ERR_OR_ZERO(cec_adap);
> > > +     if (ret < 0) {
> > > +             kfree(cec_ctrl);
> > > +             return ret;
> > > +     }
> > > +
> > > +     /* Set the logical address to Unregistered */
> > > +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
> > > +
> > > +     ret = cec_register_adapter(cec_adap, &pdev->dev);
> > > +     if (ret < 0) {
> > > +             cec_delete_adapter(cec_adap);
> > > +             return ret;
> > > +     }
> > > +
> > > +     hdmi->cec_adap = cec_adap;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +void msm_hdmi_cec_exit(struct hdmi *hdmi)
> > > +{
> > > +     cec_unregister_adapter(hdmi->cec_adap);
> > > +}



-- 
With best wishes
Dmitry

Re: [PATCH 2/4] drm/msm: add hdmi cec support

[Hans Verkuil]

Hi Arnaud,

Some review comments below...

On 4/18/23 20:10, Arnaud Vrac wrote:
> Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
> and MSM8998. The hardware block can handle a single CEC logical address
> and broadcast messages.
> 
> Port the CEC driver from downstream msm-4.4 kernel. It has been tested
> on MSM8998 and passes the cec-compliance tool tests.

Just to verify: did you run the cec-compliance --test-adapter test? That's
the important one to verify your own driver.

> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> ---
>  drivers/gpu/drm/msm/Kconfig         |   8 ++
>  drivers/gpu/drm/msm/Makefile        |   1 +
>  drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
>  drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
>  drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 322 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index 85f5ab1d552c4..2a02c74207935 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
>  	default y
>  	help
>  	  Choose this option to enable HDCP state machine
> +
> +config DRM_MSM_HDMI_CEC
> +	bool "Enable HDMI CEC support in MSM DRM driver"
> +	depends on DRM_MSM && DRM_MSM_HDMI
> +	select CEC_CORE
> +	default y
> +	help
> +	  Choose this option to enable CEC support
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 7274c41228ed9..0237a2f219ac2 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>  
>  msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
>  
> +msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
>  msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
>  
>  msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 3132105a2a433..1dde3890e25c0 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -11,6 +11,8 @@
>  #include <drm/drm_bridge_connector.h>
>  #include <drm/drm_of.h>
>  
> +#include <media/cec.h>
> +
>  #include <sound/hdmi-codec.h>
>  #include "hdmi.h"
>  
> @@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
>  	if (hdmi->hdcp_ctrl)
>  		msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
>  
> +	/* Process CEC: */
> +	msm_hdmi_cec_irq(hdmi);
> +
>  	/* TODO audio.. */
>  
>  	return IRQ_HANDLED;
> @@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>  	 */
>  	if (hdmi->workq)
>  		destroy_workqueue(hdmi->workq);
> +
> +	msm_hdmi_cec_exit(hdmi);
>  	msm_hdmi_hdcp_destroy(hdmi);
>  
>  	if (hdmi->i2c)
> @@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>  		hdmi->hdcp_ctrl = NULL;
>  	}
>  
> +	msm_hdmi_cec_init(hdmi);
> +
>  	return 0;
>  
>  fail:
> @@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>  
>  	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>  
> +	if (hdmi->cec_adap) {
> +		struct cec_connector_info conn_info;
> +		cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
> +		cec_s_conn_info(hdmi->cec_adap, &conn_info);
> +	}
> +
>  	ret = devm_request_irq(dev->dev, hdmi->irq,
>  			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>  			"hdmi_isr", hdmi);
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index e8dbee50637fa..c639bd87f4b8f 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -29,6 +29,7 @@ struct hdmi_audio {
>  };
>  
>  struct hdmi_hdcp_ctrl;
> +struct cec_adapter;
>  
>  struct hdmi {
>  	struct drm_device *dev;
> @@ -73,6 +74,7 @@ struct hdmi {
>  	struct workqueue_struct *workq;
>  
>  	struct hdmi_hdcp_ctrl *hdcp_ctrl;
> +	struct cec_adapter *cec_adap;
>  
>  	/*
>  	* spinlock to protect registers shared by different execution
> @@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
>  static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
>  #endif
>  
> +/*
> + * cec
> + */
> +#ifdef CONFIG_DRM_MSM_HDMI_CEC
> +int msm_hdmi_cec_init(struct hdmi *hdmi);
> +void msm_hdmi_cec_exit(struct hdmi *hdmi);
> +void msm_hdmi_cec_irq(struct hdmi *hdmi);
> +#else
> +static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
> +{
> +	return -ENXIO;
> +}
> +static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
> +static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
> +#endif
> +
>  #endif /* __HDMI_CONNECTOR_H__ */
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> new file mode 100644
> index 0000000000000..51326e493e5da
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> @@ -0,0 +1,280 @@
> +#include <linux/iopoll.h>
> +#include <media/cec.h>
> +
> +#include "hdmi.h"
> +
> +#define HDMI_CEC_INT_MASK ( \
> +	HDMI_CEC_INT_TX_DONE_MASK | \
> +	HDMI_CEC_INT_TX_ERROR_MASK | \
> +	HDMI_CEC_INT_RX_DONE_MASK)
> +
> +struct hdmi_cec_ctrl {
> +	struct hdmi *hdmi;
> +	struct work_struct work;
> +	spinlock_t lock;
> +	u32 irq_status;
> +	u32 tx_status;
> +	u32 tx_retransmits;
> +};
> +
> +static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +
> +	if (enable) {
> +		/* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
> +		hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
> +			   HDMI_CEC_REFTIMER_REFTIMER(960) |
> +			   HDMI_CEC_REFTIMER_ENABLE);
> +
> +		/* read and write timings */
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);
> +		hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
> +
> +		/* start bit low pulse duration, 3.7ms */
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
> +
> +		/* signal free time, 7 * 2.4ms */
> +		hdmi_write(hdmi, REG_HDMI_CEC_TIME,
> +			   HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
> +			   HDMI_CEC_TIME_ENABLE);

The Signal Free Time changes depending on the situation (3, 5 or 7 bit
periods). Does the hardware take care of that, or do you need to update
this register in the transmit op as well?

> +
> +		hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
> +		hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
> +		hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
> +
> +		hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
> +		hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> +	} else {
> +		hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
> +		hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> +		cancel_work_sync(&cec_ctrl->work);
> +	}
> +
> +	return 0;
> +}
> +
> +static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +
> +	hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);

So to disable the logical address you set this to 0xf, right? Since
CEC_LOG_ADDR_INVALID is 0xff.

> +
> +	return 0;
> +}
> +
> +static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				      u32 signal_free_time, struct cec_msg *msg)

Note that the SFT is passed in as an argument for those hardware devices
that do not keep track of it themselves.

> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +	u8 retransmits;
> +	u32 broadcast;
> +	u32 status;
> +	int i;
> +
> +	/* toggle cec in order to flush out bad hw state, if any */
> +	hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> +	hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> +
> +	/* flush register writes */
> +	wmb();
> +
> +	retransmits = attempts ? (attempts - 1) : 0;
> +	hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
> +		   HDMI_CEC_RETRANSMIT_ENABLE |
> +		   HDMI_CEC_RETRANSMIT_COUNT(retransmits));
> +
> +	broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
> +	for (i = 0; i < msg->len; i++) {
> +		hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
> +			   HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
> +	}
> +
> +	/* check line status */
> +	if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
> +			      5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
> +		pr_err("CEC line is busy. Retry failed\n");

This doesn't look right. Normally it is the CEC hardware that will wait for the
bus to become free, and then it will start the transmit. That is not something
you should have to do in the driver. And this waits for just 1 ms, right? That's
much too short if a message is currently being received.

Is there documentation of the CEC hardware available somewhere? Or can you
explain a bit about it?

> +		return -EBUSY;
> +	}
> +
> +	cec_ctrl->tx_retransmits = retransmits;
> +
> +	/* start transmission */
> +	hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
> +		   HDMI_CEC_CTRL_ENABLE |
> +		   HDMI_CEC_CTRL_SEND_TRIGGER |
> +		   HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
> +		   HDMI_CEC_CTRL_LINE_OE);
> +
> +	return 0;
> +}
> +
> +static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> +
> +	cec_ctrl->hdmi->cec_adap = NULL;
> +	kfree(cec_ctrl);
> +}
> +
> +static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
> +	.adap_enable = msm_hdmi_cec_adap_enable,
> +	.adap_log_addr = msm_hdmi_cec_adap_log_addr,
> +	.adap_transmit = msm_hdmi_cec_adap_transmit,
> +	.adap_free = msm_hdmi_cec_adap_free,
> +};
> +
> +#define CEC_IRQ_FRAME_WR_DONE 0x01
> +#define CEC_IRQ_FRAME_RD_DONE 0x02
> +
> +static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
> +{
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +	struct cec_msg msg = {};
> +	u32 data;
> +	int i;
> +
> +	data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
> +	msg.len = (data & 0x1f00) >> 8;
> +	if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
> +		return;
> +
> +	msg.msg[0] = data & 0xff;
> +	for (i = 1; i < msg.len; i++)
> +		msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
> +
> +	cec_received_msg(hdmi->cec_adap, &msg);
> +}
> +
> +static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
> +{
> +	struct hdmi *hdmi = cec_ctrl->hdmi;
> +	u32 tx_status;
> +
> +	tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
> +		HDMI_CEC_STATUS_TX_STATUS__SHIFT;
> +
> +	switch (tx_status) {
> +	case 0:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_OK, 0, 0, 0, 0);
> +		break;
> +	case 1:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_NACK, 0, 1, 0, 0);

It's not clear to me who does the retransmits. There are two possibilities:
the hardware takes care of that, and so you just get the final result
and you OR this status with CEC_TX_STATUS_MAX_RETRIES to indicate that
the CEC framework shouldn't attempt to retry.

Or the hardware just does a single transmit, and in that case you never
supply the CEC_TX_STATUS_MAX_RETRIES and just leave it up to the framework
to reissue a transmit.

So here you do no supply MAX_RETRIES...

> +		break;
> +	case 2:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);

... and also not here...

> +		break;
> +	case 3:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_MAX_RETRIES |
> +				  CEC_TX_STATUS_NACK,
> +				  0, cec_ctrl->tx_retransmits + 1, 0, 0);

...but here you do.

> +		break;
> +	default:
> +		cec_transmit_done(hdmi->cec_adap,
> +				  CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
> +		break;
> +	}
> +}
> +
> +static void msm_hdmi_cec_work(struct work_struct *work)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl =
> +		container_of(work, struct hdmi_cec_ctrl, work);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cec_ctrl->lock, flags);
> +
> +	if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
> +		msm_hdmi_cec_handle_tx_done(cec_ctrl);
> +
> +	if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
> +		msm_hdmi_cec_handle_rx_done(cec_ctrl);
> +
> +	cec_ctrl->irq_status = 0;
> +	cec_ctrl->tx_status = 0;
> +
> +	spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> +}
> +
> +void msm_hdmi_cec_irq(struct hdmi *hdmi)
> +{
> +	struct hdmi_cec_ctrl *cec_ctrl;
> +	unsigned long flags;
> +	u32 int_status;
> +
> +	if (!hdmi->cec_adap)
> +		return;
> +
> +	cec_ctrl = hdmi->cec_adap->priv;
> +
> +	int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
> +	if (!(int_status & HDMI_CEC_INT_MASK))
> +		return;
> +
> +	spin_lock_irqsave(&cec_ctrl->lock, flags);
> +
> +	if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
> +		cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
> +		cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
> +	}
> +
> +	if (int_status & HDMI_CEC_INT_RX_DONE)
> +		cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
> +
> +	spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> +
> +	hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
> +	queue_work(hdmi->workq, &cec_ctrl->work);
> +}
> +
> +int msm_hdmi_cec_init(struct hdmi *hdmi)
> +{
> +	struct platform_device *pdev = hdmi->pdev;
> +	struct hdmi_cec_ctrl *cec_ctrl;
> +	struct cec_adapter *cec_adap;
> +	int ret;
> +
> +	cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
> +	if (!cec_ctrl)
> +		return -ENOMEM;
> +
> +	cec_ctrl->hdmi = hdmi;
> +	INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
> +
> +	cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
> +					cec_ctrl, "msm",
> +					CEC_CAP_DEFAULTS |
> +					CEC_CAP_CONNECTOR_INFO, 1);
> +	ret = PTR_ERR_OR_ZERO(cec_adap);
> +	if (ret < 0) {
> +		kfree(cec_ctrl);
> +		return ret;
> +	}
> +
> +	/* Set the logical address to Unregistered */
> +	hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
> +
> +	ret = cec_register_adapter(cec_adap, &pdev->dev);
> +	if (ret < 0) {
> +		cec_delete_adapter(cec_adap);
> +		return ret;
> +	}
> +
> +	hdmi->cec_adap = cec_adap;
> +
> +	return 0;
> +}
> +
> +void msm_hdmi_cec_exit(struct hdmi *hdmi)
> +{
> +	cec_unregister_adapter(hdmi->cec_adap);
> +}
> 

Final question: is this CEC device able to transmit messages when the hotplug detect
pin is low? Some displays pull the HPD low when in Standby, but it is still possible
to wake them up with a <Image View On> message. It is important to check that.

If this is really not possible, then the CEC_CAP_NEEDS_HPD should be set.

Regards,

	Hans

Re: [PATCH 2/4] drm/msm: add hdmi cec support

[Arnaud Vrac]

Le ven. 21 avr. 2023 à 15:27, Hans Verkuil <hverkuil-cisco@xs4all.nl> a écrit :
>
> Hi Arnaud,
>
> Some review comments below...

Hi Hans,

For context, I first based my work on the fbdev driver from Qualcomm a
few years ago, on our own CEC framework which does not implement any
CEC protocol logic (as android does). At the time I verified that the
messages were matching the electrical and protocol spec, using manual
tests and a QD882EA analyzer. I also passed HDMI and CEC certs.

I simply ported this work more recently to a newer kernel and the
media-cec framework, also checking the port that Qualcomm did later
on.

> On 4/18/23 20:10, Arnaud Vrac wrote:
> > Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
> > and MSM8998. The hardware block can handle a single CEC logical address
> > and broadcast messages.
> >
> > Port the CEC driver from downstream msm-4.4 kernel. It has been tested
> > on MSM8998 and passes the cec-compliance tool tests.
>
> Just to verify: did you run the cec-compliance --test-adapter test? That's
> the important one to verify your own driver.

Yes, and I also ran the cec-compliance -r 0 with a pulse8 emulating a
tv on the bus. Here's the result of cec-compliance --test-adapter:

Find remote devices:
        Polling: OK

CEC API:
        CEC_ADAP_G_CAPS: OK
        Invalid ioctls: OK
        CEC_DQEVENT: OK
        CEC_ADAP_G/S_PHYS_ADDR: OK
        CEC_ADAP_G/S_LOG_ADDRS: OK
        CEC_TRANSMIT: OK
        CEC_RECEIVE: OK
        CEC_TRANSMIT/RECEIVE (non-blocking): OK (Presumed)
        CEC_G/S_MODE: OK
        warn: cec-test-adapter.cpp(1189): Too many transmits (3)
without receives
                SFTs for repeating messages (>= 7): 7: 38, 8: 2
                SFTs for newly transmitted messages (>= 5): 6: 2, 7: 17
                SFTs for newly transmitted remote messages (>= 5): 6: 20
        CEC_EVENT_LOST_MSGS: OK

Network topology:
[...]

Total for hdmi_msm device /dev/cec0: 11, Succeeded: 11, Failed: 0, Warnings: 1

>
> >
> > Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> > ---
> >  drivers/gpu/drm/msm/Kconfig         |   8 ++
> >  drivers/gpu/drm/msm/Makefile        |   1 +
> >  drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
> >  drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
> >  drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
> >  5 files changed, 322 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> > index 85f5ab1d552c4..2a02c74207935 100644
> > --- a/drivers/gpu/drm/msm/Kconfig
> > +++ b/drivers/gpu/drm/msm/Kconfig
> > @@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
> >       default y
> >       help
> >         Choose this option to enable HDCP state machine
> > +
> > +config DRM_MSM_HDMI_CEC
> > +     bool "Enable HDMI CEC support in MSM DRM driver"
> > +     depends on DRM_MSM && DRM_MSM_HDMI
> > +     select CEC_CORE
> > +     default y
> > +     help
> > +       Choose this option to enable CEC support
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 7274c41228ed9..0237a2f219ac2 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
> >
> >  msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
> >
> > +msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
> >  msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
> >
> >  msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > index 3132105a2a433..1dde3890e25c0 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > @@ -11,6 +11,8 @@
> >  #include <drm/drm_bridge_connector.h>
> >  #include <drm/drm_of.h>
> >
> > +#include <media/cec.h>
> > +
> >  #include <sound/hdmi-codec.h>
> >  #include "hdmi.h"
> >
> > @@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
> >       if (hdmi->hdcp_ctrl)
> >               msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
> >
> > +     /* Process CEC: */
> > +     msm_hdmi_cec_irq(hdmi);
> > +
> >       /* TODO audio.. */
> >
> >       return IRQ_HANDLED;
> > @@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
> >        */
> >       if (hdmi->workq)
> >               destroy_workqueue(hdmi->workq);
> > +
> > +     msm_hdmi_cec_exit(hdmi);
> >       msm_hdmi_hdcp_destroy(hdmi);
> >
> >       if (hdmi->i2c)
> > @@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
> >               hdmi->hdcp_ctrl = NULL;
> >       }
> >
> > +     msm_hdmi_cec_init(hdmi);
> > +
> >       return 0;
> >
> >  fail:
> > @@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
> >
> >       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
> >
> > +     if (hdmi->cec_adap) {
> > +             struct cec_connector_info conn_info;
> > +             cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
> > +             cec_s_conn_info(hdmi->cec_adap, &conn_info);
> > +     }
> > +
> >       ret = devm_request_irq(dev->dev, hdmi->irq,
> >                       msm_hdmi_irq, IRQF_TRIGGER_HIGH,
> >                       "hdmi_isr", hdmi);
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > index e8dbee50637fa..c639bd87f4b8f 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > @@ -29,6 +29,7 @@ struct hdmi_audio {
> >  };
> >
> >  struct hdmi_hdcp_ctrl;
> > +struct cec_adapter;
> >
> >  struct hdmi {
> >       struct drm_device *dev;
> > @@ -73,6 +74,7 @@ struct hdmi {
> >       struct workqueue_struct *workq;
> >
> >       struct hdmi_hdcp_ctrl *hdcp_ctrl;
> > +     struct cec_adapter *cec_adap;
> >
> >       /*
> >       * spinlock to protect registers shared by different execution
> > @@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
> >  static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
> >  #endif
> >
> > +/*
> > + * cec
> > + */
> > +#ifdef CONFIG_DRM_MSM_HDMI_CEC
> > +int msm_hdmi_cec_init(struct hdmi *hdmi);
> > +void msm_hdmi_cec_exit(struct hdmi *hdmi);
> > +void msm_hdmi_cec_irq(struct hdmi *hdmi);
> > +#else
> > +static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
> > +{
> > +     return -ENXIO;
> > +}
> > +static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
> > +static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
> > +#endif
> > +
> >  #endif /* __HDMI_CONNECTOR_H__ */
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> > new file mode 100644
> > index 0000000000000..51326e493e5da
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
> > @@ -0,0 +1,280 @@
> > +#include <linux/iopoll.h>
> > +#include <media/cec.h>
> > +
> > +#include "hdmi.h"
> > +
> > +#define HDMI_CEC_INT_MASK ( \
> > +     HDMI_CEC_INT_TX_DONE_MASK | \
> > +     HDMI_CEC_INT_TX_ERROR_MASK | \
> > +     HDMI_CEC_INT_RX_DONE_MASK)
> > +
> > +struct hdmi_cec_ctrl {
> > +     struct hdmi *hdmi;
> > +     struct work_struct work;
> > +     spinlock_t lock;
> > +     u32 irq_status;
> > +     u32 tx_status;
> > +     u32 tx_retransmits;
> > +};
> > +
> > +static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +
> > +     if (enable) {
> > +             /* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
> > +                        HDMI_CEC_REFTIMER_REFTIMER(960) |
> > +                        HDMI_CEC_REFTIMER_ENABLE);
> > +
> > +             /* read and write timings */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
> > +
> > +             /* start bit low pulse duration, 3.7ms */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
> > +
> > +             /* signal free time, 7 * 2.4ms */
> > +             hdmi_write(hdmi, REG_HDMI_CEC_TIME,
> > +                        HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
> > +                        HDMI_CEC_TIME_ENABLE);
>
> The Signal Free Time changes depending on the situation (3, 5 or 7 bit
> periods). Does the hardware take care of that, or do you need to update
> this register in the transmit op as well?

In case of retries, which are handled by the hardware, I'm not sure if
the hardware will use the period set in this register or 3. I believe
it should be 3 otherwise we would have problems when probing addresses
on a busy bus.

Otherwise for new message transmissions, the register value is used
with caveats.

I've just tried to use set the register value during transmit using
the signal_free_time parameter, and am getting wrong results. To
emphasize the problem I set signal_free_time to 3 instead of 5 and 8
instead of 7, and am getting the following in cec-compliance:

                SFTs for repeating messages (>= 7): 3: 10, 4: 6, 8: 14, 9: 6
                SFTs for newly transmitted messages (>= 5): 7: 6, 8: 8

If I instead set signal_free_time to 3 just after receiving a message
and 8 just after sending, I get better results:

                SFTs for repeating messages (>= 7): 8: 26, 9: 10
                SFTs for newly transmitted messages (>= 5): 2: 7, 3: 12

But it's not clear to me why the change is not effective immediatly in
the first test, so I'm not confortable changing the fixed value of 7
periods.

>
> > +
> > +             hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
> > +
> > +             hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> > +     } else {
> > +             hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
> > +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> > +             cancel_work_sync(&cec_ctrl->work);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +
> > +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);
>
> So to disable the logical address you set this to 0xf, right? Since
> CEC_LOG_ADDR_INVALID is 0xff.

Right.

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > +                                   u32 signal_free_time, struct cec_msg *msg)
>
> Note that the SFT is passed in as an argument for those hardware devices
> that do not keep track of it themselves.
>
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +     u8 retransmits;
> > +     u32 broadcast;
> > +     u32 status;
> > +     int i;
> > +
> > +     /* toggle cec in order to flush out bad hw state, if any */
> > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
> > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
> > +
> > +     /* flush register writes */
> > +     wmb();
> > +
> > +     retransmits = attempts ? (attempts - 1) : 0;
> > +     hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
> > +                HDMI_CEC_RETRANSMIT_ENABLE |
> > +                HDMI_CEC_RETRANSMIT_COUNT(retransmits));
> > +
> > +     broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
> > +     for (i = 0; i < msg->len; i++) {
> > +             hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
> > +                        HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
> > +     }
> > +
> > +     /* check line status */
> > +     if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
> > +                           5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
> > +             pr_err("CEC line is busy. Retry failed\n");
>
> This doesn't look right. Normally it is the CEC hardware that will wait for the
> bus to become free, and then it will start the transmit. That is not something
> you should have to do in the driver. And this waits for just 1 ms, right? That's
> much too short if a message is currently being received.
>
> Is there documentation of the CEC hardware available somewhere? Or can you
> explain a bit about it?

I'm not sure why this code is here, it's a check that was done in the
original Qualcomm driver using a busy loop with a schedule():

https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/media/platform/msm/sde/cec/sde_hdmi_cec.c#L174
https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/video/fbdev/msm/mdss_hdmi_cec.c#L109

In practice I don't think I've seen the status being busy, but I
ported the check in case it's still useful. Maybe an hw engineer from
Qualcomm can chime in.

>
> > +             return -EBUSY;
> > +     }
> > +
> > +     cec_ctrl->tx_retransmits = retransmits;
> > +
> > +     /* start transmission */
> > +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
> > +                HDMI_CEC_CTRL_ENABLE |
> > +                HDMI_CEC_CTRL_SEND_TRIGGER |
> > +                HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
> > +                HDMI_CEC_CTRL_LINE_OE);
> > +
> > +     return 0;
> > +}
> > +
> > +static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
> > +
> > +     cec_ctrl->hdmi->cec_adap = NULL;
> > +     kfree(cec_ctrl);
> > +}
> > +
> > +static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
> > +     .adap_enable = msm_hdmi_cec_adap_enable,
> > +     .adap_log_addr = msm_hdmi_cec_adap_log_addr,
> > +     .adap_transmit = msm_hdmi_cec_adap_transmit,
> > +     .adap_free = msm_hdmi_cec_adap_free,
> > +};
> > +
> > +#define CEC_IRQ_FRAME_WR_DONE 0x01
> > +#define CEC_IRQ_FRAME_RD_DONE 0x02
> > +
> > +static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
> > +{
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +     struct cec_msg msg = {};
> > +     u32 data;
> > +     int i;
> > +
> > +     data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
> > +     msg.len = (data & 0x1f00) >> 8;
> > +     if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
> > +             return;
> > +
> > +     msg.msg[0] = data & 0xff;
> > +     for (i = 1; i < msg.len; i++)
> > +             msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
> > +
> > +     cec_received_msg(hdmi->cec_adap, &msg);
> > +}
> > +
> > +static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
> > +{
> > +     struct hdmi *hdmi = cec_ctrl->hdmi;
> > +     u32 tx_status;
> > +
> > +     tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
> > +             HDMI_CEC_STATUS_TX_STATUS__SHIFT;
> > +
> > +     switch (tx_status) {
> > +     case 0:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_OK, 0, 0, 0, 0);
> > +             break;
> > +     case 1:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_NACK, 0, 1, 0, 0);
>
> It's not clear to me who does the retransmits. There are two possibilities:
> the hardware takes care of that, and so you just get the final result
> and you OR this status with CEC_TX_STATUS_MAX_RETRIES to indicate that
> the CEC framework shouldn't attempt to retry.
>
> Or the hardware just does a single transmit, and in that case you never
> supply the CEC_TX_STATUS_MAX_RETRIES and just leave it up to the framework
> to reissue a transmit.

After fiddling with the registers, my understanding is the following:

When arbitration loss happens, the hardware returns the ARB_LOSS
status without retrying afterwards, and I don't think the hardware
reports the number of NACKs that were attempted before the arbitration
loss. So in this case I believe it makes sense to only report the
arbitration loss to the framework.

The hardware will otherwise retry up to the number of retransmits
indicated in the REG_HDMI_CEC_RETRANSMIT register set during transmit
and return either OK or MAX_RETRIES (=NACK). I haven't seen the NACK
status (case 1) in practice, even if I disable retransmits in the
dedicated register.

>
> So here you do no supply MAX_RETRIES...

I chose to handle case 1 as a single NACK answer, if it could still
happen for some reason.

>
> > +             break;
> > +     case 2:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);
>
> ... and also not here...

See explanation above.

>
> > +             break;
> > +     case 3:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_MAX_RETRIES |
> > +                               CEC_TX_STATUS_NACK,
> > +                               0, cec_ctrl->tx_retransmits + 1, 0, 0);
>
> ...but here you do.

See explanation above.

>
> > +             break;
> > +     default:
> > +             cec_transmit_done(hdmi->cec_adap,
> > +                               CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
> > +             break;
> > +     }
> > +}
> > +
> > +static void msm_hdmi_cec_work(struct work_struct *work)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl =
> > +             container_of(work, struct hdmi_cec_ctrl, work);
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&cec_ctrl->lock, flags);

Just a note for myself, I need to avoid calling the cec framework
functions with the spinlock taken.

> > +
> > +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
> > +             msm_hdmi_cec_handle_tx_done(cec_ctrl);
> > +
> > +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
> > +             msm_hdmi_cec_handle_rx_done(cec_ctrl);
> > +
> > +     cec_ctrl->irq_status = 0;
> > +     cec_ctrl->tx_status = 0;
> > +
> > +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> > +}
> > +
> > +void msm_hdmi_cec_irq(struct hdmi *hdmi)
> > +{
> > +     struct hdmi_cec_ctrl *cec_ctrl;
> > +     unsigned long flags;
> > +     u32 int_status;
> > +
> > +     if (!hdmi->cec_adap)
> > +             return;
> > +
> > +     cec_ctrl = hdmi->cec_adap->priv;
> > +
> > +     int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
> > +     if (!(int_status & HDMI_CEC_INT_MASK))
> > +             return;
> > +
> > +     spin_lock_irqsave(&cec_ctrl->lock, flags);
> > +
> > +     if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
> > +             cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
> > +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
> > +     }
> > +
> > +     if (int_status & HDMI_CEC_INT_RX_DONE)
> > +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
> > +
> > +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
> > +
> > +     hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
> > +     queue_work(hdmi->workq, &cec_ctrl->work);
> > +}
> > +
> > +int msm_hdmi_cec_init(struct hdmi *hdmi)
> > +{
> > +     struct platform_device *pdev = hdmi->pdev;
> > +     struct hdmi_cec_ctrl *cec_ctrl;
> > +     struct cec_adapter *cec_adap;
> > +     int ret;
> > +
> > +     cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
> > +     if (!cec_ctrl)
> > +             return -ENOMEM;
> > +
> > +     cec_ctrl->hdmi = hdmi;
> > +     INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
> > +
> > +     cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
> > +                                     cec_ctrl, "msm",
> > +                                     CEC_CAP_DEFAULTS |
> > +                                     CEC_CAP_CONNECTOR_INFO, 1);
> > +     ret = PTR_ERR_OR_ZERO(cec_adap);
> > +     if (ret < 0) {
> > +             kfree(cec_ctrl);
> > +             return ret;
> > +     }
> > +
> > +     /* Set the logical address to Unregistered */
> > +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
> > +
> > +     ret = cec_register_adapter(cec_adap, &pdev->dev);
> > +     if (ret < 0) {
> > +             cec_delete_adapter(cec_adap);
> > +             return ret;
> > +     }
> > +
> > +     hdmi->cec_adap = cec_adap;
> > +
> > +     return 0;
> > +}
> > +
> > +void msm_hdmi_cec_exit(struct hdmi *hdmi)
> > +{
> > +     cec_unregister_adapter(hdmi->cec_adap);
> > +}
> >
>
> Final question: is this CEC device able to transmit messages when the hotplug detect
> pin is low? Some displays pull the HPD low when in Standby, but it is still possible
> to wake them up with a <Image View On> message. It is important to check that.
>
> If this is really not possible, then the CEC_CAP_NEEDS_HPD should be set.

Yes, messages can be sent with HPD low, as long as the HDMI PLL is up.

Regards,
-Arnaud

Re: [PATCH 2/4] drm/msm: add hdmi cec support

[Hans Verkuil]

Hi Arnaud,

My apologies for the long delay in replying, it's been very, very busy lately.
I hope I'll be able to be more responsive going forward.

On 21/04/2023 18:58, Arnaud Vrac wrote:
> Le ven. 21 avr. 2023 à 15:27, Hans Verkuil <hverkuil-cisco@xs4all.nl> a écrit :
>>
>> Hi Arnaud,
>>
>> Some review comments below...
> 
> Hi Hans,
> 
> For context, I first based my work on the fbdev driver from Qualcomm a
> few years ago, on our own CEC framework which does not implement any
> CEC protocol logic (as android does). At the time I verified that the
> messages were matching the electrical and protocol spec, using manual
> tests and a QD882EA analyzer. I also passed HDMI and CEC certs.
> 
> I simply ported this work more recently to a newer kernel and the
> media-cec framework, also checking the port that Qualcomm did later
> on.
> 
>> On 4/18/23 20:10, Arnaud Vrac wrote:
>>> Some Qualcomm SoCs that support HDMI also support CEC, including MSM8996
>>> and MSM8998. The hardware block can handle a single CEC logical address
>>> and broadcast messages.
>>>
>>> Port the CEC driver from downstream msm-4.4 kernel. It has been tested
>>> on MSM8998 and passes the cec-compliance tool tests.
>>
>> Just to verify: did you run the cec-compliance --test-adapter test? That's
>> the important one to verify your own driver.
> 
> Yes, and I also ran the cec-compliance -r 0 with a pulse8 emulating a
> tv on the bus. Here's the result of cec-compliance --test-adapter:
> 
> Find remote devices:
>         Polling: OK
> 
> CEC API:
>         CEC_ADAP_G_CAPS: OK
>         Invalid ioctls: OK
>         CEC_DQEVENT: OK
>         CEC_ADAP_G/S_PHYS_ADDR: OK
>         CEC_ADAP_G/S_LOG_ADDRS: OK
>         CEC_TRANSMIT: OK
>         CEC_RECEIVE: OK
>         CEC_TRANSMIT/RECEIVE (non-blocking): OK (Presumed)
>         CEC_G/S_MODE: OK
>         warn: cec-test-adapter.cpp(1189): Too many transmits (3)
> without receives
>                 SFTs for repeating messages (>= 7): 7: 38, 8: 2
>                 SFTs for newly transmitted messages (>= 5): 6: 2, 7: 17
>                 SFTs for newly transmitted remote messages (>= 5): 6: 20
>         CEC_EVENT_LOST_MSGS: OK
> 
> Network topology:
> [...]
> 
> Total for hdmi_msm device /dev/cec0: 11, Succeeded: 11, Failed: 0, Warnings: 1
> 
>>
>>>
>>> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
>>> ---
>>>  drivers/gpu/drm/msm/Kconfig         |   8 ++
>>>  drivers/gpu/drm/msm/Makefile        |   1 +
>>>  drivers/gpu/drm/msm/hdmi/hdmi.c     |  15 ++
>>>  drivers/gpu/drm/msm/hdmi/hdmi.h     |  18 +++
>>>  drivers/gpu/drm/msm/hdmi/hdmi_cec.c | 280 ++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 322 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
>>> index 85f5ab1d552c4..2a02c74207935 100644
>>> --- a/drivers/gpu/drm/msm/Kconfig
>>> +++ b/drivers/gpu/drm/msm/Kconfig
>>> @@ -165,3 +165,11 @@ config DRM_MSM_HDMI_HDCP
>>>       default y
>>>       help
>>>         Choose this option to enable HDCP state machine
>>> +
>>> +config DRM_MSM_HDMI_CEC
>>> +     bool "Enable HDMI CEC support in MSM DRM driver"
>>> +     depends on DRM_MSM && DRM_MSM_HDMI
>>> +     select CEC_CORE
>>> +     default y
>>> +     help
>>> +       Choose this option to enable CEC support
>>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>>> index 7274c41228ed9..0237a2f219ac2 100644
>>> --- a/drivers/gpu/drm/msm/Makefile
>>> +++ b/drivers/gpu/drm/msm/Makefile
>>> @@ -131,6 +131,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>>>
>>>  msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
>>>
>>> +msm-$(CONFIG_DRM_MSM_HDMI_CEC) += hdmi/hdmi_cec.o
>>>  msm-$(CONFIG_DRM_MSM_HDMI_HDCP) += hdmi/hdmi_hdcp.o
>>>
>>>  msm-$(CONFIG_DRM_MSM_DSI) += dsi/dsi.o \
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> index 3132105a2a433..1dde3890e25c0 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> @@ -11,6 +11,8 @@
>>>  #include <drm/drm_bridge_connector.h>
>>>  #include <drm/drm_of.h>
>>>
>>> +#include <media/cec.h>
>>> +
>>>  #include <sound/hdmi-codec.h>
>>>  #include "hdmi.h"
>>>
>>> @@ -53,6 +55,9 @@ static irqreturn_t msm_hdmi_irq(int irq, void *dev_id)
>>>       if (hdmi->hdcp_ctrl)
>>>               msm_hdmi_hdcp_irq(hdmi->hdcp_ctrl);
>>>
>>> +     /* Process CEC: */
>>> +     msm_hdmi_cec_irq(hdmi);
>>> +
>>>       /* TODO audio.. */
>>>
>>>       return IRQ_HANDLED;
>>> @@ -66,6 +71,8 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>>>        */
>>>       if (hdmi->workq)
>>>               destroy_workqueue(hdmi->workq);
>>> +
>>> +     msm_hdmi_cec_exit(hdmi);
>>>       msm_hdmi_hdcp_destroy(hdmi);
>>>
>>>       if (hdmi->i2c)
>>> @@ -139,6 +146,8 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>>>               hdmi->hdcp_ctrl = NULL;
>>>       }
>>>
>>> +     msm_hdmi_cec_init(hdmi);
>>> +
>>>       return 0;
>>>
>>>  fail:
>>> @@ -198,6 +207,12 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>
>>>       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>>>
>>> +     if (hdmi->cec_adap) {
>>> +             struct cec_connector_info conn_info;
>>> +             cec_fill_conn_info_from_drm(&conn_info, hdmi->connector);
>>> +             cec_s_conn_info(hdmi->cec_adap, &conn_info);
>>> +     }
>>> +
>>>       ret = devm_request_irq(dev->dev, hdmi->irq,
>>>                       msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>>>                       "hdmi_isr", hdmi);
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
>>> index e8dbee50637fa..c639bd87f4b8f 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
>>> @@ -29,6 +29,7 @@ struct hdmi_audio {
>>>  };
>>>
>>>  struct hdmi_hdcp_ctrl;
>>> +struct cec_adapter;
>>>
>>>  struct hdmi {
>>>       struct drm_device *dev;
>>> @@ -73,6 +74,7 @@ struct hdmi {
>>>       struct workqueue_struct *workq;
>>>
>>>       struct hdmi_hdcp_ctrl *hdcp_ctrl;
>>> +     struct cec_adapter *cec_adap;
>>>
>>>       /*
>>>       * spinlock to protect registers shared by different execution
>>> @@ -261,4 +263,20 @@ static inline void msm_hdmi_hdcp_off(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
>>>  static inline void msm_hdmi_hdcp_irq(struct hdmi_hdcp_ctrl *hdcp_ctrl) {}
>>>  #endif
>>>
>>> +/*
>>> + * cec
>>> + */
>>> +#ifdef CONFIG_DRM_MSM_HDMI_CEC
>>> +int msm_hdmi_cec_init(struct hdmi *hdmi);
>>> +void msm_hdmi_cec_exit(struct hdmi *hdmi);
>>> +void msm_hdmi_cec_irq(struct hdmi *hdmi);
>>> +#else
>>> +static inline int msm_hdmi_cec_init(struct hdmi *hdmi)
>>> +{
>>> +     return -ENXIO;
>>> +}
>>> +static inline void msm_hdmi_cec_exit(struct hdmi *hdmi) {}
>>> +static inline void msm_hdmi_cec_irq(struct hdmi *hdmi) {}
>>> +#endif
>>> +
>>>  #endif /* __HDMI_CONNECTOR_H__ */
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_cec.c b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
>>> new file mode 100644
>>> index 0000000000000..51326e493e5da
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_cec.c
>>> @@ -0,0 +1,280 @@
>>> +#include <linux/iopoll.h>
>>> +#include <media/cec.h>
>>> +
>>> +#include "hdmi.h"
>>> +
>>> +#define HDMI_CEC_INT_MASK ( \
>>> +     HDMI_CEC_INT_TX_DONE_MASK | \
>>> +     HDMI_CEC_INT_TX_ERROR_MASK | \
>>> +     HDMI_CEC_INT_RX_DONE_MASK)
>>> +
>>> +struct hdmi_cec_ctrl {
>>> +     struct hdmi *hdmi;
>>> +     struct work_struct work;
>>> +     spinlock_t lock;
>>> +     u32 irq_status;
>>> +     u32 tx_status;
>>> +     u32 tx_retransmits;
>>> +};
>>> +
>>> +static int msm_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>> +{
>>> +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
>>> +     struct hdmi *hdmi = cec_ctrl->hdmi;
>>> +
>>> +     if (enable) {
>>> +             /* timer frequency, 19.2Mhz * 0.05ms / 1000ms = 960 */
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_REFTIMER,
>>> +                        HDMI_CEC_REFTIMER_REFTIMER(960) |
>>> +                        HDMI_CEC_REFTIMER_ENABLE);
>>> +
>>> +             /* read and write timings */
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_RD_RANGE, 0x30AB9888);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_WR_RANGE, 0x888AA888);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_RD_START_RANGE, 0x88888888);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_RD_TOTAL_RANGE, 0x99);
>>> +
>>> +             /* start bit low pulse duration, 3.7ms */
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_RD_ERR_RESP_LO, 74);
>>> +
>>> +             /* signal free time, 7 * 2.4ms */
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_TIME,
>>> +                        HDMI_CEC_TIME_SIGNAL_FREE_TIME(7 * 48) |
>>> +                        HDMI_CEC_TIME_ENABLE);
>>
>> The Signal Free Time changes depending on the situation (3, 5 or 7 bit
>> periods). Does the hardware take care of that, or do you need to update
>> this register in the transmit op as well?
> 
> In case of retries, which are handled by the hardware, I'm not sure if
> the hardware will use the period set in this register or 3. I believe
> it should be 3 otherwise we would have problems when probing addresses
> on a busy bus.
> 
> Otherwise for new message transmissions, the register value is used
> with caveats.
> 
> I've just tried to use set the register value during transmit using
> the signal_free_time parameter, and am getting wrong results. To
> emphasize the problem I set signal_free_time to 3 instead of 5 and 8
> instead of 7, and am getting the following in cec-compliance:
> 
>                 SFTs for repeating messages (>= 7): 3: 10, 4: 6, 8: 14, 9: 6
>                 SFTs for newly transmitted messages (>= 5): 7: 6, 8: 8
> 
> If I instead set signal_free_time to 3 just after receiving a message
> and 8 just after sending, I get better results:
> 
>                 SFTs for repeating messages (>= 7): 8: 26, 9: 10
>                 SFTs for newly transmitted messages (>= 5): 2: 7, 3: 12
> 
> But it's not clear to me why the change is not effective immediatly in
> the first test, so I'm not confortable changing the fixed value of 7
> periods.

See my comment below in the transmit function.

> 
>>
>>> +
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_COMPL_CTL, 0xF);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_WR_CHECK_CONFIG, 0x4);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_RD_FILTER, BIT(0) | (0x7FF << 4));
>>> +
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_INT, HDMI_CEC_INT_MASK);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
>>> +     } else {
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_INT, 0);
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
>>> +             cancel_work_sync(&cec_ctrl->work);
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int msm_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>> +{
>>> +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
>>> +     struct hdmi *hdmi = cec_ctrl->hdmi;
>>> +
>>> +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, logical_addr & 0xF);
>>
>> So to disable the logical address you set this to 0xf, right? Since
>> CEC_LOG_ADDR_INVALID is 0xff.
> 
> Right.
> 
>>
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int msm_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>> +                                   u32 signal_free_time, struct cec_msg *msg)
>>
>> Note that the SFT is passed in as an argument for those hardware devices
>> that do not keep track of it themselves.

What happens if you update the REG_HDMI_CEC_TIME here based on the
signal_free_time argument? Does that improve the cec-compliance results?

>>
>>> +{
>>> +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
>>> +     struct hdmi *hdmi = cec_ctrl->hdmi;
>>> +     u8 retransmits;
>>> +     u32 broadcast;
>>> +     u32 status;
>>> +     int i;
>>> +
>>> +     /* toggle cec in order to flush out bad hw state, if any */
>>> +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, 0);
>>> +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL, HDMI_CEC_CTRL_ENABLE);
>>> +
>>> +     /* flush register writes */
>>> +     wmb();
>>> +
>>> +     retransmits = attempts ? (attempts - 1) : 0;

attempts is guaranteed to be non-zero, so you can just set this to attempts - 1.

>>> +     hdmi_write(hdmi, REG_HDMI_CEC_RETRANSMIT,
>>> +                HDMI_CEC_RETRANSMIT_ENABLE |
>>> +                HDMI_CEC_RETRANSMIT_COUNT(retransmits));
>>> +
>>> +     broadcast = cec_msg_is_broadcast(msg) ? HDMI_CEC_WR_DATA_BROADCAST : 0;
>>> +     for (i = 0; i < msg->len; i++) {
>>> +             hdmi_write(hdmi, REG_HDMI_CEC_WR_DATA,
>>> +                        HDMI_CEC_WR_DATA_DATA(msg->msg[i]) | broadcast);
>>> +     }
>>> +
>>> +     /* check line status */
>>> +     if (read_poll_timeout(hdmi_read, status, !(status & HDMI_CEC_STATUS_BUSY),
>>> +                           5, 1000, false, hdmi, REG_HDMI_CEC_STATUS)) {
>>> +             pr_err("CEC line is busy. Retry failed\n");
>>
>> This doesn't look right. Normally it is the CEC hardware that will wait for the
>> bus to become free, and then it will start the transmit. That is not something
>> you should have to do in the driver. And this waits for just 1 ms, right? That's
>> much too short if a message is currently being received.
>>
>> Is there documentation of the CEC hardware available somewhere? Or can you
>> explain a bit about it?
> 
> I'm not sure why this code is here, it's a check that was done in the
> original Qualcomm driver using a busy loop with a schedule():
> 
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/media/platform/msm/sde/cec/sde_hdmi_cec.c#L174
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/video/fbdev/msm/mdss_hdmi_cec.c#L109
> 
> In practice I don't think I've seen the status being busy, but I
> ported the check in case it's still useful. Maybe an hw engineer from
> Qualcomm can chime in.

I think this code should be dropped.

Normally the transmitter in the hardware will have to wait for the bus to
be free (that's what the Signal Free Time is all about). If the hardware
cannot do that, then the driver needs to be modified so that it can return
here, but kick off a worker thread or something that waits for the bus to
become free. But that is hard to get right.

So in either case this code doesn't belong here, adap_transmit is meant to
be fast.

One option is to create a CEC debugger where you can see what is going
on. See "4.7. Making a CEC debugger" here:

https://linuxtv.org/downloads/v4l-dvb-apis-new/admin-guide/cec.html

You need a way to hook up the CEC pin of an HDMI cable to a GPIO. I
suggest an HDMI breakout board that you can buy. Then you can use that
in combination with the cec-gpio driver to do low-level testing and see
the exact behavior of the CEC transmitter.

While I use a RPi 4B for that, it should work fine with other SoCs.

I can help with instruction on how to use such a setup, if needed.

> 
>>
>>> +             return -EBUSY;
>>> +     }
>>> +
>>> +     cec_ctrl->tx_retransmits = retransmits;
>>> +
>>> +     /* start transmission */
>>> +     hdmi_write(hdmi, REG_HDMI_CEC_CTRL,
>>> +                HDMI_CEC_CTRL_ENABLE |
>>> +                HDMI_CEC_CTRL_SEND_TRIGGER |
>>> +                HDMI_CEC_CTRL_FRAME_SIZE(msg->len) |
>>> +                HDMI_CEC_CTRL_LINE_OE);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void msm_hdmi_cec_adap_free(struct cec_adapter *adap)
>>> +{
>>> +     struct hdmi_cec_ctrl *cec_ctrl = adap->priv;
>>> +
>>> +     cec_ctrl->hdmi->cec_adap = NULL;
>>> +     kfree(cec_ctrl);
>>> +}
>>> +
>>> +static const struct cec_adap_ops msm_hdmi_cec_adap_ops = {
>>> +     .adap_enable = msm_hdmi_cec_adap_enable,
>>> +     .adap_log_addr = msm_hdmi_cec_adap_log_addr,
>>> +     .adap_transmit = msm_hdmi_cec_adap_transmit,
>>> +     .adap_free = msm_hdmi_cec_adap_free,
>>> +};
>>> +
>>> +#define CEC_IRQ_FRAME_WR_DONE 0x01
>>> +#define CEC_IRQ_FRAME_RD_DONE 0x02
>>> +
>>> +static void msm_hdmi_cec_handle_rx_done(struct hdmi_cec_ctrl *cec_ctrl)
>>> +{
>>> +     struct hdmi *hdmi = cec_ctrl->hdmi;
>>> +     struct cec_msg msg = {};
>>> +     u32 data;
>>> +     int i;
>>> +
>>> +     data = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA);
>>> +     msg.len = (data & 0x1f00) >> 8;
>>> +     if (msg.len < 1 || msg.len > CEC_MAX_MSG_SIZE)
>>> +             return;
>>> +
>>> +     msg.msg[0] = data & 0xff;
>>> +     for (i = 1; i < msg.len; i++)
>>> +             msg.msg[i] = hdmi_read(hdmi, REG_HDMI_CEC_RD_DATA) & 0xff;
>>> +
>>> +     cec_received_msg(hdmi->cec_adap, &msg);
>>> +}
>>> +
>>> +static void msm_hdmi_cec_handle_tx_done(struct hdmi_cec_ctrl *cec_ctrl)
>>> +{
>>> +     struct hdmi *hdmi = cec_ctrl->hdmi;
>>> +     u32 tx_status;
>>> +
>>> +     tx_status = (cec_ctrl->tx_status & HDMI_CEC_STATUS_TX_STATUS__MASK) >>
>>> +             HDMI_CEC_STATUS_TX_STATUS__SHIFT;
>>> +
>>> +     switch (tx_status) {
>>> +     case 0:
>>> +             cec_transmit_done(hdmi->cec_adap,
>>> +                               CEC_TX_STATUS_OK, 0, 0, 0, 0);
>>> +             break;
>>> +     case 1:
>>> +             cec_transmit_done(hdmi->cec_adap,
>>> +                               CEC_TX_STATUS_NACK, 0, 1, 0, 0);
>>
>> It's not clear to me who does the retransmits. There are two possibilities:
>> the hardware takes care of that, and so you just get the final result
>> and you OR this status with CEC_TX_STATUS_MAX_RETRIES to indicate that
>> the CEC framework shouldn't attempt to retry.
>>
>> Or the hardware just does a single transmit, and in that case you never
>> supply the CEC_TX_STATUS_MAX_RETRIES and just leave it up to the framework
>> to reissue a transmit.
> 
> After fiddling with the registers, my understanding is the following:
> 
> When arbitration loss happens, the hardware returns the ARB_LOSS
> status without retrying afterwards, and I don't think the hardware
> reports the number of NACKs that were attempted before the arbitration
> loss. So in this case I believe it makes sense to only report the
> arbitration loss to the framework.
> 
> The hardware will otherwise retry up to the number of retransmits
> indicated in the REG_HDMI_CEC_RETRANSMIT register set during transmit
> and return either OK or MAX_RETRIES (=NACK). I haven't seen the NACK
> status (case 1) in practice, even if I disable retransmits in the
> dedicated register.

I wonder if case 1 is actually a catch-all ERROR condition or possibly
it is returned in case of a Low Drive. It's hard to test without proper
test equipment (the cec-gpio driver can inject such error conditions).

If a normal NACK situation results in case 3, then I would suggest
returning CEC_TX_STATUS_ERROR for case 1, with a comment that this is
a guess only. Unless you are able to confirm it based on cec-gpio tests
or documentation.

> 
>>
>> So here you do no supply MAX_RETRIES...
> 
> I chose to handle case 1 as a single NACK answer, if it could still
> happen for some reason.
> 
>>
>>> +             break;
>>> +     case 2:
>>> +             cec_transmit_done(hdmi->cec_adap,
>>> +                               CEC_TX_STATUS_ARB_LOST, 1, 0, 0, 0);
>>
>> ... and also not here...
> 
> See explanation above.

So this needs to be documented: the HW doesn't do retransmits for ARB LOST,
but it does for NACK.

> 
>>
>>> +             break;
>>> +     case 3:
>>> +             cec_transmit_done(hdmi->cec_adap,
>>> +                               CEC_TX_STATUS_MAX_RETRIES |
>>> +                               CEC_TX_STATUS_NACK,
>>> +                               0, cec_ctrl->tx_retransmits + 1, 0, 0);
>>
>> ...but here you do.
> 
> See explanation above.
> 
>>
>>> +             break;
>>> +     default:
>>> +             cec_transmit_done(hdmi->cec_adap,
>>> +                               CEC_TX_STATUS_ERROR, 0, 0, 0, 1);
>>> +             break;
>>> +     }
>>> +}
>>> +
>>> +static void msm_hdmi_cec_work(struct work_struct *work)
>>> +{
>>> +     struct hdmi_cec_ctrl *cec_ctrl =
>>> +             container_of(work, struct hdmi_cec_ctrl, work);
>>> +     unsigned long flags;
>>> +
>>> +     spin_lock_irqsave(&cec_ctrl->lock, flags);
> 
> Just a note for myself, I need to avoid calling the cec framework
> functions with the spinlock taken.
> 
>>> +
>>> +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_WR_DONE)
>>> +             msm_hdmi_cec_handle_tx_done(cec_ctrl);
>>> +
>>> +     if (cec_ctrl->irq_status & CEC_IRQ_FRAME_RD_DONE)
>>> +             msm_hdmi_cec_handle_rx_done(cec_ctrl);
>>> +
>>> +     cec_ctrl->irq_status = 0;
>>> +     cec_ctrl->tx_status = 0;
>>> +
>>> +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
>>> +}
>>> +
>>> +void msm_hdmi_cec_irq(struct hdmi *hdmi)
>>> +{
>>> +     struct hdmi_cec_ctrl *cec_ctrl;
>>> +     unsigned long flags;
>>> +     u32 int_status;
>>> +
>>> +     if (!hdmi->cec_adap)
>>> +             return;
>>> +
>>> +     cec_ctrl = hdmi->cec_adap->priv;
>>> +
>>> +     int_status = hdmi_read(hdmi, REG_HDMI_CEC_INT);
>>> +     if (!(int_status & HDMI_CEC_INT_MASK))
>>> +             return;
>>> +
>>> +     spin_lock_irqsave(&cec_ctrl->lock, flags);
>>> +
>>> +     if (int_status & (HDMI_CEC_INT_TX_DONE | HDMI_CEC_INT_TX_ERROR)) {
>>> +             cec_ctrl->tx_status = hdmi_read(hdmi, REG_HDMI_CEC_STATUS);
>>> +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_WR_DONE;
>>> +     }
>>> +
>>> +     if (int_status & HDMI_CEC_INT_RX_DONE)
>>> +             cec_ctrl->irq_status |= CEC_IRQ_FRAME_RD_DONE;
>>> +
>>> +     spin_unlock_irqrestore(&cec_ctrl->lock, flags);
>>> +
>>> +     hdmi_write(hdmi, REG_HDMI_CEC_INT, int_status);
>>> +     queue_work(hdmi->workq, &cec_ctrl->work);
>>> +}
>>> +
>>> +int msm_hdmi_cec_init(struct hdmi *hdmi)
>>> +{
>>> +     struct platform_device *pdev = hdmi->pdev;
>>> +     struct hdmi_cec_ctrl *cec_ctrl;
>>> +     struct cec_adapter *cec_adap;
>>> +     int ret;
>>> +
>>> +     cec_ctrl = kzalloc(sizeof (*cec_ctrl), GFP_KERNEL);
>>> +     if (!cec_ctrl)
>>> +             return -ENOMEM;
>>> +
>>> +     cec_ctrl->hdmi = hdmi;
>>> +     INIT_WORK(&cec_ctrl->work, msm_hdmi_cec_work);
>>> +
>>> +     cec_adap = cec_allocate_adapter(&msm_hdmi_cec_adap_ops,
>>> +                                     cec_ctrl, "msm",
>>> +                                     CEC_CAP_DEFAULTS |
>>> +                                     CEC_CAP_CONNECTOR_INFO, 1);
>>> +     ret = PTR_ERR_OR_ZERO(cec_adap);
>>> +     if (ret < 0) {
>>> +             kfree(cec_ctrl);
>>> +             return ret;
>>> +     }
>>> +
>>> +     /* Set the logical address to Unregistered */
>>> +     hdmi_write(hdmi, REG_HDMI_CEC_ADDR, 0xf);
>>> +
>>> +     ret = cec_register_adapter(cec_adap, &pdev->dev);
>>> +     if (ret < 0) {
>>> +             cec_delete_adapter(cec_adap);
>>> +             return ret;
>>> +     }
>>> +
>>> +     hdmi->cec_adap = cec_adap;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +void msm_hdmi_cec_exit(struct hdmi *hdmi)
>>> +{
>>> +     cec_unregister_adapter(hdmi->cec_adap);
>>> +}
>>>
>>
>> Final question: is this CEC device able to transmit messages when the hotplug detect
>> pin is low? Some displays pull the HPD low when in Standby, but it is still possible
>> to wake them up with a <Image View On> message. It is important to check that.
>>
>> If this is really not possible, then the CEC_CAP_NEEDS_HPD should be set.
> 
> Yes, messages can be sent with HPD low, as long as the HDMI PLL is up.

Nice!

Regards,

	Hans

[PATCH 3/4] drm/msm: expose edid to hdmi cec adapter

[Arnaud Vrac]

When edid has been read after hpd, pass it to the cec adapter so that it
can extract the physical address of the device on the cec bus.
Invalidate the physical address when hpd is low.

Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 ++
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 17 +++++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 9b1391d27ed39..efc3bd4908e83 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -7,6 +7,7 @@
 #include <linux/delay.h>
 #include <drm/drm_bridge_connector.h>
 #include <drm/drm_edid.h>
+#include <media/cec.h>
 
 #include "msm_kms.h"
 #include "hdmi.h"
@@ -256,6 +257,7 @@ static struct edid *msm_hdmi_bridge_get_edid(struct drm_bridge *bridge,
 	hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl | HDMI_CTRL_ENABLE);
 
 	edid = drm_get_edid(connector, hdmi->i2c);
+	cec_s_phys_addr_from_edid(hdmi->cec_adap, edid);
 
 	hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl);
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index bfa827b479897..cb3eb2625ff63 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -7,6 +7,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/pinctrl/consumer.h>
+#include <media/cec.h>
 
 #include "msm_kms.h"
 #include "hdmi.h"
@@ -230,15 +231,17 @@ enum drm_connector_status msm_hdmi_bridge_detect(
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	enum drm_connector_status stat_gpio, stat_reg;
+	enum drm_connector_status status, stat_gpio, stat_reg;
 	int retry = 20;
 
 	/*
 	 * some platforms may not have hpd gpio. Rely only on the status
 	 * provided by REG_HDMI_HPD_INT_STATUS in this case.
 	 */
-	if (!hdmi->hpd_gpiod)
-		return detect_reg(hdmi);
+	if (!hdmi->hpd_gpiod) {
+		status = detect_reg(hdmi);
+		goto out;
+	}
 
 	do {
 		stat_gpio = detect_gpio(hdmi);
@@ -259,5 +262,11 @@ enum drm_connector_status msm_hdmi_bridge_detect(
 		DBG("hpd gpio tells us: %d", stat_gpio);
 	}
 
-	return stat_gpio;
+	status = stat_gpio;
+
+out:
+	if (!status)
+		cec_phys_addr_invalidate(hdmi->cec_adap);
+
+	return status;
 }

-- 
2.40.0

Re: [PATCH 3/4] drm/msm: expose edid to hdmi cec adapter

[Dmitry Baryshkov]

On 18/04/2023 21:10, Arnaud Vrac wrote:
> When edid has been read after hpd, pass it to the cec adapter so that it
> can extract the physical address of the device on the cec bus.
> Invalidate the physical address when hpd is low.

If there is another bridge in a chain (e.g. display-connector) which 
handles HPD, then the msm_hdmi_bridge_detect() might get skipped. Please 
also add the hpd_notify() callback which invalidate the physical 
address. See adv7511, which does that both in its own HPD path and in 
the hpd_notify().

> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 ++
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 17 +++++++++++++----
>   2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 9b1391d27ed39..efc3bd4908e83 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -7,6 +7,7 @@
>   #include <linux/delay.h>
>   #include <drm/drm_bridge_connector.h>
>   #include <drm/drm_edid.h>
> +#include <media/cec.h>
>   
>   #include "msm_kms.h"
>   #include "hdmi.h"
> @@ -256,6 +257,7 @@ static struct edid *msm_hdmi_bridge_get_edid(struct drm_bridge *bridge,
>   	hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl | HDMI_CTRL_ENABLE);
>   
>   	edid = drm_get_edid(connector, hdmi->i2c);
> +	cec_s_phys_addr_from_edid(hdmi->cec_adap, edid);
>   
>   	hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl);
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index bfa827b479897..cb3eb2625ff63 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -7,6 +7,7 @@
>   #include <linux/delay.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/pinctrl/consumer.h>
> +#include <media/cec.h>
>   
>   #include "msm_kms.h"
>   #include "hdmi.h"
> @@ -230,15 +231,17 @@ enum drm_connector_status msm_hdmi_bridge_detect(
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	enum drm_connector_status stat_gpio, stat_reg;
> +	enum drm_connector_status status, stat_gpio, stat_reg;
>   	int retry = 20;
>   
>   	/*
>   	 * some platforms may not have hpd gpio. Rely only on the status
>   	 * provided by REG_HDMI_HPD_INT_STATUS in this case.
>   	 */
> -	if (!hdmi->hpd_gpiod)
> -		return detect_reg(hdmi);
> +	if (!hdmi->hpd_gpiod) {
> +		status = detect_reg(hdmi);
> +		goto out;
> +	}
>   
>   	do {
>   		stat_gpio = detect_gpio(hdmi);
> @@ -259,5 +262,11 @@ enum drm_connector_status msm_hdmi_bridge_detect(
>   		DBG("hpd gpio tells us: %d", stat_gpio);
>   	}
>   
> -	return stat_gpio;
> +	status = stat_gpio;
> +
> +out:
> +	if (!status)
> +		cec_phys_addr_invalidate(hdmi->cec_adap);
> +
> +	return status;
>   }
> 

-- 
With best wishes
Dmitry

[PATCH 4/4] arm64: dts: qcom: msm8998: add hdmi cec pinctrl nodes

[Arnaud Vrac]

HDMI is not enabled yet on msm8998 so the pinctrl nodes are not
used.

Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index b150437a83558..fb4aa376ef117 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -1312,6 +1312,20 @@ blsp2_i2c6_sleep: blsp2-i2c6-sleep-state {
 				drive-strength = <2>;
 				bias-pull-up;
 			};
+
+			hdmi_cec_default: hdmi-cec-default-state {
+				pins = "gpio31";
+				function = "hdmi_cec";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			hdmi_cec_sleep: hdmi-cec-sleep-state {
+				pins = "gpio31";
+				function = "hdmi_cec";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
 		};
 
 		remoteproc_mss: remoteproc@4080000 {

-- 
2.40.0

Re: [PATCH 4/4] arm64: dts: qcom: msm8998: add hdmi cec pinctrl nodes

[Konrad Dybcio]

On 18.04.2023 20:10, Arnaud Vrac wrote:
> HDMI is not enabled yet on msm8998 so the pinctrl nodes are not
> used.
> 
> Signed-off-by: Arnaud Vrac <avrac@freebox.fr>
> ---
Are they supposed to be identical?

Konrad
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index b150437a83558..fb4aa376ef117 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -1312,6 +1312,20 @@ blsp2_i2c6_sleep: blsp2-i2c6-sleep-state {
>  				drive-strength = <2>;
>  				bias-pull-up;
>  			};
> +
> +			hdmi_cec_default: hdmi-cec-default-state {
> +				pins = "gpio31";
> +				function = "hdmi_cec";
> +				drive-strength = <2>;
> +				bias-pull-up;
> +			};
> +
> +			hdmi_cec_sleep: hdmi-cec-sleep-state {
> +				pins = "gpio31";
> +				function = "hdmi_cec";
> +				drive-strength = <2>;
> +				bias-pull-up;
> +			};
>  		};
>  
>  		remoteproc_mss: remoteproc@4080000 {
>