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 <jsa...@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

Reply via email to