Re: [RFC PATCH v3 7/7] drm: exynos: mixer: Add interconnect support

[Artur ÅwigoÅ]

Hi,

On Tue, 2019-12-24 at 13:56 +0900, Inki Dae wrote:
> Hi,
> 
> 19. 12. 20. ìí 8:56ì Artur ÅwigoÅ ì(ê) ì ê:
> > From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > 
> > This patch adds interconnect support to exynos-mixer. The mixer works
> > the same as before when CONFIG_INTERCONNECT is 'n'.
> > 
> > Co-developed-by: Artur ÅwigoÅ <a.swigon@xxxxxxxxxxx>
> > Signed-off-by: Artur ÅwigoÅ <a.swigon@xxxxxxxxxxx>
> > Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/exynos/exynos_mixer.c | 71 +++++++++++++++++++++++++--
> >  1 file changed, 66 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> > index 6cfdb95fef2f..a7e7240a055f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/component.h>
> >  #include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/interconnect.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> > @@ -97,6 +98,7 @@ struct mixer_context {
> >  	struct exynos_drm_crtc	*crtc;
> >  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
> >  	unsigned long		flags;
> > +	struct icc_path		*soc_path;
> >  
> >  	int			irq;
> >  	void __iomem		*mixer_regs;
> > @@ -931,6 +933,40 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
> >  	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
> >  }
> >  
> > +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc)
> > +{
> > +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
> > +	struct mixer_context *ctx = crtc->ctx;
> > +	unsigned long bw, bandwidth = 0;
> > +	int i, j, sub;
> > +
> > +	if (!ctx->soc_path)
> > +		return;
> > +
> > +	for (i = 0; i < MIXER_WIN_NR; i++) {
> > +		struct drm_plane *plane = &ctx->planes[i].base;
> > +		const struct drm_format_info *format;
> > +
> > +		if (plane->state && plane->state->crtc && plane->state->fb) {
> > +			format = plane->state->fb->format;
> > +			bw = mode->hdisplay * mode->vdisplay *
> > +							drm_mode_vrefresh(mode);
> > +			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > +				bw /= 2;
> > +			for (j = 0; j < format->num_planes; j++) {
> > +				sub = j ? (format->vsub * format->hsub) : 1;
> > +				bandwidth += format->cpp[j] * bw / sub;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* add 20% safety margin */
> > +	bandwidth = bandwidth / 4 * 5;
> > +
> > +	dev_dbg(ctx->dev, "exynos-mixer: safe bandwidth %ld Bps\n", bandwidth);
> > +	icc_set_bw(ctx->soc_path, Bps_to_icc(bandwidth), 0);
> > +}
> > +
> >  static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
> >  {
> >  	struct mixer_context *ctx = crtc->ctx;
> > @@ -982,6 +1018,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
> >  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
> >  		return;
> >  
> > +	mixer_set_memory_bandwidth(crtc);
> >  	mixer_enable_sync(mixer_ctx);
> >  	exynos_crtc_handle_event(crtc);
> >  }
> > @@ -1029,6 +1066,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc)
> >  	for (i = 0; i < MIXER_WIN_NR; i++)
> >  		mixer_disable_plane(crtc, &ctx->planes[i]);
> >  > +	mixer_set_memory_bandwidth(crtc);
> 
> Your intention is to set peak and average bandwidth to 0 at disabling mixer device?

Yes. In general, setting the requested bandwidth to zero means "do not override
the devfreq setting" because only constraints of type DEV_PM_QOS_MIN_FREQUENCY
are used (cf. patch 05 of this series). I will make sure to reflect that in the
commit message.

Moreover, this RFC does not really make use of the peak bandwidth (yet). It is
set to zero in this patch and ignored in patch 05 (cf. exynos_bus_icc_set()).
Only the average bandwidth is translated to a minimum frequency constraint,
overriding devfreq if necessary.

A possible modification to mixer_set_memory_bandwidth() could be:
- bandwidth = bandwidth / 4 * 5;
+ peak_bandwidth = bandwidth / 4 * 5;
in mixer_set_memory_bandwidth() plus some additional logic in exynos_bus_icc_set().

Best regards,
-- 
Artur ÅwigoÅ
Samsung R&D Institute Poland
Samsung Electronics