Re: [PATCH v3 1/2] drm/ast: Add BMC virtual connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/07/2023 12:44, Thomas Zimmermann wrote:
Hi,

thanks for this patch.

Am 12.07.23 um 10:35 schrieb Jocelyn Falempe:
Most aspeed devices have a BMC, which allows to remotely see the screen.
Also in the common use case, those servers don't have a display connected.
So add a Virtual connector, to reflect that even if no display is
connected, the framebuffer can still be seen remotely.
This prepares the work to implement a detect_ctx() for the Display port
connector.

Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---
  drivers/gpu/drm/ast/ast_drv.h  |  4 ++
  drivers/gpu/drm/ast/ast_mode.c | 67 ++++++++++++++++++++++++++++++++++
  2 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 3f6e0c984523..c9659e72002f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -214,6 +214,10 @@ struct ast_device {
              struct drm_encoder encoder;
              struct drm_connector connector;
          } astdp;
+        struct {
+            struct drm_encoder encoder;
+            struct drm_connector connector;
+        } bmc;
      } output;
      bool support_wide_screen;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index f711d592da52..8896b22eb5cf 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1735,6 +1735,70 @@ static int ast_astdp_output_init(struct ast_device *ast)
      return 0;
  }
+/*
+ * BMC virtual Connector
+ */
+
+static int ast_bmc_connector_helper_get_modes(struct drm_connector *connector)
+{
+    return drm_add_modes_noedid(connector, 1024, 768);

That's probably too small. The CRTC lists resolutions up to 1920x1200. Returning 1024x768 could easily filter out those higher-res modes.

The BMC can probably just use whatever the CRTC offers. So rather call drm_add_modes_noedid() with 4096x4096. At 32 bpp, this covers the max memory of 64 MiB.  The CRTC will filter out unsupported modes.

Thanks for pointing this, I didn't realize it will prevent higher resolutions. With this change, the bmc resolution becomes 1920x1200 (with "disabled vga" and no DP connected), which is much nicer.

With vga, it stays at 1024x768. So maybe adding a .detect_ctx() for vga too would be worth it. But that's for another series.



+}
+
+static const struct drm_connector_helper_funcs ast_bmc_connector_helper_funcs = {
+    .get_modes = ast_bmc_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs ast_bmc_connector_funcs = {
+    .reset = drm_atomic_helper_connector_reset,
+    .fill_modes = drm_helper_probe_single_connector_modes,
+    .destroy = drm_connector_cleanup,
+    .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+    .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int ast_bmc_connector_init(struct drm_device *dev,
+                  struct drm_connector *connector)
+{
+    int ret;
+
+    ret = drm_connector_init(dev, connector, &ast_bmc_connector_funcs,
+                 DRM_MODE_CONNECTOR_VIRTUAL);
+    if (ret)
+        return ret;
+
+    drm_connector_helper_add(connector, &ast_bmc_connector_helper_funcs);
+
+    connector->interlace_allowed = 0;
+    connector->doublescan_allowed = 0;
+    connector->polled = 0;

It's zero-allocated memory. Please don't clear these fields manually. (I know that ast doesn't get this right.)

Done


+
+    return 0;
+}
+
+static int ast_bmc_output_init(struct ast_device *ast)
+{
+    struct drm_device *dev = &ast->base;
+    struct drm_crtc *crtc = &ast->crtc;
+    struct drm_encoder *encoder = &ast->output.bmc.encoder;
+    struct drm_connector *connector = &ast->output.bmc.connector;
+    int ret;
+
+    ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);

Adding the simple_encoder helper was a mistake. Please open-code its functionality in ast. (Also something that ast currently does not.)

ok, it's simple enough to call drm_encoder_init() directly.


+    if (ret)
+        return ret;
+    encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+    ret = ast_bmc_connector_init(dev, connector);

Maybe just inline this call. It's simple enough.
Done


Best regards
Thomas

Thanks for the review, I will push a v4 shortly.

--

Jocelyn


+    if (ret)
+        return ret;
+
+    ret = drm_connector_attach_encoder(connector, encoder);
+    if (ret)
+        return ret;
+
+    return 0;
+}
+
  /*
   * Mode config
   */
@@ -1842,6 +1906,9 @@ int ast_mode_config_init(struct ast_device *ast)
          if (ret)
              return ret;
      }
+    ret = ast_bmc_output_init(ast);
+    if (ret)
+        return ret;
      drm_mode_config_reset(dev);

base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux