[PATCH v2 0/3] drm/omap: Make omapdss API more generic + related patches

[PATCH v2 0/3] drm/omap: Make omapdss API more generic + related patches

[Jyri Sarha]

This the v2 rouns of a this RFC patch:
https://patchwork.kernel.org/patch/10066245/

The first patch is a simple fix that should be applied in any case.

I did not split the mgr_has_framedone() callback as a separate patch. It
quite literally replaces the mgr_get_framedone_irq() and makes no
sense without the "drm/omap: Make omapdss API more generic"-patch.

The patches have been rebased on top of the latest drm-next
(350877626faba5d60cbb8cef2bdeb524212c780b).

Best regards,
Jyri

Jyri Sarha (3):
  drm/omap: Fail probe if irq registration fails
  drm/omap: Add get_ovl_name() and get_mgr_name() to dispc_ops
  drm/omap: Make omapdss API more generic

 drivers/gpu/drm/omapdrm/dss/dispc.c   | 159 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/omapdrm/dss/dispc.h   |  33 +++++++
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  67 +++++++-------
 drivers/gpu/drm/omapdrm/omap_crtc.c   |  27 +++---
 drivers/gpu/drm/omapdrm/omap_crtc.h   |   2 +-
 drivers/gpu/drm/omapdrm/omap_drv.c    |   4 +-
 drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +-
 drivers/gpu/drm/omapdrm/omap_irq.c    | 141 ++++++++++++++----------------
 drivers/gpu/drm/omapdrm/omap_irq.h    |   2 +-
 drivers/gpu/drm/omapdrm/omap_plane.c  |  18 ++--
 drivers/gpu/drm/omapdrm/omap_plane.h  |   1 +
 11 files changed, 292 insertions(+), 165 deletions(-)

-- 
1.9.1

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] drm/omap: Fail probe if irq registration fails

[Jyri Sarha]

Call to omap_drm_irq_install() may fail with an error code. In such a
case the driver probe should fail.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index dd68b25..5fe2fcb 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -321,9 +321,9 @@ static int omap_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_reset(dev);
 
-	omap_drm_irq_install(dev);
+	ret = omap_drm_irq_install(dev);
 
-	return 0;
+	return ret;
 }
 
 /*
-- 
1.9.1

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm/omap: Fail probe if irq registration fails

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Monday, 1 January 2018 13:55:10 EET Jyri Sarha wrote:
> Call to omap_drm_irq_install() may fail with an error code. In such a
> case the driver probe should fail.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b25..5fe2fcb 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -321,9 +321,9 @@ static int omap_modeset_init(struct drm_device *dev)
> 
>  	drm_mode_config_reset(dev);
> 
> -	omap_drm_irq_install(dev);
> +	ret = omap_drm_irq_install(dev);
> 
> -	return 0;
> +	return ret;

Nitpicking, how about just

	return omap_drm_irq_install(dev);

? With this changed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
> 
>  /*

-- 
Regards,

Laurent Pinchart

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

[PATCH v2 2/3] drm/omap: Add get_ovl_name() and get_mgr_name() to dispc_ops

[Jyri Sarha]

Add get_ovl_name() and get_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>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 21 +++++++++++++++++++++
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  3 +++
 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, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 4e8f68e..070053f 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -680,6 +680,24 @@ void dispc_runtime_put(void)
 	WARN_ON(r < 0 && r != -ENOSYS);
 }
 
+static const char *dispc_get_ovl_name(enum omap_plane_id plane)
+{
+	static const char *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_get_mgr_name(enum omap_channel channel)
+{
+	return mgr_desc[channel].name;
+}
+
 static u32 dispc_mgr_get_vsync_irq(enum omap_channel channel)
 {
 	return mgr_desc[channel].vsync_irq;
@@ -4506,6 +4524,9 @@ static void dispc_errata_i734_wa(void)
 	.get_num_ovls = dispc_get_num_ovls,
 	.get_num_mgrs = dispc_get_num_mgrs,
 
+	.get_ovl_name = dispc_get_ovl_name,
+	.get_mgr_name = dispc_get_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 f8f83e8..d7ed1a4 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -691,6 +691,9 @@ struct dispc_ops {
 	int (*get_num_ovls)(void);
 	int (*get_num_mgrs)(void);
 
+	const char *(*get_ovl_name)(enum omap_plane_id plane);
+	const char *(*get_mgr_name)(enum omap_channel channel);
+
 	u32 (*get_memory_bandwidth_limit)(void);
 
 	void (*mgr_enable)(enum omap_channel channel, bool enable);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 1b8154e..fee8a63 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -662,13 +662,6 @@ static void omap_crtc_reset(struct drm_crtc *crtc)
  * 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(void)
 {
 	memset(omap_crtcs, 0, sizeof(omap_crtcs));
@@ -696,7 +689,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->get_mgr_name(channel));
 
 	/* Multiple displays on same channel is not allowed */
 	if (WARN_ON(omap_crtcs[channel] != NULL))
@@ -711,7 +704,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->get_mgr_name(channel);
 
 	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 53ba424..b0f6850 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -144,15 +144,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 };
 
 	const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
 		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
@@ -172,9 +167,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->get_ovl_name(i));
 	}
 
 	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 7d789d1..6f9d9ef 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -239,13 +239,6 @@ static int omap_plane_atomic_get_property(struct drm_plane *plane,
 	.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->get_ovl_name(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->get_ovl_name(id);
 
 	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->get_ovl_name(id));
 
 	kfree(omap_plane);
 	return NULL;
-- 
1.9.1

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/omap: Make omapdss API more generic

[Tomi Valkeinen]

Hi,

On 01/01/18 13:55, Jyri Sarha wrote:
> The new omapdss API is HW independent and cleans up some of the DSS5
> specific hacks from the omapdrm side and gets rid off the DSS5 IRQ
> register bits and replace them with HW independent generic u64 based
> macros. This new macros make it more straight forward to implement the
> IRQ code for the future DSS versions that do not share the same
> register structure as DSS2 to DSS5 has.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 148 ++++++++++++++++++++++++++++------
>  drivers/gpu/drm/omapdrm/dss/dispc.h   |  33 ++++++++
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  64 +++++++--------
>  drivers/gpu/drm/omapdrm/omap_crtc.c   |  16 ++--
>  drivers/gpu/drm/omapdrm/omap_crtc.h   |   2 +-
>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +-
>  drivers/gpu/drm/omapdrm/omap_irq.c    | 136 +++++++++++++++----------------
>  drivers/gpu/drm/omapdrm/omap_irq.h    |   2 +-
>  drivers/gpu/drm/omapdrm/omap_plane.c  |   7 ++
>  drivers/gpu/drm/omapdrm/omap_plane.h  |   1 +
>  10 files changed, 267 insertions(+), 145 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 070053f..a80ebe1 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -698,27 +698,12 @@ static const char *dispc_get_mgr_name(enum omap_channel channel)
>  	return mgr_desc[channel].name;
>  }
>  
> -static u32 dispc_mgr_get_vsync_irq(enum omap_channel channel)
> -{
> -	return mgr_desc[channel].vsync_irq;
> -}
> -
> -static u32 dispc_mgr_get_framedone_irq(enum omap_channel channel)
> +static bool dispc_mgr_has_framedone(enum omap_channel channel)
>  {
>  	if (channel == OMAP_DSS_CHANNEL_DIGIT && dispc.feat->no_framedone_tv)
> -		return 0;
> -
> -	return mgr_desc[channel].framedone_irq;
> -}
> -
> -static u32 dispc_mgr_get_sync_lost_irq(enum omap_channel channel)
> -{
> -	return mgr_desc[channel].sync_lost_irq;
> -}
> +		return false;
>  
> -u32 dispc_wb_get_framedone_irq(void)
> -{
> -	return DISPC_IRQ_FRAMEDONEWB;
> +	return true;
>  }

You can just do

return channel != OMAP_DSS_CHANNEL_DIGIT || !dispc.feat->no_framedone_tv;

>  
>  static void dispc_mgr_enable(enum omap_channel channel, bool enable)
> @@ -3604,6 +3589,121 @@ static void dispc_write_irqenable(u32 mask)
>  	dispc_read_reg(DISPC_IRQENABLE);
>  }
>  
> +static u64 dispc_hw_to_api_irq(u32 hw)
> +{
> +	u64 api = 0;
> +
> +	if (hw & DISPC_IRQ_OCP_ERR)
> +		api |= DSS_IRQ_DEVICE_OCP_ERR;
> +
> +	if (hw & DISPC_IRQ_FRAMEDONE)
> +		api |= DSS_IRQ_MGR_FRAME_DONE(0);
> +	if (hw & DISPC_IRQ_VSYNC)
> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(0);
> +	if (hw & DISPC_IRQ_SYNC_LOST)
> +		api |= DSS_IRQ_MGR_SYNC_LOST(0);
> +
> +	if (hw & DISPC_IRQ_EVSYNC_EVEN)
> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(1);
> +	if (hw & DISPC_IRQ_EVSYNC_ODD)
> +		api |= DSS_IRQ_MGR_VSYNC_ODD(1);
> +	if (hw & DISPC_IRQ_SYNC_LOST_DIGIT)
> +		api |= DSS_IRQ_MGR_SYNC_LOST(1);
> +	if (hw & DISPC_IRQ_FRAMEDONETV)
> +		api |= DSS_IRQ_MGR_FRAME_DONE(1);
> +
> +	if (hw & DISPC_IRQ_SYNC_LOST2)
> +		api |= DSS_IRQ_MGR_SYNC_LOST(2);
> +	if (hw & DISPC_IRQ_VSYNC2)
> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(2);
> +	if (hw & DISPC_IRQ_FRAMEDONE2)
> +		api |= DSS_IRQ_MGR_FRAME_DONE(2);
> +
> +	if (hw & DISPC_IRQ_SYNC_LOST3)
> +		api |= DSS_IRQ_MGR_SYNC_LOST(3);
> +	if (hw & DISPC_IRQ_VSYNC3)
> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(3);
> +	if (hw & DISPC_IRQ_FRAMEDONE3)
> +		api |= DSS_IRQ_MGR_FRAME_DONE(3);
> +
> +	if (hw & DISPC_IRQ_GFX_FIFO_UNDERFLOW)
> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(0);
> +	if (hw & DISPC_IRQ_VID1_FIFO_UNDERFLOW)
> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(1);
> +	if (hw & DISPC_IRQ_VID2_FIFO_UNDERFLOW)
> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(2);
> +	if (hw & DISPC_IRQ_VID3_FIFO_UNDERFLOW)
> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(3);
> +
> +	return api;
> +}
> +
> +static u32 dispc_api_to_hw_irq(u64 api)
> +{
> +	u32 hw = 0;
> +
> +	if (api & DSS_IRQ_DEVICE_OCP_ERR)
> +		hw |= DISPC_IRQ_OCP_ERR;
> +
> +	if (api & DSS_IRQ_MGR_FRAME_DONE(0))
> +		hw |= DISPC_IRQ_FRAMEDONE;
> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(0))
> +		hw |= DISPC_IRQ_VSYNC;
> +	if (api & DSS_IRQ_MGR_SYNC_LOST(0))
> +		hw |= DISPC_IRQ_SYNC_LOST;
> +
> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(1))
> +		hw |= DISPC_IRQ_EVSYNC_EVEN;
> +	if (api & DSS_IRQ_MGR_VSYNC_ODD(1))
> +		hw |= DISPC_IRQ_EVSYNC_ODD;
> +	if (api & DSS_IRQ_MGR_SYNC_LOST(1))
> +		hw |= DISPC_IRQ_SYNC_LOST_DIGIT;
> +	if (api & DSS_IRQ_MGR_FRAME_DONE(1))
> +		hw |= DISPC_IRQ_FRAMEDONETV;
> +
> +	if (api & DSS_IRQ_MGR_SYNC_LOST(2))
> +		hw |= DISPC_IRQ_SYNC_LOST2;
> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(2))
> +		hw |= DISPC_IRQ_VSYNC2;
> +	if (api & DSS_IRQ_MGR_FRAME_DONE(2))
> +		hw |= DISPC_IRQ_FRAMEDONE2;
> +
> +	if (api & DSS_IRQ_MGR_SYNC_LOST(3))
> +		hw |= DISPC_IRQ_SYNC_LOST3;
> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(3))
> +		hw |= DISPC_IRQ_VSYNC3;
> +	if (api & DSS_IRQ_MGR_FRAME_DONE(3))
> +		hw |= DISPC_IRQ_FRAMEDONE3;
> +
> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(0))
> +		hw |= DISPC_IRQ_GFX_FIFO_UNDERFLOW;
> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(1))
> +		hw |= DISPC_IRQ_VID1_FIFO_UNDERFLOW;
> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(2))
> +		hw |= DISPC_IRQ_VID2_FIFO_UNDERFLOW;
> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(3))
> +		hw |= DISPC_IRQ_VID3_FIFO_UNDERFLOW;
> +
> +	return hw;
> +}

I think you could have a simple table with hw:api bit pairs. And then a
simple for loop in both the functions above to do the work.

> +
> +static u64 dispc_api_read_irqstatus(u64 clearmask)
> +{
> +	u32 hw_clearmask = dispc_api_to_hw_irq(clearmask);
> +	u32 hw_status = dispc_read_irqstatus();
> +
> +	dispc_clear_irqstatus(hw_clearmask & hw_status);
> +
> +	return dispc_hw_to_api_irq(hw_status);
> +}

I think we always want to read the whole irqstatus, and clear it. You
can do that with the function above, but I'm not sure if clearmask
offers us anything we ever need to use.

Also, this function also clears the irqstatus, so perhaps rename the
function to read_and_clear_irqstatus to make it obvious, as it's quite
an important side effect.

> +
> +static void dispc_api_write_irqenable(u64 enable)
> +{
> +	u32 hw_enable = dispc_api_to_hw_irq(enable);
> +
> +	dispc_write_irqenable(hw_enable);
> +}
> +
>  void dispc_enable_sidle(void)
>  {
>  	REG_FLD_MOD(DISPC_SYSCONFIG, 2, 4, 3);	/* SIDLEMODE: smart idle */
> @@ -4453,7 +4553,7 @@ static void dispc_errata_i734_wa_fini(void)
>  
>  static void dispc_errata_i734_wa(void)
>  {
> -	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
> +	u32 framedone_irq = DISPC_IRQ_FRAMEDONE;
>  	struct omap_overlay_info ovli;
>  	struct dss_lcd_mgr_config lcd_conf;
>  	u32 gatestate;
> @@ -4511,9 +4611,8 @@ static void dispc_errata_i734_wa(void)
>  }
>  
>  static const struct dispc_ops dispc_ops = {
> -	.read_irqstatus = dispc_read_irqstatus,
> -	.clear_irqstatus = dispc_clear_irqstatus,
> -	.write_irqenable = dispc_write_irqenable,
> +	.read_irqstatus = dispc_api_read_irqstatus,
> +	.write_irqenable = dispc_api_write_irqenable,
>  
>  	.request_irq = dispc_request_irq,
>  	.free_irq = dispc_free_irq,
> @@ -4527,13 +4626,12 @@ static void dispc_errata_i734_wa(void)
>  	.get_ovl_name = dispc_get_ovl_name,
>  	.get_mgr_name = dispc_get_mgr_name,
>  
> +	.mgr_has_framedone = dispc_mgr_has_framedone,
> +
>  	.get_memory_bandwidth_limit = dispc_get_memory_bandwidth_limit,
>  
>  	.mgr_enable = dispc_mgr_enable,
>  	.mgr_is_enabled = dispc_mgr_is_enabled,
> -	.mgr_get_vsync_irq = dispc_mgr_get_vsync_irq,
> -	.mgr_get_framedone_irq = dispc_mgr_get_framedone_irq,
> -	.mgr_get_sync_lost_irq = dispc_mgr_get_sync_lost_irq,
>  	.mgr_go_busy = dispc_mgr_go_busy,
>  	.mgr_go = dispc_mgr_go,
>  	.mgr_set_lcd_config = dispc_mgr_set_lcd_config,
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.h b/drivers/gpu/drm/omapdrm/dss/dispc.h
> index e901dd1..e71266e 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.h
> @@ -18,6 +18,39 @@
>  #ifndef __OMAP2_DISPC_REG_H
>  #define __OMAP2_DISPC_REG_H
>  
> +/* DISPC IRQ bits */
> +#define DISPC_IRQ_FRAMEDONE		(1 << 0)
> +#define DISPC_IRQ_VSYNC			(1 << 1)
> +#define DISPC_IRQ_EVSYNC_EVEN		(1 << 2)
> +#define DISPC_IRQ_EVSYNC_ODD		(1 << 3)
> +#define DISPC_IRQ_ACBIAS_COUNT_STAT	(1 << 4)
> +#define DISPC_IRQ_PROG_LINE_NUM		(1 << 5)
> +#define DISPC_IRQ_GFX_FIFO_UNDERFLOW	(1 << 6)
> +#define DISPC_IRQ_GFX_END_WIN		(1 << 7)
> +#define DISPC_IRQ_PAL_GAMMA_MASK	(1 << 8)
> +#define DISPC_IRQ_OCP_ERR		(1 << 9)
> +#define DISPC_IRQ_VID1_FIFO_UNDERFLOW	(1 << 10)
> +#define DISPC_IRQ_VID1_END_WIN		(1 << 11)
> +#define DISPC_IRQ_VID2_FIFO_UNDERFLOW	(1 << 12)
> +#define DISPC_IRQ_VID2_END_WIN		(1 << 13)
> +#define DISPC_IRQ_SYNC_LOST		(1 << 14)
> +#define DISPC_IRQ_SYNC_LOST_DIGIT	(1 << 15)
> +#define DISPC_IRQ_WAKEUP		(1 << 16)
> +#define DISPC_IRQ_SYNC_LOST2		(1 << 17)
> +#define DISPC_IRQ_VSYNC2		(1 << 18)
> +#define DISPC_IRQ_VID3_END_WIN		(1 << 19)
> +#define DISPC_IRQ_VID3_FIFO_UNDERFLOW	(1 << 20)
> +#define DISPC_IRQ_ACBIAS_COUNT_STAT2	(1 << 21)
> +#define DISPC_IRQ_FRAMEDONE2		(1 << 22)
> +#define DISPC_IRQ_FRAMEDONEWB		(1 << 23)
> +#define DISPC_IRQ_FRAMEDONETV		(1 << 24)
> +#define DISPC_IRQ_WBBUFFEROVERFLOW	(1 << 25)
> +#define DISPC_IRQ_WBUNCOMPLETEERROR	(1 << 26)
> +#define DISPC_IRQ_SYNC_LOST3		(1 << 27)
> +#define DISPC_IRQ_VSYNC3		(1 << 28)
> +#define DISPC_IRQ_ACBIAS_COUNT_STAT3	(1 << 29)
> +#define DISPC_IRQ_FRAMEDONE3		(1 << 30)
> +
>  /* DISPC common registers */
>  #define DISPC_REVISION			0x0000
>  #define DISPC_SYSCONFIG			0x0010
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index d7ed1a4..b1499c2 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -27,37 +27,29 @@
>  #include <uapi/drm/drm_mode.h>
>  #include <drm/drm_crtc.h>
>  
> -#define DISPC_IRQ_FRAMEDONE		(1 << 0)
> -#define DISPC_IRQ_VSYNC			(1 << 1)
> -#define DISPC_IRQ_EVSYNC_EVEN		(1 << 2)
> -#define DISPC_IRQ_EVSYNC_ODD		(1 << 3)
> -#define DISPC_IRQ_ACBIAS_COUNT_STAT	(1 << 4)
> -#define DISPC_IRQ_PROG_LINE_NUM		(1 << 5)
> -#define DISPC_IRQ_GFX_FIFO_UNDERFLOW	(1 << 6)
> -#define DISPC_IRQ_GFX_END_WIN		(1 << 7)
> -#define DISPC_IRQ_PAL_GAMMA_MASK	(1 << 8)
> -#define DISPC_IRQ_OCP_ERR		(1 << 9)
> -#define DISPC_IRQ_VID1_FIFO_UNDERFLOW	(1 << 10)
> -#define DISPC_IRQ_VID1_END_WIN		(1 << 11)
> -#define DISPC_IRQ_VID2_FIFO_UNDERFLOW	(1 << 12)
> -#define DISPC_IRQ_VID2_END_WIN		(1 << 13)
> -#define DISPC_IRQ_SYNC_LOST		(1 << 14)
> -#define DISPC_IRQ_SYNC_LOST_DIGIT	(1 << 15)
> -#define DISPC_IRQ_WAKEUP		(1 << 16)
> -#define DISPC_IRQ_SYNC_LOST2		(1 << 17)
> -#define DISPC_IRQ_VSYNC2		(1 << 18)
> -#define DISPC_IRQ_VID3_END_WIN		(1 << 19)
> -#define DISPC_IRQ_VID3_FIFO_UNDERFLOW	(1 << 20)
> -#define DISPC_IRQ_ACBIAS_COUNT_STAT2	(1 << 21)
> -#define DISPC_IRQ_FRAMEDONE2		(1 << 22)
> -#define DISPC_IRQ_FRAMEDONEWB		(1 << 23)
> -#define DISPC_IRQ_FRAMEDONETV		(1 << 24)
> -#define DISPC_IRQ_WBBUFFEROVERFLOW	(1 << 25)
> -#define DISPC_IRQ_WBUNCOMPLETEERROR	(1 << 26)
> -#define DISPC_IRQ_SYNC_LOST3		(1 << 27)
> -#define DISPC_IRQ_VSYNC3		(1 << 28)
> -#define DISPC_IRQ_ACBIAS_COUNT_STAT3	(1 << 29)
> -#define DISPC_IRQ_FRAMEDONE3		(1 << 30)
> +#define DSS_MAX_CHANNELS 8
> +#define DSS_MAX_OVLS 8
> +
> +#define DSS_IRQ_DEVICE_OCP_ERR		BIT_ULL(0)
> +
> +#define DSS_IRQ_MGR_BIT_N(ch, bit)	(4 + 4 * ch + bit)
> +#define DSS_IRQ_OVL_BIT_N(ovl, bit) \
> +	(DSS_IRQ_MGR_BIT_N(DSS_MAX_CHANNELS, 0) + 1 * ovl + bit)
> +
> +#define DSS_IRQ_MGR_BIT(ch, bit)	BIT_ULL(DSS_IRQ_MGR_BIT_N(ch, bit))
> +#define DSS_IRQ_OVL_BIT(ovl, bit)	BIT_ULL(DSS_IRQ_OVL_BIT_N(ovl, bit))
> +
> +#define DSS_IRQ_MGR_MASK(ch) \
> +	GENMASK_ULL(DSS_IRQ_MGR_BIT_N(ch, 3), DSS_IRQ_MGR_BIT_N(ch, 0))
> +#define DSS_IRQ_OVL_MASK(ovl) \
> +	GENMASK_ULL(DSS_IRQ_OVL_BIT_N(ovl, 0), DSS_IRQ_OVL_BIT_N(ovl, 0))
> +
> +#define DSS_IRQ_MGR_FRAME_DONE(ch)	DSS_IRQ_MGR_BIT(ch, 0)
> +#define DSS_IRQ_MGR_VSYNC_EVEN(ch)	DSS_IRQ_MGR_BIT(ch, 1)
> +#define DSS_IRQ_MGR_VSYNC_ODD(ch)	DSS_IRQ_MGR_BIT(ch, 2)
> +#define DSS_IRQ_MGR_SYNC_LOST(ch)	DSS_IRQ_MGR_BIT(ch, 3)
> +
> +#define DSS_IRQ_OVL_FIFO_UNDERFLOW(ovl)	DSS_IRQ_OVL_BIT(ovl, 0)
>  
>  struct omap_dss_device;
>  struct dss_lcd_mgr_config;
> @@ -678,9 +670,8 @@ void dss_mgr_unregister_framedone_handler(enum omap_channel channel,
>  /* dispc ops */
>  
>  struct dispc_ops {
> -	u32 (*read_irqstatus)(void);
> -	void (*clear_irqstatus)(u32 mask);
> -	void (*write_irqenable)(u32 mask);
> +	u64 (*read_irqstatus)(u64 clearmask);
> +	void (*write_irqenable)(u64 enable);
>  
>  	int (*request_irq)(irq_handler_t handler, void *dev_id);
>  	void (*free_irq)(void *dev_id);
> @@ -694,13 +685,12 @@ struct dispc_ops {
>  	const char *(*get_ovl_name)(enum omap_plane_id plane);
>  	const char *(*get_mgr_name)(enum omap_channel channel);
>  
> +	bool (*mgr_has_framedone)(enum omap_channel channel);
> +
>  	u32 (*get_memory_bandwidth_limit)(void);
>  
>  	void (*mgr_enable)(enum omap_channel channel, bool enable);
>  	bool (*mgr_is_enabled)(enum omap_channel channel);
> -	u32 (*mgr_get_vsync_irq)(enum omap_channel channel);
> -	u32 (*mgr_get_framedone_irq)(enum omap_channel channel);
> -	u32 (*mgr_get_sync_lost_irq)(enum omap_channel channel);
>  	bool (*mgr_go_busy)(enum omap_channel channel);
>  	void (*mgr_go)(enum omap_channel channel);
>  	void (*mgr_set_lcd_config)(enum omap_channel channel,
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index fee8a63..f7e1668 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -149,7 +149,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>  	enum omap_channel channel = omap_crtc->channel;
>  	struct omap_irq_wait *wait;
> -	u32 framedone_irq, vsync_irq;
> +	u64 vsync_irq, framedone_irq;
>  	int ret;
>  
>  	if (WARN_ON(omap_crtc->enabled == enable))
> @@ -169,8 +169,10 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  		omap_crtc->ignore_digit_sync_lost = true;
>  	}
>  
> -	framedone_irq = priv->dispc_ops->mgr_get_framedone_irq(channel);
> -	vsync_irq = priv->dispc_ops->mgr_get_vsync_irq(channel);
> +
> +	vsync_irq = (DSS_IRQ_MGR_VSYNC_EVEN(channel) |
> +		     DSS_IRQ_MGR_VSYNC_ODD(channel));
> +	framedone_irq = DSS_IRQ_MGR_FRAME_DONE(channel);
>  
>  	if (enable) {
>  		wait = omap_irq_wait_init(dev, vsync_irq, 1);
> @@ -184,7 +186,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  		 * even and odd frames.
>  		 */
>  
> -		if (framedone_irq)
> +		if (priv->dispc_ops->mgr_has_framedone(channel))
>  			wait = omap_irq_wait_init(dev, framedone_irq, 1);
>  		else
>  			wait = omap_irq_wait_init(dev, vsync_irq, 2);
> @@ -272,17 +274,17 @@ static void omap_crtc_dss_unregister_framedone(
>   * Setup, Flush and Page Flip
>   */
>  
> -void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus)
> +void omap_crtc_error_irq(struct drm_crtc *crtc, u64 irqstatus)
>  {
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>  
>  	if (omap_crtc->ignore_digit_sync_lost) {
> -		irqstatus &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
> +		irqstatus &= ~DSS_IRQ_MGR_SYNC_LOST(omap_crtc->channel);
>  		if (!irqstatus)
>  			return;
>  	}
>  
> -	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus);
> +	DRM_ERROR_RATELIMITED("%s: errors: %016llx\n", omap_crtc->name, irqstatus);
>  }
>  
>  void omap_crtc_vblank_irq(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.h b/drivers/gpu/drm/omapdrm/omap_crtc.h
> index ad7b007..55e2e02 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.h
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.h
> @@ -37,7 +37,7 @@
>  struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  		struct drm_plane *plane, struct omap_dss_device *dssdev);
>  int omap_crtc_wait_pending(struct drm_crtc *crtc);
> -void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus);
> +void omap_crtc_error_irq(struct drm_crtc *crtc, u64 irqstatus);
>  void omap_crtc_vblank_irq(struct drm_crtc *crtc);
>  
>  #endif /* __OMAPDRM_CRTC_H__ */
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 0ac97fe..1a479ea 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -81,8 +81,7 @@ struct omap_drm_private {
>  	/* irq handling: */
>  	spinlock_t wait_lock;		/* protects the wait_list */
>  	struct list_head wait_list;	/* list of omap_irq_wait */
> -	uint32_t irq_mask;		/* enabled irqs in addition to wait_list */
> -
> +	u64 irq_mask;			/* enabled irqs in addition to wait_list */
>  	/* memory bandwidth limit if it is needed on the platform */
>  	unsigned int max_bandwidth;
>  };
> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
> index b0f6850..57ea08e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> @@ -20,25 +20,24 @@
>  struct omap_irq_wait {
>  	struct list_head node;
>  	wait_queue_head_t wq;
> -	uint32_t irqmask;
> +	u64 irqmask;
>  	int count;
>  };
>  
>  /* call with wait_lock and dispc runtime held */
> -static void omap_irq_update(struct drm_device *dev)
> +static void omap_irq_full_mask(struct drm_device *dev, u64 *irqmask)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
>  	struct omap_irq_wait *wait;
> -	uint32_t irqmask = priv->irq_mask;
>  
>  	assert_spin_locked(&priv->wait_lock);
>  
> -	list_for_each_entry(wait, &priv->wait_list, node)
> -		irqmask |= wait->irqmask;
> +	*irqmask = priv->irq_mask;
>  
> -	DBG("irqmask=%08x", irqmask);
> +	list_for_each_entry(wait, &priv->wait_list, node)
> +		*irqmask |= wait->irqmask;
>  
> -	priv->dispc_ops->write_irqenable(irqmask);
> +	DBG("irqmask 0x%016llx", *irqmask);
>  }
>  
>  static void omap_irq_wait_handler(struct omap_irq_wait *wait)
> @@ -48,19 +47,24 @@ static void omap_irq_wait_handler(struct omap_irq_wait *wait)
>  }
>  
>  struct omap_irq_wait * omap_irq_wait_init(struct drm_device *dev,
> -		uint32_t irqmask, int count)
> +		u64 waitmask, int count)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
>  	struct omap_irq_wait *wait = kzalloc(sizeof(*wait), GFP_KERNEL);
>  	unsigned long flags;
> +	u64 irqmask;
> +
> +	if (!wait)
> +		return NULL;
>  
>  	init_waitqueue_head(&wait->wq);
> -	wait->irqmask = irqmask;
> +	wait->irqmask = waitmask;
>  	wait->count = count;
>  
>  	spin_lock_irqsave(&priv->wait_lock, flags);
>  	list_add(&wait->node, &priv->wait_list);
> -	omap_irq_update(dev);
> +	omap_irq_full_mask(dev, &irqmask);
> +	priv->dispc_ops->write_irqenable(irqmask);
>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>  
>  	return wait;
> @@ -71,13 +75,15 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
>  	unsigned long flags;
> +	u64 irqmask;
>  	int ret;
>  
>  	ret = wait_event_timeout(wait->wq, (wait->count <= 0), timeout);
>  
>  	spin_lock_irqsave(&priv->wait_lock, flags);
>  	list_del(&wait->node);
> -	omap_irq_update(dev);
> +	omap_irq_full_mask(dev, &irqmask);
> +	priv->dispc_ops->write_irqenable(irqmask);
>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>  
>  	kfree(wait);
> @@ -104,12 +110,15 @@ int omap_irq_enable_vblank(struct drm_crtc *crtc)
>  	struct omap_drm_private *priv = dev->dev_private;
>  	unsigned long flags;
>  	enum omap_channel channel = omap_crtc_channel(crtc);
> +	u64 irqmask;
>  
>  	DBG("dev=%p, crtc=%u", dev, channel);
>  
>  	spin_lock_irqsave(&priv->wait_lock, flags);
> -	priv->irq_mask |= priv->dispc_ops->mgr_get_vsync_irq(channel);
> -	omap_irq_update(dev);
> +	priv->irq_mask |= DSS_IRQ_MGR_VSYNC_EVEN(channel) |
> +		DSS_IRQ_MGR_VSYNC_ODD(channel);
> +	omap_irq_full_mask(dev, &irqmask);
> +	priv->dispc_ops->write_irqenable(irqmask);
>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>  
>  	return 0;
> @@ -130,36 +139,36 @@ void omap_irq_disable_vblank(struct drm_crtc *crtc)
>  	struct omap_drm_private *priv = dev->dev_private;
>  	unsigned long flags;
>  	enum omap_channel channel = omap_crtc_channel(crtc);
> +	u64 irqmask;
>  
>  	DBG("dev=%p, crtc=%u", dev, channel);
>  
>  	spin_lock_irqsave(&priv->wait_lock, flags);
> -	priv->irq_mask &= ~priv->dispc_ops->mgr_get_vsync_irq(channel);
> -	omap_irq_update(dev);
> +	priv->irq_mask &= ~(DSS_IRQ_MGR_VSYNC_EVEN(channel) |
> +			    DSS_IRQ_MGR_VSYNC_ODD(channel));
> +	omap_irq_full_mask(dev, &irqmask);
> +	priv->dispc_ops->write_irqenable(irqmask);
>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>  }
>  
>  static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
> -				    u32 irqstatus)
> +				    u64 irqstatus)
>  {
>  	static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
>  				      DEFAULT_RATELIMIT_BURST);
> -	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
> -		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
> -		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
> +	static const u64 uf_mask =
> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(0) | DSS_IRQ_OVL_FIFO_UNDERFLOW(1) |
> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(2) | DSS_IRQ_OVL_FIFO_UNDERFLOW(3) |
> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(4) | DSS_IRQ_OVL_FIFO_UNDERFLOW(5) |
> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(6) |	DSS_IRQ_OVL_FIFO_UNDERFLOW(7);

Use a for loop to setup the mask. With the above, if in the future we
change the irq bits and, say, change the num of ovls to 4, the above
will break.

>  	unsigned int i;
> +	u64 masked;
>  
>  	spin_lock(&priv->wait_lock);
> -	irqstatus &= priv->irq_mask & mask;
> +	masked = irqstatus & uf_mask & priv->irq_mask;
>  	spin_unlock(&priv->wait_lock);
>  
> -	if (!irqstatus)
> +	if (!masked)
>  		return;
>  
>  	if (!__ratelimit(&_rs))
> @@ -167,21 +176,19 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>  
>  	DRM_ERROR("FIFO underflow on ");
>  
> -	for (i = 0; i < ARRAY_SIZE(irqbits); ++i) {
> -		if (irqbits[i] & irqstatus)
> -			pr_cont("%s ", priv->dispc_ops->get_ovl_name(i));
> +	for (i = 0; i < DSS_MAX_OVLS; ++i) {
> +		if (masked & DSS_IRQ_OVL_FIFO_UNDERFLOW(i))
> +			pr_cont("%u:%s ", i, priv->dispc_ops->get_ovl_name(i));
>  	}
>  
> -	pr_cont("(0x%08x)\n", irqstatus);
> +	pr_cont("(%016llx)\n", irqstatus);
>  }
>  
>  static void omap_irq_ocp_error_handler(struct drm_device *dev,
> -	u32 irqstatus)
> +				       u64 irqstatus)
>  {
> -	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
> -		return;
> -
> -	dev_err_ratelimited(dev->dev, "OCP error\n");
> +	if (irqstatus & DSS_IRQ_DEVICE_OCP_ERR)
> +		dev_err_ratelimited(dev->dev, "OCP error\n");
>  }
>  
>  static irqreturn_t omap_irq_handler(int irq, void *arg)
> @@ -191,47 +198,40 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
>  	struct omap_irq_wait *wait, *n;
>  	unsigned long flags;
>  	unsigned int id;
> -	u32 irqstatus;
> +	u64 irqstatus, clearmask;
>  
> -	irqstatus = priv->dispc_ops->read_irqstatus();
> -	priv->dispc_ops->clear_irqstatus(irqstatus);
> -	priv->dispc_ops->read_irqstatus();        /* flush posted write */
> +	spin_lock_irqsave(&priv->wait_lock, flags);
> +	omap_irq_full_mask(dev, &clearmask);
> +	irqstatus = priv->dispc_ops->read_irqstatus(clearmask);
> +
> +	list_for_each_entry_safe(wait, n, &priv->wait_list, node) {
> +		if (irqstatus & wait->irqmask)
> +			omap_irq_wait_handler(wait);
> +	}
> +	spin_unlock_irqrestore(&priv->wait_lock, flags);
>  
> -	VERB("irqs: %08x", irqstatus);
> +	VERB("irqs: 0x%016llx\n", irqstatus);
>  
>  	for (id = 0; id < priv->num_crtcs; id++) {
>  		struct drm_crtc *crtc = priv->crtcs[id];
>  		enum omap_channel channel = omap_crtc_channel(crtc);
>  
> -		if (irqstatus & priv->dispc_ops->mgr_get_vsync_irq(channel)) {
> +		if (irqstatus & (DSS_IRQ_MGR_VSYNC_EVEN(channel) |
> +				 DSS_IRQ_MGR_VSYNC_ODD(channel))) {
>  			drm_handle_vblank(dev, id);
>  			omap_crtc_vblank_irq(crtc);
>  		}
>  
> -		if (irqstatus & priv->dispc_ops->mgr_get_sync_lost_irq(channel))
> +		if (irqstatus & DSS_IRQ_MGR_SYNC_LOST(channel))
>  			omap_crtc_error_irq(crtc, irqstatus);
>  	}
>  
>  	omap_irq_ocp_error_handler(dev, irqstatus);
>  	omap_irq_fifo_underflow(priv, irqstatus);
>  
> -	spin_lock_irqsave(&priv->wait_lock, flags);
> -	list_for_each_entry_safe(wait, n, &priv->wait_list, node) {
> -		if (wait->irqmask & irqstatus)
> -			omap_irq_wait_handler(wait);
> -	}
> -	spin_unlock_irqrestore(&priv->wait_lock, flags);
> -

Why did you change the location where wait_handler is being called?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/omap: Make omapdss API more generic

[Jyri Sarha]

On 01/04/18 13:17, Tomi Valkeinen wrote:
> Hi,
> 
> On 01/01/18 13:55, Jyri Sarha wrote:
>> The new omapdss API is HW independent and cleans up some of the DSS5
>> specific hacks from the omapdrm side and gets rid off the DSS5 IRQ
>> register bits and replace them with HW independent generic u64 based
>> macros. This new macros make it more straight forward to implement the
>> IRQ code for the future DSS versions that do not share the same
>> register structure as DSS2 to DSS5 has.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 148 ++++++++++++++++++++++++++++------
>>  drivers/gpu/drm/omapdrm/dss/dispc.h   |  33 ++++++++
>>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  64 +++++++--------
>>  drivers/gpu/drm/omapdrm/omap_crtc.c   |  16 ++--
>>  drivers/gpu/drm/omapdrm/omap_crtc.h   |   2 +-
>>  drivers/gpu/drm/omapdrm/omap_drv.h    |   3 +-
>>  drivers/gpu/drm/omapdrm/omap_irq.c    | 136 +++++++++++++++----------------
>>  drivers/gpu/drm/omapdrm/omap_irq.h    |   2 +-
>>  drivers/gpu/drm/omapdrm/omap_plane.c  |   7 ++
>>  drivers/gpu/drm/omapdrm/omap_plane.h  |   1 +
>>  10 files changed, 267 insertions(+), 145 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> index 070053f..a80ebe1 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -698,27 +698,12 @@ static const char *dispc_get_mgr_name(enum omap_channel channel)
>>  	return mgr_desc[channel].name;
>>  }
>>  
>> -static u32 dispc_mgr_get_vsync_irq(enum omap_channel channel)
>> -{
>> -	return mgr_desc[channel].vsync_irq;
>> -}
>> -
>> -static u32 dispc_mgr_get_framedone_irq(enum omap_channel channel)
>> +static bool dispc_mgr_has_framedone(enum omap_channel channel)
>>  {
>>  	if (channel == OMAP_DSS_CHANNEL_DIGIT && dispc.feat->no_framedone_tv)
>> -		return 0;
>> -
>> -	return mgr_desc[channel].framedone_irq;
>> -}
>> -
>> -static u32 dispc_mgr_get_sync_lost_irq(enum omap_channel channel)
>> -{
>> -	return mgr_desc[channel].sync_lost_irq;
>> -}
>> +		return false;
>>  
>> -u32 dispc_wb_get_framedone_irq(void)
>> -{
>> -	return DISPC_IRQ_FRAMEDONEWB;
>> +	return true;
>>  }
> 
> You can just do
> 
> return channel != OMAP_DSS_CHANNEL_DIGIT || !dispc.feat->no_framedone_tv;
> 

Sure, I'll change that.

>>  
>>  static void dispc_mgr_enable(enum omap_channel channel, bool enable)
>> @@ -3604,6 +3589,121 @@ static void dispc_write_irqenable(u32 mask)
>>  	dispc_read_reg(DISPC_IRQENABLE);
>>  }
>>  
>> +static u64 dispc_hw_to_api_irq(u32 hw)
>> +{
>> +	u64 api = 0;
>> +
>> +	if (hw & DISPC_IRQ_OCP_ERR)
>> +		api |= DSS_IRQ_DEVICE_OCP_ERR;
>> +
>> +	if (hw & DISPC_IRQ_FRAMEDONE)
>> +		api |= DSS_IRQ_MGR_FRAME_DONE(0);
>> +	if (hw & DISPC_IRQ_VSYNC)
>> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(0);
>> +	if (hw & DISPC_IRQ_SYNC_LOST)
>> +		api |= DSS_IRQ_MGR_SYNC_LOST(0);
>> +
>> +	if (hw & DISPC_IRQ_EVSYNC_EVEN)
>> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(1);
>> +	if (hw & DISPC_IRQ_EVSYNC_ODD)
>> +		api |= DSS_IRQ_MGR_VSYNC_ODD(1);
>> +	if (hw & DISPC_IRQ_SYNC_LOST_DIGIT)
>> +		api |= DSS_IRQ_MGR_SYNC_LOST(1);
>> +	if (hw & DISPC_IRQ_FRAMEDONETV)
>> +		api |= DSS_IRQ_MGR_FRAME_DONE(1);
>> +
>> +	if (hw & DISPC_IRQ_SYNC_LOST2)
>> +		api |= DSS_IRQ_MGR_SYNC_LOST(2);
>> +	if (hw & DISPC_IRQ_VSYNC2)
>> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(2);
>> +	if (hw & DISPC_IRQ_FRAMEDONE2)
>> +		api |= DSS_IRQ_MGR_FRAME_DONE(2);
>> +
>> +	if (hw & DISPC_IRQ_SYNC_LOST3)
>> +		api |= DSS_IRQ_MGR_SYNC_LOST(3);
>> +	if (hw & DISPC_IRQ_VSYNC3)
>> +		api |= DSS_IRQ_MGR_VSYNC_EVEN(3);
>> +	if (hw & DISPC_IRQ_FRAMEDONE3)
>> +		api |= DSS_IRQ_MGR_FRAME_DONE(3);
>> +
>> +	if (hw & DISPC_IRQ_GFX_FIFO_UNDERFLOW)
>> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(0);
>> +	if (hw & DISPC_IRQ_VID1_FIFO_UNDERFLOW)
>> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(1);
>> +	if (hw & DISPC_IRQ_VID2_FIFO_UNDERFLOW)
>> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(2);
>> +	if (hw & DISPC_IRQ_VID3_FIFO_UNDERFLOW)
>> +		api |= DSS_IRQ_OVL_FIFO_UNDERFLOW(3);
>> +
>> +	return api;
>> +}
>> +
>> +static u32 dispc_api_to_hw_irq(u64 api)
>> +{
>> +	u32 hw = 0;
>> +
>> +	if (api & DSS_IRQ_DEVICE_OCP_ERR)
>> +		hw |= DISPC_IRQ_OCP_ERR;
>> +
>> +	if (api & DSS_IRQ_MGR_FRAME_DONE(0))
>> +		hw |= DISPC_IRQ_FRAMEDONE;
>> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(0))
>> +		hw |= DISPC_IRQ_VSYNC;
>> +	if (api & DSS_IRQ_MGR_SYNC_LOST(0))
>> +		hw |= DISPC_IRQ_SYNC_LOST;
>> +
>> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(1))
>> +		hw |= DISPC_IRQ_EVSYNC_EVEN;
>> +	if (api & DSS_IRQ_MGR_VSYNC_ODD(1))
>> +		hw |= DISPC_IRQ_EVSYNC_ODD;
>> +	if (api & DSS_IRQ_MGR_SYNC_LOST(1))
>> +		hw |= DISPC_IRQ_SYNC_LOST_DIGIT;
>> +	if (api & DSS_IRQ_MGR_FRAME_DONE(1))
>> +		hw |= DISPC_IRQ_FRAMEDONETV;
>> +
>> +	if (api & DSS_IRQ_MGR_SYNC_LOST(2))
>> +		hw |= DISPC_IRQ_SYNC_LOST2;
>> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(2))
>> +		hw |= DISPC_IRQ_VSYNC2;
>> +	if (api & DSS_IRQ_MGR_FRAME_DONE(2))
>> +		hw |= DISPC_IRQ_FRAMEDONE2;
>> +
>> +	if (api & DSS_IRQ_MGR_SYNC_LOST(3))
>> +		hw |= DISPC_IRQ_SYNC_LOST3;
>> +	if (api & DSS_IRQ_MGR_VSYNC_EVEN(3))
>> +		hw |= DISPC_IRQ_VSYNC3;
>> +	if (api & DSS_IRQ_MGR_FRAME_DONE(3))
>> +		hw |= DISPC_IRQ_FRAMEDONE3;
>> +
>> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(0))
>> +		hw |= DISPC_IRQ_GFX_FIFO_UNDERFLOW;
>> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(1))
>> +		hw |= DISPC_IRQ_VID1_FIFO_UNDERFLOW;
>> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(2))
>> +		hw |= DISPC_IRQ_VID2_FIFO_UNDERFLOW;
>> +	if (api & DSS_IRQ_OVL_FIFO_UNDERFLOW(3))
>> +		hw |= DISPC_IRQ_VID3_FIFO_UNDERFLOW;
>> +
>> +	return hw;
>> +}
> 
> I think you could have a simple table with hw:api bit pairs. And then a
> simple for loop in both the functions above to do the work.
> 

Ok, I'll do that.

>> +
>> +static u64 dispc_api_read_irqstatus(u64 clearmask)
>> +{
>> +	u32 hw_clearmask = dispc_api_to_hw_irq(clearmask);
>> +	u32 hw_status = dispc_read_irqstatus();
>> +
>> +	dispc_clear_irqstatus(hw_clearmask & hw_status);
>> +
>> +	return dispc_hw_to_api_irq(hw_status);
>> +}
> 
> I think we always want to read the whole irqstatus, and clear it. You
> can do that with the function above, but I'm not sure if clearmask
> offers us anything we ever need to use.
> 

But the semantically correct way is to clear only the interrupts we
handle. In theory there could be some other entity following some other
interrupts and clearing the interrupts we are not handling could ruin
that. But in practice at the moment everything would work fine without
the clearmask too. Is that reason enough to remove it?

> Also, this function also clears the irqstatus, so perhaps rename the
> function to read_and_clear_irqstatus to make it obvious, as it's quite
> an important side effect.
> 

Sure, I'll change that.

>> +
>> +static void dispc_api_write_irqenable(u64 enable)
>> +{
>> +	u32 hw_enable = dispc_api_to_hw_irq(enable);
>> +
>> +	dispc_write_irqenable(hw_enable);
>> +}
>> +
>>  void dispc_enable_sidle(void)
>>  {
>>  	REG_FLD_MOD(DISPC_SYSCONFIG, 2, 4, 3);	/* SIDLEMODE: smart idle */
>> @@ -4453,7 +4553,7 @@ static void dispc_errata_i734_wa_fini(void)
>>  
>>  static void dispc_errata_i734_wa(void)
>>  {
>> -	u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD);
>> +	u32 framedone_irq = DISPC_IRQ_FRAMEDONE;
>>  	struct omap_overlay_info ovli;
>>  	struct dss_lcd_mgr_config lcd_conf;
>>  	u32 gatestate;
>> @@ -4511,9 +4611,8 @@ static void dispc_errata_i734_wa(void)
>>  }
>>  
>>  static const struct dispc_ops dispc_ops = {
>> -	.read_irqstatus = dispc_read_irqstatus,
>> -	.clear_irqstatus = dispc_clear_irqstatus,
>> -	.write_irqenable = dispc_write_irqenable,
>> +	.read_irqstatus = dispc_api_read_irqstatus,
>> +	.write_irqenable = dispc_api_write_irqenable,
>>  
>>  	.request_irq = dispc_request_irq,
>>  	.free_irq = dispc_free_irq,
>> @@ -4527,13 +4626,12 @@ static void dispc_errata_i734_wa(void)
>>  	.get_ovl_name = dispc_get_ovl_name,
>>  	.get_mgr_name = dispc_get_mgr_name,
>>  
>> +	.mgr_has_framedone = dispc_mgr_has_framedone,
>> +
>>  	.get_memory_bandwidth_limit = dispc_get_memory_bandwidth_limit,
>>  
>>  	.mgr_enable = dispc_mgr_enable,
>>  	.mgr_is_enabled = dispc_mgr_is_enabled,
>> -	.mgr_get_vsync_irq = dispc_mgr_get_vsync_irq,
>> -	.mgr_get_framedone_irq = dispc_mgr_get_framedone_irq,
>> -	.mgr_get_sync_lost_irq = dispc_mgr_get_sync_lost_irq,
>>  	.mgr_go_busy = dispc_mgr_go_busy,
>>  	.mgr_go = dispc_mgr_go,
>>  	.mgr_set_lcd_config = dispc_mgr_set_lcd_config,
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.h b/drivers/gpu/drm/omapdrm/dss/dispc.h
>> index e901dd1..e71266e 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.h
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.h
>> @@ -18,6 +18,39 @@
>>  #ifndef __OMAP2_DISPC_REG_H
>>  #define __OMAP2_DISPC_REG_H
>>  
>> +/* DISPC IRQ bits */
>> +#define DISPC_IRQ_FRAMEDONE		(1 << 0)
>> +#define DISPC_IRQ_VSYNC			(1 << 1)
>> +#define DISPC_IRQ_EVSYNC_EVEN		(1 << 2)
>> +#define DISPC_IRQ_EVSYNC_ODD		(1 << 3)
>> +#define DISPC_IRQ_ACBIAS_COUNT_STAT	(1 << 4)
>> +#define DISPC_IRQ_PROG_LINE_NUM		(1 << 5)
>> +#define DISPC_IRQ_GFX_FIFO_UNDERFLOW	(1 << 6)
>> +#define DISPC_IRQ_GFX_END_WIN		(1 << 7)
>> +#define DISPC_IRQ_PAL_GAMMA_MASK	(1 << 8)
>> +#define DISPC_IRQ_OCP_ERR		(1 << 9)
>> +#define DISPC_IRQ_VID1_FIFO_UNDERFLOW	(1 << 10)
>> +#define DISPC_IRQ_VID1_END_WIN		(1 << 11)
>> +#define DISPC_IRQ_VID2_FIFO_UNDERFLOW	(1 << 12)
>> +#define DISPC_IRQ_VID2_END_WIN		(1 << 13)
>> +#define DISPC_IRQ_SYNC_LOST		(1 << 14)
>> +#define DISPC_IRQ_SYNC_LOST_DIGIT	(1 << 15)
>> +#define DISPC_IRQ_WAKEUP		(1 << 16)
>> +#define DISPC_IRQ_SYNC_LOST2		(1 << 17)
>> +#define DISPC_IRQ_VSYNC2		(1 << 18)
>> +#define DISPC_IRQ_VID3_END_WIN		(1 << 19)
>> +#define DISPC_IRQ_VID3_FIFO_UNDERFLOW	(1 << 20)
>> +#define DISPC_IRQ_ACBIAS_COUNT_STAT2	(1 << 21)
>> +#define DISPC_IRQ_FRAMEDONE2		(1 << 22)
>> +#define DISPC_IRQ_FRAMEDONEWB		(1 << 23)
>> +#define DISPC_IRQ_FRAMEDONETV		(1 << 24)
>> +#define DISPC_IRQ_WBBUFFEROVERFLOW	(1 << 25)
>> +#define DISPC_IRQ_WBUNCOMPLETEERROR	(1 << 26)
>> +#define DISPC_IRQ_SYNC_LOST3		(1 << 27)
>> +#define DISPC_IRQ_VSYNC3		(1 << 28)
>> +#define DISPC_IRQ_ACBIAS_COUNT_STAT3	(1 << 29)
>> +#define DISPC_IRQ_FRAMEDONE3		(1 << 30)
>> +
>>  /* DISPC common registers */
>>  #define DISPC_REVISION			0x0000
>>  #define DISPC_SYSCONFIG			0x0010
>> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
>> index d7ed1a4..b1499c2 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
>> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
>> @@ -27,37 +27,29 @@
>>  #include <uapi/drm/drm_mode.h>
>>  #include <drm/drm_crtc.h>
>>  
>> -#define DISPC_IRQ_FRAMEDONE		(1 << 0)
>> -#define DISPC_IRQ_VSYNC			(1 << 1)
>> -#define DISPC_IRQ_EVSYNC_EVEN		(1 << 2)
>> -#define DISPC_IRQ_EVSYNC_ODD		(1 << 3)
>> -#define DISPC_IRQ_ACBIAS_COUNT_STAT	(1 << 4)
>> -#define DISPC_IRQ_PROG_LINE_NUM		(1 << 5)
>> -#define DISPC_IRQ_GFX_FIFO_UNDERFLOW	(1 << 6)
>> -#define DISPC_IRQ_GFX_END_WIN		(1 << 7)
>> -#define DISPC_IRQ_PAL_GAMMA_MASK	(1 << 8)
>> -#define DISPC_IRQ_OCP_ERR		(1 << 9)
>> -#define DISPC_IRQ_VID1_FIFO_UNDERFLOW	(1 << 10)
>> -#define DISPC_IRQ_VID1_END_WIN		(1 << 11)
>> -#define DISPC_IRQ_VID2_FIFO_UNDERFLOW	(1 << 12)
>> -#define DISPC_IRQ_VID2_END_WIN		(1 << 13)
>> -#define DISPC_IRQ_SYNC_LOST		(1 << 14)
>> -#define DISPC_IRQ_SYNC_LOST_DIGIT	(1 << 15)
>> -#define DISPC_IRQ_WAKEUP		(1 << 16)
>> -#define DISPC_IRQ_SYNC_LOST2		(1 << 17)
>> -#define DISPC_IRQ_VSYNC2		(1 << 18)
>> -#define DISPC_IRQ_VID3_END_WIN		(1 << 19)
>> -#define DISPC_IRQ_VID3_FIFO_UNDERFLOW	(1 << 20)
>> -#define DISPC_IRQ_ACBIAS_COUNT_STAT2	(1 << 21)
>> -#define DISPC_IRQ_FRAMEDONE2		(1 << 22)
>> -#define DISPC_IRQ_FRAMEDONEWB		(1 << 23)
>> -#define DISPC_IRQ_FRAMEDONETV		(1 << 24)
>> -#define DISPC_IRQ_WBBUFFEROVERFLOW	(1 << 25)
>> -#define DISPC_IRQ_WBUNCOMPLETEERROR	(1 << 26)
>> -#define DISPC_IRQ_SYNC_LOST3		(1 << 27)
>> -#define DISPC_IRQ_VSYNC3		(1 << 28)
>> -#define DISPC_IRQ_ACBIAS_COUNT_STAT3	(1 << 29)
>> -#define DISPC_IRQ_FRAMEDONE3		(1 << 30)
>> +#define DSS_MAX_CHANNELS 8
>> +#define DSS_MAX_OVLS 8
>> +
>> +#define DSS_IRQ_DEVICE_OCP_ERR		BIT_ULL(0)
>> +
>> +#define DSS_IRQ_MGR_BIT_N(ch, bit)	(4 + 4 * ch + bit)
>> +#define DSS_IRQ_OVL_BIT_N(ovl, bit) \
>> +	(DSS_IRQ_MGR_BIT_N(DSS_MAX_CHANNELS, 0) + 1 * ovl + bit)
>> +
>> +#define DSS_IRQ_MGR_BIT(ch, bit)	BIT_ULL(DSS_IRQ_MGR_BIT_N(ch, bit))
>> +#define DSS_IRQ_OVL_BIT(ovl, bit)	BIT_ULL(DSS_IRQ_OVL_BIT_N(ovl, bit))
>> +
>> +#define DSS_IRQ_MGR_MASK(ch) \
>> +	GENMASK_ULL(DSS_IRQ_MGR_BIT_N(ch, 3), DSS_IRQ_MGR_BIT_N(ch, 0))
>> +#define DSS_IRQ_OVL_MASK(ovl) \
>> +	GENMASK_ULL(DSS_IRQ_OVL_BIT_N(ovl, 0), DSS_IRQ_OVL_BIT_N(ovl, 0))
>> +
>> +#define DSS_IRQ_MGR_FRAME_DONE(ch)	DSS_IRQ_MGR_BIT(ch, 0)
>> +#define DSS_IRQ_MGR_VSYNC_EVEN(ch)	DSS_IRQ_MGR_BIT(ch, 1)
>> +#define DSS_IRQ_MGR_VSYNC_ODD(ch)	DSS_IRQ_MGR_BIT(ch, 2)
>> +#define DSS_IRQ_MGR_SYNC_LOST(ch)	DSS_IRQ_MGR_BIT(ch, 3)
>> +
>> +#define DSS_IRQ_OVL_FIFO_UNDERFLOW(ovl)	DSS_IRQ_OVL_BIT(ovl, 0)
>>  
>>  struct omap_dss_device;
>>  struct dss_lcd_mgr_config;
>> @@ -678,9 +670,8 @@ void dss_mgr_unregister_framedone_handler(enum omap_channel channel,
>>  /* dispc ops */
>>  
>>  struct dispc_ops {
>> -	u32 (*read_irqstatus)(void);
>> -	void (*clear_irqstatus)(u32 mask);
>> -	void (*write_irqenable)(u32 mask);
>> +	u64 (*read_irqstatus)(u64 clearmask);
>> +	void (*write_irqenable)(u64 enable);
>>  
>>  	int (*request_irq)(irq_handler_t handler, void *dev_id);
>>  	void (*free_irq)(void *dev_id);
>> @@ -694,13 +685,12 @@ struct dispc_ops {
>>  	const char *(*get_ovl_name)(enum omap_plane_id plane);
>>  	const char *(*get_mgr_name)(enum omap_channel channel);
>>  
>> +	bool (*mgr_has_framedone)(enum omap_channel channel);
>> +
>>  	u32 (*get_memory_bandwidth_limit)(void);
>>  
>>  	void (*mgr_enable)(enum omap_channel channel, bool enable);
>>  	bool (*mgr_is_enabled)(enum omap_channel channel);
>> -	u32 (*mgr_get_vsync_irq)(enum omap_channel channel);
>> -	u32 (*mgr_get_framedone_irq)(enum omap_channel channel);
>> -	u32 (*mgr_get_sync_lost_irq)(enum omap_channel channel);
>>  	bool (*mgr_go_busy)(enum omap_channel channel);
>>  	void (*mgr_go)(enum omap_channel channel);
>>  	void (*mgr_set_lcd_config)(enum omap_channel channel,
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> index fee8a63..f7e1668 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -149,7 +149,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>>  	enum omap_channel channel = omap_crtc->channel;
>>  	struct omap_irq_wait *wait;
>> -	u32 framedone_irq, vsync_irq;
>> +	u64 vsync_irq, framedone_irq;
>>  	int ret;
>>  
>>  	if (WARN_ON(omap_crtc->enabled == enable))
>> @@ -169,8 +169,10 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>>  		omap_crtc->ignore_digit_sync_lost = true;
>>  	}
>>  
>> -	framedone_irq = priv->dispc_ops->mgr_get_framedone_irq(channel);
>> -	vsync_irq = priv->dispc_ops->mgr_get_vsync_irq(channel);
>> +
>> +	vsync_irq = (DSS_IRQ_MGR_VSYNC_EVEN(channel) |
>> +		     DSS_IRQ_MGR_VSYNC_ODD(channel));
>> +	framedone_irq = DSS_IRQ_MGR_FRAME_DONE(channel);
>>  
>>  	if (enable) {
>>  		wait = omap_irq_wait_init(dev, vsync_irq, 1);
>> @@ -184,7 +186,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>>  		 * even and odd frames.
>>  		 */
>>  
>> -		if (framedone_irq)
>> +		if (priv->dispc_ops->mgr_has_framedone(channel))
>>  			wait = omap_irq_wait_init(dev, framedone_irq, 1);
>>  		else
>>  			wait = omap_irq_wait_init(dev, vsync_irq, 2);
>> @@ -272,17 +274,17 @@ static void omap_crtc_dss_unregister_framedone(
>>   * Setup, Flush and Page Flip
>>   */
>>  
>> -void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus)
>> +void omap_crtc_error_irq(struct drm_crtc *crtc, u64 irqstatus)
>>  {
>>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>>  
>>  	if (omap_crtc->ignore_digit_sync_lost) {
>> -		irqstatus &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
>> +		irqstatus &= ~DSS_IRQ_MGR_SYNC_LOST(omap_crtc->channel);
>>  		if (!irqstatus)
>>  			return;
>>  	}
>>  
>> -	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus);
>> +	DRM_ERROR_RATELIMITED("%s: errors: %016llx\n", omap_crtc->name, irqstatus);
>>  }
>>  
>>  void omap_crtc_vblank_irq(struct drm_crtc *crtc)
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.h b/drivers/gpu/drm/omapdrm/omap_crtc.h
>> index ad7b007..55e2e02 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.h
>> @@ -37,7 +37,7 @@
>>  struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>>  		struct drm_plane *plane, struct omap_dss_device *dssdev);
>>  int omap_crtc_wait_pending(struct drm_crtc *crtc);
>> -void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus);
>> +void omap_crtc_error_irq(struct drm_crtc *crtc, u64 irqstatus);
>>  void omap_crtc_vblank_irq(struct drm_crtc *crtc);
>>  
>>  #endif /* __OMAPDRM_CRTC_H__ */
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
>> index 0ac97fe..1a479ea 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
>> @@ -81,8 +81,7 @@ struct omap_drm_private {
>>  	/* irq handling: */
>>  	spinlock_t wait_lock;		/* protects the wait_list */
>>  	struct list_head wait_list;	/* list of omap_irq_wait */
>> -	uint32_t irq_mask;		/* enabled irqs in addition to wait_list */
>> -
>> +	u64 irq_mask;			/* enabled irqs in addition to wait_list */
>>  	/* memory bandwidth limit if it is needed on the platform */
>>  	unsigned int max_bandwidth;
>>  };
>> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
>> index b0f6850..57ea08e 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
>> @@ -20,25 +20,24 @@
>>  struct omap_irq_wait {
>>  	struct list_head node;
>>  	wait_queue_head_t wq;
>> -	uint32_t irqmask;
>> +	u64 irqmask;
>>  	int count;
>>  };
>>  
>>  /* call with wait_lock and dispc runtime held */
>> -static void omap_irq_update(struct drm_device *dev)
>> +static void omap_irq_full_mask(struct drm_device *dev, u64 *irqmask)
>>  {
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	struct omap_irq_wait *wait;
>> -	uint32_t irqmask = priv->irq_mask;
>>  
>>  	assert_spin_locked(&priv->wait_lock);
>>  
>> -	list_for_each_entry(wait, &priv->wait_list, node)
>> -		irqmask |= wait->irqmask;
>> +	*irqmask = priv->irq_mask;
>>  
>> -	DBG("irqmask=%08x", irqmask);
>> +	list_for_each_entry(wait, &priv->wait_list, node)
>> +		*irqmask |= wait->irqmask;
>>  
>> -	priv->dispc_ops->write_irqenable(irqmask);
>> +	DBG("irqmask 0x%016llx", *irqmask);
>>  }
>>  
>>  static void omap_irq_wait_handler(struct omap_irq_wait *wait)
>> @@ -48,19 +47,24 @@ static void omap_irq_wait_handler(struct omap_irq_wait *wait)
>>  }
>>  
>>  struct omap_irq_wait * omap_irq_wait_init(struct drm_device *dev,
>> -		uint32_t irqmask, int count)
>> +		u64 waitmask, int count)
>>  {
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	struct omap_irq_wait *wait = kzalloc(sizeof(*wait), GFP_KERNEL);
>>  	unsigned long flags;
>> +	u64 irqmask;
>> +
>> +	if (!wait)
>> +		return NULL;
>>  
>>  	init_waitqueue_head(&wait->wq);
>> -	wait->irqmask = irqmask;
>> +	wait->irqmask = waitmask;
>>  	wait->count = count;
>>  
>>  	spin_lock_irqsave(&priv->wait_lock, flags);
>>  	list_add(&wait->node, &priv->wait_list);
>> -	omap_irq_update(dev);
>> +	omap_irq_full_mask(dev, &irqmask);
>> +	priv->dispc_ops->write_irqenable(irqmask);
>>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>>  
>>  	return wait;
>> @@ -71,13 +75,15 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
>>  {
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	unsigned long flags;
>> +	u64 irqmask;
>>  	int ret;
>>  
>>  	ret = wait_event_timeout(wait->wq, (wait->count <= 0), timeout);
>>  
>>  	spin_lock_irqsave(&priv->wait_lock, flags);
>>  	list_del(&wait->node);
>> -	omap_irq_update(dev);
>> +	omap_irq_full_mask(dev, &irqmask);
>> +	priv->dispc_ops->write_irqenable(irqmask);
>>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>>  
>>  	kfree(wait);
>> @@ -104,12 +110,15 @@ int omap_irq_enable_vblank(struct drm_crtc *crtc)
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	unsigned long flags;
>>  	enum omap_channel channel = omap_crtc_channel(crtc);
>> +	u64 irqmask;
>>  
>>  	DBG("dev=%p, crtc=%u", dev, channel);
>>  
>>  	spin_lock_irqsave(&priv->wait_lock, flags);
>> -	priv->irq_mask |= priv->dispc_ops->mgr_get_vsync_irq(channel);
>> -	omap_irq_update(dev);
>> +	priv->irq_mask |= DSS_IRQ_MGR_VSYNC_EVEN(channel) |
>> +		DSS_IRQ_MGR_VSYNC_ODD(channel);
>> +	omap_irq_full_mask(dev, &irqmask);
>> +	priv->dispc_ops->write_irqenable(irqmask);
>>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>>  
>>  	return 0;
>> @@ -130,36 +139,36 @@ void omap_irq_disable_vblank(struct drm_crtc *crtc)
>>  	struct omap_drm_private *priv = dev->dev_private;
>>  	unsigned long flags;
>>  	enum omap_channel channel = omap_crtc_channel(crtc);
>> +	u64 irqmask;
>>  
>>  	DBG("dev=%p, crtc=%u", dev, channel);
>>  
>>  	spin_lock_irqsave(&priv->wait_lock, flags);
>> -	priv->irq_mask &= ~priv->dispc_ops->mgr_get_vsync_irq(channel);
>> -	omap_irq_update(dev);
>> +	priv->irq_mask &= ~(DSS_IRQ_MGR_VSYNC_EVEN(channel) |
>> +			    DSS_IRQ_MGR_VSYNC_ODD(channel));
>> +	omap_irq_full_mask(dev, &irqmask);
>> +	priv->dispc_ops->write_irqenable(irqmask);
>>  	spin_unlock_irqrestore(&priv->wait_lock, flags);
>>  }
>>  
>>  static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>> -				    u32 irqstatus)
>> +				    u64 irqstatus)
>>  {
>>  	static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
>>  				      DEFAULT_RATELIMIT_BURST);
>> -	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
>> -		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
>> -		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
>> +	static const u64 uf_mask =
>> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(0) | DSS_IRQ_OVL_FIFO_UNDERFLOW(1) |
>> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(2) | DSS_IRQ_OVL_FIFO_UNDERFLOW(3) |
>> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(4) | DSS_IRQ_OVL_FIFO_UNDERFLOW(5) |
>> +		DSS_IRQ_OVL_FIFO_UNDERFLOW(6) |	DSS_IRQ_OVL_FIFO_UNDERFLOW(7);
> 
> Use a for loop to setup the mask. With the above, if in the future we
> change the irq bits and, say, change the num of ovls to 4, the above
> will break.
> 
>>  	unsigned int i;
>> +	u64 masked;
>>  
>>  	spin_lock(&priv->wait_lock);
>> -	irqstatus &= priv->irq_mask & mask;
>> +	masked = irqstatus & uf_mask & priv->irq_mask;
>>  	spin_unlock(&priv->wait_lock);
>>  
>> -	if (!irqstatus)
>> +	if (!masked)
>>  		return;
>>  
>>  	if (!__ratelimit(&_rs))
>> @@ -167,21 +176,19 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>>  
>>  	DRM_ERROR("FIFO underflow on ");
>>  
>> -	for (i = 0; i < ARRAY_SIZE(irqbits); ++i) {
>> -		if (irqbits[i] & irqstatus)
>> -			pr_cont("%s ", priv->dispc_ops->get_ovl_name(i));
>> +	for (i = 0; i < DSS_MAX_OVLS; ++i) {
>> +		if (masked & DSS_IRQ_OVL_FIFO_UNDERFLOW(i))
>> +			pr_cont("%u:%s ", i, priv->dispc_ops->get_ovl_name(i));
>>  	}
>>  
>> -	pr_cont("(0x%08x)\n", irqstatus);
>> +	pr_cont("(%016llx)\n", irqstatus);
>>  }
>>  
>>  static void omap_irq_ocp_error_handler(struct drm_device *dev,
>> -	u32 irqstatus)
>> +				       u64 irqstatus)
>>  {
>> -	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
>> -		return;
>> -
>> -	dev_err_ratelimited(dev->dev, "OCP error\n");
>> +	if (irqstatus & DSS_IRQ_DEVICE_OCP_ERR)
>> +		dev_err_ratelimited(dev->dev, "OCP error\n");
>>  }
>>  
>>  static irqreturn_t omap_irq_handler(int irq, void *arg)
>> @@ -191,47 +198,40 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
>>  	struct omap_irq_wait *wait, *n;
>>  	unsigned long flags;
>>  	unsigned int id;
>> -	u32 irqstatus;
>> +	u64 irqstatus, clearmask;
>>  
>> -	irqstatus = priv->dispc_ops->read_irqstatus();
>> -	priv->dispc_ops->clear_irqstatus(irqstatus);
>> -	priv->dispc_ops->read_irqstatus();        /* flush posted write */
>> +	spin_lock_irqsave(&priv->wait_lock, flags);
>> +	omap_irq_full_mask(dev, &clearmask);
>> +	irqstatus = priv->dispc_ops->read_irqstatus(clearmask);
>> +
>> +	list_for_each_entry_safe(wait, n, &priv->wait_list, node) {
>> +		if (irqstatus & wait->irqmask)
>> +			omap_irq_wait_handler(wait);
>> +	}
>> +	spin_unlock_irqrestore(&priv->wait_lock, flags);
>>  
>> -	VERB("irqs: %08x", irqstatus);
>> +	VERB("irqs: 0x%016llx\n", irqstatus);
>>  
>>  	for (id = 0; id < priv->num_crtcs; id++) {
>>  		struct drm_crtc *crtc = priv->crtcs[id];
>>  		enum omap_channel channel = omap_crtc_channel(crtc);
>>  
>> -		if (irqstatus & priv->dispc_ops->mgr_get_vsync_irq(channel)) {
>> +		if (irqstatus & (DSS_IRQ_MGR_VSYNC_EVEN(channel) |
>> +				 DSS_IRQ_MGR_VSYNC_ODD(channel))) {
>>  			drm_handle_vblank(dev, id);
>>  			omap_crtc_vblank_irq(crtc);
>>  		}
>>  
>> -		if (irqstatus & priv->dispc_ops->mgr_get_sync_lost_irq(channel))
>> +		if (irqstatus & DSS_IRQ_MGR_SYNC_LOST(channel))
>>  			omap_crtc_error_irq(crtc, irqstatus);
>>  	}
>>  
>>  	omap_irq_ocp_error_handler(dev, irqstatus);
>>  	omap_irq_fifo_underflow(priv, irqstatus);
>>  
>> -	spin_lock_irqsave(&priv->wait_lock, flags);
>> -	list_for_each_entry_safe(wait, n, &priv->wait_list, node) {
>> -		if (wait->irqmask & irqstatus)
>> -			omap_irq_wait_handler(wait);
>> -	}
>> -	spin_unlock_irqrestore(&priv->wait_lock, flags);
>> -
> 
> Why did you change the location where wait_handler is being called?
> 

So that I would not need to take the &priv->wait_lock twice. First for
calculating the proper clearmask and the again for checking the wait_list.

Cheers,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/omap: Make omapdss API more generic

[Tomi Valkeinen]

On 07/01/18 22:14, Jyri Sarha wrote:

>>> +static u64 dispc_api_read_irqstatus(u64 clearmask)
>>> +{
>>> +	u32 hw_clearmask = dispc_api_to_hw_irq(clearmask);
>>> +	u32 hw_status = dispc_read_irqstatus();
>>> +
>>> +	dispc_clear_irqstatus(hw_clearmask & hw_status);
>>> +
>>> +	return dispc_hw_to_api_irq(hw_status);
>>> +}
>>
>> I think we always want to read the whole irqstatus, and clear it. You
>> can do that with the function above, but I'm not sure if clearmask
>> offers us anything we ever need to use.
>>
> 
> But the semantically correct way is to clear only the interrupts we
> handle. In theory there could be some other entity following some other
> interrupts and clearing the interrupts we are not handling could ruin

If that would be the case, then we need to ensure that the irqenable and
irqstatus are handled race free, and we need to make sure those entities
never touch the same bits, even if they'd use proper locking.

Yes, it's possible, but we don't do it and I hope we never do.

> that. But in practice at the moment everything would work fine without
> the clearmask too. Is that reason enough to remove it?

Well, I would ask if there's enough reason to add it? What's the
scenario you see that it would be used (not theoretical, but real one)?
Or is there any other downside to just clearing all irqstatus bits?

My main worry with this is that we somehow (well, bug) would end up
having a dispc irq enabled in irqenable, and we would not have that bit
in clearmask. The end result would be an endless irq flood.

If in the future there's a use case that needs this, it would be trivial
to add it at that point of time. So my guideline would be to keep things
as simple as possible, and only add unused features if there's a
realistic near-future use case for it which you know you'll be implementing.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/omap: Make omapdss API more generic + related patches

[Tomi Valkeinen]

On 01/01/18 13:55, Jyri Sarha wrote:
> This the v2 rouns of a this RFC patch:
> https://patchwork.kernel.org/patch/10066245/
> 
> The first patch is a simple fix that should be applied in any case.
> 
> I did not split the mgr_has_framedone() callback as a separate patch. It
> quite literally replaces the mgr_get_framedone_irq() and makes no
> sense without the "drm/omap: Make omapdss API more generic"-patch.
> 
> The patches have been rebased on top of the latest drm-next
> (350877626faba5d60cbb8cef2bdeb524212c780b).
> 
> Best regards,
> Jyri
> 
> Jyri Sarha (3):
>   drm/omap: Fail probe if irq registration fails
>   drm/omap: Add get_ovl_name() and get_mgr_name() to dispc_ops
>   drm/omap: Make omapdss API more generic

I think the first two patches look fine. The third patch is, of course,
the interesting one. I did have some comments for that.

To be honest, I'm not sure what kind of irqhandling we'll end up with
when we get the omapdss & omapdrm merge completed and add support for
DSS6. But I still think this cleans up things. So even if in the end
we'd end up not using common irq code, the way there will be easier with
these changes.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel