Hi Jyri,

Thank you for the patch.

On Friday, 16 February 2018 13:25:04 EET Jyri Sarha wrote:
> Add ovl_name() and mgr_name() to dispc_ops and get rid of adhoc names
> here and there in the omapdrm code. This moves the names of hardware
> entities to omapdss side where they have to be when new omapdss
> backend drivers are introduced.
> 
> Signed-off-by: Jyri Sarha <jsa...@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkei...@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 23 +++++++++++++++++++++++
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  5 +++++
>  drivers/gpu/drm/omapdrm/omap_crtc.c   | 11 ++---------
>  drivers/gpu/drm/omapdrm/omap_irq.c    | 19 +++++++------------
>  drivers/gpu/drm/omapdrm/omap_plane.c  | 13 +++----------
>  5 files changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 338490d..6f83b3e 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -694,6 +694,26 @@ void dispc_runtime_put(struct dispc_device *dispc)
>       WARN_ON(r < 0 && r != -ENOSYS);
>  }
> 
> +static const char *dispc_ovl_name(struct dispc_device *dispc,
> +                               enum omap_plane_id plane)
> +{
> +     static const char * const ovl_names[] = {
> +             [OMAP_DSS_GFX]          = "GFX",
> +             [OMAP_DSS_VIDEO1]       = "VID1",
> +             [OMAP_DSS_VIDEO2]       = "VID2",
> +             [OMAP_DSS_VIDEO3]       = "VID3",
> +             [OMAP_DSS_WB]           = "WB",
> +     };
> +
> +     return ovl_names[plane];
> +}
> +
> +static const char *dispc_mgr_name(struct dispc_device *dispc,
> +                               enum omap_channel channel)
> +{
> +     return mgr_desc[channel].name;
> +}
> +
>  static u32 dispc_mgr_get_vsync_irq(struct dispc_device *dispc,
>                                  enum omap_channel channel)
>  {
> @@ -4662,6 +4682,9 @@ static const struct dispc_ops dispc_ops = {
>       .get_num_ovls = dispc_get_num_ovls,
>       .get_num_mgrs = dispc_get_num_mgrs,
> 
> +     .ovl_name = dispc_ovl_name,
> +     .mgr_name = dispc_mgr_name,
> +
>       .get_memory_bandwidth_limit = dispc_get_memory_bandwidth_limit,
> 
>       .mgr_enable = dispc_mgr_enable,
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 1299dd6..b84cfd8 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -711,6 +711,11 @@ struct dispc_ops {
>       int (*get_num_ovls)(struct dispc_device *dispc);
>       int (*get_num_mgrs)(struct dispc_device *dispc);
> 
> +     const char *(*ovl_name)(struct dispc_device *dispc,
> +                             enum omap_plane_id plane);
> +     const char *(*mgr_name)(struct dispc_device *dispc,
> +                             enum omap_channel channel);
> +
>       u32 (*get_memory_bandwidth_limit)(struct dispc_device *dispc);
> 
>       void (*mgr_enable)(struct dispc_device *dispc,
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6c4d40b..00ec959 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -672,13 +672,6 @@ static const struct drm_crtc_helper_funcs
> omap_crtc_helper_funcs = { * Init and Cleanup
>   */
> 
> -static const char *channel_names[] = {
> -     [OMAP_DSS_CHANNEL_LCD] = "lcd",
> -     [OMAP_DSS_CHANNEL_DIGIT] = "tv",
> -     [OMAP_DSS_CHANNEL_LCD2] = "lcd2",
> -     [OMAP_DSS_CHANNEL_LCD3] = "lcd3",
> -};
> -
>  void omap_crtc_pre_init(struct omap_drm_private *priv)
>  {
>       memset(omap_crtcs, 0, sizeof(omap_crtcs));
> @@ -706,7 +699,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>       channel = out->dispc_channel;
>       omap_dss_put_device(out);
> 
> -     DBG("%s", channel_names[channel]);
> +     DBG("%s", priv->dispc_ops->mgr_name(priv->dispc, channel));
> 
>       /* Multiple displays on same channel is not allowed */
>       if (WARN_ON(omap_crtcs[channel] != NULL))
> @@ -721,7 +714,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>       init_waitqueue_head(&omap_crtc->pending_wait);
> 
>       omap_crtc->channel = channel;
> -     omap_crtc->name = channel_names[channel];
> +     omap_crtc->name = priv->dispc_ops->mgr_name(priv->dispc, channel);

Possibly a small improvement here, you could cache the name in a local 
variable instead of calling the mgr_name operation twice.

> 
>       ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
>                                       &omap_crtc_funcs, NULL);
> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c
> b/drivers/gpu/drm/omapdrm/omap_irq.c index c8511504..5cc88b6 100644
> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> @@ -146,15 +146,10 @@ static void omap_irq_fifo_underflow(struct
> omap_drm_private *priv, {
>       static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
>                                     DEFAULT_RATELIMIT_BURST);
> -     static const struct {
> -             const char *name;
> -             u32 mask;
> -     } sources[] = {
> -             { "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW },
> -             { "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW },
> -             { "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW },
> -             { "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW },
> -     };
> +     static const u32 irqbits[] = { DISPC_IRQ_GFX_FIFO_UNDERFLOW,
> +                                    DISPC_IRQ_VID1_FIFO_UNDERFLOW,
> +                                    DISPC_IRQ_VID2_FIFO_UNDERFLOW,
> +                                    DISPC_IRQ_VID3_FIFO_UNDERFLOW };

The indentation looks weird, I'd write it

        static const u32 irqbits[] = {
                DISPC_IRQ_GFX_FIFO_UNDERFLOW,
                DISPC_IRQ_VID1_FIFO_UNDERFLOW,
                DISPC_IRQ_VID2_FIFO_UNDERFLOW,
                DISPC_IRQ_VID3_FIFO_UNDERFLOW,
        };

>       const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
>                      | DISPC_IRQ_VID1_FIFO_UNDERFLOW
> 
> @@ -174,9 +169,9 @@ static void omap_irq_fifo_underflow(struct
> omap_drm_private *priv,
> 
>       DRM_ERROR("FIFO underflow on ");
> 
> -     for (i = 0; i < ARRAY_SIZE(sources); ++i) {
> -             if (sources[i].mask & irqstatus)
> -                     pr_cont("%s ", sources[i].name);
> +     for (i = 0; i < ARRAY_SIZE(irqbits); ++i) {
> +             if (irqbits[i] & irqstatus)
> +                     pr_cont("%s ", priv->dispc_ops->ovl_name(priv->dispc, 
> i));

I wonder if it's worth it here, in the sense that you're splitting the name 
and mask, which are both DISPC-specific, in two. Would it make sense to move 
the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ 
handling to the DSS side, as they're not DRM/KMS-related ?

>       }
> 
>       pr_cont("(0x%08x)\n", irqstatus);
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 2899435..61b0753 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -239,13 +239,6 @@ static const struct drm_plane_funcs omap_plane_funcs =
> { .atomic_get_property = omap_plane_atomic_get_property,
>  };
> 
> -static const char *plane_id_to_name[] = {
> -     [OMAP_DSS_GFX] = "gfx",
> -     [OMAP_DSS_VIDEO1] = "vid1",
> -     [OMAP_DSS_VIDEO2] = "vid2",
> -     [OMAP_DSS_VIDEO3] = "vid3",
> -};
> -
>  static const enum omap_plane_id plane_idx_to_id[] = {
>       OMAP_DSS_GFX,
>       OMAP_DSS_VIDEO1,
> @@ -272,7 +265,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev,
> 
>       id = plane_idx_to_id[idx];
> 
> -     DBG("%s: type=%d", plane_id_to_name[id], type);
> +     DBG("%s: type=%d", priv->dispc_ops->ovl_name(priv->dispc, id), type);
> 
>       omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL);
>       if (!omap_plane)
> @@ -282,7 +275,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev, for (nformats = 0; formats[nformats]; ++nformats)
>               ;
>       omap_plane->id = id;
> -     omap_plane->name = plane_id_to_name[id];
> +     omap_plane->name = priv->dispc_ops->ovl_name(priv->dispc, id);

Same here, we could cache the name.

> 
>       plane = &omap_plane->base;
> 
> @@ -301,7 +294,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev,
> 
>  error:
>       dev_err(dev->dev, "%s(): could not create plane: %s\n",
> -             __func__, plane_id_to_name[id]);
> +             __func__, priv->dispc_ops->ovl_name(priv->dispc, id));

You could use omap_plane->name here.

> 
>       kfree(omap_plane);
>       return NULL;

-- 
Regards,

Laurent Pinchart

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

Reply via email to