Probing modes and validating them

Probing modes and validating them

[Jose Abreu]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@synopsys.com>
From: Jose Abreu <joabreu@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@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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Probing modes and validating them

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 02:29:12PM +0100, Jose Abreu wrote:
> ++ 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@synopsys.com>
> From: Jose Abreu <joabreu@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@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 &&

Basing this decision on whichever crtc might happen to be driving the
connector at the time is not what we want.

This would need to loop over all the crtc that could potentially drive
the connector, and reject the mode only if all the crtcs rejected it.

> +        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
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Probing modes and validating them

[Daniel Vetter]

On Fri, Apr 21, 2017 at 02:29:12PM +0100, Jose Abreu wrote:
> ++ 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.

This is a faq, and your patch isn't the solution. Adding John (who made a
different spin on this, differently broken).

I guess I to write a todo.rst entry about this ...
-Daniel

> >
> >
> > 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@synopsys.com>
> From: Jose Abreu <joabreu@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@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
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel