Re: [PATCH v2 6/6] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

2021-10-18 Thread Neil Armstrong 
Hi,

On 16/10/2021 00:34, Martin Blumenstingl wrote:
> Hi Neil, Hi Sam,
> 
> On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  
> wrote:
> [...]
>> +static const struct drm_bridge_funcs meson_encoder_cvbs_bridge_funcs = {
>> +   .attach = meson_encoder_cvbs_attach,
>> +   .enable = meson_encoder_cvbs_enable,
>> +   .disable = meson_encoder_cvbs_disable,
>> +   .mode_valid = meson_encoder_cvbs_mode_valid,
>> +   .get_modes = meson_encoder_cvbs_get_modes,
>> +   .atomic_enable = meson_encoder_cvbs_atomic_enable,
> I did some testing on boards where u-boot doesn't initialize the video 
> outputs.
> It seems that meson_encoder_cvbs_enable() is never called with this patch.
> meson_encoder_cvbs_atomic_enable() is called though.
> 
> From what I can see in drm_bridge.c [0] this is even expected.
> Does this mean that we need to move all logic from .enable to
> .atomic_enable (and same with .disable -> moving that logic to
> .atomic_disable)?
> 
> The same comment applies to the HDMI patch.

Good point, I'll fix that for both patches !

Neil

> 
> 
> Best regards,
> Martin
> 
> 
> [0] 
> https://elixir.bootlin.com/linux/v5.15-rc5/source/drivers/gpu/drm/drm_bridge.c#L717
>

Re: [PATCH v2 6/6] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

2021-10-15 Thread Martin Blumenstingl 
Hi Neil, Hi Sam,

On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
[...]
> +static const struct drm_bridge_funcs meson_encoder_cvbs_bridge_funcs = {
> +   .attach = meson_encoder_cvbs_attach,
> +   .enable = meson_encoder_cvbs_enable,
> +   .disable = meson_encoder_cvbs_disable,
> +   .mode_valid = meson_encoder_cvbs_mode_valid,
> +   .get_modes = meson_encoder_cvbs_get_modes,
> +   .atomic_enable = meson_encoder_cvbs_atomic_enable,
I did some testing on boards where u-boot doesn't initialize the video outputs.
It seems that meson_encoder_cvbs_enable() is never called with this patch.
meson_encoder_cvbs_atomic_enable() is called though.

>From what I can see in drm_bridge.c [0] this is even expected.
Does this mean that we need to move all logic from .enable to
.atomic_enable (and same with .disable -> moving that logic to
.atomic_disable)?

The same comment applies to the HDMI patch.


Best regards,
Martin


[0] 
https://elixir.bootlin.com/linux/v5.15-rc5/source/drivers/gpu/drm/drm_bridge.c#L717

Re: [PATCH v2 6/6] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

2021-10-15 Thread Martin Blumenstingl 
On Fri, Oct 15, 2021 at 4:11 PM Neil Armstrong  wrote:
>
> Drop the local connector and move all callback to bridge funcs in order
> to leverage the generic CVBS display connector.
>
> This will also permit adding custom cvbs connectors for ADC based HPD
> detection on some Amlogic SoC based boards.
>
> Signed-off-by: Neil Armstrong 
> Acked-by: Sam Ravnborg 
Acked-by: Martin Blumenstingl

[PATCH v2 6/6] drm/meson: encoder_cvbs: switch to bridge with ATTACH_NO_CONNECTOR

2021-10-15 Thread Neil Armstrong 
Drop the local connector and move all callback to bridge funcs in order
to leverage the generic CVBS display connector.

This will also permit adding custom cvbs connectors for ADC based HPD
detection on some Amlogic SoC based boards.

Signed-off-by: Neil Armstrong 
Acked-by: Sam Ravnborg 
---
 drivers/gpu/drm/meson/meson_encoder_cvbs.c | 208 ++---
 1 file changed, 103 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c 
b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
index 01024c5f610c..5b9704d78cf9 100644
--- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
@@ -13,10 +13,12 @@
 #include 
 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 
 #include "meson_registers.h"
 #include "meson_vclk.h"
@@ -30,14 +32,13 @@
 
 struct meson_encoder_cvbs {
struct drm_encoder  encoder;
-   struct drm_connectorconnector;
+   struct drm_bridge   bridge;
+   struct drm_bridge   *next_bridge;
struct meson_drm*priv;
 };
-#define encoder_to_meson_encoder_cvbs(x) \
-   container_of(x, struct meson_encoder_cvbs, encoder)
 
-#define connector_to_meson_encoder_cvbs(x) \
-   container_of(x, struct meson_encoder_cvbs, connector)
+#define bridge_to_meson_encoder_cvbs(x) \
+   container_of(x, struct meson_encoder_cvbs, bridge)
 
 /* Supported Modes */
 
@@ -81,32 +82,31 @@ meson_cvbs_get_mode(const struct drm_display_mode *req_mode)
return NULL;
 }
 
-/* Connector */
-
-static void meson_cvbs_connector_destroy(struct drm_connector *connector)
+static int meson_encoder_cvbs_attach(struct drm_bridge *bridge,
+enum drm_bridge_attach_flags flags)
 {
-   drm_connector_cleanup(connector);
-}
+   struct meson_encoder_cvbs *meson_encoder_cvbs =
+   bridge_to_meson_encoder_cvbs(bridge);
 
-static enum drm_connector_status
-meson_cvbs_connector_detect(struct drm_connector *connector, bool force)
-{
-   /* FIXME: Add load-detect or jack-detect if possible */
-   return connector_status_connected;
+   return drm_bridge_attach(bridge->encoder, 
meson_encoder_cvbs->next_bridge,
+_encoder_cvbs->bridge, flags);
 }
 
-static int meson_cvbs_connector_get_modes(struct drm_connector *connector)
+static int meson_encoder_cvbs_get_modes(struct drm_bridge *bridge,
+   struct drm_connector *connector)
 {
-   struct drm_device *dev = connector->dev;
+   struct meson_encoder_cvbs *meson_encoder_cvbs =
+   bridge_to_meson_encoder_cvbs(bridge);
+   struct meson_drm *priv = meson_encoder_cvbs->priv;
struct drm_display_mode *mode;
int i;
 
for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
struct meson_cvbs_mode *meson_mode = _cvbs_modes[i];
 
-   mode = drm_mode_duplicate(dev, _mode->mode);
+   mode = drm_mode_duplicate(priv->drm, _mode->mode);
if (!mode) {
-   DRM_ERROR("Failed to create a new display mode\n");
+   dev_err(priv->dev, "Failed to create a new display 
mode\n");
return 0;
}
 
@@ -116,40 +116,18 @@ static int meson_cvbs_connector_get_modes(struct 
drm_connector *connector)
return i;
 }
 
-static int meson_cvbs_connector_mode_valid(struct drm_connector *connector,
-  struct drm_display_mode *mode)
+static int meson_encoder_cvbs_mode_valid(struct drm_bridge *bridge,
+   const struct drm_display_info 
*display_info,
+   const struct drm_display_mode *mode)
 {
-   /* Validate the modes added in get_modes */
-   return MODE_OK;
-}
-
-static const struct drm_connector_funcs meson_cvbs_connector_funcs = {
-   .detect = meson_cvbs_connector_detect,
-   .fill_modes = drm_helper_probe_single_connector_modes,
-   .destroy= meson_cvbs_connector_destroy,
-   .reset  = drm_atomic_helper_connector_reset,
-   .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-   .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
-};
-
-static const
-struct drm_connector_helper_funcs meson_cvbs_connector_helper_funcs = {
-   .get_modes  = meson_cvbs_connector_get_modes,
-   .mode_valid = meson_cvbs_connector_mode_valid,
-};
-
-/* Encoder */
+   if (meson_cvbs_get_mode(mode))
+   return MODE_OK;
 
-static void meson_encoder_cvbs_encoder_destroy(struct drm_encoder *encoder)
-{
-   drm_encoder_cleanup(encoder);
+   return MODE_BAD;
 }
 
-static const struct drm_encoder_funcs meson_encoder_cvbs_encoder_funcs = {
-   .destroy=