On 08/02/18 13:43, Jyri Sarha wrote:
> 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
> */
I think it would be enough just saying that first 4 bits are reserved
for generic irqs, then 8 managers each with 4 bits, and then 8 overlays
with 1 bit. The defines below are readable, but if there's a comment
with a hint on how to decipher them, everything becomes much clearer.
But now that you already wrote a full description, it's fine =).
Although more work to change it later.
>>> +#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.
Ok. Yes, I think it's ok as it is. Or, well, I think framedone_irq
variable is only used once, so... perhaps the whole variable can be dropped.
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
