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
