[PATCH 0/4] [media] exynos-gsc: Cleanup and fixes

[PATCH 0/4] [media] exynos-gsc: Cleanup and fixes

[Javier Martinez Canillas]

Hello,

This patch series contains a cleanup and some fixes for the exynos-gsc
driver. I found these issues when trying to use the driver with the
GStreamer v4l2videoconvert element. I wasn't able to display to Exynos
DRM/KMS driver and only a small set of formats were working correctly.

After these patches, I'm able to display the captured frames to the
Exynos DRM driver using kmssink or chain two v4l2videoconvert elements
using the two GSC instances. Also, most supported formats are working.

I've tested all supported formats with the following script:

FORMATS="BGRx YUY2 UYVY YVYU Y42B NV16 NV61 YV12 NV21 NV12 I420"

for IN in ${FORMATS}; do
    for OUT in ${FORMATS}; do
        gst-launch-1.0 videotestsrc num-buffers=30 ! video/x-raw,format=${IN} ! \
        v4l2video$1convert ! video/x-raw,format=${OUT} ! videoconvert ! kmssink
    done
done

There are though still two issues remaining after these patches:

1) The NV21 and NV61 formats aren't show correctly (NV12 and NV16 works).
2) The Y42B format works when used as input but no when used as output.

But those can be addressed as a follow-up since I believe are not related,
and the fixes in the series improve the support for most exposed formats.

Best regards,
Javier


Javier Martinez Canillas (4):
  [media] exynos-gsc: change spamming try_fmt log message to debug
  [media] exynos-gsc: don't clear format when freeing buffers with
    REQBUFS(0)
  [media] exynos-gsc: fix supported RGB pixel format
  [media] exynos-gsc: do proper bytesperline and sizeimage calculation

 drivers/media/platform/exynos-gsc/gsc-core.c | 27 ++++++++++++++++++++-------
 drivers/media/platform/exynos-gsc/gsc-m2m.c  |  8 +-------
 2 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH 1/4] [media] exynos-gsc: change spamming try_fmt log message to debug

[Javier Martinez Canillas]

The driver try_fmt handler prints a message each time that the image
size has been changed due the maximum and minimum width and height.

Since user-space can try different format and sizes, this logs a lot
of unnecessary messages. Change the message log level to debug and
while being there, also add a new line to the message.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 787bd16c19e5..fac0c0246ad4 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -441,7 +441,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	v4l_bound_align_image(&pix_mp->width, min_w, max_w, mod_x,
 		&pix_mp->height, min_h, max_h, mod_y, 0);
 	if (tmp_w != pix_mp->width || tmp_h != pix_mp->height)
-		pr_info("Image size has been modified from %dx%d to %dx%d",
+		pr_debug("Image size has been modified from %dx%d to %dx%d\n",
 			 tmp_w, tmp_h, pix_mp->width, pix_mp->height);
 
 	pix_mp->num_planes = fmt->num_planes;
-- 
2.7.4

[PATCH 2/4] [media] exynos-gsc: don't clear format when freeing buffers with REQBUFS(0)

[Javier Martinez Canillas]

User-space applications can use the VIDIOC_REQBUFS ioctl to determine if a
memory mapped, user pointer or DMABUF based I/O is supported by a driver.

For example, GStreamer attempts to determine the I/O methods supported by
the driver by doing many VIDIOC_REQBUFS ioctl calls with different memory
types and count 0. And then the real VIDIOC_REQBUFS call with count == n
is be made to allocate the buffers. But for count 0, the driver not only
frees the buffers but also clears the format set before with VIDIOC_S_FMT.

This is a problem since STREAMON fails if a format isn't set but GStreamer
first sets a format and then tries to determine the supported I/O methods,
so the format will be cleared on REQBUFS(0), before the call to STREAMON.

To avoid this issue, only free the buffers on VIDIOC_REQBUFS(0) but don't
clear the format. Since is completely valid to set the format and then do
different calls to REQBUFS before a call to STREAMON.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-m2m.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 9f03b791b711..e2a16b52f87d 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -365,14 +365,8 @@ static int gsc_m2m_reqbufs(struct file *file, void *fh,
 
 	max_cnt = (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ?
 		gsc->variant->in_buf_cnt : gsc->variant->out_buf_cnt;
-	if (reqbufs->count > max_cnt) {
+	if (reqbufs->count > max_cnt)
 		return -EINVAL;
-	} else if (reqbufs->count == 0) {
-		if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
-			gsc_ctx_state_lock_clear(GSC_SRC_FMT, ctx);
-		else
-			gsc_ctx_state_lock_clear(GSC_DST_FMT, ctx);
-	}
 
 	return v4l2_m2m_reqbufs(file, ctx->m2m_ctx, reqbufs);
 }
-- 
2.7.4

[PATCH 3/4] [media] exynos-gsc: fix supported RGB pixel format

[Javier Martinez Canillas]

The driver exposes 32-bit A/XRGB 8-8-8-8 as supported format but testing
shows that using this format produces frames with wrong colors. The test
was done with the following GStreamer pipeline:

$ gst-launch-1.0 videotestsrc num-buffers=20 ! video/x-raw,format=UYVY \
! v4l2video3convert ! video/x-raw,format=xRGB ! videoconvert ! kmssink

The manual seems to state that the Pixel Format are in Little Endianness
so instead use the 32-bit BGRA/X 8-8-8-8 pixel format. This format works
correctly when using the following pipeline:

$ gst-launch-1.0 videotestsrc num-buffers=20 ! video/x-raw,format=UYVY \
! v4l2video3convert ! video/x-raw,format=BGRx ! kmssink

This change is similar to commit 7f2816e51ea1 ("[media] s5p-fimc: Changed
RGB32 to BGR32") that fixed the same issue on a different Samsung driver.

Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index fac0c0246ad4..8bb1d2be7234 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -39,8 +39,8 @@ static const struct gsc_fmt gsc_formats[] = {
 		.num_planes	= 1,
 		.num_comp	= 1,
 	}, {
-		.name		= "XRGB-8-8-8-8, 32 bpp",
-		.pixelformat	= V4L2_PIX_FMT_RGB32,
+		.name		= "BGRX-8-8-8-8, 32 bpp",
+		.pixelformat	= V4L2_PIX_FMT_BGR32,
 		.depth		= { 32 },
 		.color		= GSC_RGB,
 		.num_planes	= 1,
-- 
2.7.4

[PATCH 4/4] [media] exynos-gsc: do proper bytesperline and sizeimage calculation

[Javier Martinez Canillas]

The driver don't take into account the differences between packed, semi
planar and multi planar formats when calculating the pixel format bytes
per lines and image size values. This makes GStreamer to fail when the
following formats are used NV12, NV21, NV16, NV61, YV12, I420 and Y42B:

"gst_video_frame_map_id: failed to map video frame plane 1"

Nicolas suggested to use the logic found in the Exynos FIMC v4l2 driver
since does this correctly. So this patch changes the bytes per line and
image size calculation according to what's done in this media driver.

After this patch most supported formats work correctly. There are still
issues with the NV21 and NV61 formats, but that seems to be a separate
problem since NV12 and NV16 work and these formats use the same values.

So this can be fixed as a follow-up and shouldn't be a blocker for this
change that improves the driver's support.

Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/media/platform/exynos-gsc/gsc-core.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 8bb1d2be7234..a6c47deba3b7 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -451,12 +451,25 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	else /* SD */
 		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
-
 	for (i = 0; i < pix_mp->num_planes; ++i) {
-		int bpl = (pix_mp->width * fmt->depth[i]) >> 3;
-		pix_mp->plane_fmt[i].bytesperline = bpl;
-		pix_mp->plane_fmt[i].sizeimage = bpl * pix_mp->height;
+		struct v4l2_plane_pix_format *plane_fmt = &pix_mp->plane_fmt[i];
+		u32 bpl = plane_fmt->bytesperline;
+
+		if (fmt->num_comp == 1 && /* Packed */
+		    (bpl == 0 || (bpl * 8 / fmt->depth[i]) < pix_mp->width))
+			bpl = pix_mp->width * fmt->depth[i] / 8;
+
+		if (fmt->num_comp > 1 && /* Planar */
+		    (bpl == 0 || bpl < pix_mp->width))
+			bpl = pix_mp->width;
+
+		if (i != 0 && fmt->num_comp == 3)
+			bpl /= 2;
 
+		plane_fmt->bytesperline = bpl;
+		plane_fmt->sizeimage = max(pix_mp->width * pix_mp->height *
+					   fmt->depth[i] / 8,
+					   plane_fmt->sizeimage);
 		pr_debug("[%d]: bpl: %d, sizeimage: %d",
 				i, bpl, pix_mp->plane_fmt[i].sizeimage);
 	}
-- 
2.7.4