Re: [PATCH 6/9] drm/amdgpu: separate IH and IRQ funcs

["Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>]

Am 26.09.2018 um 11:16 schrieb Huang, Ray:
>> -----Original Message-----
>> From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com]
>> Sent: Wednesday, September 26, 2018 4:09 PM
>> To: Huang, Ray <Ray.Huang@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH 6/9] drm/amdgpu: separate IH and IRQ funcs
>>
>> Am 26.09.2018 um 08:05 schrieb Huang Rui:
>>> On Mon, Sep 24, 2018 at 02:38:17PM +0200, Christian König wrote:
>>>> One for the ring buffer and one for the IV handling.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> Reviewed-by: Huang Rui <ray.huang@amd.com>
>>>
>>> How about merge amdgpu_ih.c into amdgpu_irq.c? As I think, we don't
>>> need two common interrupt handle files. IH is actually the hw ip block
>>> name, and the meaning is actually the same with irq. We can put all
>>> common irq handle include ih ring init functions into irq.c. If you
>>> also agree, I will file the patch.
>> I actually dropped this patch, but we certainly need two different files.
>>
>> The one is for the IH ring buffer, which Vega10 actually has 3 of.
>>
>> The other one is for the IRQ handling which does the tracking how often
>> interrupt sources are enabled, how to route them etc...
>>
>> E.g. for that we would still have one instance per device.
>>
> So you want to separate the hardware-layer abstraction function and IRQ handling for upper-layer on different file?

Yes, exactly.

> That's also fine.

I'm just not sure about the naming of files, functions and structures. 
Any better suggestion would be welcome.

Christian.

>
> Thanks,
> Ray
>
>> Regards,
>> Christian.
>>
>>> Thanks,
>>> Ray
>>>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h  | 11 ++++-------
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  4 ++--
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h | 11 ++++++++++-
>>>>    drivers/gpu/drm/amd/amdgpu/cik_ih.c     |  8 ++++++--
>>>>    drivers/gpu/drm/amd/amdgpu/cz_ih.c      |  8 ++++++--
>>>>    drivers/gpu/drm/amd/amdgpu/iceland_ih.c |  8 ++++++--
>>>>    drivers/gpu/drm/amd/amdgpu/si_ih.c      |  8 ++++++--
>>>>    drivers/gpu/drm/amd/amdgpu/tonga_ih.c   |  8 ++++++--
>>>>    drivers/gpu/drm/amd/amdgpu/vega10_ih.c  |  8 ++++++--
>>>>    9 files changed, 52 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>> index 9ce8c93ec19b..d88f82321ee4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.h
>>>> @@ -45,22 +45,19 @@ struct amdgpu_ih_ring {
>>>>    	bool			use_doorbell;
>>>>    	bool			use_bus_addr;
>>>>    	dma_addr_t		rb_dma_addr; /* only used when
>> use_bus_addr = true */
>>>> +
>>>> +	const struct amdgpu_ih_funcs	*funcs;
>>>>    };
>>>>
>>>>    /* provided by the ih block */
>>>>    struct amdgpu_ih_funcs {
>>>>    	/* ring read/write ptr handling, called from interrupt context */
>>>>    	u32 (*get_wptr)(struct amdgpu_device *adev);
>>>> -	bool (*prescreen_iv)(struct amdgpu_device *adev);
>>>> -	void (*decode_iv)(struct amdgpu_device *adev,
>>>> -			  struct amdgpu_iv_entry *entry);
>>>>    	void (*set_rptr)(struct amdgpu_device *adev);
>>>>    };
>>>>
>>>> -#define amdgpu_ih_get_wptr(adev)
>>>> (adev)->irq.ih_funcs->get_wptr((adev))
>>>> -#define amdgpu_ih_prescreen_iv(adev)
>>>> (adev)->irq.ih_funcs->prescreen_iv((adev))
>>>> -#define amdgpu_ih_decode_iv(adev, iv)
>>>> (adev)->irq.ih_funcs->decode_iv((adev), (iv)) -#define
>>>> amdgpu_ih_set_rptr(adev) (adev)->irq.ih_funcs->set_rptr((adev))
>>>> +#define amdgpu_ih_get_wptr(adev)
>>>> +(adev)->irq.ih.funcs->get_wptr((adev))
>>>> +#define amdgpu_ih_set_rptr(adev)
>>>> +(adev)->irq.ih.funcs->set_rptr((adev))
>>>>
>>>>    int amdgpu_ih_ring_init(struct amdgpu_device *adev, struct
>> amdgpu_ih_ring *ih,
>>>>    			unsigned ring_size, bool use_bus_addr); diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>>> index 52c17f6219a7..8e5ce25f3fe1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>>>> @@ -163,14 +163,14 @@ static void amdgpu_irq_callback(struct
>> amdgpu_device *adev,
>>>>    	struct amdgpu_iv_entry entry;
>>>>
>>>>    	/* Prescreening of high-frequency interrupts */
>>>> -	if (!amdgpu_ih_prescreen_iv(adev))
>>>> +	if (!amdgpu_irq_prescreen_iv(adev))
>>>>    		return;
>>>>
>>>>    	/* Before dispatching irq to IP blocks, send it to amdkfd */
>>>>    	amdgpu_amdkfd_interrupt(adev, (const void *)
>>>> &ih->ring[ring_index]);
>>>>
>>>>    	entry.iv_entry = (const uint32_t *)&ih->ring[ring_index];
>>>> -	amdgpu_ih_decode_iv(adev, &entry);
>>>> +	amdgpu_irq_decode_iv(adev, &entry);
>>>>
>>>>    	amdgpu_irq_dispatch(adev, &entry);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>>>> index f6ce171cb8aa..3cc0e7ce40a0 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>>>> @@ -68,6 +68,12 @@ struct amdgpu_irq_client {
>>>>    	struct amdgpu_irq_src **sources;
>>>>    };
>>>>
>>>> +struct amdgpu_irq_funcs {
>>>> +	bool (*prescreen_iv)(struct amdgpu_device *adev);
>>>> +	void (*decode_iv)(struct amdgpu_device *adev,
>>>> +			  struct amdgpu_iv_entry *entry); };
>>>> +
>>>>    /* provided by interrupt generating IP blocks */
>>>>    struct amdgpu_irq_src_funcs {
>>>>    	int (*set)(struct amdgpu_device *adev, struct amdgpu_irq_src
>>>> *source, @@ -89,12 +95,12 @@ struct amdgpu_irq {
>>>>
>>>>    	/* interrupt ring */
>>>>    	struct amdgpu_ih_ring		ih;
>>>> -	const struct amdgpu_ih_funcs	*ih_funcs;
>>>>
>>>>    	/* gen irq stuff */
>>>>    	struct irq_domain		*domain; /* GPU irq controller
>> domain */
>>>>    	unsigned			virq[AMDGPU_MAX_IRQ_SRC_ID];
>>>>    	uint32_t                        srbm_soft_reset;
>>>> +	const struct amdgpu_irq_funcs	*funcs;
>>>>    };
>>>>
>>>>    void amdgpu_irq_disable_all(struct amdgpu_device *adev); @@ -121,4
>>>> +127,7 @@ int amdgpu_irq_add_domain(struct amdgpu_device *adev);
>>>>    void amdgpu_irq_remove_domain(struct amdgpu_device *adev);
>>>>    unsigned amdgpu_irq_create_mapping(struct amdgpu_device *adev,
>>>> unsigned src_id);
>>>>
>>>> +#define amdgpu_irq_prescreen_iv(adev)
>>>> +(adev)->irq.funcs->prescreen_iv((adev))
>>>> +#define amdgpu_irq_decode_iv(adev, iv)
>>>> +(adev)->irq.funcs->decode_iv((adev), (iv))
>>>> +
>>>>    #endif
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_ih.c
>>>> b/drivers/gpu/drm/amd/amdgpu/cik_ih.c
>>>> index b5775c6a857b..161f0225749c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/cik_ih.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik_ih.c
>>>> @@ -461,14 +461,18 @@ static const struct amd_ip_funcs
>>>> cik_ih_ip_funcs = {
>>>>
>>>>    static const struct amdgpu_ih_funcs cik_ih_funcs = {
>>>>    	.get_wptr = cik_ih_get_wptr,
>>>> +	.set_rptr = cik_ih_set_rptr
>>>> +};
>>>> +
>>>> +static const struct amdgpu_irq_funcs cik_irq_funcs = {
>>>>    	.prescreen_iv = cik_ih_prescreen_iv,
>>>>    	.decode_iv = cik_ih_decode_iv,
>>>> -	.set_rptr = cik_ih_set_rptr
>>>>    };
>>>>
>>>>    static void cik_ih_set_interrupt_funcs(struct amdgpu_device *adev)
>>>>    {
>>>> -	adev->irq.ih_funcs = &cik_ih_funcs;
>>>> +	adev->irq.ih.funcs = &cik_ih_funcs;
>>>> +	adev->irq.funcs = &cik_irq_funcs;
>>>>    }
>>>>
>>>>    const struct amdgpu_ip_block_version cik_ih_ip_block = diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/cz_ih.c
>>>> b/drivers/gpu/drm/amd/amdgpu/cz_ih.c
>>>> index df5ac4d85a00..648ecd774611 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/cz_ih.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/cz_ih.c
>>>> @@ -442,14 +442,18 @@ static const struct amd_ip_funcs cz_ih_ip_funcs
>>>> = {
>>>>
>>>>    static const struct amdgpu_ih_funcs cz_ih_funcs = {
>>>>    	.get_wptr = cz_ih_get_wptr,
>>>> +	.set_rptr = cz_ih_set_rptr
>>>> +};
>>>> +
>>>> +static const struct amdgpu_irq_funcs cz_irq_funcs = {
>>>>    	.prescreen_iv = cz_ih_prescreen_iv,
>>>>    	.decode_iv = cz_ih_decode_iv,
>>>> -	.set_rptr = cz_ih_set_rptr
>>>>    };
>>>>
>>>>    static void cz_ih_set_interrupt_funcs(struct amdgpu_device *adev)
>>>>    {
>>>> -	adev->irq.ih_funcs = &cz_ih_funcs;
>>>> +	adev->irq.ih.funcs = &cz_ih_funcs;
>>>> +	adev->irq.funcs = &cz_irq_funcs;
>>>>    }
>>>>
>>>>    const struct amdgpu_ip_block_version cz_ih_ip_block = diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
>>>> b/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
>>>> index cf0fc61aebe6..6139186ccd36 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/iceland_ih.c
>>>> @@ -440,14 +440,18 @@ static const struct amd_ip_funcs
>>>> iceland_ih_ip_funcs = {
>>>>
>>>>    static const struct amdgpu_ih_funcs iceland_ih_funcs = {
>>>>    	.get_wptr = iceland_ih_get_wptr,
>>>> +	.set_rptr = iceland_ih_set_rptr
>>>> +};
>>>> +
>>>> +static const struct amdgpu_irq_funcs iceland_irq_funcs = {
>>>>    	.prescreen_iv = iceland_ih_prescreen_iv,
>>>>    	.decode_iv = iceland_ih_decode_iv,
>>>> -	.set_rptr = iceland_ih_set_rptr
>>>>    };
>>>>
>>>>    static void iceland_ih_set_interrupt_funcs(struct amdgpu_device *adev)
>>>>    {
>>>> -	adev->irq.ih_funcs = &iceland_ih_funcs;
>>>> +	adev->irq.ih.funcs = &iceland_ih_funcs;
>>>> +	adev->irq.funcs = &iceland_irq_funcs;
>>>>    }
>>>>
>>>>    const struct amdgpu_ip_block_version iceland_ih_ip_block = diff
>>>> --git a/drivers/gpu/drm/amd/amdgpu/si_ih.c
>>>> b/drivers/gpu/drm/amd/amdgpu/si_ih.c
>>>> index b3d7d9f83202..16f212f3b534 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/si_ih.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/si_ih.c
>>>> @@ -301,14 +301,18 @@ static const struct amd_ip_funcs si_ih_ip_funcs
>>>> = {
>>>>
>>>>    static const struct amdgpu_ih_funcs si_ih_funcs = {
>>>>    	.get_wptr = si_ih_get_wptr,
>>>> +	.set_rptr = si_ih_set_rptr
>>>> +};
>>>> +
>>>> +static const struct amdgpu_irq_funcs si_irq_funcs = {
>>>>    	.prescreen_iv = si_ih_prescreen_iv,
>>>>    	.decode_iv = si_ih_decode_iv,
>>>> -	.set_rptr = si_ih_set_rptr
>>>>    };
>>>>
>>>>    static void si_ih_set_interrupt_funcs(struct amdgpu_device *adev)
>>>>    {
>>>> -	adev->irq.ih_funcs = &si_ih_funcs;
>>>> +	adev->irq.ih.funcs = &si_ih_funcs;
>>>> +	adev->irq.funcs = &si_irq_funcs;
>>>>    }
>>>>
>>>>    const struct amdgpu_ip_block_version si_ih_ip_block = diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
>>>> b/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
>>>> index 3abffd06b5c7..6120ac3c7667 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/tonga_ih.c
>>>> @@ -506,14 +506,18 @@ static const struct amd_ip_funcs
>>>> tonga_ih_ip_funcs = {
>>>>
>>>>    static const struct amdgpu_ih_funcs tonga_ih_funcs = {
>>>>    	.get_wptr = tonga_ih_get_wptr,
>>>> +	.set_rptr = tonga_ih_set_rptr
>>>> +};
>>>> +
>>>> +static const struct amdgpu_irq_funcs tonga_irq_funcs = {
>>>>    	.prescreen_iv = tonga_ih_prescreen_iv,
>>>>    	.decode_iv = tonga_ih_decode_iv,
>>>> -	.set_rptr = tonga_ih_set_rptr
>>>>    };
>>>>
>>>>    static void tonga_ih_set_interrupt_funcs(struct amdgpu_device *adev)
>>>>    {
>>>> -	adev->irq.ih_funcs = &tonga_ih_funcs;
>>>> +	adev->irq.ih.funcs = &tonga_ih_funcs;
>>>> +	adev->irq.funcs = &tonga_irq_funcs;
>>>>    }
>>>>
>>>>    const struct amdgpu_ip_block_version tonga_ih_ip_block = diff --git
>>>> a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>> b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>> index a99f71797aa3..a9737adf8392 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vega10_ih.c
>>>> @@ -487,14 +487,18 @@ const struct amd_ip_funcs vega10_ih_ip_funcs
>> =
>>>> {
>>>>
>>>>    static const struct amdgpu_ih_funcs vega10_ih_funcs = {
>>>>    	.get_wptr = vega10_ih_get_wptr,
>>>> +	.set_rptr = vega10_ih_set_rptr
>>>> +};
>>>> +
>>>> +static const struct amdgpu_irq_funcs vega10_irq_funcs = {
>>>>    	.prescreen_iv = vega10_ih_prescreen_iv,
>>>>    	.decode_iv = vega10_ih_decode_iv,
>>>> -	.set_rptr = vega10_ih_set_rptr
>>>>    };
>>>>
>>>>    static void vega10_ih_set_interrupt_funcs(struct amdgpu_device *adev)
>>>>    {
>>>> -	adev->irq.ih_funcs = &vega10_ih_funcs;
>>>> +	adev->irq.ih.funcs = &vega10_ih_funcs;
>>>> +	adev->irq.funcs = &vega10_irq_funcs;
>>>>    }
>>>>
>>>>    const struct amdgpu_ip_block_version vega10_ih_ip_block =
>>>> --
>>>> 2.14.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx