Re: [RFC PATCH v5 6/6] drm: exynos: mixer: Add interconnect support

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi Chanwoo,

On 01.06.2020 09:58, Chanwoo Choi wrote:
> On 5/30/20 1:32 AM, Sylwester Nawrocki wrote:
>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> This patch adds interconnect support to exynos-mixer. The mixer works
>> the same as before when CONFIG_INTERCONNECT is 'n'.
>>
>> For proper operation of the video mixer block we need to ensure the
>> interconnect busses like DMC or LEFTBUS provide enough bandwidth so
>> as to avoid DMA buffer underruns in the mixer block. i.e we need to
>> prevent those busses from operating in low perfomance OPPs when
>> the mixer is running.
>> In this patch the bus bandwidth request is done through the interconnect
>> API, the bandiwidth value is calculated from selected DRM mode, i.e.
>> video plane width, height, refresh rate and pixel format.
>>
>> Co-developed-by: Artur Świgoń <a.swigon@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> [s.nawrocki: renamed soc_path variable to icc_path, edited commit desc.]
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

>>  drivers/gpu/drm/exynos/exynos_mixer.c | 73 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 68 insertions(+), 5 deletions(-)
  
>> +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;
>> +	u32 avg_bw, peak_bw;
>> +	int i, j, sub;
>> +
>> +	if (!ctx->icc_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;
> 
> First of all, I agree this approach.
> 
> Could you please add more detailed comments for understadning
> about this calculation? As you commented, it seems that
> the final bandwidth contains the width/height/refresh rate
> and pixel format. If you add one real example, it will be very helpful.

OK, I will improve the commit message and add a comment to the function.
As far as I understand this simply calculates amount of data in bytes that
needs to be read from the system memory per second for given video stream,
by summing values for each mixer window and for each color plane within 
a window.

Anyway, the patch will to be changed as after some more tests it seems
the calculation works for LEFTBUS but DMC clock on Odroid U3 needs to
be set to 400 MHz, rather than to at least 160 MHz. With any lower value 
the mixer underflow error interrupts are being triggered and eventually 
the IOMMU page fault occurs.

--
Regards,
Sylwester