Re: [PATCH v2 4/6] drm/meson: encoder_hdmi: switch to bridge DRM_BRIDGE_ATTACH_NO_CONNECTOR

2021-10-18 Thread Neil Armstrong 
Hi,

On 16/10/2021 00:07, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  
> wrote:
>>
>> This implements the necessary change to no more use the embedded
>> connector in dw-hdmi and use the dedicated bridge connector driver
>> by passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to the bridge attach call.
>>
>> The necessary connector properties are added to handle the same
>> functionalities as the embedded dw-hdmi connector, i.e. the HDR
>> metadata, the CEC notifier & other flags.
>>
>> The dw-hdmi output_port is set to 1 in order to look for a connector
>> next bridge in order to get DRM_BRIDGE_ATTACH_NO_CONNECTOR working.
>>
>> Signed-off-by: Neil Armstrong 
>> Acked-by: Sam Ravnborg 
> another great piece which helps a lot with HDMI support for the 32-bit SoCs!
> I have one question below - but regardless of the answer there this gets my:
> Acked-by: Martin Blumenstingl 
> 
> [...]
>> +   pdev = of_find_device_by_node(remote);
> I am wondering if we should use something like:
> encoder_hdmi->cec_notifier_pdev
> 
>> +   if (pdev) {
>> +   struct cec_connector_info conn_info;
>> +   struct cec_notifier *notifier;
>> +
>> +   cec_fill_conn_info_from_drm(_info, 
>> meson_encoder_hdmi->connector);
>> +
>> +   notifier = cec_notifier_conn_register(>dev, NULL, 
>> _info);
>> +   if (!notifier)
>> +   return -ENOMEM;
>> +
>> +   meson_encoder_hdmi->cec_notifier = notifier;
>> +   }
> and then move this logic to meson_encoder_hdmi_attach()

We can't because we create the connector after the attach.

> This would be important if .detach() and .attach() can be called
> multiple times (for example during suspend and resume). But I am not
> sure if that's a supported use-case.

Attach for now will only be done once at probe, and detach only a remove,
we don't change the bridge chaining while suspend/resume, we only reset the
pipeline state and put the old state back when resuming.

Neil

> 
> 
> Best regards,
> Martin
>

Re: [PATCH v2 4/6] drm/meson: encoder_hdmi: switch to bridge DRM_BRIDGE_ATTACH_NO_CONNECTOR

2021-10-15 Thread Martin Blumenstingl 
Hi Neil,

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> This implements the necessary change to no more use the embedded
> connector in dw-hdmi and use the dedicated bridge connector driver
> by passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to the bridge attach call.
>
> The necessary connector properties are added to handle the same
> functionalities as the embedded dw-hdmi connector, i.e. the HDR
> metadata, the CEC notifier & other flags.
>
> The dw-hdmi output_port is set to 1 in order to look for a connector
> next bridge in order to get DRM_BRIDGE_ATTACH_NO_CONNECTOR working.
>
> Signed-off-by: Neil Armstrong 
> Acked-by: Sam Ravnborg 
another great piece which helps a lot with HDMI support for the 32-bit SoCs!
I have one question below - but regardless of the answer there this gets my:
Acked-by: Martin Blumenstingl 

[...]
> +   pdev = of_find_device_by_node(remote);
I am wondering if we should use something like:
encoder_hdmi->cec_notifier_pdev

> +   if (pdev) {
> +   struct cec_connector_info conn_info;
> +   struct cec_notifier *notifier;
> +
> +   cec_fill_conn_info_from_drm(_info, 
> meson_encoder_hdmi->connector);
> +
> +   notifier = cec_notifier_conn_register(>dev, NULL, 
> _info);
> +   if (!notifier)
> +   return -ENOMEM;
> +
> +   meson_encoder_hdmi->cec_notifier = notifier;
> +   }
and then move this logic to meson_encoder_hdmi_attach()
This would be important if .detach() and .attach() can be called
multiple times (for example during suspend and resume). But I am not
sure if that's a supported use-case.


Best regards,
Martin

[PATCH v2 4/6] drm/meson: encoder_hdmi: switch to bridge DRM_BRIDGE_ATTACH_NO_CONNECTOR

2021-10-15 Thread Neil Armstrong 
This implements the necessary change to no more use the embedded
connector in dw-hdmi and use the dedicated bridge connector driver
by passing DRM_BRIDGE_ATTACH_NO_CONNECTOR to the bridge attach call.

The necessary connector properties are added to handle the same
functionalities as the embedded dw-hdmi connector, i.e. the HDR
metadata, the CEC notifier & other flags.

The dw-hdmi output_port is set to 1 in order to look for a connector
next bridge in order to get DRM_BRIDGE_ATTACH_NO_CONNECTOR working.

Signed-off-by: Neil Armstrong 
Acked-by: Sam Ravnborg 
---
 drivers/gpu/drm/meson/Kconfig  |  2 +
 drivers/gpu/drm/meson/meson_dw_hdmi.c  |  1 +
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 81 +-
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig
index 9f9281dd49f8..a4e1ed96e5e8 100644
--- a/drivers/gpu/drm/meson/Kconfig
+++ b/drivers/gpu/drm/meson/Kconfig
@@ -6,9 +6,11 @@ config DRM_MESON
select DRM_KMS_HELPER
select DRM_KMS_CMA_HELPER
select DRM_GEM_CMA_HELPER
+   select DRM_DISPLAY_CONNECTOR
select VIDEOMODE_HELPERS
select REGMAP_MMIO
select MESON_CANVAS
+   select CEC_CORE if CEC_NOTIFIER
 
 config DRM_MESON_DW_HDMI
tristate "HDMI Synopsys Controller support for Amlogic Meson Display"
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index fb540a503efe..5cd2b2ebbbd3 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -803,6 +803,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct 
device *master,
dw_plat_data->input_bus_encoding = V4L2_YCBCR_ENC_709;
dw_plat_data->ycbcr_420_allowed = true;
dw_plat_data->disable_cec = true;
+   dw_plat_data->output_port = 1;
 
if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c 
b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 971da662c954..32f52f1c423b 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -14,8 +14,11 @@
 #include 
 #include 
 
+#include 
+
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -33,8 +36,10 @@ struct meson_encoder_hdmi {
struct drm_encoder encoder;
struct drm_bridge bridge;
struct drm_bridge *next_bridge;
+   struct drm_connector *connector;
struct meson_drm *priv;
unsigned long output_bus_fmt;
+   struct cec_notifier *cec_notifier;
 };
 
 #define bridge_to_meson_encoder_hdmi(x) \
@@ -49,6 +54,14 @@ static int meson_encoder_hdmi_attach(struct drm_bridge 
*bridge,
 _hdmi->bridge, flags);
 }
 
+static void meson_encoder_hdmi_detach(struct drm_bridge *bridge)
+{
+   struct meson_encoder_hdmi *encoder_hdmi = 
bridge_to_meson_encoder_hdmi(bridge);
+
+   cec_notifier_conn_unregister(encoder_hdmi->cec_notifier);
+   encoder_hdmi->cec_notifier = NULL;
+}
+
 static void meson_encoder_hdmi_enable(struct drm_bridge *bridge)
 {
struct meson_encoder_hdmi *encoder_hdmi = 
bridge_to_meson_encoder_hdmi(bridge);
@@ -302,11 +315,32 @@ static int meson_encoder_hdmi_atomic_check(struct 
drm_bridge *bridge,
return 0;
 }
 
+static void meson_encoder_hdmi_hpd_notify(struct drm_bridge *bridge,
+ enum drm_connector_status status)
+{
+   struct meson_encoder_hdmi *encoder_hdmi = 
bridge_to_meson_encoder_hdmi(bridge);
+   struct edid *edid;
+
+   if (!encoder_hdmi->cec_notifier)
+   return;
+
+   if (status == connector_status_connected) {
+   edid = drm_bridge_get_edid(encoder_hdmi->next_bridge, 
encoder_hdmi->connector);
+   if (!edid)
+   return;
+
+   
cec_notifier_set_phys_addr_from_edid(encoder_hdmi->cec_notifier, edid);
+   } else
+   cec_notifier_phys_addr_invalidate(encoder_hdmi->cec_notifier);
+}
+
 static const struct drm_bridge_funcs meson_encoder_hdmi_bridge_funcs = {
.attach = meson_encoder_hdmi_attach,
+   .detach = meson_encoder_hdmi_detach,
.enable = meson_encoder_hdmi_enable,
.disable = meson_encoder_hdmi_disable,
.mode_valid = meson_encoder_hdmi_mode_valid,
+   .hpd_notify = meson_encoder_hdmi_hpd_notify,
.atomic_enable = meson_encoder_hdmi_atomic_enable,
.atomic_get_input_bus_fmts = meson_encoder_hdmi_get_inp_bus_fmts,
.atomic_check = meson_encoder_hdmi_atomic_check,
@@ -318,6 +352,7 @@ static const struct drm_bridge_funcs 
meson_encoder_hdmi_bridge_funcs = {
 int meson_encoder_hdmi_init(struct meson_drm *priv)
 {
struct meson_encoder_hdmi *meson_encoder_hdmi;
+   struct platform_device *pdev;