[PATCH 0/4] drm/exynos/mixer: small cleanups

[PATCH 0/4] drm/exynos/mixer: small cleanups

[Daniel Kurtz]

I don't actually have a way of testing the video processor changes, but they
seem correct from looking at the code.  Hopefully someone has a way of testing
them.

Daniel Kurtz (4):
  drm/exynos/mixer: move format definitions to regs-mixer
  drm/exynos/mixer: use MXR_GRP_SXY_SY
  drm/exynos/mixer: planes are not disabled by setting dma_addr to zero
  drm/exynos/mixer: add support for NV21

 drivers/gpu/drm/exynos/exynos_drm_plane.c |  1 +
 drivers/gpu/drm/exynos/exynos_mixer.c     | 54 +++++++++++--------------------
 drivers/gpu/drm/exynos/regs-mixer.h       |  4 +++
 3 files changed, 23 insertions(+), 36 deletions(-)

-- 
1.9.1.423.g4596e3a

[PATCH 1/4] drm/exynos/mixer: move format definitions to regs-mixer

[Daniel Kurtz]

These constants directly define register values, so move them to the
register definition header.

Also, the logic used for setting fmt from bpp is either/or, so just use
if/else.

** No functional change

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 19 ++++---------------
 drivers/gpu/drm/exynos/regs-mixer.h   |  4 ++++
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index ce28881..475eb49 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -518,21 +518,10 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 
 	win_data = &ctx->win_data[win];
 
-	#define RGB565 4
-	#define ARGB1555 5
-	#define ARGB4444 6
-	#define ARGB8888 7
-
-	switch (win_data->bpp) {
-	case 16:
-		fmt = ARGB4444;
-		break;
-	case 32:
-		fmt = ARGB8888;
-		break;
-	default:
-		fmt = ARGB8888;
-	}
+	if (win_data->bpp == 16)
+		fmt = MXR_GRP_CFG_FORMAT_ARGB4444;
+	else
+		fmt = MXR_GRP_CFG_FORMAT_ARGB8888;
 
 	/* 2x scaling feature */
 	x_ratio = 0;
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index 4537026..785a97a 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -114,6 +114,10 @@
 #define MXR_GRP_CFG_PIXEL_BLEND_EN	(1 << 16)
 #define MXR_GRP_CFG_FORMAT_VAL(x)	MXR_MASK_VAL(x, 11, 8)
 #define MXR_GRP_CFG_FORMAT_MASK		MXR_GRP_CFG_FORMAT_VAL(~0)
+#define MXR_GRP_CFG_FORMAT_RGB565	4
+#define MXR_GRP_CFG_FORMAT_ARGB1555	5
+#define MXR_GRP_CFG_FORMAT_ARGB4444	6
+#define MXR_GRP_CFG_FORMAT_ARGB8888	7
 #define MXR_GRP_CFG_ALPHA_VAL(x)	MXR_MASK_VAL(x, 7, 0)
 
 /* bits for MXR_GRAPHICn_WH */
-- 
1.9.1.423.g4596e3a

[PATCH 2/4] drm/exynos/mixer: use MXR_GRP_SXY_SY

[Daniel Kurtz]

Mixer hardware supports offsetting dma from start of source buffer using
the MXR_GRP_SXY register.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 475eb49..40cf39b 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -529,13 +529,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 
 	dst_x_offset = win_data->crtc_x;
 	dst_y_offset = win_data->crtc_y;
+	src_x_offset = win_data->fb_x;
+	src_y_offset = win_data->fb_y;
 
 	/* converting dma address base and source offset */
-	dma_addr = win_data->dma_addr
-		+ (win_data->fb_x * win_data->bpp >> 3)
-		+ (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
-	src_x_offset = 0;
-	src_y_offset = 0;
+	dma_addr = win_data->dma_addr;
 
 	if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE)
 		ctx->interlace = true;
-- 
1.9.1.423.g4596e3a

[PATCH 3/4] drm/exynos/mixer: planes are not disabled by setting dma_addr to zero

[Daniel Kurtz]

Planes are disabled by calling the win_mode_disable() callback, not by
calling win_mode_commit()[->vp_video_buffer] with dma_addr set to zero.

Thus, the comment in the pixel_format switch default clause is obsolete,
we should always check if the pixel_format is supported, and therefore,
since the driver does not actually supported single-buffer formats,
buf_num will always be 2, and we drop the broken 1-buffer case.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 40cf39b..b252ec7 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -382,7 +382,6 @@ static void vp_video_buffer(struct mixer_context *ctx, int win)
 	unsigned long flags;
 	struct hdmi_win_data *win_data;
 	unsigned int x_ratio, y_ratio;
-	unsigned int buf_num = 1;
 	dma_addr_t luma_addr[2], chroma_addr[2];
 	bool tiled_mode = false;
 	bool crcb_mode = false;
@@ -393,16 +392,12 @@ static void vp_video_buffer(struct mixer_context *ctx, int win)
 	switch (win_data->pixel_format) {
 	case DRM_FORMAT_NV12MT:
 		tiled_mode = true;
+		/* fall through */
 	case DRM_FORMAT_NV12:
 		crcb_mode = false;
-		buf_num = 2;
 		break;
 	/* TODO: single buffer format NV12, NV21 */
 	default:
-		/* ignore pixel format at disable time */
-		if (!win_data->dma_addr)
-			break;
-
 		DRM_ERROR("pixel format for vp is wrong [%d].\n",
 				win_data->pixel_format);
 		return;
@@ -412,14 +407,8 @@ static void vp_video_buffer(struct mixer_context *ctx, int win)
 	x_ratio = (win_data->src_width << 16) / win_data->crtc_width;
 	y_ratio = (win_data->src_height << 16) / win_data->crtc_height;
 
-	if (buf_num == 2) {
-		luma_addr[0] = win_data->dma_addr;
-		chroma_addr[0] = win_data->chroma_dma_addr;
-	} else {
-		luma_addr[0] = win_data->dma_addr;
-		chroma_addr[0] = win_data->dma_addr
-			+ (win_data->fb_width * win_data->fb_height);
-	}
+	luma_addr[0] = win_data->dma_addr;
+	chroma_addr[0] = win_data->chroma_dma_addr;
 
 	if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE) {
 		ctx->interlace = true;
-- 
1.9.1.423.g4596e3a

[PATCH 4/4] drm/exynos/mixer: add support for NV21

[Daniel Kurtz]

AFAICT, the only difference between NV12 and NV21 is Cr:Cb vs Cb:Cr.
Since the video processor can handle either order, it should be able to
handle both formats.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  1 +
 drivers/gpu/drm/exynos/exynos_mixer.c     | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 8371cbd..bf2be7a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -31,6 +31,7 @@ static const uint32_t formats[] = {
 	DRM_FORMAT_ARGB8888,
 	DRM_FORMAT_NV12,
 	DRM_FORMAT_NV12MT,
+	DRM_FORMAT_NV21,
 };
 
 /*
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index b252ec7..52a94d9 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -383,8 +383,8 @@ static void vp_video_buffer(struct mixer_context *ctx, int win)
 	struct hdmi_win_data *win_data;
 	unsigned int x_ratio, y_ratio;
 	dma_addr_t luma_addr[2], chroma_addr[2];
-	bool tiled_mode = false;
-	bool crcb_mode = false;
+	bool tiled_mode;
+	bool crcb_mode;
 	u32 val;
 
 	win_data = &ctx->win_data[win];
@@ -392,10 +392,16 @@ static void vp_video_buffer(struct mixer_context *ctx, int win)
 	switch (win_data->pixel_format) {
 	case DRM_FORMAT_NV12MT:
 		tiled_mode = true;
-		/* fall through */
+		crcb_mode = false;
+		break;
 	case DRM_FORMAT_NV12:
+		tiled_mode = false;
 		crcb_mode = false;
 		break;
+	case DRM_FORMAT_NV21:
+		tiled_mode = false;
+		crcb_mode = true;
+		break;
 	/* TODO: single buffer format NV12, NV21 */
 	default:
 		DRM_ERROR("pixel format for vp is wrong [%d].\n",
-- 
1.9.1.423.g4596e3a

Re: [PATCH 2/4] drm/exynos/mixer: use MXR_GRP_SXY_SY

[Seung-Woo Kim]

Hi Daniel,

On 2014년 05월 05일 00:26, Daniel Kurtz wrote:
> Mixer hardware supports offsetting dma from start of source buffer using
> the MXR_GRP_SXY register.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 475eb49..40cf39b 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -529,13 +529,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
>  
>  	dst_x_offset = win_data->crtc_x;
>  	dst_y_offset = win_data->crtc_y;
> +	src_x_offset = win_data->fb_x;
> +	src_y_offset = win_data->fb_y;
>  
>  	/* converting dma address base and source offset */
> -	dma_addr = win_data->dma_addr
> -		+ (win_data->fb_x * win_data->bpp >> 3)
> -		+ (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
> -	src_x_offset = 0;
> -	src_y_offset = 0;
> +	dma_addr = win_data->dma_addr;

Basically, you are right and source offset register can be used. But
because of limitation of resolution for mixer up to 1920x1080, I
considered modified soruce dma address to set one frame buffer, which is
bigger than 1920x1080, on to both fimd and hdmi.

Regards,
- Seung-Woo Kim

>  
>  	if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE)
>  		ctx->interlace = true;
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: [PATCH 2/4] drm/exynos/mixer: use MXR_GRP_SXY_SY

[Daniel Kurtz]

On Wed, May 7, 2014 at 1:14 PM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> Hi Daniel,
>
> On 2014년 05월 05일 00:26, Daniel Kurtz wrote:
>> Mixer hardware supports offsetting dma from start of source buffer using
>> the MXR_GRP_SXY register.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 475eb49..40cf39b 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -529,13 +529,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
>>
>>       dst_x_offset = win_data->crtc_x;
>>       dst_y_offset = win_data->crtc_y;
>> +     src_x_offset = win_data->fb_x;
>> +     src_y_offset = win_data->fb_y;
>>
>>       /* converting dma address base and source offset */
>> -     dma_addr = win_data->dma_addr
>> -             + (win_data->fb_x * win_data->bpp >> 3)
>> -             + (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
>> -     src_x_offset = 0;
>> -     src_y_offset = 0;
>> +     dma_addr = win_data->dma_addr;
>
> Basically, you are right and source offset register can be used. But
> because of limitation of resolution for mixer up to 1920x1080, I
> considered modified soruce dma address to set one frame buffer, which is
> bigger than 1920x1080, on to both fimd and hdmi.

Hi Seung-Woo,

I do not see why the maximum MIXER resolution matters for choosing
between offsetting BASE or using SXY.

Let's say you have one big 1920x1908 framebuffer, with a span of 1920,
starting at dma_addr (there is no extra padding at the end of the
line).
Let's say you wanted the mixer to scan out 1920x1080 pixels starting
from (0, 800) in the framebuffer, and start drawing them at (0,0) on
the screen.

What we currently do is:
  BASE = dma_addr + (800 * 1080 * 4)
  SPAN = 1920
  SXY = SX(0) | SY(0)
  WH = W(1920) | H(1080)
  DXY = DX(0) | DY(0)

I am proposing we do:
  BASE = dma_addr
  SPAN = 1920
  SXY = SX(0) | SY(800)
  WH = W(1920) | H(1080)
  DXY = DX(0) | DY(0)

In both cases, the mixer resolution is 1920x1080.

My motivation for wanting to program an un-modified dma_addr into BASE
is so we can then just check BASE_S to determine from which buffer the
mixer is actively being scanned out without worrying about the source
offset, since the source offset can change for a given framebuffer
(for example, when doing panning, or if an overlay is used for a HW
cursor).

Best Regards,
-Daniel

>
> Regards,
> - Seung-Woo Kim
>
>>
>>       if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE)
>>               ctx->interlace = true;
>>
>
> --
> Seung-Woo Kim
> Samsung Software R&D Center
> --
>

Re: [PATCH 2/4] drm/exynos/mixer: use MXR_GRP_SXY_SY

[Seung-Woo Kim]

Hello Daniel,

On 2014년 05월 07일 23:14, Daniel Kurtz wrote:
> On Wed, May 7, 2014 at 1:14 PM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>> Hi Daniel,
>>
>> On 2014년 05월 05일 00:26, Daniel Kurtz wrote:
>>> Mixer hardware supports offsetting dma from start of source buffer using
>>> the MXR_GRP_SXY register.
>>>
>>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 475eb49..40cf39b 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -529,13 +529,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
>>>
>>>       dst_x_offset = win_data->crtc_x;
>>>       dst_y_offset = win_data->crtc_y;
>>> +     src_x_offset = win_data->fb_x;
>>> +     src_y_offset = win_data->fb_y;
>>>
>>>       /* converting dma address base and source offset */
>>> -     dma_addr = win_data->dma_addr
>>> -             + (win_data->fb_x * win_data->bpp >> 3)
>>> -             + (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
>>> -     src_x_offset = 0;
>>> -     src_y_offset = 0;
>>> +     dma_addr = win_data->dma_addr;
>>
>> Basically, you are right and source offset register can be used. But
>> because of limitation of resolution for mixer up to 1920x1080, I
>> considered modified soruce dma address to set one frame buffer, which is
>> bigger than 1920x1080, on to both fimd and hdmi.
> 
> Hi Seung-Woo,
> 
> I do not see why the maximum MIXER resolution matters for choosing
> between offsetting BASE or using SXY.
> 
> Let's say you have one big 1920x1908 framebuffer, with a span of 1920,
> starting at dma_addr (there is no extra padding at the end of the
> line).
> Let's say you wanted the mixer to scan out 1920x1080 pixels starting
> from (0, 800) in the framebuffer, and start drawing them at (0,0) on
> the screen.
> 
> What we currently do is:
>   BASE = dma_addr + (800 * 1080 * 4)
>   SPAN = 1920
>   SXY = SX(0) | SY(0)
>   WH = W(1920) | H(1080)
>   DXY = DX(0) | DY(0)
> 
> I am proposing we do:
>   BASE = dma_addr
>   SPAN = 1920
>   SXY = SX(0) | SY(800)
>   WH = W(1920) | H(1080)
>   DXY = DX(0) | DY(0)
> 
> In both cases, the mixer resolution is 1920x1080.

In my test to show each half of big one framebuffer (3840 x 1080) to
FIMD from 0 to 1079 and MIXER from 1080 to 3839 with exynos4210 and
exynos4412, it was failed to show proper hdmi display. Also it is same
for framebuffer (1920 x 2160). AFAIK, it is mainly because mixer dma has
limitation of dma memory size.

In this case, I set register as like:
  BASE = dma_addr /* 3840 x 1080 x 4 */
  SPAN = 3840
  SXY = SX(1920) | SY(0)
  WH = W(1920) | H(1080)
  DXY = DX(0) | DY(0)
or:
  BASE = dma_addr /* 1920 x 2160 x 4 */
  SPAN = 1920
  SXY = SX(0) | SY(1080)
  WH = W(1920) | H(1080)
  DXY = DX(0) | DY(0)
but these two setting did not show hdmi display as I expected. So I used
modified dma address.

> 
> My motivation for wanting to program an un-modified dma_addr into BASE
> is so we can then just check BASE_S to determine from which buffer the
> mixer is actively being scanned out without worrying about the source
> offset, since the source offset can change for a given framebuffer
> (for example, when doing panning, or if an overlay is used for a HW
> cursor).

Actually, this patch is exactly same with my first implementation, so I
completely understand your motivation. Anyway, I was focus on extended
displays with one buffer, so I wrote modified dma base address.

Thanks and Regards,
- Seung-Woo Kim

> 
> Best Regards,
> -Daniel
> 
>>
>> Regards,
>> - Seung-Woo Kim
>>
>>>
>>>       if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE)
>>>               ctx->interlace = true;
>>>
>>
>> --
>> Seung-Woo Kim
>> Samsung Software R&D Center
>> --
>>
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: [PATCH 2/4] drm/exynos/mixer: use MXR_GRP_SXY_SY

[Daniel Kurtz]

On Thu, May 8, 2014 at 12:33 PM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>
> Hello Daniel,
>
> On 2014년 05월 07일 23:14, Daniel Kurtz wrote:
> > On Wed, May 7, 2014 at 1:14 PM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
> >> Hi Daniel,
> >>
> >> On 2014년 05월 05일 00:26, Daniel Kurtz wrote:
> >>> Mixer hardware supports offsetting dma from start of source buffer using
> >>> the MXR_GRP_SXY register.
> >>>
> >>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> >>> ---
> >>>  drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
> >>>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> >>> index 475eb49..40cf39b 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> >>> @@ -529,13 +529,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
> >>>
> >>>       dst_x_offset = win_data->crtc_x;
> >>>       dst_y_offset = win_data->crtc_y;
> >>> +     src_x_offset = win_data->fb_x;
> >>> +     src_y_offset = win_data->fb_y;
> >>>
> >>>       /* converting dma address base and source offset */
> >>> -     dma_addr = win_data->dma_addr
> >>> -             + (win_data->fb_x * win_data->bpp >> 3)
> >>> -             + (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
> >>> -     src_x_offset = 0;
> >>> -     src_y_offset = 0;
> >>> +     dma_addr = win_data->dma_addr;
> >>
> >> Basically, you are right and source offset register can be used. But
> >> because of limitation of resolution for mixer up to 1920x1080, I
> >> considered modified soruce dma address to set one frame buffer, which is
> >> bigger than 1920x1080, on to both fimd and hdmi.
> >
> > Hi Seung-Woo,
> >
> > I do not see why the maximum MIXER resolution matters for choosing
> > between offsetting BASE or using SXY.
> >
> > Let's say you have one big 1920x1908 framebuffer, with a span of 1920,
> > starting at dma_addr (there is no extra padding at the end of the
> > line).
> > Let's say you wanted the mixer to scan out 1920x1080 pixels starting
> > from (0, 800) in the framebuffer, and start drawing them at (0,0) on
> > the screen.
> >
> > What we currently do is:
> >   BASE = dma_addr + (800 * 1080 * 4)
> >   SPAN = 1920
> >   SXY = SX(0) | SY(0)
> >   WH = W(1920) | H(1080)
> >   DXY = DX(0) | DY(0)
> >
> > I am proposing we do:
> >   BASE = dma_addr
> >   SPAN = 1920
> >   SXY = SX(0) | SY(800)
> >   WH = W(1920) | H(1080)
> >   DXY = DX(0) | DY(0)
> >
> > In both cases, the mixer resolution is 1920x1080.
>
> In my test to show each half of big one framebuffer (3840 x 1080) to
> FIMD from 0 to 1079 and MIXER from 1080 to 3839 with exynos4210 and
> exynos4412, it was failed to show proper hdmi display. Also it is same
> for framebuffer (1920 x 2160). AFAIK, it is mainly because mixer dma has
> limitation of dma memory size.


That is unfortunate, but thank you for your explanation.

Is this limitation exynos4 specific, or does it also apply for the
exynos5 mixer?
Was the limitation perhaps the number of bits in the SXY register
fields (< 11) on those devices?
For 5250/5420 these fields are 11 bits, and I don't see any
restrictions about "dma memory size" in the documentation that I have.

Thanks,
-Dan

> In this case, I set register as like:
>   BASE = dma_addr /* 3840 x 1080 x 4 */
>   SPAN = 3840
>   SXY = SX(1920) | SY(0)
>   WH = W(1920) | H(1080)
>   DXY = DX(0) | DY(0)
> or:
>   BASE = dma_addr /* 1920 x 2160 x 4 */
>   SPAN = 1920
>   SXY = SX(0) | SY(1080)
>   WH = W(1920) | H(1080)
>   DXY = DX(0) | DY(0)
> but these two setting did not show hdmi display as I expected. So I used
> modified dma address.
>
> >
> > My motivation for wanting to program an un-modified dma_addr into BASE
> > is so we can then just check BASE_S to determine from which buffer the
> > mixer is actively being scanned out without worrying about the source
> > offset, since the source offset can change for a given framebuffer
> > (for example, when doing panning, or if an overlay is used for a HW
> > cursor).
>
> Actually, this patch is exactly same with my first implementation, so I
> completely understand your motivation. Anyway, I was focus on extended
> displays with one buffer, so I wrote modified dma base address.
>
> Thanks and Regards,
> - Seung-Woo Kim
>
> >
> > Best Regards,
> > -Daniel
> >
> >>
> >> Regards,
> >> - Seung-Woo Kim
> >>
> >>>
> >>>       if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE)
> >>>               ctx->interlace = true;
> >>>
> >>
> >> --
> >> Seung-Woo Kim
> >> Samsung Software R&D Center
> >> --
> >>
> >
>
> --
> Seung-Woo Kim
> Samsung Software R&D Center
> --
>

Re: [PATCH 2/4] drm/exynos/mixer: use MXR_GRP_SXY_SY

[Inki Dae]

Hi Daniel,

On 2014년 05월 08일 18:21, Daniel Kurtz wrote:
> On Thu, May 8, 2014 at 12:33 PM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>>
>> Hello Daniel,
>>
>> On 2014년 05월 07일 23:14, Daniel Kurtz wrote:
>>> On Wed, May 7, 2014 at 1:14 PM, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>>>> Hi Daniel,
>>>>
>>>> On 2014년 05월 05일 00:26, Daniel Kurtz wrote:
>>>>> Mixer hardware supports offsetting dma from start of source buffer using
>>>>> the MXR_GRP_SXY register.
>>>>>
>>>>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>>>>> ---
>>>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
>>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>> index 475eb49..40cf39b 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>>> @@ -529,13 +529,11 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
>>>>>
>>>>>       dst_x_offset = win_data->crtc_x;
>>>>>       dst_y_offset = win_data->crtc_y;
>>>>> +     src_x_offset = win_data->fb_x;
>>>>> +     src_y_offset = win_data->fb_y;
>>>>>
>>>>>       /* converting dma address base and source offset */
>>>>> -     dma_addr = win_data->dma_addr
>>>>> -             + (win_data->fb_x * win_data->bpp >> 3)
>>>>> -             + (win_data->fb_y * win_data->fb_width * win_data->bpp >> 3);
>>>>> -     src_x_offset = 0;
>>>>> -     src_y_offset = 0;
>>>>> +     dma_addr = win_data->dma_addr;
>>>>
>>>> Basically, you are right and source offset register can be used. But
>>>> because of limitation of resolution for mixer up to 1920x1080, I
>>>> considered modified soruce dma address to set one frame buffer, which is
>>>> bigger than 1920x1080, on to both fimd and hdmi.
>>>
>>> Hi Seung-Woo,
>>>
>>> I do not see why the maximum MIXER resolution matters for choosing
>>> between offsetting BASE or using SXY.
>>>
>>> Let's say you have one big 1920x1908 framebuffer, with a span of 1920,
>>> starting at dma_addr (there is no extra padding at the end of the
>>> line).
>>> Let's say you wanted the mixer to scan out 1920x1080 pixels starting
>>> from (0, 800) in the framebuffer, and start drawing them at (0,0) on
>>> the screen.
>>>
>>> What we currently do is:
>>>   BASE = dma_addr + (800 * 1080 * 4)
>>>   SPAN = 1920
>>>   SXY = SX(0) | SY(0)
>>>   WH = W(1920) | H(1080)
>>>   DXY = DX(0) | DY(0)
>>>
>>> I am proposing we do:
>>>   BASE = dma_addr
>>>   SPAN = 1920
>>>   SXY = SX(0) | SY(800)
>>>   WH = W(1920) | H(1080)
>>>   DXY = DX(0) | DY(0)
>>>
>>> In both cases, the mixer resolution is 1920x1080.
>>
>> In my test to show each half of big one framebuffer (3840 x 1080) to
>> FIMD from 0 to 1079 and MIXER from 1080 to 3839 with exynos4210 and
>> exynos4412, it was failed to show proper hdmi display. Also it is same
>> for framebuffer (1920 x 2160). AFAIK, it is mainly because mixer dma has
>> limitation of dma memory size.
> 
> 
> That is unfortunate, but thank you for your explanation.
> 
> Is this limitation exynos4 specific, or does it also apply for the
> exynos5 mixer?

It seems that exynos5260/5420 mixers also have same limitation: SX/Y
fields is 11 bits.

> Was the limitation perhaps the number of bits in the SXY register
> fields (< 11) on those devices?
> For 5250/5420 these fields are 11 bits, and I don't see any
> restrictions about "dma memory size" in the documentation that I have.
> 

So I guess that the reason Mixer controller was operated incorrectly is
that SX/Y fields exceed maximum value (2047) by itself when he
calculates DMA address to access the memory region: the value of SX/Y
fields would be increased by Mixer controller internally until reaching
width or height, and in turn the value would exceed 2047.

So I think it'd better to use existing codes because this patch isn't
tested so not safe. You can add more comments enough to existing codes
if needed.

In addition, it seems that Exynos5422/5430 has no such limitation but
they are much different from old ones so we would need new drivers for them.

Thanks,
Inki Dae

> Thanks,
> -Dan
> 
>> In this case, I set register as like:
>>   BASE = dma_addr /* 3840 x 1080 x 4 */
>>   SPAN = 3840
>>   SXY = SX(1920) | SY(0)
>>   WH = W(1920) | H(1080)
>>   DXY = DX(0) | DY(0)
>> or:
>>   BASE = dma_addr /* 1920 x 2160 x 4 */
>>   SPAN = 1920
>>   SXY = SX(0) | SY(1080)
>>   WH = W(1920) | H(1080)
>>   DXY = DX(0) | DY(0)
>> but these two setting did not show hdmi display as I expected. So I used
>> modified dma address.
>>
>>>
>>> My motivation for wanting to program an un-modified dma_addr into BASE
>>> is so we can then just check BASE_S to determine from which buffer the
>>> mixer is actively being scanned out without worrying about the source
>>> offset, since the source offset can change for a given framebuffer
>>> (for example, when doing panning, or if an overlay is used for a HW
>>> cursor).
>>
>> Actually, this patch is exactly same with my first implementation, so I
>> completely understand your motivation. Anyway, I was focus on extended
>> displays with one buffer, so I wrote modified dma base address.
>>
>> Thanks and Regards,
>> - Seung-Woo Kim
>>
>>>
>>> Best Regards,
>>> -Daniel
>>>
>>>>
>>>> Regards,
>>>> - Seung-Woo Kim
>>>>
>>>>>
>>>>>       if (win_data->scan_flags & DRM_MODE_FLAG_INTERLACE)
>>>>>               ctx->interlace = true;
>>>>>
>>>>
>>>> --
>>>> Seung-Woo Kim
>>>> Samsung Software R&D Center
>>>> --
>>>>
>>>
>>
>> --
>> Seung-Woo Kim
>> Samsung Software R&D Center
>> --
>>
>