Probing modes and validating them

[Jose Abreu]

++ Daniel

++ Dave


On 21-04-2017 13:59, Jose Abreu wrote:
> Hi All,
>
>
> Is there any callback available, at the crtc level, that can
> validate the probed modes before making them available for
> userspace? I mean: I know that there is connector->mode_valid(),
> but I couldn't find any equivalent crtc->mode_valid(). I found
> mode_fixup() but this is called after giving the mode to
> userspace, which is not what I need.
>
> For reference here is the situation I'm facing:
>
> ARCPGU is a DRM driver that uses ADV7511 as connector/bridge. The
> PGU encodes raw video into a stream that ADV can understands and
> in order to do this it needs a pixel clock. Currently this pixel
> clock value is fixed, so technically arcpgu driver supports only
> one video mode. There is a patch currently on discussion that
> adds more supported frequencies for arcpgu, but there are still
> some frequencies that will not be supported. This is NOT a
> limitation in ADV7511, so it doesn't make sense to limit the
> available modes in adv, we could instead limit the modes in the
> crtc level (i.e. the pgu).
>
> In order to know if a mode can be displayed or not it is as
> simple as call clk_round_rate() and check if the returned
> frequency is the same. But in order to do this I need some sort
> of callback that is called at the crtc level and before
> delivering modes to userspace.
>
> I believe this would be a good addition to arcpgu because this
> way we wouldn't "fool" userspace by delivering modes that will
> fail in atomic check. The user may still specify manually a mode,
> this will still fail in atomic check, but the difference is that
> this mode will not be in the probed list.
>
>
> Best regards,
>
> Jose Miguel Abreu
>

With this simple patch I could fix my problem, what do you think?
Is this ok to be added? I guess some connectors may not have the
crtc linked at probbing stage but this is handled.

>From 252b7cb5ad9999eaf16be95988d17468eea2895b Mon Sep 17 00:00:00
2001
Message-Id:
<252b7cb5ad9999eaf16be95988d17468eea2895b.1492781127.git.joabreu at synopsys.com>
From: Jose Abreu <joabreu at synopsys.com>
Date: Fri, 21 Apr 2017 14:24:16 +0100
Subject: [PATCH] drm: Introduce crtc->mode_valid() callback

Introduce a new crtc callback which validates the probbed modes,
just like connector->mode_valid() does.

Signed-off-by: Jose Abreu <joabreu at synopsys.com>
---
 drivers/gpu/drm/arc/arcpgu_crtc.c        | 14 ++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c       | 12 ++++++++++++
 include/drm/drm_modeset_helper_vtables.h |  3 +++
 3 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c
b/drivers/gpu/drm/arc/arcpgu_crtc.c
index 7130b04..e43e8d6 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -63,6 +63,19 @@ static void arc_pgu_set_pxl_fmt(struct
drm_crtc *crtc)
     .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
+enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
+                         struct drm_display_mode *mode)
+{
+    struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
+    long rate, clk_rate = mode->clock * 1000;
+
+    rate = clk_round_rate(arcpgu->clk, clk_rate);
+    if (rate != clk_rate)
+        return MODE_NOCLOCK;
+
+    return MODE_OK;
+}
+
 static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
     struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
@@ -157,6 +170,7 @@ static void arc_pgu_crtc_atomic_begin(struct
drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs
arc_pgu_crtc_helper_funcs = {
+    .mode_valid    = arc_pgu_crtc_mode_valid,
     .mode_set    = drm_helper_crtc_mode_set,
     .mode_set_base    = drm_helper_crtc_mode_set_base,
     .mode_set_nofb    = arc_pgu_crtc_mode_set_nofb,
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index cf8f012..6c9af88 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -233,6 +233,8 @@ int
drm_helper_probe_single_connector_modes(struct drm_connector
*connector,
     struct drm_display_mode *mode;
     const struct drm_connector_helper_funcs *connector_funcs =
         connector->helper_private;
+    struct drm_crtc *crtc = NULL;
+    const struct drm_crtc_helper_funcs *crtc_funcs = NULL;
     int count = 0;
     int mode_flags = 0;
     bool verbose_prune = true;
@@ -242,6 +244,13 @@ int
drm_helper_probe_single_connector_modes(struct drm_connector
*connector,
 
     DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
             connector->name);
+
+    if (connector->encoder && connector->encoder->crtc &&
+        connector->encoder->crtc->helper_private) {
+        crtc = connector->encoder->crtc;
+        crtc_funcs = crtc->helper_private;
+    }
+
     /* set all old modes to the stale state */
     list_for_each_entry(mode, &connector->modes, head)
         mode->status = MODE_STALE;
@@ -338,6 +347,9 @@ int
drm_helper_probe_single_connector_modes(struct drm_connector
*connector,
         if (mode->status == MODE_OK && connector_funcs->mode_valid)
             mode->status = connector_funcs->mode_valid(connector,
                                    mode);
+
+        if (mode->status == MODE_OK && crtc &&
crtc_funcs->mode_valid)
+            mode->status = crtc_funcs->mode_valid(crtc, mode);
     }
 
 prune:
diff --git a/include/drm/drm_modeset_helper_vtables.h
b/include/drm/drm_modeset_helper_vtables.h
index 69c3974..776c9d0 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -104,6 +104,9 @@ struct drm_crtc_helper_funcs {
      */
     void (*commit)(struct drm_crtc *crtc);
 
+    enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+                       struct drm_display_mode *mode);
+
     /**
      * @mode_fixup:
      *
-- 
1.9.1