Re: drm/etnaviv: disable MLCG and pulse eater on GPU reset
OK,
On 2023/6/15 17:14, Lucas Stach wrote:
Am Donnerstag, dem 15.06.2023 um 01:49 +0800 schrieb Sui Jingfeng:
Hi
On 2023/6/14 15:45, Lucas Stach wrote:
Hi,
Am Mittwoch, dem 14.06.2023 um 00:42 +0800 schrieb Sui Jingfeng:
Hi, Lucas
I love your patch, perhaps something to improve:
The MLCG stand for "Module Level Clock Gating",
without reading the commit message, I guess there may have people don't
know its meaning.
Yea, I expect people to read the commit message and not just the
subject if they want to know what a patch does. The MLCG abbreviation
is already well established within the driver code.
Yeah, I agree with you that the reviewer should read the commit message
and the patch itself(code)
But after look the code quite a while, I'm wondering that
1) is the "Module Level" absolutely needed ?
2) is it accurate enough ?
For question in 1), I meant that is it better by saying: "drm/etnaviv:
disable clock gating and pulse eater on GPU reset"
For question in 2), I mean that the code inside the
etnaviv_hw_reset(struct etnaviv_gpu *gpu) function are per GPU instance
level.
Every GPU instance managed by the drm/etnaviv will run those clock
gating related code.
So it is per GPU instance level.
As far as I can understand, the "Module Level" stand for the
drm/etnaviv(etnaviv.ko) as a whole
Nearly all patches for the gpu/drm/drivers are module level by default,
What's you really want to emphasize?
I think you probably want to drop the "ML", and replace the "MLCG" with
"clock gating".
explain the "ML" in the commit message should be enough.
No "module level" here has nothing to do with the kernel module at
all.
MLCG is the GPU internal clock gating mechanism implemented in the
Vivante GPU cores. The module level here means that the GPU can gate
individual modules of the core like the texture engine, pixel engine,
etc. when the they are either idle or stalled. It's a fine grained
clock gating mechanism, in contrast to the more coarse-grained platform
level mechanisms, which may be able to gate the clock for the GPU as a
whole.
So in essence MLCG is the absolutely right and most specific term that
need to be used here to describe what is being done in this patch.
OK, that sound fine then If no other experts come here to battle.
Since you are more profession in this domain, I choose to believe what
you have said.
Thanks for the educating.
Regards,
Lucas
There are still more thing in this patch can only be understand relay on
guessing... :-)
That's unfortunately true. I don't know exactly what the pulse eater
control bits mean either. I've taken them verbatim from the reset
sequence in the Vivante kernel driver, which is also why I didn't try
to assign some meaning by giving them a name, but keep them as BITs in
the driver code.
Regards,
Lucas
But drm/etnaviv drvier still works with this patch applied, so
On 2023/6/7 20:58, Lucas Stach wrote:
Module level clock gating and the pulse eater might interfere with
the GPU reset, as they both have the potential to stop the clock
and thus reset propagation to parts of the GPU.
Signed-off-by: Lucas Stach
Reviewed-by: Christian Gmeiner
Tested-by: Sui Jingfeng
---
I'm not aware of any cases where this would have been an issue, but
it is what the downstream driver does and fundametally seems like
the right thing to do.
---
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index de8c9894967c..41aab1aa330b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
timeout = jiffies + msecs_to_jiffies(1000);
while (time_is_after_jiffies(timeout)) {
- /* enable clock */
unsigned int fscale = 1 << (6 - gpu->freq_scale);
+ u32 pulse_eater = 0x01590880;
+
+ /* disable clock gating */
+ gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
+
+ /* disable pulse eater */
+ pulse_eater |= BIT(17);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ pulse_eater |= BIT(0);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+
+ /* enable clock */
control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
etnaviv_gpu_load_clock(gpu, control);
--
Jingfeng
Re: drm/etnaviv: disable MLCG and pulse eater on GPU reset
Am Donnerstag, dem 15.06.2023 um 01:49 +0800 schrieb Sui Jingfeng:
> Hi
>
> On 2023/6/14 15:45, Lucas Stach wrote:
> > Hi,
> >
> > Am Mittwoch, dem 14.06.2023 um 00:42 +0800 schrieb Sui Jingfeng:
> > > Hi, Lucas
> > >
> > >
> > > I love your patch, perhaps something to improve:
> > >
> > > The MLCG stand for "Module Level Clock Gating",
> > >
> > > without reading the commit message, I guess there may have people don't
> > > know its meaning.
> > >
> > Yea, I expect people to read the commit message and not just the
> > subject if they want to know what a patch does. The MLCG abbreviation
> > is already well established within the driver code.
>
> Yeah, I agree with you that the reviewer should read the commit message
> and the patch itself(code)
>
> But after look the code quite a while, I'm wondering that
>
> 1) is the "Module Level" absolutely needed ?
>
> 2) is it accurate enough ?
>
>
> For question in 1), I meant that is it better by saying: "drm/etnaviv:
> disable clock gating and pulse eater on GPU reset"
>
> For question in 2), I mean that the code inside the
> etnaviv_hw_reset(struct etnaviv_gpu *gpu) function are per GPU instance
> level.
>
>
> Every GPU instance managed by the drm/etnaviv will run those clock
> gating related code.
>
> So it is per GPU instance level.
>
>
> As far as I can understand, the "Module Level" stand for the
> drm/etnaviv(etnaviv.ko) as a whole
>
> Nearly all patches for the gpu/drm/drivers are module level by default,
>
> What's you really want to emphasize?
>
>
> I think you probably want to drop the "ML", and replace the "MLCG" with
> "clock gating".
>
> explain the "ML" in the commit message should be enough.
>
No "module level" here has nothing to do with the kernel module at
all.
MLCG is the GPU internal clock gating mechanism implemented in the
Vivante GPU cores. The module level here means that the GPU can gate
individual modules of the core like the texture engine, pixel engine,
etc. when the they are either idle or stalled. It's a fine grained
clock gating mechanism, in contrast to the more coarse-grained platform
level mechanisms, which may be able to gate the clock for the GPU as a
whole.
So in essence MLCG is the absolutely right and most specific term that
need to be used here to describe what is being done in this patch.
Regards,
Lucas
> > > There are still more thing in this patch can only be understand relay on
> > > guessing... :-)
> > >
> > That's unfortunately true. I don't know exactly what the pulse eater
> > control bits mean either. I've taken them verbatim from the reset
> > sequence in the Vivante kernel driver, which is also why I didn't try
> > to assign some meaning by giving them a name, but keep them as BITs in
> > the driver code.
> >
> > Regards,
> > Lucas
> >
> > > But drm/etnaviv drvier still works with this patch applied, so
> > >
> > >
> > > On 2023/6/7 20:58, Lucas Stach wrote:
> > > > Module level clock gating and the pulse eater might interfere with
> > > > the GPU reset, as they both have the potential to stop the clock
> > > > and thus reset propagation to parts of the GPU.
> > > >
> > > > Signed-off-by: Lucas Stach
> > > > Reviewed-by: Christian Gmeiner
> > >
> > > Tested-by: Sui Jingfeng
> > >
> > >
> > > > ---
> > > > I'm not aware of any cases where this would have been an issue, but
> > > > it is what the downstream driver does and fundametally seems like
> > > > the right thing to do.
> > > > ---
> > > >drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
> > > >1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > > > b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > > > index de8c9894967c..41aab1aa330b 100644
> > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > > > @@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu
> > > > *gpu)
> > > > timeout = jiffies + msecs_to_jiffies(1000);
> > > >
> > > > while (time_is_after_jiffies(timeout)) {
> > > > - /* enable clock */
> > > > unsigned int fscale = 1 << (6 - gpu->freq_scale);
> > > > + u32 pulse_eater = 0x01590880;
> > > > +
> > > > + /* disable clock gating */
> > > > + gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
> > > > +
> > > > + /* disable pulse eater */
> > > > + pulse_eater |= BIT(17);
> > > > + gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> > > > + pulse_eater |= BIT(0);
> > > > + gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> > > > +
> > > > + /* enable clock */
> > > > control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
> > > > etnaviv_gpu_load_clock(gpu, control);
> > > >
>
Re: drm/etnaviv: disable MLCG and pulse eater on GPU reset
Hi
On 2023/6/14 15:45, Lucas Stach wrote:
Hi,
Am Mittwoch, dem 14.06.2023 um 00:42 +0800 schrieb Sui Jingfeng:
Hi, Lucas
I love your patch, perhaps something to improve:
The MLCG stand for "Module Level Clock Gating",
without reading the commit message, I guess there may have people don't
know its meaning.
Yea, I expect people to read the commit message and not just the
subject if they want to know what a patch does. The MLCG abbreviation
is already well established within the driver code.
Yeah, I agree with you that the reviewer should read the commit message
and the patch itself(code)
But after look the code quite a while, I'm wondering that
1) is the "Module Level" absolutely needed ?
2) is it accurate enough ?
For question in 1), I meant that is it better by saying: "drm/etnaviv:
disable clock gating and pulse eater on GPU reset"
For question in 2), I mean that the code inside the
etnaviv_hw_reset(struct etnaviv_gpu *gpu) function are per GPU instance
level.
Every GPU instance managed by the drm/etnaviv will run those clock
gating related code.
So it is per GPU instance level.
As far as I can understand, the "Module Level" stand for the
drm/etnaviv(etnaviv.ko) as a whole
Nearly all patches for the gpu/drm/drivers are module level by default,
What's you really want to emphasize?
I think you probably want to drop the "ML", and replace the "MLCG" with
"clock gating".
explain the "ML" in the commit message should be enough.
There are still more thing in this patch can only be understand relay on
guessing... :-)
That's unfortunately true. I don't know exactly what the pulse eater
control bits mean either. I've taken them verbatim from the reset
sequence in the Vivante kernel driver, which is also why I didn't try
to assign some meaning by giving them a name, but keep them as BITs in
the driver code.
Regards,
Lucas
But drm/etnaviv drvier still works with this patch applied, so
On 2023/6/7 20:58, Lucas Stach wrote:
Module level clock gating and the pulse eater might interfere with
the GPU reset, as they both have the potential to stop the clock
and thus reset propagation to parts of the GPU.
Signed-off-by: Lucas Stach
Reviewed-by: Christian Gmeiner
Tested-by: Sui Jingfeng
---
I'm not aware of any cases where this would have been an issue, but
it is what the downstream driver does and fundametally seems like
the right thing to do.
---
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index de8c9894967c..41aab1aa330b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
timeout = jiffies + msecs_to_jiffies(1000);
while (time_is_after_jiffies(timeout)) {
- /* enable clock */
unsigned int fscale = 1 << (6 - gpu->freq_scale);
+ u32 pulse_eater = 0x01590880;
+
+ /* disable clock gating */
+ gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
+
+ /* disable pulse eater */
+ pulse_eater |= BIT(17);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ pulse_eater |= BIT(0);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+
+ /* enable clock */
control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
etnaviv_gpu_load_clock(gpu, control);
--
Jingfeng
Re: [PATCH] drm/etnaviv: disable MLCG and pulse eater on GPU reset
On Wed, Jun 07, 2023 at 02:58:41PM +0200, Lucas Stach wrote: > Module level clock gating and the pulse eater might interfere with > the GPU reset, as they both have the potential to stop the clock > and thus reset propagation to parts of the GPU. > > Signed-off-by: Lucas Stach > --- > I'm not aware of any cases where this would have been an issue, but > it is what the downstream driver does and fundametally seems like > the right thing to do. Maybe Dove, where the PLLs generating the clock to the GPU can be changed, iirc? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: drm/etnaviv: disable MLCG and pulse eater on GPU reset
Hi,
Am Mittwoch, dem 14.06.2023 um 00:42 +0800 schrieb Sui Jingfeng:
> Hi, Lucas
>
>
> I love your patch, perhaps something to improve:
>
> The MLCG stand for "Module Level Clock Gating",
>
> without reading the commit message, I guess there may have people don't
> know its meaning.
>
Yea, I expect people to read the commit message and not just the
subject if they want to know what a patch does. The MLCG abbreviation
is already well established within the driver code.
> There are still more thing in this patch can only be understand relay on
> guessing... :-)
>
That's unfortunately true. I don't know exactly what the pulse eater
control bits mean either. I've taken them verbatim from the reset
sequence in the Vivante kernel driver, which is also why I didn't try
to assign some meaning by giving them a name, but keep them as BITs in
the driver code.
Regards,
Lucas
>
> But drm/etnaviv drvier still works with this patch applied, so
>
>
> On 2023/6/7 20:58, Lucas Stach wrote:
> > Module level clock gating and the pulse eater might interfere with
> > the GPU reset, as they both have the potential to stop the clock
> > and thus reset propagation to parts of the GPU.
> >
> > Signed-off-by: Lucas Stach
> > Reviewed-by: Christian Gmeiner
>
>
> Tested-by: Sui Jingfeng
>
>
> > ---
> > I'm not aware of any cases where this would have been an issue, but
> > it is what the downstream driver does and fundametally seems like
> > the right thing to do.
> > ---
> > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > index de8c9894967c..41aab1aa330b 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> > @@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
> > timeout = jiffies + msecs_to_jiffies(1000);
> >
> > while (time_is_after_jiffies(timeout)) {
> > - /* enable clock */
> > unsigned int fscale = 1 << (6 - gpu->freq_scale);
> > + u32 pulse_eater = 0x01590880;
> > +
> > + /* disable clock gating */
> > + gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
> > +
> > + /* disable pulse eater */
> > + pulse_eater |= BIT(17);
> > + gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> > + pulse_eater |= BIT(0);
> > + gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> > +
> > + /* enable clock */
> > control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
> > etnaviv_gpu_load_clock(gpu, control);
> >
>
Re: drm/etnaviv: disable MLCG and pulse eater on GPU reset
Hi, Lucas
I love your patch, perhaps something to improve:
The MLCG stand for "Module Level Clock Gating",
without reading the commit message, I guess there may have people don't
know its meaning.
There are still more thing in this patch can only be understand relay on
guessing... :-)
But drm/etnaviv drvier still works with this patch applied, so
On 2023/6/7 20:58, Lucas Stach wrote:
Module level clock gating and the pulse eater might interfere with
the GPU reset, as they both have the potential to stop the clock
and thus reset propagation to parts of the GPU.
Signed-off-by: Lucas Stach
Reviewed-by: Christian Gmeiner
Tested-by: Sui Jingfeng
---
I'm not aware of any cases where this would have been an issue, but
it is what the downstream driver does and fundametally seems like
the right thing to do.
---
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index de8c9894967c..41aab1aa330b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
timeout = jiffies + msecs_to_jiffies(1000);
while (time_is_after_jiffies(timeout)) {
- /* enable clock */
unsigned int fscale = 1 << (6 - gpu->freq_scale);
+ u32 pulse_eater = 0x01590880;
+
+ /* disable clock gating */
+ gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
+
+ /* disable pulse eater */
+ pulse_eater |= BIT(17);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ pulse_eater |= BIT(0);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+
+ /* enable clock */
control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
etnaviv_gpu_load_clock(gpu, control);
--
Jingfeng
Re: [PATCH] drm/etnaviv: disable MLCG and pulse eater on GPU reset
>
> Module level clock gating and the pulse eater might interfere with
> the GPU reset, as they both have the potential to stop the clock
> and thus reset propagation to parts of the GPU.
>
> Signed-off-by: Lucas Stach
Reviewed-by: Christian Gmeiner
> ---
> I'm not aware of any cases where this would have been an issue, but
> it is what the downstream driver does and fundametally seems like
> the right thing to do.
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index de8c9894967c..41aab1aa330b 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
> timeout = jiffies + msecs_to_jiffies(1000);
>
> while (time_is_after_jiffies(timeout)) {
> - /* enable clock */
> unsigned int fscale = 1 << (6 - gpu->freq_scale);
> + u32 pulse_eater = 0x01590880;
> +
> + /* disable clock gating */
> + gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
> +
> + /* disable pulse eater */
> + pulse_eater |= BIT(17);
> + gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> + pulse_eater |= BIT(0);
> + gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> +
> + /* enable clock */
> control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
> etnaviv_gpu_load_clock(gpu, control);
>
> --
> 2.39.2
>
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
[PATCH] drm/etnaviv: disable MLCG and pulse eater on GPU reset
Module level clock gating and the pulse eater might interfere with
the GPU reset, as they both have the potential to stop the clock
and thus reset propagation to parts of the GPU.
Signed-off-by: Lucas Stach
---
I'm not aware of any cases where this would have been an issue, but
it is what the downstream driver does and fundametally seems like
the right thing to do.
---
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index de8c9894967c..41aab1aa330b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -505,8 +505,19 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
timeout = jiffies + msecs_to_jiffies(1000);
while (time_is_after_jiffies(timeout)) {
- /* enable clock */
unsigned int fscale = 1 << (6 - gpu->freq_scale);
+ u32 pulse_eater = 0x01590880;
+
+ /* disable clock gating */
+ gpu_write_power(gpu, VIVS_PM_POWER_CONTROLS, 0x0);
+
+ /* disable pulse eater */
+ pulse_eater |= BIT(17);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ pulse_eater |= BIT(0);
+ gpu_write_power(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+
+ /* enable clock */
control = VIVS_HI_CLOCK_CONTROL_FSCALE_VAL(fscale);
etnaviv_gpu_load_clock(gpu, control);
--
2.39.2
