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 <jsarha@xxxxxx>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
