Re: [PATCH 6/9] drm/amdgpu: separate IH and IRQ funcs
Am 26.09.2018 um 11:16 schrieb Huang, Ray:
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
Sent: Wednesday, September 26, 2018 4:09 PM
To: Huang, Ray
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
Reviewed-by: Huang Rui
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 {
booluse_doorbell;
booluse_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
RE: [PATCH 6/9] drm/amdgpu: separate IH and IRQ funcs
> -Original Message-
> From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
> Sent: Wednesday, September 26, 2018 4:09 PM
> To: Huang, Ray
> 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
> > Reviewed-by: Huang Rui
> >
> > 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?
That's also fine.
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 {
> >>booluse_doorbell;
> >>booluse_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))
> >>r
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
Reviewed-by: Huang Rui
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.
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 {
booluse_doorbell;
booluse_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 */
unsignedvirq[AMDGPU_MAX_IRQ_SRC_ID];
uint32_tsrbm_soft_reset;
+ const struct amdgpu_irq_funcs *funcs;
};
Re: [PATCH 6/9] drm/amdgpu: separate IH and IRQ funcs
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
Reviewed-by: Huang Rui
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.
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 {
> booluse_doorbell;
> booluse_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 */
> unsignedvirq[AMDGPU_MAX_IRQ_SRC_ID];
> uint32_tsrbm_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, unsig
