diff mbox series

[v2,3/3] drm/ingenic: ipu: Only enable clock when needed

Message ID 20200730144830.10479-4-paul@crapouillou.net (mailing list archive)
State New, archived
Headers show
Series More IPU cleanups v2 | expand

Commit Message

Paul Cercueil July 30, 2020, 2:48 p.m. UTC 
Instead of keeping the IPU clock enabled constantly, enable and disable
it on demand, when the IPU plane is used. That way, we won't use any
extra power when the IPU is not used.

v2: Explain the reason of this patch

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Paul Cercueil July 30, 2020, 4:21 p.m. UTC | #1 
Le jeu. 30 juil. 2020 à 17:29, Sam Ravnborg <sam@ravnborg.org> a 
écrit :
> On Thu, Jul 30, 2020 at 04:48:30PM +0200, Paul Cercueil wrote:
>>  Instead of keeping the IPU clock enabled constantly, enable and 
>> disable
>>  it on demand, when the IPU plane is used. That way, we won't use any
>>  extra power when the IPU is not used.
>> 
>>  v2: Explain the reason of this patch
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> And thanks for the quick update!

Pushed to drm-misc-next. Thanks!

Cheers,
-Paul

> 
> 	Sam
> 
>>  ---
>>   drivers/gpu/drm/ingenic/ingenic-ipu.c | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c 
>> b/drivers/gpu/drm/ingenic/ingenic-ipu.c
>>  index 7dd2a6ae4994..fc8c6e970ee3 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
>>  @@ -49,6 +49,7 @@ struct ingenic_ipu {
>>   	struct regmap *map;
>>   	struct clk *clk;
>>   	const struct soc_info *soc_info;
>>  +	bool clk_enabled;
>> 
>>   	unsigned int num_w, num_h, denom_w, denom_h;
>> 
>>  @@ -288,12 +289,23 @@ static void 
>> ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
>>   	const struct drm_format_info *finfo;
>>   	u32 ctrl, stride = 0, coef_index = 0, format = 0;
>>   	bool needs_modeset, upscaling_w, upscaling_h;
>>  +	int err;
>> 
>>   	if (!state || !state->fb)
>>   		return;
>> 
>>   	finfo = drm_format_info(state->fb->format->format);
>> 
>>  +	if (!ipu->clk_enabled) {
>>  +		err = clk_enable(ipu->clk);
>>  +		if (err) {
>>  +			dev_err(ipu->dev, "Unable to enable clock: %d\n", err);
>>  +			return;
>>  +		}
>>  +
>>  +		ipu->clk_enabled = true;
>>  +	}
>>  +
>>   	/* Reset all the registers if needed */
>>   	needs_modeset = drm_atomic_crtc_needs_modeset(state->crtc->state);
>>   	if (needs_modeset) {
>>  @@ -578,6 +590,11 @@ static void 
>> ingenic_ipu_plane_atomic_disable(struct drm_plane *plane,
>>   	regmap_clear_bits(ipu->map, JZ_REG_IPU_CTRL, JZ_IPU_CTRL_CHIP_EN);
>> 
>>   	ingenic_drm_plane_disable(ipu->master, plane);
>>  +
>>  +	if (ipu->clk_enabled) {
>>  +		clk_disable(ipu->clk);
>>  +		ipu->clk_enabled = false;
>>  +	}
>>   }
>> 
>>   static const struct drm_plane_helper_funcs 
>> ingenic_ipu_plane_helper_funcs = {
>>  @@ -761,9 +778,9 @@ static int ingenic_ipu_bind(struct device *dev, 
>> struct device *master, void *d)
>>   	drm_object_attach_property(&plane->base, ipu->sharpness_prop,
>>   				   ipu->sharpness);
>> 
>>  -	err = clk_prepare_enable(ipu->clk);
>>  +	err = clk_prepare(ipu->clk);
>>   	if (err) {
>>  -		dev_err(dev, "Unable to enable clock\n");
>>  +		dev_err(dev, "Unable to prepare clock\n");
>>   		return err;
>>   	}
>> 
>>  @@ -775,7 +792,7 @@ static void ingenic_ipu_unbind(struct device 
>> *dev,
>>   {
>>   	struct ingenic_ipu *ipu = dev_get_drvdata(dev);
>> 
>>  -	clk_disable_unprepare(ipu->clk);
>>  +	clk_unprepare(ipu->clk);
>>   }
>> 
>>   static const struct component_ops ingenic_ipu_ops = {
>>  --
>>  2.27.0
Daniel Vetter Aug. 4, 2020, 9:31 a.m. UTC | #2 
On Thu, Jul 30, 2020 at 06:21:05PM +0200, Paul Cercueil wrote:
>
>
> Le jeu. 30 juil. 2020 à 17:29, Sam Ravnborg <sam@ravnborg.org> a écrit :
> > On Thu, Jul 30, 2020 at 04:48:30PM +0200, Paul Cercueil wrote:
> > >  Instead of keeping the IPU clock enabled constantly, enable and
> > > disable
> > >  it on demand, when the IPU plane is used. That way, we won't use any
> > >  extra power when the IPU is not used.
> > >
> > >  v2: Explain the reason of this patch
> > >
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> >
> > And thanks for the quick update!
>
> Pushed to drm-misc-next. Thanks!

Hi Paul,

Maybe I'm mixing up people, but iirc you're the one very much interested
in running fbdev userspace. Did you think about building up a set of
regression tests in igt (which we could eventually even run on something
like vkms for hw-free regression testing) so that we make sure fbdev keeps
wroking?

Defacto drm_fb_helper.c has become the fbdev reference implementation
already anyway (e.g. with stuff like stanardizing vblank waiting
behaviour), so this would only be the next logical step.

Also adding Bart.

Cheers, Daniel


>
> Cheers,
> -Paul
>
> >
> >     Sam
> >
> > >  ---
> > >   drivers/gpu/drm/ingenic/ingenic-ipu.c | 23 ++++++++++++++++++++---
> > >   1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > >  diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> > > b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> > >  index 7dd2a6ae4994..fc8c6e970ee3 100644
> > >  --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> > >  +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> > >  @@ -49,6 +49,7 @@ struct ingenic_ipu {
> > >           struct regmap *map;
> > >           struct clk *clk;
> > >           const struct soc_info *soc_info;
> > >  +        bool clk_enabled;
> > >
> > >           unsigned int num_w, num_h, denom_w, denom_h;
> > >
> > >  @@ -288,12 +289,23 @@ static void
> > > ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
> > >           const struct drm_format_info *finfo;
> > >           u32 ctrl, stride = 0, coef_index = 0, format = 0;
> > >           bool needs_modeset, upscaling_w, upscaling_h;
> > >  +        int err;
> > >
> > >           if (!state || !state->fb)
> > >                   return;
> > >
> > >           finfo = drm_format_info(state->fb->format->format);
> > >
> > >  +        if (!ipu->clk_enabled) {
> > >  +                err = clk_enable(ipu->clk);
> > >  +                if (err) {
> > >  +                        dev_err(ipu->dev, "Unable to enable clock: %d\n", err);
> > >  +                        return;
> > >  +                }
> > >  +
> > >  +                ipu->clk_enabled = true;
> > >  +        }
> > >  +
> > >           /* Reset all the registers if needed */
> > >           needs_modeset = drm_atomic_crtc_needs_modeset(state->crtc->state);
> > >           if (needs_modeset) {
> > >  @@ -578,6 +590,11 @@ static void
> > > ingenic_ipu_plane_atomic_disable(struct drm_plane *plane,
> > >           regmap_clear_bits(ipu->map, JZ_REG_IPU_CTRL, JZ_IPU_CTRL_CHIP_EN);
> > >
> > >           ingenic_drm_plane_disable(ipu->master, plane);
> > >  +
> > >  +        if (ipu->clk_enabled) {
> > >  +                clk_disable(ipu->clk);
> > >  +                ipu->clk_enabled = false;
> > >  +        }
> > >   }
> > >
> > >   static const struct drm_plane_helper_funcs
> > > ingenic_ipu_plane_helper_funcs = {
> > >  @@ -761,9 +778,9 @@ static int ingenic_ipu_bind(struct device *dev,
> > > struct device *master, void *d)
> > >           drm_object_attach_property(&plane->base, ipu->sharpness_prop,
> > >                                      ipu->sharpness);
> > >
> > >  -        err = clk_prepare_enable(ipu->clk);
> > >  +        err = clk_prepare(ipu->clk);
> > >           if (err) {
> > >  -                dev_err(dev, "Unable to enable clock\n");
> > >  +                dev_err(dev, "Unable to prepare clock\n");
> > >                   return err;
> > >           }
> > >
> > >  @@ -775,7 +792,7 @@ static void ingenic_ipu_unbind(struct device
> > > *dev,
> > >   {
> > >           struct ingenic_ipu *ipu = dev_get_drvdata(dev);
> > >
> > >  -        clk_disable_unprepare(ipu->clk);
> > >  +        clk_unprepare(ipu->clk);
> > >   }
> > >
> > >   static const struct component_ops ingenic_ipu_ops = {
> > >  --
> > >  2.27.0
>
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 7dd2a6ae4994..fc8c6e970ee3 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -49,6 +49,7 @@  struct ingenic_ipu {
 	struct regmap *map;
 	struct clk *clk;
 	const struct soc_info *soc_info;
+	bool clk_enabled;
 
 	unsigned int num_w, num_h, denom_w, denom_h;
 
@@ -288,12 +289,23 @@  static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
 	const struct drm_format_info *finfo;
 	u32 ctrl, stride = 0, coef_index = 0, format = 0;
 	bool needs_modeset, upscaling_w, upscaling_h;
+	int err;
 
 	if (!state || !state->fb)
 		return;
 
 	finfo = drm_format_info(state->fb->format->format);
 
+	if (!ipu->clk_enabled) {
+		err = clk_enable(ipu->clk);
+		if (err) {
+			dev_err(ipu->dev, "Unable to enable clock: %d\n", err);
+			return;
+		}
+
+		ipu->clk_enabled = true;
+	}
+
 	/* Reset all the registers if needed */
 	needs_modeset = drm_atomic_crtc_needs_modeset(state->crtc->state);
 	if (needs_modeset) {
@@ -578,6 +590,11 @@  static void ingenic_ipu_plane_atomic_disable(struct drm_plane *plane,
 	regmap_clear_bits(ipu->map, JZ_REG_IPU_CTRL, JZ_IPU_CTRL_CHIP_EN);
 
 	ingenic_drm_plane_disable(ipu->master, plane);
+
+	if (ipu->clk_enabled) {
+		clk_disable(ipu->clk);
+		ipu->clk_enabled = false;
+	}
 }
 
 static const struct drm_plane_helper_funcs ingenic_ipu_plane_helper_funcs = {
@@ -761,9 +778,9 @@  static int ingenic_ipu_bind(struct device *dev, struct device *master, void *d)
 	drm_object_attach_property(&plane->base, ipu->sharpness_prop,
 				   ipu->sharpness);
 
-	err = clk_prepare_enable(ipu->clk);
+	err = clk_prepare(ipu->clk);
 	if (err) {
-		dev_err(dev, "Unable to enable clock\n");
+		dev_err(dev, "Unable to prepare clock\n");
 		return err;
 	}
 
@@ -775,7 +792,7 @@  static void ingenic_ipu_unbind(struct device *dev,
 {
 	struct ingenic_ipu *ipu = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(ipu->clk);
+	clk_unprepare(ipu->clk);
 }
 
 static const struct component_ops ingenic_ipu_ops = {