Probing modes and validating them

Daniel Vetter daniel at ffwll.ch
Tue May 2 08:36:51 UTC 2017


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 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
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list