Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jyri Sarha <jsarha@ti.com>
Cc: tomi.valkeinen@ti.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops
Date: Tue, 27 Feb 2018 16:35:42 +0200	[thread overview]
Message-ID: <1952497.C2LRMoVbBQ@avalon> (raw)
In-Reply-To: <ab1c46aac17a9aaeed525167bb30bea7851d9738.1518780268.git.jsarha@ti.com>

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@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@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