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

[Jyri Sarha <jsarha@ti.com>]

Argh, sorry I forgot about these in the previous post. (And the comment
for "drm/omap: Fail probe if irq registration fails").

On 02/08/18 10:53, Tomi Valkeinen wrote:
> On 07/02/18 16:11, 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. The 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>
> <snip>
>
> Can you add a comment here that describes the layout of the u64 irq bits
> container.
Would this do:

/*
 * Based on the above 2 defines the bellow defines describe following
 * u64 IRQ bits:
 * bit group |dev |mrg0|mrg1|mrg2|mrg3|mrg4|mrg5|mrg6|mrg7| olv0-7 
|<unused> |
 * bit use   |D   |FEOL|FEOL|FEOL|FEOL|FEOL|FEOL|FEOL|FEOL|UUUU|UUUU| |
| | | |
 * bit number|0-3 |4-7 |8-11|            12-35            |  36-43  | 
44-63  |
 *
 * device bits: D = OCP error
 * mgr bits:    F = frame done, E = vsync even, O = vsync odd,
 * ovl bits:    U = fifo underflow
 */

>> +#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_and_clear_irqstatus)(void);
>> +	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))
> Is it valid to do DSS_IRQ_MGR_FRAME_DONE(channel) above, even if there's
> no framedone irq for the channel? Well, I know it won't crash, but from
> code-cleanliness point of view, perhaps it's better to first check if we
> have a framedone, and only then get it.

Yes it is valid. Those are the generic IRQ bits, the mgr_has_framedone()
just checks whether the back end can produce such and IRQ. But certainly
I can remove the framedone_irq variable and use literal in
omap_irq_wait_init(), if you like.
>>  			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..22f88b5 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
>> @@ -81,7 +81,8 @@ 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 */
>> +	u64 irq_uf_mask;		/* underflow irq bits for all planes */
>>  
>>  	/* 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..a411ef2 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);
>>  }
> Wouldn't it make more sense for omap_irq_full_mask() to return the mask?

Yes it would. I'll change that.

>  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