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. > +#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. > 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? 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