Add MPEG-2 decoding to Rockchip VPU

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

drivers/media/v4l2-core/v4l2-common.c +11 -0

+			plane->bytesperline = info->bpp[i] *
+					      DIV_ROUND_UP(aligned_width, hdiv);
 
+
+			/*
+			 * The user might have specified a custom sizeimage,
+			 * only override it if it's not big enough.
+			 */
+			plane->sizeimage = max_t(u32,
+						 plane->bytesperline *
+						 DIV_ROUND_UP(aligned_height, vdiv),
+						 plane->sizeimage);
 		}

drivers/media/v4l2-core/v4l2-common.c +4 -6

 	}
 	return 0;

@@ -627,6 +647,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 		     u32 width, u32 height)
 {
 	const struct v4l2_format_info *info;
+	u32 sizeimage = 0;
 	int i;
 
 	info = v4l2_format_info(pixelformat);

@@ -640,8 +661,6 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-	pixfmt->sizeimage = 0;
 
 	for (i = 0; i < info->comp_planes; i++) {
 		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -651,11 +670,20 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 
 		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv);
+		sizeimage += info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) *
+			     DIV_ROUND_UP(aligned_height, vdiv);
 	}
+

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

or something similar that would be reasonable.

Quoted 5 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 629

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +2 -4

@@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
 			plane = &pixfmt->plane_fmt[i];
-			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
 
+
+			/* Custom bytesperline value is not supported yet. */

Supporting custom bytesperline seems too risky of breaking apps. So, I would drop this comment.

Quoted 44 lines

drivers/media/v4l2-core/v4l2-common.c +11 -0

+			plane->bytesperline = info->bpp[i] *
+					      DIV_ROUND_UP(aligned_width, hdiv);
 
+
+			/*
+			 * The user might have specified a custom sizeimage,
+			 * only override it if it's not big enough.
+			 */
+			plane->sizeimage = max_t(u32,
+						 plane->bytesperline *
+						 DIV_ROUND_UP(aligned_height, vdiv),
+						 plane->sizeimage);
 		}

drivers/media/v4l2-core/v4l2-common.c +4 -6

 	}
 	return 0;

@@ -627,6 +647,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 		     u32 width, u32 height)
 {
 	const struct v4l2_format_info *info;
+	u32 sizeimage = 0;
 	int i;
 
 	info = v4l2_format_info(pixelformat);

@@ -640,8 +661,6 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-	pixfmt->sizeimage = 0;
 
 	for (i = 0; i < info->comp_planes; i++) {
 		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -651,11 +670,20 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 
 		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv);
+		sizeimage += info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) *
+			     DIV_ROUND_UP(aligned_height, vdiv);
 	}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, · line 686

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+	/* Custom bytesperline value is not supported yet. */
+	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) *
+			       info->bpp[0];
+
+	/*
+	 * The user might have specified its own sizeimage value, only override
+	 * it if it's not big enough.
+	 */
+	pixfmt->sizeimage = max(sizeimage, pixfmt->sizeimage);
+

Same comment applies here: We need to sanitize it from too big sizeimages.

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, · line 689

Quoted 3 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);

Thanks,
Mauro

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);  

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Regards,

Hans

Quoted 86 lines

Quoted 5 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 629

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +2 -4

@@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
 			plane = &pixfmt->plane_fmt[i];
-			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);  
 
+
+			/* Custom bytesperline value is not supported yet. */  

Supporting custom bytesperline seems too risky of breaking apps. So, I would drop this comment.

Quoted 44 lines

drivers/media/v4l2-core/v4l2-common.c +11 -0

+			plane->bytesperline = info->bpp[i] *
+					      DIV_ROUND_UP(aligned_width, hdiv);  
 
+
+			/*
+			 * The user might have specified a custom sizeimage,
+			 * only override it if it's not big enough.
+			 */
+			plane->sizeimage = max_t(u32,
+						 plane->bytesperline *
+						 DIV_ROUND_UP(aligned_height, vdiv),
+						 plane->sizeimage);
 		}

drivers/media/v4l2-core/v4l2-common.c +4 -6

 	}
 	return 0;

@@ -627,6 +647,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 		     u32 width, u32 height)
 {
 	const struct v4l2_format_info *info;
+	u32 sizeimage = 0;
 	int i;
 
 	info = v4l2_format_info(pixelformat);

@@ -640,8 +661,6 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-	pixfmt->sizeimage = 0;
 
 	for (i = 0; i < info->comp_planes; i++) {
 		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -651,11 +670,20 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 
 		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv);
+		sizeimage += info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) *
+			     DIV_ROUND_UP(aligned_height, vdiv);
 	}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, · line 686

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+	/* Custom bytesperline value is not supported yet. */
+	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) *
+			       info->bpp[0];
+
+	/*
+	 * The user might have specified its own sizeimage value, only override
+	 * it if it's not big enough.
+	 */
+	pixfmt->sizeimage = max(sizeimage, pixfmt->sizeimage);
+  

Same comment applies here: We need to sanitize it from too big sizeimages.

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, · line 689

Quoted 3 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);  

Thanks,
Mauro

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);    

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

Quoted 95 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);      

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

Quoted 3 lines

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

If that works for you, we'll send a new version of this patch alone (unless you want us to send the whole series again).

Em Wed, 29 May 2019 14:16:33 +0200
Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]> escreveu:

Quoted 98 lines

Quoted 95 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);        

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

Quoted 6 lines

Quoted 3 lines

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Works for me.

Quoted 2 lines

If that works for you, we'll send a new version of this patch alone (unless you want us to send the whole series again).

If the other patches on this series don't depend on this, I can apply the PR just skipping this one, applying your patch afterwards.

Thanks,
Mauro

Em Wed, 29 May 2019 14:16:33 +0200
Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]> escreveu:

Quoted 98 lines

Quoted 95 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);        

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

Quoted 6 lines

Quoted 3 lines

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Works for me.

Quoted 2 lines

If that works for you, we'll send a new version of this patch alone (unless you want us to send the whole series again).

If the other patches on this series don't depend on this, I can apply the PR just skipping this one, applying your patch afterwards.

Quoted 95 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);    

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

Quoted 3 lines

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

If that works for you, we'll send a new version of this patch alone (unless you want us to send the whole series again).

Quoted 102 lines · Boris Brezillon

Quoted 98 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);      

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

Quoted 2 lines

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Looking more closely, only compressed formats can accept a user-specified sizeimage value, and this function is only called for uncompressed formats.

So doesn't that mean that this sizeimage override code can be dropped?

Quoted 102 lines · Boris Brezillon

Quoted 98 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);    

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

Quoted 2 lines

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Looking more closely, only compressed formats can accept a user-specified sizeimage value, and this function is only called for uncompressed formats.

So doesn't that mean that this sizeimage override code can be dropped?

Quoted 103 lines · Hans Verkuil

Quoted 102 lines · Boris Brezillon

Quoted 98 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);      

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

Quoted 8 lines

Quoted 2 lines

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Looking more closely, only compressed formats can accept a user-specified sizeimage value, and this function is only called for uncompressed formats.

So doesn't that mean that this sizeimage override code can be dropped?

I think this is a good idea, which means just picking all patches except this one (and the RK3328 one).

So it would be patches: 1,2 and 4 to 15.

Boris, Hans: are you OK with this?

Quoted 113 lines · On Wed, 29 May 2019 11:04:35 -0300 Ezequiel Garcia…

Quoted 111 lines · Hans Verkuil

Quoted 98 lines · Boris Brezillon

Quoted 95 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);        

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

Quoted 6 lines

Quoted 3 lines

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Looking more closely, only compressed formats can accept a user-specified sizeimage value, and this function is only called for uncompressed formats.

So doesn't that mean that this sizeimage override code can be dropped?

Quoted 6 lines

I think this is a good idea, which means just picking all patches except this one (and the RK3328 one).

So it would be patches: 1,2 and 4 to 15.

Boris, Hans: are you OK with this?

I doesn't work => the prototype of the fill_pixfmt() changed.

Quoted 119 lines · On Wed, 29 May 2019 16:06:10 +0200 Boris Brezillon…

Quoted 116 lines · On Wed, 29 May 2019 11:04:35 -0300 Ezequiel Garcia…

Quoted 103 lines · Hans Verkuil

Quoted 102 lines · Boris Brezillon

Quoted 98 lines · On Wed, 29 May 2019 08:58:54 -0300 Mauro Carvalho Chehab…

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]> escreveu:

Quoted 73 lines · Mauro Carvalho Chehab

Quoted 72 lines

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);        

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

Quoted 8 lines

Quoted 1 line

or something similar that would be reasonable.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can use min(sizeimage, 0x10000000)? Although we should probably use the clamp function instead of min/max.

Well, the max is driver-specific.

For example, for a camera with a max resolution of 640x480 with 2 bytes per pixel as the max format can only be

max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function or value filled by the drivers that would support custom sizeimage.

The core could actually calculate during init (by asking the driver to a very big resolution and getting the returned value), but it sounds better to let the drivers to explicitly calculate it.

Quoted 8 lines

Quoted 2 lines

If we want max_sizeimage to be driver specific I can add it as an extra arg to the fill_pixfmt() funcs.

Looking more closely, only compressed formats can accept a user-specified sizeimage value, and this function is only called for uncompressed formats.

So doesn't that mean that this sizeimage override code can be dropped?

I think this is a good idea, which means just picking all patches except this one (and the RK3328 one).

Quoted 5 lines

Quoted 3 lines

So it would be patches: 1,2 and 4 to 15.

Boris, Hans: are you OK with this?

I doesn't work => the prototype of the fill_pixfmt() changed.

Forget what I said, dropping patch 3 should work just fine.

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <boris.brezillon-ZGY8ohtN/[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

or something similar that would be reasonable.

Quoted 6 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 628

Quoted 7 lines

drivers/media/v4l2-core/v4l2-common.c +2 -4

 			plane = &pixfmt->plane_fmt[i];
-			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+
+			/* Custom bytesperline value is not supported yet. */

Supporting custom bytesperline seems too risky of breaking apps. So, I would drop this comment.

Quoted 45 lines

drivers/media/v4l2-core/v4l2-common.c +15 -6

+			plane->bytesperline = info->bpp[i] *
+					      DIV_ROUND_UP(aligned_width, hdiv);
+
+			/*
+			 * The user might have specified a custom sizeimage,
+			 * only override it if it's not big enough.
+			 */
+			plane->sizeimage = max_t(u32,
+						 plane->bytesperline *
+						 DIV_ROUND_UP(aligned_height, vdiv),
+						 plane->sizeimage);
 		}
 	}
 	return 0;

@@ -627,6 +647,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 		     u32 width, u32 height)
 {
 	const struct v4l2_format_info *info;
+	u32 sizeimage = 0;
 	int i;
 
 	info = v4l2_format_info(pixelformat);

@@ -640,8 +661,6 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-	pixfmt->sizeimage = 0;
 
 	for (i = 0; i < info->comp_planes; i++) {
 		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -651,11 +670,20 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 
 		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv);
+		sizeimage += info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) *
+			     DIV_ROUND_UP(aligned_height, vdiv);
 	}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, · line 686

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+	/* Custom bytesperline value is not supported yet. */
+	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) *
+			       info->bpp[0];
+
+	/*
+	 * The user might have specified its own sizeimage value, only override
+	 * it if it's not big enough.
+	 */
+	pixfmt->sizeimage = max(sizeimage, pixfmt->sizeimage);
+

Same comment applies here: We need to sanitize it from too big sizeimages.

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, · line 689

Quoted 3 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);

Thanks,
Mauro

Hi Mauro,

Quoted 80 lines · Mauro Carvalho Chehab

Em Tue, 28 May 2019 14:02:19 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 50 lines

From: Boris Brezillon <[email protected]>

Users can define custom sizeimage as long as they're big enough to store the amount of pixels required for a specific width/height under a specific format. Avoid overriding those fields in this case.

We could possibly do the same for bytesperline, but it gets tricky when dealing with !MPLANE definitions, so this case is omitted for now and ->bytesperline is always overwritten with the value calculated in fill_pixfmt().

Signed-off-by Boris Brezillon <[email protected]>

---
Changes from v5:
* Overwrite bytesperline with the value calculated in fill_pixfmt()

Changes from v4:
* New patch

 drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 15 deletions(-)

drivers/media/v4l2-core/v4l2-common.c +6 -5

index b2d1e55d9561..fd286f6e17d7 100644
@@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 	pixfmt->num_planes = info->mem_planes;
 
 	if (info->mem_planes == 1) {
+		u32 sizeimage = 0;
+
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
-		plane->sizeimage = 0;
 
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
+			sizeimage += info->bpp[i] *
+				     DIV_ROUND_UP(aligned_width, hdiv) *
+				     DIV_ROUND_UP(aligned_height, vdiv);
 		}
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 615

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+		/* Custom bytesperline value is not supported yet. */
+		plane->bytesperline = ALIGN(width,
+					    v4l2_format_block_width(info, 0)) *
+				      info->bpp[0];
+
+		/*
+		 * The user might have specified a custom sizeimage, only
+		 * override it if it's not big enough.
+		 */
+		plane->sizeimage = max(sizeimage, plane->sizeimage);

No upper limit? That doesn't sound a good idea to me, specially since some (broken) app might not be memset the format to zero before filling the ioctl structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

or something similar that would be reasonable.

Quoted 6 lines

drivers/media/v4l2-core/v4l2-common.c +0 -0

 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
 			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;

@@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
 

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, · line 628

Quoted 7 lines

drivers/media/v4l2-core/v4l2-common.c +2 -4

 			plane = &pixfmt->plane_fmt[i];
-			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+
+			/* Custom bytesperline value is not supported yet. */

Supporting custom bytesperline seems too risky of breaking apps. So, I would drop this comment.

We will really need this in the long run in many drivers in order to allow import/export of DMABuf. Without such adaptive feature, we have a software limitation that forces bouncing memory. I have already discussed about adding this feature notably in vivid and uvcvideo on IRC and in conference, which both have no restriction the memory alignment, so should allow importing any kind of video layout.

We already have a partial userspace implementation for this in GStreamer and upstream driver submission should come when the IP is considered stable enough.

Why I think it won't break userspace is that the correct way to use these read-only members of V4L2 struct is to set these to 0, which is also documented.

Adding upper bound seems like a good idea though.

From: Jonas Karlman <[email protected]>

Add necessary bits to support MPEG2 decoding on RK3328.

Signed-off-by Jonas Karlman <[email protected]>

Signed-off-by Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]>

--
Changes from v5:
* New patch.

 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c   | 12 ++++++++++++
 .../staging/media/rockchip/vpu/rockchip_vpu_drv.c    |  1 +
 drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h |  1 +
 3 files changed, 14 insertions(+)

drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c +12 -0

index 2b3689968ef4..341f8d69c33d 100644
@@ -175,3 +175,15 @@ const struct rockchip_vpu_variant rk3399_vpu_variant = {
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2
 };
+
+const struct rockchip_vpu_variant rk3328_vpu_variant = {
+	.dec_offset = 0x400,
+	.dec_fmts = rk3399_vpu_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.codec = RK_VPU_MPEG2_DECODER,
+	.codec_ops = rk3399_vpu_codec_ops,
+	.vdpu_irq = rk3399_vdpu_irq,
+	.init = rk3399_vpu_hw_init,
+	.clk_names = {"aclk", "hclk"},
+	.num_clocks = 2
+};

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

index b94ff97451db..2e22009b6583 100644

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

 	{ .compatible = "rockchip,rk3288-vpu", .data = &rk3288_vpu_variant, },
 	{ /* sentinel */ }
 };

drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h +1 -0

index 6cecb528f994..3d6b97af90fb 100644
@@ -79,6 +79,7 @@ enum rockchip_vpu_enc_fmt {
 };
 
 extern const struct rockchip_vpu_variant rk3399_vpu_variant;
+extern const struct rockchip_vpu_variant rk3328_vpu_variant;
 extern const struct rockchip_vpu_variant rk3288_vpu_variant;
 
 void rockchip_vpu_watchdog(struct work_struct *work);

Quoted 41 lines · Ezequiel Garcia

From: Jonas Karlman <[email protected]>

Add necessary bits to support MPEG2 decoding on RK3328.

Signed-off-by Jonas Karlman <[email protected]>

Signed-off-by Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]>

--
Changes from v5:
* New patch.

 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c   | 12 ++++++++++++
 .../staging/media/rockchip/vpu/rockchip_vpu_drv.c    |  1 +
 drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h |  1 +
 3 files changed, 14 insertions(+)

drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c +12 -0

index 2b3689968ef4..341f8d69c33d 100644
@@ -175,3 +175,15 @@ const struct rockchip_vpu_variant rk3399_vpu_variant = {
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2
 };
+
+const struct rockchip_vpu_variant rk3328_vpu_variant = {
+	.dec_offset = 0x400,
+	.dec_fmts = rk3399_vpu_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.codec = RK_VPU_MPEG2_DECODER,
+	.codec_ops = rk3399_vpu_codec_ops,
+	.vdpu_irq = rk3399_vdpu_irq,
+	.init = rk3399_vpu_hw_init,
+	.clk_names = {"aclk", "hclk"},
+	.num_clocks = 2
+};

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

index b94ff97451db..2e22009b6583 100644
@@ -419,6 +419,7 @@ static const struct v4l2_file_operations rockchip_vpu_fops = {
 

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c static const struct v4l2_file_operations rockchip_vpu_fops = { · line 422

Quoted 3 lines

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +1 -0

 static const struct of_device_id of_rockchip_vpu_match[] = {
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
+	{ .compatible = "rockchip,rk3328-vpu", .data = &rk3328_vpu_variant, },

This new compatible string should be documented in Documentation/devicetree/bindings/media/rockchip-vpu.txt as well.

I'll take patches 1-15 and drop this one. This patch can be merged once the bindings file is updated as well.

Regards,

Hans

Quoted 16 lines

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

 	{ .compatible = "rockchip,rk3288-vpu", .data = &rk3288_vpu_variant, },
 	{ /* sentinel */ }
 };

drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h +1 -0

index 6cecb528f994..3d6b97af90fb 100644
@@ -79,6 +79,7 @@ enum rockchip_vpu_enc_fmt {
 };
 
 extern const struct rockchip_vpu_variant rk3399_vpu_variant;
+extern const struct rockchip_vpu_variant rk3328_vpu_variant;
 extern const struct rockchip_vpu_variant rk3288_vpu_variant;
 
 void rockchip_vpu_watchdog(struct work_struct *work);

Quoted 48 lines · Hans Verkuil

Quoted 40 lines · Ezequiel Garcia

From: Jonas Karlman <[email protected]>

Add necessary bits to support MPEG2 decoding on RK3328.

Signed-off-by Jonas Karlman <[email protected]>

Signed-off-by Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]>

--
Changes from v5:
* New patch.

 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c   | 12 ++++++++++++
 .../staging/media/rockchip/vpu/rockchip_vpu_drv.c    |  1 +
 drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h |  1 +
 3 files changed, 14 insertions(+)

drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c +12 -0

index 2b3689968ef4..341f8d69c33d 100644
@@ -175,3 +175,15 @@ const struct rockchip_vpu_variant rk3399_vpu_variant = {
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2
 };
+
+const struct rockchip_vpu_variant rk3328_vpu_variant = {
+	.dec_offset = 0x400,
+	.dec_fmts = rk3399_vpu_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.codec = RK_VPU_MPEG2_DECODER,
+	.codec_ops = rk3399_vpu_codec_ops,
+	.vdpu_irq = rk3399_vdpu_irq,
+	.init = rk3399_vpu_hw_init,
+	.clk_names = {"aclk", "hclk"},
+	.num_clocks = 2
+};

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

index b94ff97451db..2e22009b6583 100644

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c static const struct v4l2_file_operations rockchip_vpu_fops = { · line 422

Quoted 4 lines

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +1 -0

@@ -419,6 +419,7 @@ static const struct v4l2_file_operations rockchip_vpu_fops = {
 
 static const struct of_device_id of_rockchip_vpu_match[] = {
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
+	{ .compatible = "rockchip,rk3328-vpu", .data = &rk3328_vpu_variant, },

This new compatible string should be documented in Documentation/devicetree/bindings/media/rockchip-vpu.txt as well.

Quoted 2 lines

I'll take patches 1-15 and drop this one. This patch can be merged once the bindings file is updated as well.

Looks like the bindings patch did not get picked for v6 :-)

I am a bit unclear on how to handle patch submission that covers multiple subtrees. Should I send a single series including three patches: bindings update, this patch and device tree update? Or is a series with only bindings update and this patch preferred?

Regards,
Jonas

Quoted 21 lines

Regards,

Hans

Quoted 16 lines

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

 	{ .compatible = "rockchip,rk3288-vpu", .data = &rk3288_vpu_variant, },
 	{ /* sentinel */ }
 };

drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h +1 -0

index 6cecb528f994..3d6b97af90fb 100644
@@ -79,6 +79,7 @@ enum rockchip_vpu_enc_fmt {
 };
 
 extern const struct rockchip_vpu_variant rk3399_vpu_variant;
+extern const struct rockchip_vpu_variant rk3328_vpu_variant;
 extern const struct rockchip_vpu_variant rk3288_vpu_variant;
 
 void rockchip_vpu_watchdog(struct work_struct *work);

Quoted 48 lines · Hans Verkuil

Quoted 40 lines · Ezequiel Garcia

From: Jonas Karlman <[email protected]>

Add necessary bits to support MPEG2 decoding on RK3328.

Signed-off-by Jonas Karlman <[email protected]>

Signed-off-by Ezequiel Garcia <ezequiel-ZGY8ohtN/[email protected]>

--
Changes from v5:
* New patch.

 drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c   | 12 ++++++++++++
 .../staging/media/rockchip/vpu/rockchip_vpu_drv.c    |  1 +
 drivers/staging/media/rockchip/vpu/rockchip_vpu_hw.h |  1 +
 3 files changed, 14 insertions(+)

drivers/staging/media/rockchip/vpu/rk3399_vpu_hw.c +12 -0

index 2b3689968ef4..341f8d69c33d 100644
@@ -175,3 +175,15 @@ const struct rockchip_vpu_variant rk3399_vpu_variant = {
 	.clk_names = {"aclk", "hclk"},
 	.num_clocks = 2
 };
+
+const struct rockchip_vpu_variant rk3328_vpu_variant = {
+	.dec_offset = 0x400,
+	.dec_fmts = rk3399_vpu_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(rk3399_vpu_dec_fmts),
+	.codec = RK_VPU_MPEG2_DECODER,
+	.codec_ops = rk3399_vpu_codec_ops,
+	.vdpu_irq = rk3399_vdpu_irq,
+	.init = rk3399_vpu_hw_init,
+	.clk_names = {"aclk", "hclk"},
+	.num_clocks = 2
+};

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +0 -0

index b94ff97451db..2e22009b6583 100644

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c static const struct v4l2_file_operations rockchip_vpu_fops = { · line 422

Quoted 4 lines

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +1 -0

@@ -419,6 +419,7 @@ static const struct v4l2_file_operations rockchip_vpu_fops = {
 
 static const struct of_device_id of_rockchip_vpu_match[] = {
 	{ .compatible = "rockchip,rk3399-vpu", .data = &rk3399_vpu_variant, },
+	{ .compatible = "rockchip,rk3328-vpu", .data = &rk3328_vpu_variant, },

This new compatible string should be documented in Documentation/devicetree/bindings/media/rockchip-vpu.txt as well.

Quoted 2 lines

I'll take patches 1-15 and drop this one. This patch can be merged once the bindings file is updated as well.

Looks like the bindings patch did not get picked for v6 :-)

I am a bit unclear on how to handle patch submission that covers multiple subtrees. Should I send a single series including three patches: bindings update, this patch and device tree update? Or is a series with only bindings update and this patch preferred?