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@xxxxxx>
> <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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
