[Patch 00/13] media: am437x-vpfe: overdue maintenance

[Patch 00/13] media: am437x-vpfe: overdue maintenance

[Benoit Parrot]

This patch series is a collection of patches we have been carrying for a
while.

A few patches do fix actual bug and v4l2-compliance errors/warnings.
Other are drivers re-work to simplify/clarify the code for easier
maintenance.

We also include the SPDX Licensing update which seemed to have been
missed by the global script thus far.

Changes since v1:
- Address review comment from Joe, Hans and Prabhakar
- Cleaned-up the function entry debug log
- Split off the pcr change into its own patch
- Rework/combine two patches but remove code churn
- fix miscellaneous typos

=============================

v4l2-compliance SHA: 5b168dc8473911227890526bad26553d9e8ff81b, 32 bits

Compliance test for vpfe device /dev/video0:

Driver Info:
	Driver name      : vpfe
	Card type        : TI AM437x VPFE
	Bus info         : platform:vpfe 48326000.vpfe
	Driver version   : 5.3.0
	Capabilities     : 0x85200001
		Vide o Capture
		Read/Write
		Streaming
		Extended Pix Format
		D evice Capabilities
	Device Caps      : 0x05200001
		Video Capt ure
		Read/Write
		Streaming
		Extended Pix Format

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second /dev/video0 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 3 Private Controls: 0

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
		fail: v4l2-test-formats.cpp(1419): node->frmsizes_count[pixfmt] > 1
	test Cropping: FAIL
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
	test read/write: OK
	test blocking wait: OK
	test MMAP (no poll): OK                           
	test MMAP (select): OK                            
	test MMAP (epoll): OK                             
	test USERPTR (no poll): OK (Not Supported)
	test USERPTR (select): OK (Not Supported)
	test DMABUF: Cannot test, specify --expbuf-device

Total for vpfe device /dev/video0: 51, Succeeded: 50, Failed: 1, Warnings: 0
dd
============================

Benoit Parrot (12):
  media: am437x-vpfe: Fix missing first line
  media: am437x-vpfe: Rework ISR routine for clarity
  media: am437x-vpfe: Wait for end of frame before tear-down
  media: am437x-vpfe: fix start streaming error path
  media: am437x-vpfe: Streamlined vb2 buffer cleanup
  media: am437x-vpfe: Setting STD to current value is not an error
  media: am437x-vpfe: Use a per instance format array instead of a
    static one
  media: am437x-vpfe: fix function trace debug log
  media: am437x-vpfe: Remove print_fourcc helper
  media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
  media: am437x-vpfe: Remove per bus width static data
  media: am437x-vpfe: Switch to SPDX Licensing

Dave Gerlach (1):
  media: am437x-vpfe: Fix suspend path to always handle pinctrl config

 drivers/media/platform/am437x/am437x-vpfe.c   | 878 ++++++++----------
 drivers/media/platform/am437x/am437x-vpfe.h   |  45 +-
 .../media/platform/am437x/am437x-vpfe_regs.h  |  10 +-
 3 files changed, 406 insertions(+), 527 deletions(-)

-- 
2.17.1

[Patch v2 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config

[Benoit Parrot]

From: Dave Gerlach <d-gerlach@ti.com>

Currently if vpfe is not active then it returns immediately in the
suspend and resume handlers. Change this so that it always performs the
pinctrl config so that we can still get proper sleep state configuration
on the pins even if we do not need to worry about fully saving and
restoring context.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 46 ++++++++++-----------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 2b42ba1f5949..a3d22f90e64c 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2653,22 +2653,21 @@ static int vpfe_suspend(struct device *dev)
 	struct vpfe_device *vpfe = dev_get_drvdata(dev);
 	struct vpfe_ccdc *ccdc = &vpfe->ccdc;
 
-	/* if streaming has not started we don't care */
-	if (!vb2_start_streaming_called(&vpfe->buffer_queue))
-		return 0;
-
-	pm_runtime_get_sync(dev);
-	vpfe_config_enable(ccdc, 1);
+	/* only do full suspend if streaming has started */
+	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
+		pm_runtime_get_sync(dev);
+		vpfe_config_enable(ccdc, 1);
 
-	/* Save VPFE context */
-	vpfe_save_context(ccdc);
+		/* Save VPFE context */
+		vpfe_save_context(ccdc);
 
-	/* Disable CCDC */
-	vpfe_pcr_enable(ccdc, 0);
-	vpfe_config_enable(ccdc, 0);
+		/* Disable CCDC */
+		vpfe_pcr_enable(ccdc, 0);
+		vpfe_config_enable(ccdc, 0);
 
-	/* Disable both master and slave clock */
-	pm_runtime_put_sync(dev);
+		/* Disable both master and slave clock */
+		pm_runtime_put_sync(dev);
+	}
 
 	/* Select sleep pin state */
 	pinctrl_pm_select_sleep_state(dev);
@@ -2710,19 +2709,18 @@ static int vpfe_resume(struct device *dev)
 	struct vpfe_device *vpfe = dev_get_drvdata(dev);
 	struct vpfe_ccdc *ccdc = &vpfe->ccdc;
 
-	/* if streaming has not started we don't care */
-	if (!vb2_start_streaming_called(&vpfe->buffer_queue))
-		return 0;
-
-	/* Enable both master and slave clock */
-	pm_runtime_get_sync(dev);
-	vpfe_config_enable(ccdc, 1);
+	/* only do full resume if streaming has started */
+	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
+		/* Enable both master and slave clock */
+		pm_runtime_get_sync(dev);
+		vpfe_config_enable(ccdc, 1);
 
-	/* Restore VPFE context */
-	vpfe_restore_context(ccdc);
+		/* Restore VPFE context */
+		vpfe_restore_context(ccdc);
 
-	vpfe_config_enable(ccdc, 0);
-	pm_runtime_put_sync(dev);
+		vpfe_config_enable(ccdc, 0);
+		pm_runtime_put_sync(dev);
+	}
 
 	/* Select default pin state */
 	pinctrl_pm_select_default_state(dev);
-- 
2.17.1

[Patch v2 02/13] media: am437x-vpfe: Fix missing first line

[Benoit Parrot]

Previous generation of this driver were hard coded to handle
encoder/decoder where the first line never contains any data and
was therefore always skipped, however when dealing with actual
camera sensors the first line is always present.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index a3d22f90e64c..1521c072f3e3 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -345,13 +345,9 @@ static void vpfe_ccdc_setwin(struct vpfe_ccdc *ccdc,
 	if (frm_fmt == CCDC_FRMFMT_INTERLACED) {
 		vert_nr_lines = (image_win->height >> 1) - 1;
 		vert_start >>= 1;
-		/* Since first line doesn't have any data */
-		vert_start += 1;
 		/* configure VDINT0 */
 		val = (vert_start << VPFE_VDINT_VDINT0_SHIFT);
 	} else {
-		/* Since first line doesn't have any data */
-		vert_start += 1;
 		vert_nr_lines = image_win->height - 1;
 		/*
 		 * configure VDINT0 and VDINT1. VDINT1 will be at half
-- 
2.17.1

[Patch v2 03/13] media: am437x-vpfe: Rework ISR routine for clarity

[Benoit Parrot]

Make the ISR code simpler to follow by removing goto and
relocating/eliminating duplicate spinlock accesses.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 128 ++++++++++----------
 1 file changed, 66 insertions(+), 62 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 1521c072f3e3..13bf4b32b40b 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1233,22 +1233,29 @@ static int vpfe_open(struct file *file)
  * This function will get next buffer from the dma queue and
  * set the buffer address in the vpfe register for capture.
  * the buffer is marked active
- *
- * Assumes caller is holding vpfe->dma_queue_lock already
  */
-static inline void vpfe_schedule_next_buffer(struct vpfe_device *vpfe)
+static void vpfe_schedule_next_buffer(struct vpfe_device *vpfe)
 {
+	dma_addr_t addr;
+
+	spin_lock(&vpfe->dma_queue_lock);
+	if (list_empty(&vpfe->dma_queue)) {
+		spin_unlock(&vpfe->dma_queue_lock);
+		return;
+	}
+
 	vpfe->next_frm = list_entry(vpfe->dma_queue.next,
 				    struct vpfe_cap_buffer, list);
 	list_del(&vpfe->next_frm->list);
+	spin_unlock(&vpfe->dma_queue_lock);
 
-	vpfe_set_sdr_addr(&vpfe->ccdc,
-	       vb2_dma_contig_plane_dma_addr(&vpfe->next_frm->vb.vb2_buf, 0));
+	addr = vb2_dma_contig_plane_dma_addr(&vpfe->next_frm->vb.vb2_buf, 0);
+	vpfe_set_sdr_addr(&vpfe->ccdc, addr);
 }
 
 static inline void vpfe_schedule_bottom_field(struct vpfe_device *vpfe)
 {
-	unsigned long addr;
+	dma_addr_t addr;
 
 	addr = vb2_dma_contig_plane_dma_addr(&vpfe->next_frm->vb.vb2_buf, 0) +
 					vpfe->field_off;
@@ -1273,6 +1280,55 @@ static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe)
 	vpfe->cur_frm = vpfe->next_frm;
 }
 
+static void vpfe_handle_interlaced_irq(struct vpfe_device *vpfe,
+				       enum v4l2_field field)
+{
+	int fid;
+
+	/* interlaced or TB capture check which field
+	 * we are in hardware
+	 */
+	fid = vpfe_ccdc_getfid(&vpfe->ccdc);
+
+	/* switch the software maintained field id */
+	vpfe->field ^= 1;
+	if (fid == vpfe->field) {
+		/* we are in-sync here,continue */
+		if (fid == 0) {
+			/*
+			 * One frame is just being captured. If the
+			 * next frame is available, release the
+			 * current frame and move on
+			 */
+			if (vpfe->cur_frm != vpfe->next_frm)
+				vpfe_process_buffer_complete(vpfe);
+
+			/*
+			 * based on whether the two fields are stored
+			 * interleave or separately in memory,
+			 * reconfigure the CCDC memory address
+			 */
+			if (field == V4L2_FIELD_SEQ_TB)
+				vpfe_schedule_bottom_field(vpfe);
+		} else {
+			/*
+			 * if one field is just being captured configure
+			 * the next frame get the next frame from the empty
+			 * queue if no frame is available hold on to the
+			 * current buffer
+			 */
+			if (vpfe->cur_frm == vpfe->next_frm)
+				vpfe_schedule_next_buffer(vpfe);
+		}
+	} else if (fid == 0) {
+		/*
+		 * out of sync. Recover from any hardware out-of-sync.
+		 * May loose one frame
+		 */
+		vpfe->field = fid;
+	}
+}
+
 /*
  * vpfe_isr : ISR handler for vpfe capture (VINT0)
  * @irq: irq number
@@ -1284,76 +1340,24 @@ static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe)
 static irqreturn_t vpfe_isr(int irq, void *dev)
 {
 	struct vpfe_device *vpfe = (struct vpfe_device *)dev;
-	enum v4l2_field field;
+	enum v4l2_field field = vpfe->fmt.fmt.pix.field;
 	int intr_status;
-	int fid;
 
 	intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS);
 
 	if (intr_status & VPFE_VDINT0) {
-		field = vpfe->fmt.fmt.pix.field;
-
 		if (field == V4L2_FIELD_NONE) {
-			/* handle progressive frame capture */
 			if (vpfe->cur_frm != vpfe->next_frm)
 				vpfe_process_buffer_complete(vpfe);
-			goto next_intr;
-		}
-
-		/* interlaced or TB capture check which field
-		   we are in hardware */
-		fid = vpfe_ccdc_getfid(&vpfe->ccdc);
-
-		/* switch the software maintained field id */
-		vpfe->field ^= 1;
-		if (fid == vpfe->field) {
-			/* we are in-sync here,continue */
-			if (fid == 0) {
-				/*
-				 * One frame is just being captured. If the
-				 * next frame is available, release the
-				 * current frame and move on
-				 */
-				if (vpfe->cur_frm != vpfe->next_frm)
-					vpfe_process_buffer_complete(vpfe);
-				/*
-				 * based on whether the two fields are stored
-				 * interleave or separately in memory,
-				 * reconfigure the CCDC memory address
-				 */
-				if (field == V4L2_FIELD_SEQ_TB)
-					vpfe_schedule_bottom_field(vpfe);
-
-				goto next_intr;
-			}
-			/*
-			 * if one field is just being captured configure
-			 * the next frame get the next frame from the empty
-			 * queue if no frame is available hold on to the
-			 * current buffer
-			 */
-			spin_lock(&vpfe->dma_queue_lock);
-			if (!list_empty(&vpfe->dma_queue) &&
-			    vpfe->cur_frm == vpfe->next_frm)
-				vpfe_schedule_next_buffer(vpfe);
-			spin_unlock(&vpfe->dma_queue_lock);
-		} else if (fid == 0) {
-			/*
-			 * out of sync. Recover from any hardware out-of-sync.
-			 * May loose one frame
-			 */
-			vpfe->field = fid;
+		} else {
+			vpfe_handle_interlaced_irq(vpfe, field);
 		}
 	}
 
-next_intr:
 	if (intr_status & VPFE_VDINT1) {
-		spin_lock(&vpfe->dma_queue_lock);
-		if (vpfe->fmt.fmt.pix.field == V4L2_FIELD_NONE &&
-		    !list_empty(&vpfe->dma_queue) &&
+		if (field == V4L2_FIELD_NONE &&
 		    vpfe->cur_frm == vpfe->next_frm)
 			vpfe_schedule_next_buffer(vpfe);
-		spin_unlock(&vpfe->dma_queue_lock);
 	}
 
 	vpfe_clear_intr(&vpfe->ccdc, intr_status);
-- 
2.17.1

[Patch v2 04/13] media: am437x-vpfe: Wait for end of frame before tear-down

[Benoit Parrot]

We were originally attempting to stop all processing as soon
as possible, but the in-progress DMA operation cannot be canceled.
This led to the module being in a busy state and prevented proper
power management functionality.

The existing implementation would attempt to clean things up by waiting
up to 50ms. However when receiving video frame at 15fps or lower,
it meant an inter frame arrival rate of 66.6 ms or higher.
In such cases upon tear down the following message could be seen:
omap_hwmod: vpfe0: _wait_target_disable failed

This patch fixes this issue by adding a stopping state where
we would wait for the next Vsync before disabling the hardware.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 52 ++++++++++-----------
 drivers/media/platform/am437x/am437x-vpfe.h |  3 ++
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 13bf4b32b40b..66df87d310a2 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -441,40 +441,25 @@ static void vpfe_ccdc_restore_defaults(struct vpfe_ccdc *ccdc)
 
 static int vpfe_ccdc_close(struct vpfe_ccdc *ccdc, struct device *dev)
 {
-	int dma_cntl, i, pcr;
+	struct vpfe_device *vpfe = container_of(ccdc, struct vpfe_device, ccdc);
+	u32 dma_cntl, pcr;
 
-	/* If the CCDC module is still busy wait for it to be done */
-	for (i = 0; i < 10; i++) {
-		usleep_range(5000, 6000);
-		pcr = vpfe_reg_read(ccdc, VPFE_PCR);
-		if (!pcr)
-			break;
+	pcr = vpfe_reg_read(ccdc, VPFE_PCR);
+	if (pcr)
+		vpfe_dbg(1, vpfe, "VPFE_PCR is still set (%x)", pcr);
 
-		/* make sure it it is disabled */
-		vpfe_pcr_enable(ccdc, 0);
-	}
+	dma_cntl = vpfe_reg_read(ccdc, VPFE_DMA_CNTL);
+	if ((dma_cntl & VPFE_DMA_CNTL_OVERFLOW))
+		vpfe_dbg(1, vpfe, "VPFE_DMA_CNTL_OVERFLOW is still set (%x)",
+			 dma_cntl);
 
 	/* Disable CCDC by resetting all register to default POR values */
 	vpfe_ccdc_restore_defaults(ccdc);
 
-	/* if DMA_CNTL overflow bit is set. Clear it
-	 *  It appears to take a while for this to become quiescent ~20ms
-	 */
-	for (i = 0; i < 10; i++) {
-		dma_cntl = vpfe_reg_read(ccdc, VPFE_DMA_CNTL);
-		if (!(dma_cntl & VPFE_DMA_CNTL_OVERFLOW))
-			break;
-
-		/* Clear the overflow bit */
-		vpfe_reg_write(ccdc, dma_cntl, VPFE_DMA_CNTL);
-		usleep_range(5000, 6000);
-	}
-
 	/* Disabled the module at the CONFIG level */
 	vpfe_config_enable(ccdc, 0);
 
 	pm_runtime_put_sync(dev);
-
 	return 0;
 }
 
@@ -1303,6 +1288,9 @@ static void vpfe_handle_interlaced_irq(struct vpfe_device *vpfe,
 			if (vpfe->cur_frm != vpfe->next_frm)
 				vpfe_process_buffer_complete(vpfe);
 
+			if (vpfe->stopping)
+				return;
+
 			/*
 			 * based on whether the two fields are stored
 			 * interleave or separately in memory,
@@ -1341,7 +1329,7 @@ static irqreturn_t vpfe_isr(int irq, void *dev)
 {
 	struct vpfe_device *vpfe = (struct vpfe_device *)dev;
 	enum v4l2_field field = vpfe->fmt.fmt.pix.field;
-	int intr_status;
+	int intr_status, stopping = vpfe->stopping;
 
 	intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS);
 
@@ -1352,9 +1340,13 @@ static irqreturn_t vpfe_isr(int irq, void *dev)
 		} else {
 			vpfe_handle_interlaced_irq(vpfe, field);
 		}
+		if (stopping) {
+			vpfe->stopping = false;
+			complete(&vpfe->capture_stop);
+		}
 	}
 
-	if (intr_status & VPFE_VDINT1) {
+	if (intr_status & VPFE_VDINT1 && !stopping) {
 		if (field == V4L2_FIELD_NONE &&
 		    vpfe->cur_frm == vpfe->next_frm)
 			vpfe_schedule_next_buffer(vpfe);
@@ -1980,6 +1972,9 @@ static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	vpfe_attach_irq(vpfe);
 
+	vpfe->stopping = false;
+	init_completion(&vpfe->capture_stop);
+
 	if (vpfe->ccdc.ccdc_cfg.if_type == VPFE_RAW_BAYER)
 		vpfe_ccdc_config_raw(&vpfe->ccdc);
 	else
@@ -2032,6 +2027,11 @@ static void vpfe_stop_streaming(struct vb2_queue *vq)
 
 	vpfe_pcr_enable(&vpfe->ccdc, 0);
 
+	/* Wait for the last frame to be captured */
+	vpfe->stopping = true;
+	wait_for_completion_timeout(&vpfe->capture_stop,
+				    msecs_to_jiffies(250));
+
 	vpfe_detach_irq(vpfe);
 
 	sdinfo = vpfe->current_subdev;
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index 4678285f34c6..2dde09780215 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -23,6 +23,7 @@
 
 #include <linux/am437x-vpfe.h>
 #include <linux/clk.h>
+#include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/i2c.h>
@@ -270,6 +271,8 @@ struct vpfe_device {
 	 */
 	u32 field_off;
 	struct vpfe_ccdc ccdc;
+	int stopping;
+	struct completion capture_stop;
 };
 
 #endif	/* AM437X_VPFE_H */
-- 
2.17.1

[Patch v2 05/13] media: am437x-vpfe: fix start streaming error path

[Benoit Parrot]

When start_streaming fails the h/w module might be left enabled
inadvertently. Make sure it is disabled in the error path.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 66df87d310a2..e0a4c8920df8 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2008,6 +2008,7 @@ static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
 		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
 	}
 
+	vpfe_pcr_enable(&vpfe->ccdc, 0);
 	return ret;
 }
 
-- 
2.17.1

[Patch v2 06/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup

[Benoit Parrot]

Returning queued vb2 buffers back to user space is a common
task best handled by a helper function.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 53 ++++++++++-----------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index e0a4c8920df8..03415c179c85 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1949,6 +1949,29 @@ static void vpfe_buffer_queue(struct vb2_buffer *vb)
 	spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags);
 }
 
+static void vpfe_return_all_buffers(struct vpfe_device *vpfe,
+				    enum vb2_buffer_state state)
+{
+	struct vpfe_cap_buffer *buf, *node;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vpfe->dma_queue_lock, flags);
+	list_for_each_entry_safe(buf, node, &vpfe->dma_queue, list) {
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
+		list_del(&buf->list);
+	}
+
+	if (vpfe->cur_frm)
+		vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf, state);
+
+	if (vpfe->next_frm && vpfe->next_frm != vpfe->cur_frm)
+		vb2_buffer_done(&vpfe->next_frm->vb.vb2_buf, state);
+
+	vpfe->cur_frm = NULL;
+	vpfe->next_frm = NULL;
+	spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags);
+}
+
 /*
  * vpfe_start_streaming : Starts the DMA engine for streaming
  * @vb: ptr to vb2_buffer
@@ -1957,7 +1980,6 @@ static void vpfe_buffer_queue(struct vb2_buffer *vb)
 static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct vpfe_device *vpfe = vb2_get_drv_priv(vq);
-	struct vpfe_cap_buffer *buf, *tmp;
 	struct vpfe_subdev_info *sdinfo;
 	unsigned long flags;
 	unsigned long addr;
@@ -2003,11 +2025,7 @@ static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 
 err:
-	list_for_each_entry_safe(buf, tmp, &vpfe->dma_queue, list) {
-		list_del(&buf->list);
-		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
-	}
-
+	vpfe_return_all_buffers(vpfe, VB2_BUF_STATE_QUEUED);
 	vpfe_pcr_enable(&vpfe->ccdc, 0);
 	return ret;
 }
@@ -2023,7 +2041,6 @@ static void vpfe_stop_streaming(struct vb2_queue *vq)
 {
 	struct vpfe_device *vpfe = vb2_get_drv_priv(vq);
 	struct vpfe_subdev_info *sdinfo;
-	unsigned long flags;
 	int ret;
 
 	vpfe_pcr_enable(&vpfe->ccdc, 0);
@@ -2041,27 +2058,7 @@ static void vpfe_stop_streaming(struct vb2_queue *vq)
 		vpfe_dbg(1, vpfe, "stream off failed in subdev\n");
 
 	/* release all active buffers */
-	spin_lock_irqsave(&vpfe->dma_queue_lock, flags);
-	if (vpfe->cur_frm == vpfe->next_frm) {
-		vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf,
-				VB2_BUF_STATE_ERROR);
-	} else {
-		if (vpfe->cur_frm != NULL)
-			vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf,
-					VB2_BUF_STATE_ERROR);
-		if (vpfe->next_frm != NULL)
-			vb2_buffer_done(&vpfe->next_frm->vb.vb2_buf,
-					VB2_BUF_STATE_ERROR);
-	}
-
-	while (!list_empty(&vpfe->dma_queue)) {
-		vpfe->next_frm = list_entry(vpfe->dma_queue.next,
-						struct vpfe_cap_buffer, list);
-		list_del(&vpfe->next_frm->list);
-		vb2_buffer_done(&vpfe->next_frm->vb.vb2_buf,
-				VB2_BUF_STATE_ERROR);
-	}
-	spin_unlock_irqrestore(&vpfe->dma_queue_lock, flags);
+	vpfe_return_all_buffers(vpfe, VB2_BUF_STATE_ERROR);
 }
 
 static int vpfe_g_pixelaspect(struct file *file, void *priv,
-- 
2.17.1

[Patch v2 07/13] media: am437x-vpfe: Setting STD to current value is not an error

[Benoit Parrot]

VIDIOC_S_STD should not return an error if the value is identical
to the current one.
This error was highlighted by the v4l2-compliance test.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 03415c179c85..8b218f48428f 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1822,6 +1822,10 @@ static int vpfe_s_std(struct file *file, void *priv, v4l2_std_id std_id)
 	if (!(sdinfo->inputs[0].capabilities & V4L2_IN_CAP_STD))
 		return -ENODATA;
 
+	/* if trying to set the same std then nothing to do */
+	if (vpfe_standards[vpfe->std_index].std_id == std_id)
+		return 0;
+
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
 		vpfe_err(vpfe, "%s device busy\n", __func__);
-- 
2.17.1

[Patch v2 08/13] media: am437x-vpfe: Use a per instance format array instead of a static one

[Benoit Parrot]

Using a statically defined format array would cause issue when
multiple vpfe instance would be connected to sub-device of
different capabilities. We need to use an instance based array
instead to properly maintain a per port/instance format list.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 110 ++++++++------------
 drivers/media/platform/am437x/am437x-vpfe.h |  36 +++++++
 2 files changed, 77 insertions(+), 69 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 8b218f48428f..569618a52082 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -69,31 +69,7 @@ static const struct vpfe_standard vpfe_standards[] = {
 	{V4L2_STD_625_50, 720, 576, {54, 59}, 1},
 };
 
-struct bus_format {
-	unsigned int width;
-	unsigned int bpp;
-};
-
-/*
- * struct vpfe_fmt - VPFE media bus format information
- * @code: V4L2 media bus format code
- * @shifted: V4L2 media bus format code for the same pixel layout but
- *	shifted to be 8 bits per pixel. =0 if format is not shiftable.
- * @pixelformat: V4L2 pixel format FCC identifier
- * @width: Bits per pixel (when transferred over a bus)
- * @bpp: Bytes per pixel (when stored in memory)
- * @supported: Indicates format supported by subdev
- */
-struct vpfe_fmt {
-	u32 fourcc;
-	u32 code;
-	struct bus_format l;
-	struct bus_format s;
-	bool supported;
-	u32 index;
-};
-
-static struct vpfe_fmt formats[] = {
+static struct vpfe_fmt formats[VPFE_MAX_ACTIVE_FMT] = {
 	{
 		.fourcc		= V4L2_PIX_FMT_YUYV,
 		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
@@ -101,7 +77,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 4,
 		.s.width	= 8,
 		.s.bpp		= 2,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_UYVY,
 		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
@@ -109,7 +84,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 4,
 		.s.width	= 8,
 		.s.bpp		= 2,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_YVYU,
 		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
@@ -117,7 +91,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 4,
 		.s.width	= 8,
 		.s.bpp		= 2,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_VYUY,
 		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
@@ -125,7 +98,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 4,
 		.s.width	= 8,
 		.s.bpp		= 2,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SBGGR8,
 		.code		= MEDIA_BUS_FMT_SBGGR8_1X8,
@@ -133,7 +105,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 2,
 		.s.width	= 8,
 		.s.bpp		= 1,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGBRG8,
 		.code		= MEDIA_BUS_FMT_SGBRG8_1X8,
@@ -141,7 +112,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 2,
 		.s.width	= 8,
 		.s.bpp		= 1,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGRBG8,
 		.code		= MEDIA_BUS_FMT_SGRBG8_1X8,
@@ -149,7 +119,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 2,
 		.s.width	= 8,
 		.s.bpp		= 1,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SRGGB8,
 		.code		= MEDIA_BUS_FMT_SRGGB8_1X8,
@@ -157,7 +126,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 2,
 		.s.width	= 8,
 		.s.bpp		= 1,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_RGB565,
 		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,
@@ -165,7 +133,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 4,
 		.s.width	= 8,
 		.s.bpp		= 2,
-		.supported	= false,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_RGB565X,
 		.code		= MEDIA_BUS_FMT_RGB565_2X8_BE,
@@ -173,7 +140,6 @@ static struct vpfe_fmt formats[] = {
 		.l.bpp		= 4,
 		.s.width	= 8,
 		.s.bpp		= 2,
-		.supported	= false,
 	},
 };
 
@@ -181,13 +147,14 @@ static int
 __vpfe_get_format(struct vpfe_device *vpfe,
 		  struct v4l2_format *format, unsigned int *bpp);
 
-static struct vpfe_fmt *find_format_by_code(unsigned int code)
+static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
+					    unsigned int code)
 {
 	struct vpfe_fmt *fmt;
 	unsigned int k;
 
-	for (k = 0; k < ARRAY_SIZE(formats); k++) {
-		fmt = &formats[k];
+	for (k = 0; k < vpfe->num_active_fmt; k++) {
+		fmt = vpfe->active_fmt[k];
 		if (fmt->code == code)
 			return fmt;
 	}
@@ -195,13 +162,14 @@ static struct vpfe_fmt *find_format_by_code(unsigned int code)
 	return NULL;
 }
 
-static struct vpfe_fmt *find_format_by_pix(unsigned int pixelformat)
+static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
+					   unsigned int pixelformat)
 {
 	struct vpfe_fmt *fmt;
 	unsigned int k;
 
-	for (k = 0; k < ARRAY_SIZE(formats); k++) {
-		fmt = &formats[k];
+	for (k = 0; k < vpfe->num_active_fmt; k++) {
+		fmt = vpfe->active_fmt[k];
 		if (fmt->fourcc == pixelformat)
 			return fmt;
 	}
@@ -218,7 +186,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
 	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
 	struct vpfe_fmt *fmt;
 
-	fmt = find_format_by_code(mbus->code);
+	fmt = find_format_by_code(vpfe, mbus->code);
 	if (WARN_ON(fmt == NULL)) {
 		pr_err("Invalid mbus code set\n");
 		*bpp = 1;
@@ -241,12 +209,12 @@ static void pix_to_mbus(struct vpfe_device *vpfe,
 {
 	struct vpfe_fmt *fmt;
 
-	fmt = find_format_by_pix(pix_fmt->pixelformat);
+	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
 	if (!fmt) {
 		/* default to first entry */
 		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
 			pix_fmt->pixelformat);
-		fmt = &formats[0];
+		fmt = vpfe->active_fmt[0];
 	}
 
 	memset(mbus_fmt, 0, sizeof(*mbus_fmt));
@@ -1494,8 +1462,7 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 	struct vpfe_subdev_info *sdinfo;
-	struct vpfe_fmt *fmt = NULL;
-	unsigned int k;
+	struct vpfe_fmt *fmt;
 
 	vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n",
 		f->index);
@@ -1504,17 +1471,10 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
 	if (!sdinfo->sd)
 		return -EINVAL;
 
-	if (f->index > ARRAY_SIZE(formats))
+	if (f->index >= vpfe->num_active_fmt)
 		return -EINVAL;
 
-	for (k = 0; k < ARRAY_SIZE(formats); k++) {
-		if (formats[k].index == f->index) {
-			fmt = &formats[k];
-			break;
-		}
-	}
-	if (!fmt)
-		return -EINVAL;
+	fmt = vpfe->active_fmt[f->index];
 
 	f->pixelformat = fmt->fourcc;
 
@@ -1593,7 +1553,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
 
 	/* check for valid format */
-	fmt = find_format_by_pix(fsize->pixel_format);
+	fmt = find_format_by_pix(vpfe, fsize->pixel_format);
 	if (!fmt) {
 		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
 			fsize->pixel_format);
@@ -2281,8 +2241,10 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
 					       struct vpfe_device, v4l2_dev);
 	struct v4l2_subdev_mbus_code_enum mbus_code;
 	struct vpfe_subdev_info *sdinfo;
+	struct vpfe_fmt *fmt;
+	int ret = 0;
 	bool found = false;
-	int i, j;
+	int i, j, k;
 
 	vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
 
@@ -2304,27 +2266,37 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
 
 	vpfe->video_dev.tvnorms |= sdinfo->inputs[0].std;
 
-	/* setup the supported formats & indexes */
-	for (j = 0, i = 0; ; ++j) {
-		struct vpfe_fmt *fmt;
-		int ret;
-
+	vpfe->num_active_fmt = 0;
+	for (j = 0, i = 0; (ret != -EINVAL); ++j) {
 		memset(&mbus_code, 0, sizeof(mbus_code));
 		mbus_code.index = j;
 		mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 		ret = v4l2_subdev_call(subdev, pad, enum_mbus_code,
-			       NULL, &mbus_code);
+				       NULL, &mbus_code);
 		if (ret)
-			break;
-
-		fmt = find_format_by_code(mbus_code.code);
-		if (!fmt)
 			continue;
 
-		fmt->supported = true;
-		fmt->index = i++;
+		vpfe_dbg(3, vpfe,
+			 "subdev %s: code: %04x idx: %d\n",
+			 subdev->name, mbus_code.code, j);
+
+		for (k = 0; k < ARRAY_SIZE(formats); k++) {
+			fmt = &formats[k];
+			if (mbus_code.code != fmt->code)
+				continue;
+			vpfe->active_fmt[i] = fmt;
+			vpfe_dbg(3, vpfe,
+				 "matched fourcc: %4.4s code: %04x idx: %d\n",
+				 (char *)&fmt->fourcc, mbus_code.code, i);
+			vpfe->num_active_fmt = ++i;
+		}
 	}
 
+	if (!i) {
+		vpfe_err(vpfe, "No suitable format reported by subdev %s\n",
+			 subdev->name);
+		return -EINVAL;
+	}
 	return 0;
 }
 
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index 2dde09780215..79a6505c083d 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -215,6 +215,39 @@ struct vpfe_ccdc {
 	u32 ccdc_ctx[VPFE_REG_END / sizeof(u32)];
 };
 
+/*
+ * struct bus_format - VPFE bus format information
+ * width: Bits per pixel (when transferred over a bus)
+ * bpp: Bytes per pixel (when stored in memory)
+ */
+struct bus_format {
+	unsigned int width;
+	unsigned int bpp;
+};
+
+/*
+ * struct vpfe_fmt - VPFE media bus format information
+ * fourcc: V4L2 pixel format code
+ * code: V4L2 media bus format code
+ * l: 10 bit bus format info
+ * s: 8 bit bus format info
+ */
+struct vpfe_fmt {
+	u32 fourcc;
+	u32 code;
+	struct bus_format l;
+	struct bus_format s;
+};
+
+/*
+ * This value needs to be at least as large as the number of entry in
+ * formats[].
+ * When formats[] is modified make sure to adjust this value also.
+ * Expect compile time warnings if VPFE_MAX_ACTIVE_FMT is smaller then
+ * the number of elements in formats[].
+ */
+#define VPFE_MAX_ACTIVE_FMT	10
+
 struct vpfe_device {
 	/* V4l2 specific parameters */
 	/* Identifies video device for this channel */
@@ -252,6 +285,9 @@ struct vpfe_device {
 	struct v4l2_format fmt;
 	/* Used to store current bytes per pixel based on current format */
 	unsigned int bpp;
+	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
+	unsigned int num_active_fmt;
+
 	/*
 	 * used when IMP is chained to store the crop window which
 	 * is different from the image window
-- 
2.17.1

Re: [Patch v2 08/13] media: am437x-vpfe: Use a per instance format array instead of a static one

[Hans Verkuil]

On 9/19/19 10:41 PM, Benoit Parrot wrote:
> Using a statically defined format array would cause issue when
> multiple vpfe instance would be connected to sub-device of
> different capabilities. We need to use an instance based array
> instead to properly maintain a per port/instance format list.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c | 110 ++++++++------------
>  drivers/media/platform/am437x/am437x-vpfe.h |  36 +++++++
>  2 files changed, 77 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 8b218f48428f..569618a52082 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -69,31 +69,7 @@ static const struct vpfe_standard vpfe_standards[] = {
>  	{V4L2_STD_625_50, 720, 576, {54, 59}, 1},
>  };
>  
> -struct bus_format {
> -	unsigned int width;
> -	unsigned int bpp;
> -};
> -
> -/*
> - * struct vpfe_fmt - VPFE media bus format information
> - * @code: V4L2 media bus format code
> - * @shifted: V4L2 media bus format code for the same pixel layout but
> - *	shifted to be 8 bits per pixel. =0 if format is not shiftable.
> - * @pixelformat: V4L2 pixel format FCC identifier
> - * @width: Bits per pixel (when transferred over a bus)
> - * @bpp: Bytes per pixel (when stored in memory)
> - * @supported: Indicates format supported by subdev
> - */
> -struct vpfe_fmt {
> -	u32 fourcc;
> -	u32 code;
> -	struct bus_format l;
> -	struct bus_format s;
> -	bool supported;
> -	u32 index;
> -};
> -
> -static struct vpfe_fmt formats[] = {
> +static struct vpfe_fmt formats[VPFE_MAX_ACTIVE_FMT] = {
>  	{
>  		.fourcc		= V4L2_PIX_FMT_YUYV,
>  		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
> @@ -101,7 +77,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 4,
>  		.s.width	= 8,
>  		.s.bpp		= 2,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_UYVY,
>  		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
> @@ -109,7 +84,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 4,
>  		.s.width	= 8,
>  		.s.bpp		= 2,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_YVYU,
>  		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
> @@ -117,7 +91,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 4,
>  		.s.width	= 8,
>  		.s.bpp		= 2,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_VYUY,
>  		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
> @@ -125,7 +98,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 4,
>  		.s.width	= 8,
>  		.s.bpp		= 2,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_SBGGR8,
>  		.code		= MEDIA_BUS_FMT_SBGGR8_1X8,
> @@ -133,7 +105,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 2,
>  		.s.width	= 8,
>  		.s.bpp		= 1,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_SGBRG8,
>  		.code		= MEDIA_BUS_FMT_SGBRG8_1X8,
> @@ -141,7 +112,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 2,
>  		.s.width	= 8,
>  		.s.bpp		= 1,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_SGRBG8,
>  		.code		= MEDIA_BUS_FMT_SGRBG8_1X8,
> @@ -149,7 +119,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 2,
>  		.s.width	= 8,
>  		.s.bpp		= 1,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_SRGGB8,
>  		.code		= MEDIA_BUS_FMT_SRGGB8_1X8,
> @@ -157,7 +126,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 2,
>  		.s.width	= 8,
>  		.s.bpp		= 1,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_RGB565,
>  		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,
> @@ -165,7 +133,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 4,
>  		.s.width	= 8,
>  		.s.bpp		= 2,
> -		.supported	= false,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_RGB565X,
>  		.code		= MEDIA_BUS_FMT_RGB565_2X8_BE,
> @@ -173,7 +140,6 @@ static struct vpfe_fmt formats[] = {
>  		.l.bpp		= 4,
>  		.s.width	= 8,
>  		.s.bpp		= 2,
> -		.supported	= false,
>  	},
>  };
>  
> @@ -181,13 +147,14 @@ static int
>  __vpfe_get_format(struct vpfe_device *vpfe,
>  		  struct v4l2_format *format, unsigned int *bpp);
>  
> -static struct vpfe_fmt *find_format_by_code(unsigned int code)
> +static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> +					    unsigned int code)
>  {
>  	struct vpfe_fmt *fmt;
>  	unsigned int k;
>  
> -	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> -		fmt = &formats[k];
> +	for (k = 0; k < vpfe->num_active_fmt; k++) {
> +		fmt = vpfe->active_fmt[k];
>  		if (fmt->code == code)
>  			return fmt;
>  	}
> @@ -195,13 +162,14 @@ static struct vpfe_fmt *find_format_by_code(unsigned int code)
>  	return NULL;
>  }
>  
> -static struct vpfe_fmt *find_format_by_pix(unsigned int pixelformat)
> +static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> +					   unsigned int pixelformat)
>  {
>  	struct vpfe_fmt *fmt;
>  	unsigned int k;
>  
> -	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> -		fmt = &formats[k];
> +	for (k = 0; k < vpfe->num_active_fmt; k++) {
> +		fmt = vpfe->active_fmt[k];
>  		if (fmt->fourcc == pixelformat)
>  			return fmt;
>  	}
> @@ -218,7 +186,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
>  	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
>  	struct vpfe_fmt *fmt;
>  
> -	fmt = find_format_by_code(mbus->code);
> +	fmt = find_format_by_code(vpfe, mbus->code);
>  	if (WARN_ON(fmt == NULL)) {
>  		pr_err("Invalid mbus code set\n");
>  		*bpp = 1;
> @@ -241,12 +209,12 @@ static void pix_to_mbus(struct vpfe_device *vpfe,
>  {
>  	struct vpfe_fmt *fmt;
>  
> -	fmt = find_format_by_pix(pix_fmt->pixelformat);
> +	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
>  	if (!fmt) {
>  		/* default to first entry */
>  		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
>  			pix_fmt->pixelformat);
> -		fmt = &formats[0];
> +		fmt = vpfe->active_fmt[0];
>  	}
>  
>  	memset(mbus_fmt, 0, sizeof(*mbus_fmt));
> @@ -1494,8 +1462,7 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
>  {
>  	struct vpfe_device *vpfe = video_drvdata(file);
>  	struct vpfe_subdev_info *sdinfo;
> -	struct vpfe_fmt *fmt = NULL;
> -	unsigned int k;
> +	struct vpfe_fmt *fmt;
>  
>  	vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n",
>  		f->index);
> @@ -1504,17 +1471,10 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
>  	if (!sdinfo->sd)
>  		return -EINVAL;
>  
> -	if (f->index > ARRAY_SIZE(formats))
> +	if (f->index >= vpfe->num_active_fmt)
>  		return -EINVAL;
>  
> -	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> -		if (formats[k].index == f->index) {
> -			fmt = &formats[k];
> -			break;
> -		}
> -	}
> -	if (!fmt)
> -		return -EINVAL;
> +	fmt = vpfe->active_fmt[f->index];
>  
>  	f->pixelformat = fmt->fourcc;
>  
> @@ -1593,7 +1553,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
>  	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
>  
>  	/* check for valid format */
> -	fmt = find_format_by_pix(fsize->pixel_format);
> +	fmt = find_format_by_pix(vpfe, fsize->pixel_format);
>  	if (!fmt) {
>  		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
>  			fsize->pixel_format);
> @@ -2281,8 +2241,10 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
>  					       struct vpfe_device, v4l2_dev);
>  	struct v4l2_subdev_mbus_code_enum mbus_code;
>  	struct vpfe_subdev_info *sdinfo;
> +	struct vpfe_fmt *fmt;
> +	int ret = 0;
>  	bool found = false;
> -	int i, j;
> +	int i, j, k;
>  
>  	vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
>  
> @@ -2304,27 +2266,37 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
>  
>  	vpfe->video_dev.tvnorms |= sdinfo->inputs[0].std;
>  
> -	/* setup the supported formats & indexes */
> -	for (j = 0, i = 0; ; ++j) {
> -		struct vpfe_fmt *fmt;
> -		int ret;
> -
> +	vpfe->num_active_fmt = 0;
> +	for (j = 0, i = 0; (ret != -EINVAL); ++j) {
>  		memset(&mbus_code, 0, sizeof(mbus_code));
>  		mbus_code.index = j;
>  		mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  		ret = v4l2_subdev_call(subdev, pad, enum_mbus_code,
> -			       NULL, &mbus_code);
> +				       NULL, &mbus_code);
>  		if (ret)
> -			break;
> -
> -		fmt = find_format_by_code(mbus_code.code);
> -		if (!fmt)
>  			continue;
>  
> -		fmt->supported = true;
> -		fmt->index = i++;
> +		vpfe_dbg(3, vpfe,
> +			 "subdev %s: code: %04x idx: %d\n",
> +			 subdev->name, mbus_code.code, j);
> +
> +		for (k = 0; k < ARRAY_SIZE(formats); k++) {
> +			fmt = &formats[k];
> +			if (mbus_code.code != fmt->code)
> +				continue;
> +			vpfe->active_fmt[i] = fmt;
> +			vpfe_dbg(3, vpfe,
> +				 "matched fourcc: %4.4s code: %04x idx: %d\n",
> +				 (char *)&fmt->fourcc, mbus_code.code, i);
> +			vpfe->num_active_fmt = ++i;
> +		}
>  	}
>  
> +	if (!i) {
> +		vpfe_err(vpfe, "No suitable format reported by subdev %s\n",
> +			 subdev->name);
> +		return -EINVAL;
> +	}
>  	return 0;
>  }
>  
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
> index 2dde09780215..79a6505c083d 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.h
> +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> @@ -215,6 +215,39 @@ struct vpfe_ccdc {
>  	u32 ccdc_ctx[VPFE_REG_END / sizeof(u32)];
>  };
>  
> +/*
> + * struct bus_format - VPFE bus format information
> + * width: Bits per pixel (when transferred over a bus)
> + * bpp: Bytes per pixel (when stored in memory)
> + */
> +struct bus_format {
> +	unsigned int width;
> +	unsigned int bpp;
> +};
> +
> +/*
> + * struct vpfe_fmt - VPFE media bus format information
> + * fourcc: V4L2 pixel format code
> + * code: V4L2 media bus format code
> + * l: 10 bit bus format info
> + * s: 8 bit bus format info
> + */
> +struct vpfe_fmt {
> +	u32 fourcc;
> +	u32 code;
> +	struct bus_format l;
> +	struct bus_format s;
> +};
> +
> +/*
> + * This value needs to be at least as large as the number of entry in
> + * formats[].
> + * When formats[] is modified make sure to adjust this value also.
> + * Expect compile time warnings if VPFE_MAX_ACTIVE_FMT is smaller then
> + * the number of elements in formats[].
> + */
> +#define VPFE_MAX_ACTIVE_FMT	10

The comment is outdated since this define is used as the size of the formats
array. So you can't really get into problems.

For the same reason the name is now wrong as well, I'd call it VPFE_NUM_FORMATS.

That's what it actually is.

Regards,

	Hans

> +
>  struct vpfe_device {
>  	/* V4l2 specific parameters */
>  	/* Identifies video device for this channel */
> @@ -252,6 +285,9 @@ struct vpfe_device {
>  	struct v4l2_format fmt;
>  	/* Used to store current bytes per pixel based on current format */
>  	unsigned int bpp;
> +	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
> +	unsigned int num_active_fmt;
> +
>  	/*
>  	 * used when IMP is chained to store the crop window which
>  	 * is different from the image window
> 

Re: [Patch v2 08/13] media: am437x-vpfe: Use a per instance format array instead of a static one

[Benoit Parrot]

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-20 10:19:21 +0200]:
> On 9/19/19 10:41 PM, Benoit Parrot wrote:
> > Using a statically defined format array would cause issue when
> > multiple vpfe instance would be connected to sub-device of
> > different capabilities. We need to use an instance based array
> > instead to properly maintain a per port/instance format list.
> > 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/platform/am437x/am437x-vpfe.c | 110 ++++++++------------
> >  drivers/media/platform/am437x/am437x-vpfe.h |  36 +++++++
> >  2 files changed, 77 insertions(+), 69 deletions(-)
> > 
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index 8b218f48428f..569618a52082 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -69,31 +69,7 @@ static const struct vpfe_standard vpfe_standards[] = {
> >  	{V4L2_STD_625_50, 720, 576, {54, 59}, 1},
> >  };
> >  
> > -struct bus_format {
> > -	unsigned int width;
> > -	unsigned int bpp;
> > -};
> > -
> > -/*
> > - * struct vpfe_fmt - VPFE media bus format information
> > - * @code: V4L2 media bus format code
> > - * @shifted: V4L2 media bus format code for the same pixel layout but
> > - *	shifted to be 8 bits per pixel. =0 if format is not shiftable.
> > - * @pixelformat: V4L2 pixel format FCC identifier
> > - * @width: Bits per pixel (when transferred over a bus)
> > - * @bpp: Bytes per pixel (when stored in memory)
> > - * @supported: Indicates format supported by subdev
> > - */
> > -struct vpfe_fmt {
> > -	u32 fourcc;
> > -	u32 code;
> > -	struct bus_format l;
> > -	struct bus_format s;
> > -	bool supported;
> > -	u32 index;
> > -};
> > -
> > -static struct vpfe_fmt formats[] = {
> > +static struct vpfe_fmt formats[VPFE_MAX_ACTIVE_FMT] = {
> >  	{
> >  		.fourcc		= V4L2_PIX_FMT_YUYV,
> >  		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
> > @@ -101,7 +77,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 4,
> >  		.s.width	= 8,
> >  		.s.bpp		= 2,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_UYVY,
> >  		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
> > @@ -109,7 +84,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 4,
> >  		.s.width	= 8,
> >  		.s.bpp		= 2,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_YVYU,
> >  		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
> > @@ -117,7 +91,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 4,
> >  		.s.width	= 8,
> >  		.s.bpp		= 2,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_VYUY,
> >  		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
> > @@ -125,7 +98,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 4,
> >  		.s.width	= 8,
> >  		.s.bpp		= 2,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_SBGGR8,
> >  		.code		= MEDIA_BUS_FMT_SBGGR8_1X8,
> > @@ -133,7 +105,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 2,
> >  		.s.width	= 8,
> >  		.s.bpp		= 1,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_SGBRG8,
> >  		.code		= MEDIA_BUS_FMT_SGBRG8_1X8,
> > @@ -141,7 +112,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 2,
> >  		.s.width	= 8,
> >  		.s.bpp		= 1,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_SGRBG8,
> >  		.code		= MEDIA_BUS_FMT_SGRBG8_1X8,
> > @@ -149,7 +119,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 2,
> >  		.s.width	= 8,
> >  		.s.bpp		= 1,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_SRGGB8,
> >  		.code		= MEDIA_BUS_FMT_SRGGB8_1X8,
> > @@ -157,7 +126,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 2,
> >  		.s.width	= 8,
> >  		.s.bpp		= 1,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_RGB565,
> >  		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,
> > @@ -165,7 +133,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 4,
> >  		.s.width	= 8,
> >  		.s.bpp		= 2,
> > -		.supported	= false,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_RGB565X,
> >  		.code		= MEDIA_BUS_FMT_RGB565_2X8_BE,
> > @@ -173,7 +140,6 @@ static struct vpfe_fmt formats[] = {
> >  		.l.bpp		= 4,
> >  		.s.width	= 8,
> >  		.s.bpp		= 2,
> > -		.supported	= false,
> >  	},
> >  };
> >  
> > @@ -181,13 +147,14 @@ static int
> >  __vpfe_get_format(struct vpfe_device *vpfe,
> >  		  struct v4l2_format *format, unsigned int *bpp);
> >  
> > -static struct vpfe_fmt *find_format_by_code(unsigned int code)
> > +static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> > +					    unsigned int code)
> >  {
> >  	struct vpfe_fmt *fmt;
> >  	unsigned int k;
> >  
> > -	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> > -		fmt = &formats[k];
> > +	for (k = 0; k < vpfe->num_active_fmt; k++) {
> > +		fmt = vpfe->active_fmt[k];
> >  		if (fmt->code == code)
> >  			return fmt;
> >  	}
> > @@ -195,13 +162,14 @@ static struct vpfe_fmt *find_format_by_code(unsigned int code)
> >  	return NULL;
> >  }
> >  
> > -static struct vpfe_fmt *find_format_by_pix(unsigned int pixelformat)
> > +static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> > +					   unsigned int pixelformat)
> >  {
> >  	struct vpfe_fmt *fmt;
> >  	unsigned int k;
> >  
> > -	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> > -		fmt = &formats[k];
> > +	for (k = 0; k < vpfe->num_active_fmt; k++) {
> > +		fmt = vpfe->active_fmt[k];
> >  		if (fmt->fourcc == pixelformat)
> >  			return fmt;
> >  	}
> > @@ -218,7 +186,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
> >  	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
> >  	struct vpfe_fmt *fmt;
> >  
> > -	fmt = find_format_by_code(mbus->code);
> > +	fmt = find_format_by_code(vpfe, mbus->code);
> >  	if (WARN_ON(fmt == NULL)) {
> >  		pr_err("Invalid mbus code set\n");
> >  		*bpp = 1;
> > @@ -241,12 +209,12 @@ static void pix_to_mbus(struct vpfe_device *vpfe,
> >  {
> >  	struct vpfe_fmt *fmt;
> >  
> > -	fmt = find_format_by_pix(pix_fmt->pixelformat);
> > +	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
> >  	if (!fmt) {
> >  		/* default to first entry */
> >  		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
> >  			pix_fmt->pixelformat);
> > -		fmt = &formats[0];
> > +		fmt = vpfe->active_fmt[0];
> >  	}
> >  
> >  	memset(mbus_fmt, 0, sizeof(*mbus_fmt));
> > @@ -1494,8 +1462,7 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
> >  {
> >  	struct vpfe_device *vpfe = video_drvdata(file);
> >  	struct vpfe_subdev_info *sdinfo;
> > -	struct vpfe_fmt *fmt = NULL;
> > -	unsigned int k;
> > +	struct vpfe_fmt *fmt;
> >  
> >  	vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n",
> >  		f->index);
> > @@ -1504,17 +1471,10 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
> >  	if (!sdinfo->sd)
> >  		return -EINVAL;
> >  
> > -	if (f->index > ARRAY_SIZE(formats))
> > +	if (f->index >= vpfe->num_active_fmt)
> >  		return -EINVAL;
> >  
> > -	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> > -		if (formats[k].index == f->index) {
> > -			fmt = &formats[k];
> > -			break;
> > -		}
> > -	}
> > -	if (!fmt)
> > -		return -EINVAL;
> > +	fmt = vpfe->active_fmt[f->index];
> >  
> >  	f->pixelformat = fmt->fourcc;
> >  
> > @@ -1593,7 +1553,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
> >  	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
> >  
> >  	/* check for valid format */
> > -	fmt = find_format_by_pix(fsize->pixel_format);
> > +	fmt = find_format_by_pix(vpfe, fsize->pixel_format);
> >  	if (!fmt) {
> >  		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
> >  			fsize->pixel_format);
> > @@ -2281,8 +2241,10 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
> >  					       struct vpfe_device, v4l2_dev);
> >  	struct v4l2_subdev_mbus_code_enum mbus_code;
> >  	struct vpfe_subdev_info *sdinfo;
> > +	struct vpfe_fmt *fmt;
> > +	int ret = 0;
> >  	bool found = false;
> > -	int i, j;
> > +	int i, j, k;
> >  
> >  	vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
> >  
> > @@ -2304,27 +2266,37 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
> >  
> >  	vpfe->video_dev.tvnorms |= sdinfo->inputs[0].std;
> >  
> > -	/* setup the supported formats & indexes */
> > -	for (j = 0, i = 0; ; ++j) {
> > -		struct vpfe_fmt *fmt;
> > -		int ret;
> > -
> > +	vpfe->num_active_fmt = 0;
> > +	for (j = 0, i = 0; (ret != -EINVAL); ++j) {
> >  		memset(&mbus_code, 0, sizeof(mbus_code));
> >  		mbus_code.index = j;
> >  		mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >  		ret = v4l2_subdev_call(subdev, pad, enum_mbus_code,
> > -			       NULL, &mbus_code);
> > +				       NULL, &mbus_code);
> >  		if (ret)
> > -			break;
> > -
> > -		fmt = find_format_by_code(mbus_code.code);
> > -		if (!fmt)
> >  			continue;
> >  
> > -		fmt->supported = true;
> > -		fmt->index = i++;
> > +		vpfe_dbg(3, vpfe,
> > +			 "subdev %s: code: %04x idx: %d\n",
> > +			 subdev->name, mbus_code.code, j);
> > +
> > +		for (k = 0; k < ARRAY_SIZE(formats); k++) {
> > +			fmt = &formats[k];
> > +			if (mbus_code.code != fmt->code)
> > +				continue;
> > +			vpfe->active_fmt[i] = fmt;
> > +			vpfe_dbg(3, vpfe,
> > +				 "matched fourcc: %4.4s code: %04x idx: %d\n",
> > +				 (char *)&fmt->fourcc, mbus_code.code, i);
> > +			vpfe->num_active_fmt = ++i;
> > +		}
> >  	}
> >  
> > +	if (!i) {
> > +		vpfe_err(vpfe, "No suitable format reported by subdev %s\n",
> > +			 subdev->name);
> > +		return -EINVAL;
> > +	}
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
> > index 2dde09780215..79a6505c083d 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.h
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> > @@ -215,6 +215,39 @@ struct vpfe_ccdc {
> >  	u32 ccdc_ctx[VPFE_REG_END / sizeof(u32)];
> >  };
> >  
> > +/*
> > + * struct bus_format - VPFE bus format information
> > + * width: Bits per pixel (when transferred over a bus)
> > + * bpp: Bytes per pixel (when stored in memory)
> > + */
> > +struct bus_format {
> > +	unsigned int width;
> > +	unsigned int bpp;
> > +};
> > +
> > +/*
> > + * struct vpfe_fmt - VPFE media bus format information
> > + * fourcc: V4L2 pixel format code
> > + * code: V4L2 media bus format code
> > + * l: 10 bit bus format info
> > + * s: 8 bit bus format info
> > + */
> > +struct vpfe_fmt {
> > +	u32 fourcc;
> > +	u32 code;
> > +	struct bus_format l;
> > +	struct bus_format s;
> > +};
> > +
> > +/*
> > + * This value needs to be at least as large as the number of entry in
> > + * formats[].
> > + * When formats[] is modified make sure to adjust this value also.
> > + * Expect compile time warnings if VPFE_MAX_ACTIVE_FMT is smaller then
> > + * the number of elements in formats[].
> > + */
> > +#define VPFE_MAX_ACTIVE_FMT	10
> 
> The comment is outdated since this define is used as the size of the formats
> array. So you can't really get into problems.
> 
> For the same reason the name is now wrong as well, I'd call it VPFE_NUM_FORMATS.
> 
> That's what it actually is.

Hmm, I guess you are right, I'll change it.

Benoit

> 
> Regards,
> 
> 	Hans
> 
> > +
> >  struct vpfe_device {
> >  	/* V4l2 specific parameters */
> >  	/* Identifies video device for this channel */
> > @@ -252,6 +285,9 @@ struct vpfe_device {
> >  	struct v4l2_format fmt;
> >  	/* Used to store current bytes per pixel based on current format */
> >  	unsigned int bpp;
> > +	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
> > +	unsigned int num_active_fmt;
> > +
> >  	/*
> >  	 * used when IMP is chained to store the crop window which
> >  	 * is different from the image window
> > 
> 

[Patch v2 09/13] media: am437x-vpfe: fix function trace debug log

[Benoit Parrot]

checkpatch.pl nows reports several:
WARNING: Prefer using '"%s...", __func__' to using '<function name>',
this function's name, in a string.

So fix these for the whole driver.
At the same time remove the function entry trace log as those can be
enabled using ftrace instead.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 72 ++++-----------------
 1 file changed, 13 insertions(+), 59 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 569618a52082..605a6bb7e6c2 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -443,8 +443,8 @@ static int vpfe_ccdc_set_params(struct vpfe_ccdc *ccdc, void __user *params)
 	x = copy_from_user(&raw_params, params, sizeof(raw_params));
 	if (x) {
 		vpfe_dbg(1, vpfe,
-			"vpfe_ccdc_set_params: error in copying ccdc params, %d\n",
-			x);
+			 "%s: error in copying ccdc params, %d\n",
+			 __func__, x);
 		return -EFAULT;
 	}
 
@@ -462,11 +462,9 @@ static int vpfe_ccdc_set_params(struct vpfe_ccdc *ccdc, void __user *params)
  */
 static void vpfe_ccdc_config_ycbcr(struct vpfe_ccdc *ccdc)
 {
-	struct vpfe_device *vpfe = container_of(ccdc, struct vpfe_device, ccdc);
 	struct ccdc_params_ycbcr *params = &ccdc->ccdc_cfg.ycbcr;
 	u32 syn_mode;
 
-	vpfe_dbg(3, vpfe, "vpfe_ccdc_config_ycbcr:\n");
 	/*
 	 * first restore the CCDC registers to default values
 	 * This is important since we assume default values to be set in
@@ -598,8 +596,6 @@ static void vpfe_ccdc_config_raw(struct vpfe_ccdc *ccdc)
 	unsigned int syn_mode;
 	unsigned int val;
 
-	vpfe_dbg(3, vpfe, "vpfe_ccdc_config_raw:\n");
-
 	/* Reset CCDC */
 	vpfe_ccdc_restore_defaults(ccdc);
 
@@ -700,8 +696,8 @@ static int vpfe_ccdc_set_pixel_format(struct vpfe_ccdc *ccdc, u32 pixfmt)
 {
 	struct vpfe_device *vpfe = container_of(ccdc, struct vpfe_device, ccdc);
 
-	vpfe_dbg(1, vpfe, "vpfe_ccdc_set_pixel_format: if_type: %d, pixfmt:%s\n",
-		 ccdc->ccdc_cfg.if_type, print_fourcc(pixfmt));
+	vpfe_dbg(1, vpfe, "%s: if_type: %d, pixfmt:%s\n",
+		 __func__, ccdc->ccdc_cfg.if_type, print_fourcc(pixfmt));
 
 	if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) {
 		ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW;
@@ -987,8 +983,6 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 	enum ccdc_frmfmt frm_fmt = CCDC_FRMFMT_INTERLACED;
 	int ret = 0;
 
-	vpfe_dbg(2, vpfe, "vpfe_config_ccdc_image_format\n");
-
 	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
 		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
 
@@ -1354,8 +1348,6 @@ static int vpfe_querycap(struct file *file, void  *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 
-	vpfe_dbg(2, vpfe, "vpfe_querycap\n");
-
 	strscpy(cap->driver, VPFE_MODULE_NAME, sizeof(cap->driver));
 	strscpy(cap->card, "TI AM437x VPFE", sizeof(cap->card));
 	snprintf(cap->bus_info, sizeof(cap->bus_info),
@@ -1400,7 +1392,7 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
 	format->type = vpfe->fmt.type;
 
 	vpfe_dbg(1, vpfe,
-		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
+		 "%s: size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
 		 __func__, format->fmt.pix.width, format->fmt.pix.height,
 		 print_fourcc(format->fmt.pix.pixelformat),
 		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
@@ -1416,8 +1408,6 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
 	struct v4l2_subdev_format fmt;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "__vpfe_set_format\n");
-
 	sdinfo = vpfe->current_subdev;
 	if (!sdinfo->sd)
 		return -EINVAL;
@@ -1450,8 +1440,6 @@ static int vpfe_g_fmt(struct file *file, void *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 
-	vpfe_dbg(2, vpfe, "vpfe_g_fmt\n");
-
 	*fmt = vpfe->fmt;
 
 	return 0;
@@ -1464,9 +1452,6 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
 	struct vpfe_subdev_info *sdinfo;
 	struct vpfe_fmt *fmt;
 
-	vpfe_dbg(2, vpfe, "vpfe_enum_format index:%d\n",
-		f->index);
-
 	sdinfo = vpfe->current_subdev;
 	if (!sdinfo->sd)
 		return -EINVAL;
@@ -1478,8 +1463,8 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
 
 	f->pixelformat = fmt->fourcc;
 
-	vpfe_dbg(1, vpfe, "vpfe_enum_format: mbus index: %d code: %x pixelformat: %s\n",
-		 f->index, fmt->code, print_fourcc(fmt->fourcc));
+	vpfe_dbg(1, vpfe, "%s: mbus index: %d code: %x pixelformat: %s\n",
+		 __func__, f->index, fmt->code, print_fourcc(fmt->fourcc));
 
 	return 0;
 }
@@ -1490,8 +1475,6 @@ static int vpfe_try_fmt(struct file *file, void *priv,
 	struct vpfe_device *vpfe = video_drvdata(file);
 	unsigned int bpp;
 
-	vpfe_dbg(2, vpfe, "vpfe_try_fmt\n");
-
 	return __vpfe_get_format(vpfe, fmt, &bpp);
 }
 
@@ -1503,8 +1486,6 @@ static int vpfe_s_fmt(struct file *file, void *priv,
 	unsigned int bpp;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_s_fmt\n");
-
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
 		vpfe_err(vpfe, "%s device busy\n", __func__);
@@ -1550,8 +1531,6 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	struct vpfe_fmt *fmt;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
-
 	/* check for valid format */
 	fmt = find_format_by_pix(vpfe, fsize->pixel_format);
 	if (!fmt) {
@@ -1584,17 +1563,17 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	if (ret)
 		return -EINVAL;
 
-	vpfe_dbg(1, vpfe, "vpfe_enum_size: index: %d code: %x W:[%d,%d] H:[%d,%d]\n",
-		fse.index, fse.code, fse.min_width, fse.max_width,
-		fse.min_height, fse.max_height);
+	vpfe_dbg(1, vpfe, "%s: index: %d code: %x W:[%d,%d] H:[%d,%d]\n",
+		 __func__, fse.index, fse.code, fse.min_width, fse.max_width,
+		 fse.min_height, fse.max_height);
 
 	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
 	fsize->discrete.width = fse.max_width;
 	fsize->discrete.height = fse.max_height;
 
-	vpfe_dbg(1, vpfe, "vpfe_enum_size: index: %d pixformat: %s size: %dx%d\n",
-		fsize->index, print_fourcc(fsize->pixel_format),
-		fsize->discrete.width, fsize->discrete.height);
+	vpfe_dbg(1, vpfe, "%s: index: %d pixformat: %4.4s size: %dx%d\n",
+		 __func__, fsize->index, (char *)&fsize->pixel_format,
+		 fsize->discrete.width, fsize->discrete.height);
 
 	return 0;
 }
@@ -1659,8 +1638,6 @@ static int vpfe_enum_input(struct file *file, void *priv,
 	struct vpfe_subdev_info *sdinfo;
 	int subdev, index;
 
-	vpfe_dbg(2, vpfe, "vpfe_enum_input\n");
-
 	if (vpfe_get_subdev_input_index(vpfe, &subdev, &index,
 					inp->index) < 0) {
 		vpfe_dbg(1, vpfe,
@@ -1677,8 +1654,6 @@ static int vpfe_g_input(struct file *file, void *priv, unsigned int *index)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 
-	vpfe_dbg(2, vpfe, "vpfe_g_input\n");
-
 	return vpfe_get_app_input_index(vpfe, index);
 }
 
@@ -1691,8 +1666,6 @@ static int vpfe_set_input(struct vpfe_device *vpfe, unsigned int index)
 	u32 input, output;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_set_input: index: %d\n", index);
-
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
 		vpfe_err(vpfe, "%s device busy\n", __func__);
@@ -1748,9 +1721,6 @@ static int vpfe_s_input(struct file *file, void *priv, unsigned int index)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 
-	vpfe_dbg(2, vpfe,
-		"vpfe_s_input: index: %d\n", index);
-
 	return vpfe_set_input(vpfe, index);
 }
 
@@ -1759,8 +1729,6 @@ static int vpfe_querystd(struct file *file, void *priv, v4l2_std_id *std_id)
 	struct vpfe_device *vpfe = video_drvdata(file);
 	struct vpfe_subdev_info *sdinfo;
 
-	vpfe_dbg(2, vpfe, "vpfe_querystd\n");
-
 	sdinfo = vpfe->current_subdev;
 	if (!(sdinfo->inputs[0].capabilities & V4L2_IN_CAP_STD))
 		return -ENODATA;
@@ -1776,8 +1744,6 @@ static int vpfe_s_std(struct file *file, void *priv, v4l2_std_id std_id)
 	struct vpfe_subdev_info *sdinfo;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_s_std\n");
-
 	sdinfo = vpfe->current_subdev;
 	if (!(sdinfo->inputs[0].capabilities & V4L2_IN_CAP_STD))
 		return -ENODATA;
@@ -1809,8 +1775,6 @@ static int vpfe_g_std(struct file *file, void *priv, v4l2_std_id *std_id)
 	struct vpfe_device *vpfe = video_drvdata(file);
 	struct vpfe_subdev_info *sdinfo;
 
-	vpfe_dbg(2, vpfe, "vpfe_g_std\n");
-
 	sdinfo = vpfe->current_subdev;
 	if (sdinfo->inputs[0].capabilities != V4L2_IN_CAP_STD)
 		return -ENODATA;
@@ -1828,8 +1792,6 @@ static void vpfe_calculate_offsets(struct vpfe_device *vpfe)
 {
 	struct v4l2_rect image_win;
 
-	vpfe_dbg(2, vpfe, "vpfe_calculate_offsets\n");
-
 	vpfe_ccdc_get_image_window(&vpfe->ccdc, &image_win);
 	vpfe->field_off = image_win.height * image_win.width;
 }
@@ -2030,8 +1992,6 @@ static int vpfe_g_pixelaspect(struct file *file, void *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 
-	vpfe_dbg(2, vpfe, "vpfe_g_pixelaspect\n");
-
 	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
 	    vpfe->std_index >= ARRAY_SIZE(vpfe_standards))
 		return -EINVAL;
@@ -2134,8 +2094,6 @@ static long vpfe_ioctl_default(struct file *file, void *priv,
 	struct vpfe_device *vpfe = video_drvdata(file);
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_ioctl_default\n");
-
 	if (!valid_prio) {
 		vpfe_err(vpfe, "%s device busy\n", __func__);
 		return -EBUSY;
@@ -2246,8 +2204,6 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
 	bool found = false;
 	int i, j, k;
 
-	vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
-
 	for (i = 0; i < ARRAY_SIZE(vpfe->cfg->asd); i++) {
 		if (vpfe->cfg->asd[i]->match.fwnode ==
 		    asd[i].match.fwnode) {
@@ -2579,8 +2535,6 @@ static int vpfe_remove(struct platform_device *pdev)
 {
 	struct vpfe_device *vpfe = platform_get_drvdata(pdev);
 
-	vpfe_dbg(2, vpfe, "vpfe_remove\n");
-
 	pm_runtime_disable(&pdev->dev);
 
 	v4l2_async_notifier_unregister(&vpfe->notifier);
-- 
2.17.1

[Patch v2 10/13] media: am437x-vpfe: Remove print_fourcc helper

[Benoit Parrot]

print_fourcc helper function was used for debug log to
convert a pixel format code into its readable form for display
purposes. But since it used a single static buffer to perform
the conversion this might lead to display format issue when more
than one instance was invoked simultaneously.

It turns out that print_fourcc can be safely replace by using
"%4pE" instead and passing the pointer to the fourcc code.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 42 +++++++--------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 605a6bb7e6c2..ad46e037fe89 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -221,20 +221,6 @@ static void pix_to_mbus(struct vpfe_device *vpfe,
 	v4l2_fill_mbus_format(mbus_fmt, pix_fmt, fmt->code);
 }
 
-/*  Print Four-character-code (FOURCC) */
-static char *print_fourcc(u32 fmt)
-{
-	static char code[5];
-
-	code[0] = (unsigned char)(fmt & 0xff);
-	code[1] = (unsigned char)((fmt >> 8) & 0xff);
-	code[2] = (unsigned char)((fmt >> 16) & 0xff);
-	code[3] = (unsigned char)((fmt >> 24) & 0xff);
-	code[4] = '\0';
-
-	return code;
-}
-
 static int
 cmp_v4l2_format(const struct v4l2_format *lhs, const struct v4l2_format *rhs)
 {
@@ -696,8 +682,8 @@ static int vpfe_ccdc_set_pixel_format(struct vpfe_ccdc *ccdc, u32 pixfmt)
 {
 	struct vpfe_device *vpfe = container_of(ccdc, struct vpfe_device, ccdc);
 
-	vpfe_dbg(1, vpfe, "%s: if_type: %d, pixfmt:%s\n",
-		 __func__, ccdc->ccdc_cfg.if_type, print_fourcc(pixfmt));
+	vpfe_dbg(1, vpfe, "%s: if_type: %d, pixfmt:%4pE\n",
+		 __func__, ccdc->ccdc_cfg.if_type, &pixfmt);
 
 	if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) {
 		ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW;
@@ -983,8 +969,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 	enum ccdc_frmfmt frm_fmt = CCDC_FRMFMT_INTERLACED;
 	int ret = 0;
 
-	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
-		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
+	vpfe_dbg(1, vpfe, "pixelformat: %4pE\n",
+		 &vpfe->fmt.fmt.pix.pixelformat);
 
 	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
 			vpfe->fmt.fmt.pix.pixelformat) < 0) {
@@ -1392,9 +1378,9 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
 	format->type = vpfe->fmt.type;
 
 	vpfe_dbg(1, vpfe,
-		 "%s: size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
+		 "%s: size %dx%d (%4pE) bytesperline = %d, size = %d, bpp = %d\n",
 		 __func__, format->fmt.pix.width, format->fmt.pix.height,
-		 print_fourcc(format->fmt.pix.pixelformat),
+		 &format->fmt.pix.pixelformat,
 		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
 
 	return 0;
@@ -1427,9 +1413,9 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
 	format->type = vpfe->fmt.type;
 
 	vpfe_dbg(1, vpfe,
-		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
+		 "%s: size %dx%d (%4pE) bytesperline = %d, size = %d, bpp = %d\n",
 		 __func__,  format->fmt.pix.width, format->fmt.pix.height,
-		 print_fourcc(format->fmt.pix.pixelformat),
+		 &format->fmt.pix.pixelformat,
 		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
 
 	return 0;
@@ -1463,8 +1449,8 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
 
 	f->pixelformat = fmt->fourcc;
 
-	vpfe_dbg(1, vpfe, "%s: mbus index: %d code: %x pixelformat: %s\n",
-		 __func__, f->index, fmt->code, print_fourcc(fmt->fourcc));
+	vpfe_dbg(1, vpfe, "%s: mbus index: %d code: %x pixelformat: %4pE\n",
+		 __func__, f->index, fmt->code, &fmt->fourcc);
 
 	return 0;
 }
@@ -1571,8 +1557,8 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	fsize->discrete.width = fse.max_width;
 	fsize->discrete.height = fse.max_height;
 
-	vpfe_dbg(1, vpfe, "%s: index: %d pixformat: %4.4s size: %dx%d\n",
-		 __func__, fsize->index, (char *)&fsize->pixel_format,
+	vpfe_dbg(1, vpfe, "%s: index: %d pixformat: %4pE size: %dx%d\n",
+		 __func__, fsize->index, &fsize->pixel_format,
 		 fsize->discrete.width, fsize->discrete.height);
 
 	return 0;
@@ -2242,8 +2228,8 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
 				continue;
 			vpfe->active_fmt[i] = fmt;
 			vpfe_dbg(3, vpfe,
-				 "matched fourcc: %4.4s code: %04x idx: %d\n",
-				 (char *)&fmt->fourcc, mbus_code.code, i);
+				 "matched fourcc: %4pE code: %04x idx: %d\n",
+				 &fmt->fourcc, mbus_code.code, i);
 			vpfe->num_active_fmt = ++i;
 		}
 	}
-- 
2.17.1

Re: [Patch v2 10/13] media: am437x-vpfe: Remove print_fourcc helper

[Hans Verkuil]

On 9/19/19 10:41 PM, Benoit Parrot wrote:
> print_fourcc helper function was used for debug log to
> convert a pixel format code into its readable form for display
> purposes. But since it used a single static buffer to perform
> the conversion this might lead to display format issue when more
> than one instance was invoked simultaneously.
> 
> It turns out that print_fourcc can be safely replace by using
> "%4pE" instead and passing the pointer to the fourcc code.

If this series is merged:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg149985.html

then this patch needs to be rewritten. So I'll skip this patch for now.

Regards,

	Hans

> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c | 42 +++++++--------------
>  1 file changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 605a6bb7e6c2..ad46e037fe89 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -221,20 +221,6 @@ static void pix_to_mbus(struct vpfe_device *vpfe,
>  	v4l2_fill_mbus_format(mbus_fmt, pix_fmt, fmt->code);
>  }
>  
> -/*  Print Four-character-code (FOURCC) */
> -static char *print_fourcc(u32 fmt)
> -{
> -	static char code[5];
> -
> -	code[0] = (unsigned char)(fmt & 0xff);
> -	code[1] = (unsigned char)((fmt >> 8) & 0xff);
> -	code[2] = (unsigned char)((fmt >> 16) & 0xff);
> -	code[3] = (unsigned char)((fmt >> 24) & 0xff);
> -	code[4] = '\0';
> -
> -	return code;
> -}
> -
>  static int
>  cmp_v4l2_format(const struct v4l2_format *lhs, const struct v4l2_format *rhs)
>  {
> @@ -696,8 +682,8 @@ static int vpfe_ccdc_set_pixel_format(struct vpfe_ccdc *ccdc, u32 pixfmt)
>  {
>  	struct vpfe_device *vpfe = container_of(ccdc, struct vpfe_device, ccdc);
>  
> -	vpfe_dbg(1, vpfe, "%s: if_type: %d, pixfmt:%s\n",
> -		 __func__, ccdc->ccdc_cfg.if_type, print_fourcc(pixfmt));
> +	vpfe_dbg(1, vpfe, "%s: if_type: %d, pixfmt:%4pE\n",
> +		 __func__, ccdc->ccdc_cfg.if_type, &pixfmt);
>  
>  	if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) {
>  		ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW;
> @@ -983,8 +969,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
>  	enum ccdc_frmfmt frm_fmt = CCDC_FRMFMT_INTERLACED;
>  	int ret = 0;
>  
> -	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
> -		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
> +	vpfe_dbg(1, vpfe, "pixelformat: %4pE\n",
> +		 &vpfe->fmt.fmt.pix.pixelformat);
>  
>  	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
>  			vpfe->fmt.fmt.pix.pixelformat) < 0) {
> @@ -1392,9 +1378,9 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
>  	format->type = vpfe->fmt.type;
>  
>  	vpfe_dbg(1, vpfe,
> -		 "%s: size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
> +		 "%s: size %dx%d (%4pE) bytesperline = %d, size = %d, bpp = %d\n",
>  		 __func__, format->fmt.pix.width, format->fmt.pix.height,
> -		 print_fourcc(format->fmt.pix.pixelformat),
> +		 &format->fmt.pix.pixelformat,
>  		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
>  
>  	return 0;
> @@ -1427,9 +1413,9 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
>  	format->type = vpfe->fmt.type;
>  
>  	vpfe_dbg(1, vpfe,
> -		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
> +		 "%s: size %dx%d (%4pE) bytesperline = %d, size = %d, bpp = %d\n",
>  		 __func__,  format->fmt.pix.width, format->fmt.pix.height,
> -		 print_fourcc(format->fmt.pix.pixelformat),
> +		 &format->fmt.pix.pixelformat,
>  		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
>  
>  	return 0;
> @@ -1463,8 +1449,8 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
>  
>  	f->pixelformat = fmt->fourcc;
>  
> -	vpfe_dbg(1, vpfe, "%s: mbus index: %d code: %x pixelformat: %s\n",
> -		 __func__, f->index, fmt->code, print_fourcc(fmt->fourcc));
> +	vpfe_dbg(1, vpfe, "%s: mbus index: %d code: %x pixelformat: %4pE\n",
> +		 __func__, f->index, fmt->code, &fmt->fourcc);
>  
>  	return 0;
>  }
> @@ -1571,8 +1557,8 @@ static int vpfe_enum_size(struct file *file, void  *priv,
>  	fsize->discrete.width = fse.max_width;
>  	fsize->discrete.height = fse.max_height;
>  
> -	vpfe_dbg(1, vpfe, "%s: index: %d pixformat: %4.4s size: %dx%d\n",
> -		 __func__, fsize->index, (char *)&fsize->pixel_format,
> +	vpfe_dbg(1, vpfe, "%s: index: %d pixformat: %4pE size: %dx%d\n",
> +		 __func__, fsize->index, &fsize->pixel_format,
>  		 fsize->discrete.width, fsize->discrete.height);
>  
>  	return 0;
> @@ -2242,8 +2228,8 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
>  				continue;
>  			vpfe->active_fmt[i] = fmt;
>  			vpfe_dbg(3, vpfe,
> -				 "matched fourcc: %4.4s code: %04x idx: %d\n",
> -				 (char *)&fmt->fourcc, mbus_code.code, i);
> +				 "matched fourcc: %4pE code: %04x idx: %d\n",
> +				 &fmt->fourcc, mbus_code.code, i);
>  			vpfe->num_active_fmt = ++i;
>  		}
>  	}
> 

Re: [Patch v2 10/13] media: am437x-vpfe: Remove print_fourcc helper

[Benoit Parrot]

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-20 10:21:17 +0200]:
> On 9/19/19 10:41 PM, Benoit Parrot wrote:
> > print_fourcc helper function was used for debug log to
> > convert a pixel format code into its readable form for display
> > purposes. But since it used a single static buffer to perform
> > the conversion this might lead to display format issue when more
> > than one instance was invoked simultaneously.
> > 
> > It turns out that print_fourcc can be safely replace by using
> > "%4pE" instead and passing the pointer to the fourcc code.
> 
> If this series is merged:
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg149985.html
> 
> then this patch needs to be rewritten. So I'll skip this patch for now.

Ok I'll monnitor the status of this patch and see if/when I'll rebase this
one.

Benoit

> 
> Regards,
> 
> 	Hans
> 

[Patch v2 11/13] media: am437x-vpfe: TRY_FMT ioctl is not really trying anything

[Benoit Parrot]

The try_fmt was not actually trying out the provided format
but merely returning the current format basically like get_fmt.
In addition set_fmt should first invoked try_fmt to validate the
given format before applying it to the hardware.

To fix all of these the whole get/try/set ioctl functions had to
be reworked.
When calculating the bytesperline/stride and sizeimage format
member we don't need to locally store the current value of
bytesperpixel as it can easily get derived dynamically.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 338 ++++++++++----------
 drivers/media/platform/am437x/am437x-vpfe.h |   4 +-
 2 files changed, 171 insertions(+), 171 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index ad46e037fe89..47404d799029 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -143,9 +143,11 @@ static struct vpfe_fmt formats[VPFE_MAX_ACTIVE_FMT] = {
 	},
 };
 
-static int
-__vpfe_get_format(struct vpfe_device *vpfe,
-		  struct v4l2_format *format, unsigned int *bpp);
+static int __subdev_get_format(struct vpfe_device *vpfe,
+			       struct v4l2_mbus_framefmt *fmt);
+static int vpfe_calc_format_size(struct vpfe_device *vpfe,
+				 const struct vpfe_fmt *fmt,
+				 struct v4l2_format *f);
 
 static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
 					    unsigned int code)
@@ -177,62 +179,16 @@ static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
 	return NULL;
 }
 
-static void
-mbus_to_pix(struct vpfe_device *vpfe,
-	    const struct v4l2_mbus_framefmt *mbus,
-	    struct v4l2_pix_format *pix, unsigned int *bpp)
+static unsigned int __get_bytesperpixel(struct vpfe_device *vpfe,
+					const struct vpfe_fmt *fmt)
 {
 	struct vpfe_subdev_info *sdinfo = vpfe->current_subdev;
 	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
-	struct vpfe_fmt *fmt;
-
-	fmt = find_format_by_code(vpfe, mbus->code);
-	if (WARN_ON(fmt == NULL)) {
-		pr_err("Invalid mbus code set\n");
-		*bpp = 1;
-		return;
-	}
+	u32 bpp;
 
-	memset(pix, 0, sizeof(*pix));
-	v4l2_fill_pix_format(pix, mbus);
-	pix->pixelformat = fmt->fourcc;
-	*bpp = (bus_width == 10) ?  fmt->l.bpp : fmt->s.bpp;
+	bpp = (bus_width == 10) ? fmt->l.bpp : fmt->s.bpp;
 
-	/* pitch should be 32 bytes aligned */
-	pix->bytesperline = ALIGN(pix->width * *bpp, 32);
-	pix->sizeimage = pix->bytesperline * pix->height;
-}
-
-static void pix_to_mbus(struct vpfe_device *vpfe,
-			struct v4l2_pix_format *pix_fmt,
-			struct v4l2_mbus_framefmt *mbus_fmt)
-{
-	struct vpfe_fmt *fmt;
-
-	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
-	if (!fmt) {
-		/* default to first entry */
-		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
-			pix_fmt->pixelformat);
-		fmt = vpfe->active_fmt[0];
-	}
-
-	memset(mbus_fmt, 0, sizeof(*mbus_fmt));
-	v4l2_fill_mbus_format(mbus_fmt, pix_fmt, fmt->code);
-}
-
-static int
-cmp_v4l2_format(const struct v4l2_format *lhs, const struct v4l2_format *rhs)
-{
-	return lhs->type == rhs->type &&
-		lhs->fmt.pix.width == rhs->fmt.pix.width &&
-		lhs->fmt.pix.height == rhs->fmt.pix.height &&
-		lhs->fmt.pix.pixelformat == rhs->fmt.pix.pixelformat &&
-		lhs->fmt.pix.field == rhs->fmt.pix.field &&
-		lhs->fmt.pix.colorspace == rhs->fmt.pix.colorspace &&
-		lhs->fmt.pix.ycbcr_enc == rhs->fmt.pix.ycbcr_enc &&
-		lhs->fmt.pix.quantization == rhs->fmt.pix.quantization &&
-		lhs->fmt.pix.xfer_func == rhs->fmt.pix.xfer_func;
+	return bpp;
 }
 
 static inline u32 vpfe_reg_read(struct vpfe_ccdc *ccdc, u32 offset)
@@ -967,6 +923,7 @@ static int vpfe_get_ccdc_image_format(struct vpfe_device *vpfe,
 static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 {
 	enum ccdc_frmfmt frm_fmt = CCDC_FRMFMT_INTERLACED;
+	u32 bpp;
 	int ret = 0;
 
 	vpfe_dbg(1, vpfe, "pixelformat: %4pE\n",
@@ -979,7 +936,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 	}
 
 	/* configure the image window */
-	vpfe_ccdc_set_image_window(&vpfe->ccdc, &vpfe->crop, vpfe->bpp);
+	bpp = __get_bytesperpixel(vpfe, vpfe->current_vpfe_fmt);
+	vpfe_ccdc_set_image_window(&vpfe->ccdc, &vpfe->crop, bpp);
 
 	switch (vpfe->fmt.fmt.pix.field) {
 	case V4L2_FIELD_INTERLACED:
@@ -1023,7 +981,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 static int vpfe_config_image_format(struct vpfe_device *vpfe,
 				    v4l2_std_id std_id)
 {
-	struct v4l2_pix_format *pix = &vpfe->fmt.fmt.pix;
+	struct vpfe_fmt *fmt;
+	struct v4l2_mbus_framefmt mbus_fmt;
 	int i, ret;
 
 	for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) {
@@ -1045,26 +1004,29 @@ static int vpfe_config_image_format(struct vpfe_device *vpfe,
 		return -EINVAL;
 	}
 
-	vpfe->crop.top = vpfe->crop.left = 0;
-	vpfe->crop.width = vpfe->std_info.active_pixels;
-	vpfe->crop.height = vpfe->std_info.active_lines;
-	pix->width = vpfe->crop.width;
-	pix->height = vpfe->crop.height;
-	pix->pixelformat = V4L2_PIX_FMT_YUYV;
-
-	/* first field and frame format based on standard frame format */
-	if (vpfe->std_info.frame_format)
-		pix->field = V4L2_FIELD_INTERLACED;
-	else
-		pix->field = V4L2_FIELD_NONE;
-
-	ret = __vpfe_get_format(vpfe, &vpfe->fmt, &vpfe->bpp);
+	ret = __subdev_get_format(vpfe, &mbus_fmt);
 	if (ret)
 		return ret;
 
+	fmt = find_format_by_code(vpfe, mbus_fmt.code);
+	if (!fmt) {
+		vpfe_dbg(3, vpfe, "mbus code format (0x%08x) not found.\n",
+			 mbus_fmt.code);
+		return -EINVAL;
+	}
+
+	/* Save current subdev format */
+	v4l2_fill_pix_format(&vpfe->fmt.fmt.pix, &mbus_fmt);
+	vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	vpfe->fmt.fmt.pix.pixelformat = fmt->fourcc;
+	vpfe_calc_format_size(vpfe, fmt, &vpfe->fmt);
+	vpfe->current_vpfe_fmt = fmt;
+
 	/* Update the crop window based on found values */
-	vpfe->crop.width = pix->width;
-	vpfe->crop.height = pix->height;
+	vpfe->crop.top = 0;
+	vpfe->crop.left = 0;
+	vpfe->crop.width = mbus_fmt.width;
+	vpfe->crop.height = mbus_fmt.height;
 
 	return vpfe_config_ccdc_image_format(vpfe);
 }
@@ -1342,81 +1304,74 @@ static int vpfe_querycap(struct file *file, void  *priv,
 }
 
 /* get the format set at output pad of the adjacent subdev */
-static int __vpfe_get_format(struct vpfe_device *vpfe,
-			     struct v4l2_format *format, unsigned int *bpp)
+static int __subdev_get_format(struct vpfe_device *vpfe,
+			       struct v4l2_mbus_framefmt *fmt)
 {
-	struct v4l2_mbus_framefmt mbus_fmt;
-	struct vpfe_subdev_info *sdinfo;
-	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *sd = vpfe->current_subdev->sd;
+	struct v4l2_subdev_format sd_fmt;
+	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
 	int ret;
 
-	sdinfo = vpfe->current_subdev;
-	if (!sdinfo->sd)
-		return -EINVAL;
-
-	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt.pad = 0;
+	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sd_fmt.pad = 0;
 
-	ret = v4l2_subdev_call(sdinfo->sd, pad, get_fmt, NULL, &fmt);
-	if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV)
+	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
+	if (ret)
 		return ret;
 
-	if (!ret) {
-		v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
-		mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
-	} else {
-		ret = v4l2_device_call_until_err(&vpfe->v4l2_dev,
-						 sdinfo->grp_id,
-						 pad, get_fmt,
-						 NULL, &fmt);
-		if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV)
-			return ret;
-		v4l2_fill_pix_format(&format->fmt.pix, &mbus_fmt);
-		mbus_to_pix(vpfe, &mbus_fmt, &format->fmt.pix, bpp);
-	}
-
-	format->type = vpfe->fmt.type;
+	*fmt = *mbus_fmt;
 
-	vpfe_dbg(1, vpfe,
-		 "%s: size %dx%d (%4pE) bytesperline = %d, size = %d, bpp = %d\n",
-		 __func__, format->fmt.pix.width, format->fmt.pix.height,
-		 &format->fmt.pix.pixelformat,
-		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
+	vpfe_dbg(1, vpfe, "%s: %dx%d code:%04X\n", __func__,
+		 fmt->width, fmt->height, fmt->code);
 
 	return 0;
 }
 
 /* set the format at output pad of the adjacent subdev */
-static int __vpfe_set_format(struct vpfe_device *vpfe,
-			     struct v4l2_format *format, unsigned int *bpp)
+static int __subdev_set_format(struct vpfe_device *vpfe,
+			       struct v4l2_mbus_framefmt *fmt)
 {
-	struct vpfe_subdev_info *sdinfo;
-	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *sd = vpfe->current_subdev->sd;
+	struct v4l2_subdev_format sd_fmt;
+	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
 	int ret;
 
-	sdinfo = vpfe->current_subdev;
-	if (!sdinfo->sd)
-		return -EINVAL;
-
-	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt.pad = 0;
-
-	pix_to_mbus(vpfe, &format->fmt.pix, &fmt.format);
+	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sd_fmt.pad = 0;
+	*mbus_fmt = *fmt;
 
-	ret = v4l2_subdev_call(sdinfo->sd, pad, set_fmt, NULL, &fmt);
+	ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sd_fmt);
 	if (ret)
 		return ret;
 
-	v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
-	mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
+	vpfe_dbg(1, vpfe, "%s %dx%d code:%04X\n", __func__,
+		 fmt->width, fmt->height, fmt->code);
 
-	format->type = vpfe->fmt.type;
+	return 0;
+}
 
-	vpfe_dbg(1, vpfe,
-		 "%s: size %dx%d (%4pE) bytesperline = %d, size = %d, bpp = %d\n",
-		 __func__,  format->fmt.pix.width, format->fmt.pix.height,
-		 &format->fmt.pix.pixelformat,
-		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
+static int vpfe_calc_format_size(struct vpfe_device *vpfe,
+				 const struct vpfe_fmt *fmt,
+				 struct v4l2_format *f)
+{
+	u32 bpp;
+
+	if (!fmt) {
+		vpfe_dbg(3, vpfe, "No vpfe_fmt provided!\n");
+		return -EINVAL;
+	}
+
+	bpp = __get_bytesperpixel(vpfe, fmt);
+
+	/* pitch should be 32 bytes aligned */
+	f->fmt.pix.bytesperline = ALIGN(f->fmt.pix.width * bpp, 32);
+	f->fmt.pix.sizeimage = f->fmt.pix.bytesperline *
+			       f->fmt.pix.height;
+
+	vpfe_dbg(3, vpfe, "%s: fourcc: %4pE size: %dx%d bpl:%d img_size:%d\n",
+		 __func__, &f->fmt.pix.pixelformat,
+		 f->fmt.pix.width, f->fmt.pix.height,
+		 f->fmt.pix.bytesperline, f->fmt.pix.sizeimage);
 
 	return 0;
 }
@@ -1456,20 +1411,70 @@ static int vpfe_enum_fmt(struct file *file, void  *priv,
 }
 
 static int vpfe_try_fmt(struct file *file, void *priv,
-			struct v4l2_format *fmt)
+			struct v4l2_format *f)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
-	unsigned int bpp;
+	struct v4l2_subdev *sd = vpfe->current_subdev->sd;
+	const struct vpfe_fmt *fmt;
+	struct v4l2_subdev_frame_size_enum fse;
+	int ret, found;
+
+	fmt = find_format_by_pix(vpfe, f->fmt.pix.pixelformat);
+	if (!fmt) {
+		/* default to first entry */
+		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
+			 f->fmt.pix.pixelformat);
+		fmt = vpfe->active_fmt[0];
+		f->fmt.pix.pixelformat = fmt->fourcc;
+	}
 
-	return __vpfe_get_format(vpfe, fmt, &bpp);
+	f->fmt.pix.field = vpfe->fmt.fmt.pix.field;
+
+	/* check for/find a valid width/height */
+	ret = 0;
+	found = false;
+	fse.pad = 0;
+	fse.code = fmt->code;
+	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	for (fse.index = 0; ; fse.index++) {
+		ret = v4l2_subdev_call(sd, pad, enum_frame_size,
+				       NULL, &fse);
+		if (ret)
+			break;
+
+		if (f->fmt.pix.width == fse.max_width &&
+		    f->fmt.pix.height == fse.max_height) {
+			found = true;
+			break;
+		} else if (f->fmt.pix.width >= fse.min_width &&
+			   f->fmt.pix.width <= fse.max_width &&
+			   f->fmt.pix.height >= fse.min_height &&
+			   f->fmt.pix.height <= fse.max_height) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		/* use existing values as default */
+		f->fmt.pix.width = vpfe->fmt.fmt.pix.width;
+		f->fmt.pix.height =  vpfe->fmt.fmt.pix.height;
+	}
+
+	/*
+	 * Use current colorspace for now, it will get
+	 * updated properly during s_fmt
+	 */
+	f->fmt.pix.colorspace = vpfe->fmt.fmt.pix.colorspace;
+	return vpfe_calc_format_size(vpfe, fmt, f);
 }
 
 static int vpfe_s_fmt(struct file *file, void *priv,
 		      struct v4l2_format *fmt)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
-	struct v4l2_format format;
-	unsigned int bpp;
+	struct vpfe_fmt *f;
+	struct v4l2_mbus_framefmt mbus_fmt;
 	int ret;
 
 	/* If streaming is started, return error */
@@ -1478,25 +1483,32 @@ static int vpfe_s_fmt(struct file *file, void *priv,
 		return -EBUSY;
 	}
 
-	ret = __vpfe_get_format(vpfe, &format, &bpp);
-	if (ret)
+	ret = vpfe_try_fmt(file, priv, fmt);
+	if (ret < 0)
 		return ret;
 
+	f = find_format_by_pix(vpfe, fmt->fmt.pix.pixelformat);
 
-	if (!cmp_v4l2_format(fmt, &format)) {
-		/* Sensor format is different from the requested format
-		 * so we need to change it
-		 */
-		ret = __vpfe_set_format(vpfe, fmt, &bpp);
-		if (ret)
-			return ret;
-	} else /* Just make sure all of the fields are consistent */
-		*fmt = format;
+	v4l2_fill_mbus_format(&mbus_fmt, &fmt->fmt.pix, f->code);
 
-	/* First detach any IRQ if currently attached */
-	vpfe_detach_irq(vpfe);
-	vpfe->fmt = *fmt;
-	vpfe->bpp = bpp;
+	ret = __subdev_set_format(vpfe, &mbus_fmt);
+	if (ret)
+		return ret;
+
+	/* Just double check nothing has gone wrong */
+	if (mbus_fmt.code != f->code) {
+		vpfe_dbg(3, vpfe,
+			 "%s subdev changed format on us, this should not happen\n",
+			 __func__);
+		return -EINVAL;
+	}
+
+	v4l2_fill_pix_format(&vpfe->fmt.fmt.pix, &mbus_fmt);
+	vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	vpfe->fmt.fmt.pix.pixelformat  = f->fourcc;
+	vpfe_calc_format_size(vpfe, f, &vpfe->fmt);
+	*fmt = vpfe->fmt;
+	vpfe->current_vpfe_fmt = f;
 
 	/* Update the crop window based on found values */
 	vpfe->crop.width = fmt->fmt.pix.width;
@@ -1511,43 +1523,28 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 	struct v4l2_subdev_frame_size_enum fse;
-	struct vpfe_subdev_info *sdinfo;
-	struct v4l2_mbus_framefmt mbus;
-	struct v4l2_pix_format pix;
+	struct v4l2_subdev *sd = vpfe->current_subdev->sd;
 	struct vpfe_fmt *fmt;
 	int ret;
 
 	/* check for valid format */
 	fmt = find_format_by_pix(vpfe, fsize->pixel_format);
 	if (!fmt) {
-		vpfe_dbg(3, vpfe, "Invalid pixel code: %x, default used instead\n",
-			fsize->pixel_format);
+		vpfe_dbg(3, vpfe, "Invalid pixel code: %x\n",
+			 fsize->pixel_format);
 		return -EINVAL;
 	}
 
 	memset(fsize->reserved, 0x0, sizeof(fsize->reserved));
 
-	sdinfo = vpfe->current_subdev;
-	if (!sdinfo->sd)
-		return -EINVAL;
-
-	memset(&pix, 0x0, sizeof(pix));
-	/* Construct pix from parameter and use default for the rest */
-	pix.pixelformat = fsize->pixel_format;
-	pix.width = 640;
-	pix.height = 480;
-	pix.colorspace = V4L2_COLORSPACE_SRGB;
-	pix.field = V4L2_FIELD_NONE;
-	pix_to_mbus(vpfe, &pix, &mbus);
-
 	memset(&fse, 0x0, sizeof(fse));
 	fse.index = fsize->index;
 	fse.pad = 0;
-	fse.code = mbus.code;
+	fse.code = fmt->code;
 	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	ret = v4l2_subdev_call(sdinfo->sd, pad, enum_frame_size, NULL, &fse);
+	ret = v4l2_subdev_call(sd, pad, enum_frame_size, NULL, &fse);
 	if (ret)
-		return -EINVAL;
+		return ret;
 
 	vpfe_dbg(1, vpfe, "%s: index: %d code: %x W:[%d,%d] H:[%d,%d]\n",
 		 __func__, fse.index, fse.code, fse.min_width, fse.max_width,
@@ -2036,6 +2033,7 @@ vpfe_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
 	struct vpfe_device *vpfe = video_drvdata(file);
 	struct v4l2_rect cr = vpfe->crop;
 	struct v4l2_rect r = s->r;
+	u32 bpp;
 
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
@@ -2061,10 +2059,12 @@ vpfe_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
 
 	s->r = vpfe->crop = r;
 
-	vpfe_ccdc_set_image_window(&vpfe->ccdc, &r, vpfe->bpp);
+	bpp = __get_bytesperpixel(vpfe, vpfe->current_vpfe_fmt);
+	vpfe_ccdc_set_image_window(&vpfe->ccdc, &r, bpp);
 	vpfe->fmt.fmt.pix.width = r.width;
 	vpfe->fmt.fmt.pix.height = r.height;
-	vpfe->fmt.fmt.pix.bytesperline = vpfe_ccdc_get_line_length(&vpfe->ccdc);
+	vpfe->fmt.fmt.pix.bytesperline =
+		vpfe_ccdc_get_line_length(&vpfe->ccdc);
 	vpfe->fmt.fmt.pix.sizeimage = vpfe->fmt.fmt.pix.bytesperline *
 						vpfe->fmt.fmt.pix.height;
 
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index 79a6505c083d..aa5ddaec774c 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -283,8 +283,8 @@ struct vpfe_device {
 	struct vpfe_cap_buffer *next_frm;
 	/* Used to store pixel format */
 	struct v4l2_format fmt;
-	/* Used to store current bytes per pixel based on current format */
-	unsigned int bpp;
+	/* Used to keep a reference to the current vpfe_fmt */
+	struct vpfe_fmt *current_vpfe_fmt;
 	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
 	unsigned int num_active_fmt;
 
-- 
2.17.1

[Patch v2 12/13] media: am437x-vpfe: Remove per bus width static data

[Benoit Parrot]

The bus related static data included in the vpfe_fmt
static table can be derived dynamically instead.
This simplify the table and it's use.

We instead replace the per bus data info with just
the usual bit per pixel value for each supported
pixel format.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 56 ++++++---------------
 drivers/media/platform/am437x/am437x-vpfe.h | 16 +-----
 2 files changed, 16 insertions(+), 56 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 47404d799029..6416b7955fb7 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -73,73 +73,43 @@ static struct vpfe_fmt formats[VPFE_MAX_ACTIVE_FMT] = {
 	{
 		.fourcc		= V4L2_PIX_FMT_YUYV,
 		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
-		.l.width	= 10,
-		.l.bpp		= 4,
-		.s.width	= 8,
-		.s.bpp		= 2,
+		.bitsperpixel	= 16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_UYVY,
 		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
-		.l.width	= 10,
-		.l.bpp		= 4,
-		.s.width	= 8,
-		.s.bpp		= 2,
+		.bitsperpixel	= 16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_YVYU,
 		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
-		.l.width	= 10,
-		.l.bpp		= 4,
-		.s.width	= 8,
-		.s.bpp		= 2,
+		.bitsperpixel	= 16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_VYUY,
 		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
-		.l.width	= 10,
-		.l.bpp		= 4,
-		.s.width	= 8,
-		.s.bpp		= 2,
+		.bitsperpixel	= 16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SBGGR8,
 		.code		= MEDIA_BUS_FMT_SBGGR8_1X8,
-		.l.width	= 10,
-		.l.bpp		= 2,
-		.s.width	= 8,
-		.s.bpp		= 1,
+		.bitsperpixel	= 8,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGBRG8,
 		.code		= MEDIA_BUS_FMT_SGBRG8_1X8,
-		.l.width	= 10,
-		.l.bpp		= 2,
-		.s.width	= 8,
-		.s.bpp		= 1,
+		.bitsperpixel	= 8,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SGRBG8,
 		.code		= MEDIA_BUS_FMT_SGRBG8_1X8,
-		.l.width	= 10,
-		.l.bpp		= 2,
-		.s.width	= 8,
-		.s.bpp		= 1,
+		.bitsperpixel	= 8,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_SRGGB8,
 		.code		= MEDIA_BUS_FMT_SRGGB8_1X8,
-		.l.width	= 10,
-		.l.bpp		= 2,
-		.s.width	= 8,
-		.s.bpp		= 1,
+		.bitsperpixel	= 8,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_RGB565,
 		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,
-		.l.width	= 10,
-		.l.bpp		= 4,
-		.s.width	= 8,
-		.s.bpp		= 2,
+		.bitsperpixel	= 16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_RGB565X,
 		.code		= MEDIA_BUS_FMT_RGB565_2X8_BE,
-		.l.width	= 10,
-		.l.bpp		= 4,
-		.s.width	= 8,
-		.s.bpp		= 2,
+		.bitsperpixel	= 16,
 	},
 };
 
@@ -184,9 +154,11 @@ static unsigned int __get_bytesperpixel(struct vpfe_device *vpfe,
 {
 	struct vpfe_subdev_info *sdinfo = vpfe->current_subdev;
 	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
-	u32 bpp;
+	u32 bpp, bus_width_bytes, clocksperpixel;
 
-	bpp = (bus_width == 10) ? fmt->l.bpp : fmt->s.bpp;
+	bus_width_bytes = ALIGN(bus_width, 8) >> 3;
+	clocksperpixel = DIV_ROUND_UP(fmt->bitsperpixel, bus_width);
+	bpp = clocksperpixel * bus_width_bytes;
 
 	return bpp;
 }
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index aa5ddaec774c..7556cca308c2 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -215,28 +215,16 @@ struct vpfe_ccdc {
 	u32 ccdc_ctx[VPFE_REG_END / sizeof(u32)];
 };
 
-/*
- * struct bus_format - VPFE bus format information
- * width: Bits per pixel (when transferred over a bus)
- * bpp: Bytes per pixel (when stored in memory)
- */
-struct bus_format {
-	unsigned int width;
-	unsigned int bpp;
-};
-
 /*
  * struct vpfe_fmt - VPFE media bus format information
  * fourcc: V4L2 pixel format code
  * code: V4L2 media bus format code
- * l: 10 bit bus format info
- * s: 8 bit bus format info
+ * bitsperpixel: Bits per pixel over the bus
  */
 struct vpfe_fmt {
 	u32 fourcc;
 	u32 code;
-	struct bus_format l;
-	struct bus_format s;
+	u32 bitsperpixel;
 };
 
 /*
-- 
2.17.1

[Patch v2 13/13] media: am437x-vpfe: Switch to SPDX Licensing

[Benoit Parrot]

Switch to SPDX licensing and drop the redundant GPL text.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c      | 14 +-------------
 drivers/media/platform/am437x/am437x-vpfe.h      | 14 +-------------
 drivers/media/platform/am437x/am437x-vpfe_regs.h | 10 +---------
 3 files changed, 3 insertions(+), 35 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 6416b7955fb7..5a66b0e9cff5 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * TI VPFE capture Driver
  *
@@ -5,19 +6,6 @@
  *
  * Benoit Parrot <bparrot@ti.com>
  * Lad, Prabhakar <prabhakar.csengg@gmail.com>
- *
- * This program is free software; you may redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 2 of the License.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
  */
 
 #include <linux/delay.h>
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index 7556cca308c2..fa5b85bcd8db 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -1,21 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
 /*
  * Copyright (C) 2013 - 2014 Texas Instruments, Inc.
  *
  * Benoit Parrot <bparrot@ti.com>
  * Lad, Prabhakar <prabhakar.csengg@gmail.com>
- *
- * This program is free software; you may redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; version 2 of the License.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
  */
 
 #ifndef AM437X_VPFE_H
diff --git a/drivers/media/platform/am437x/am437x-vpfe_regs.h b/drivers/media/platform/am437x/am437x-vpfe_regs.h
index 0746c48ec23f..63ecdca3b908 100644
--- a/drivers/media/platform/am437x/am437x-vpfe_regs.h
+++ b/drivers/media/platform/am437x/am437x-vpfe_regs.h
@@ -1,3 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
 /*
  * TI AM437x Image Sensor Interface Registers
  *
@@ -5,15 +6,6 @@
  *
  * Benoit Parrot <bparrot@ti.com>
  * Lad, Prabhakar <prabhakar.csengg@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether express or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
 #ifndef AM437X_VPFE_REGS_H
-- 
2.17.1

Re: [Patch 00/13] media: am437x-vpfe: overdue maintenance

[Hans Verkuil]

On 9/19/19 10:41 PM, Benoit Parrot wrote:
> This patch series is a collection of patches we have been carrying for a
> while.
> 
> A few patches do fix actual bug and v4l2-compliance errors/warnings.
> Other are drivers re-work to simplify/clarify the code for easier
> maintenance.
> 
> We also include the SPDX Licensing update which seemed to have been
> missed by the global script thus far.
> 
> Changes since v1:
> - Address review comment from Joe, Hans and Prabhakar
> - Cleaned-up the function entry debug log
> - Split off the pcr change into its own patch
> - Rework/combine two patches but remove code churn
> - fix miscellaneous typos

Just to verify: this patch from v1 has been dropped in this v2, right?

[Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt

Regards,

	Hans

> 
> =============================
> 
> v4l2-compliance SHA: 5b168dc8473911227890526bad26553d9e8ff81b, 32 bits
> 
> Compliance test for vpfe device /dev/video0:
> 
> Driver Info:
> 	Driver name      : vpfe
> 	Card type        : TI AM437x VPFE
> 	Bus info         : platform:vpfe 48326000.vpfe
> 	Driver version   : 5.3.0
> 	Capabilities     : 0x85200001
> 		Vide o Capture
> 		Read/Write
> 		Streaming
> 		Extended Pix Format
> 		D evice Capabilities
> 	Device Caps      : 0x05200001
> 		Video Capt ure
> 		Read/Write
> 		Streaming
> 		Extended Pix Format
> 
> Required ioctls:
> 	test VIDIOC_QUERYCAP: OK
> 
> Allow for multiple opens:
> 	test second /dev/video0 open: OK
> 	test VIDIOC_QUERYCAP: OK
> 	test VIDIOC_G/S_PRIORITY: OK
> 	test for unlimited opens: OK
> 
> Debug ioctls:
> 	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> 	test VIDIOC_LOG_STATUS: OK
> 
> Input ioctls:
> 	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> 	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> 	test VIDIOC_ENUMAUDIO: OK (Not Supported)
> 	test VIDIOC_G/S/ENUMINPUT: OK
> 	test VIDIOC_G/S_AUDIO: OK (Not Supported)
> 	Inputs: 1 Audio Inputs: 0 Tuners: 0
> 
> Output ioctls:
> 	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> 	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> 	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> 	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> 	Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Input/Output configuration ioctls:
> 	test VIDIOC_ENUM/G/S/QUERY_STD: OK
> 	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> 	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> 	test VIDIOC_G/S_EDID: OK (Not Supported)
> 
> Control ioctls (Input 0):
> 	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> 	test VIDIOC_QUERYCTRL: OK
> 	test VIDIOC_G/S_CTRL: OK
> 	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> 	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> 	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> 	Standard Controls: 3 Private Controls: 0
> 
> Format ioctls (Input 0):
> 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> 	test VIDIOC_G/S_PARM: OK
> 	test VIDIOC_G_FBUF: OK (Not Supported)
> 	test VIDIOC_G_FMT: OK
> 	test VIDIOC_TRY_FMT: OK
> 	test VIDIOC_S_FMT: OK
> 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> 		fail: v4l2-test-formats.cpp(1419): node->frmsizes_count[pixfmt] > 1
> 	test Cropping: FAIL
> 	test Composing: OK (Not Supported)
> 	test Scaling: OK (Not Supported)
> 
> Codec ioctls (Input 0):
> 	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> 	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> 	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> 
> Buffer ioctls (Input 0):
> 	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> 	test VIDIOC_EXPBUF: OK
> 	test Requests: OK (Not Supported)
> 
> Test input 0:
> 
> Streaming ioctls:
> 	test read/write: OK
> 	test blocking wait: OK
> 	test MMAP (no poll): OK                           
> 	test MMAP (select): OK                            
> 	test MMAP (epoll): OK                             
> 	test USERPTR (no poll): OK (Not Supported)
> 	test USERPTR (select): OK (Not Supported)
> 	test DMABUF: Cannot test, specify --expbuf-device
> 
> Total for vpfe device /dev/video0: 51, Succeeded: 50, Failed: 1, Warnings: 0
> dd
> ============================
> 
> Benoit Parrot (12):
>   media: am437x-vpfe: Fix missing first line
>   media: am437x-vpfe: Rework ISR routine for clarity
>   media: am437x-vpfe: Wait for end of frame before tear-down
>   media: am437x-vpfe: fix start streaming error path
>   media: am437x-vpfe: Streamlined vb2 buffer cleanup
>   media: am437x-vpfe: Setting STD to current value is not an error
>   media: am437x-vpfe: Use a per instance format array instead of a
>     static one
>   media: am437x-vpfe: fix function trace debug log
>   media: am437x-vpfe: Remove print_fourcc helper
>   media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
>   media: am437x-vpfe: Remove per bus width static data
>   media: am437x-vpfe: Switch to SPDX Licensing
> 
> Dave Gerlach (1):
>   media: am437x-vpfe: Fix suspend path to always handle pinctrl config
> 
>  drivers/media/platform/am437x/am437x-vpfe.c   | 878 ++++++++----------
>  drivers/media/platform/am437x/am437x-vpfe.h   |  45 +-
>  .../media/platform/am437x/am437x-vpfe_regs.h  |  10 +-
>  3 files changed, 406 insertions(+), 527 deletions(-)
> 

Re: [Patch 00/13] media: am437x-vpfe: overdue maintenance

[Benoit Parrot]

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-20 10:25:02 +0200]:
> On 9/19/19 10:41 PM, Benoit Parrot wrote:
> > This patch series is a collection of patches we have been carrying for a
> > while.
> > 
> > A few patches do fix actual bug and v4l2-compliance errors/warnings.
> > Other are drivers re-work to simplify/clarify the code for easier
> > maintenance.
> > 
> > We also include the SPDX Licensing update which seemed to have been
> > missed by the global script thus far.
> > 
> > Changes since v1:
> > - Address review comment from Joe, Hans and Prabhakar
> > - Cleaned-up the function entry debug log
> > - Split off the pcr change into its own patch
> > - Rework/combine two patches but remove code churn
> > - fix miscellaneous typos
> 
> Just to verify: this patch from v1 has been dropped in this v2, right?
> 
> [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt

Yes I dropped it.

Weird I seems to have missed the v2 tag on the cover-letter...
Sorry for the confusion.

Benoit

> 
> Regards,
> 
> 	Hans
> 
> > 
> > =============================
> > 
> > v4l2-compliance SHA: 5b168dc8473911227890526bad26553d9e8ff81b, 32 bits
> > 
> > Compliance test for vpfe device /dev/video0:
> > 
> > Driver Info:
> > 	Driver name      : vpfe
> > 	Card type        : TI AM437x VPFE
> > 	Bus info         : platform:vpfe 48326000.vpfe
> > 	Driver version   : 5.3.0
> > 	Capabilities     : 0x85200001
> > 		Vide o Capture
> > 		Read/Write
> > 		Streaming
> > 		Extended Pix Format
> > 		D evice Capabilities
> > 	Device Caps      : 0x05200001
> > 		Video Capt ure
> > 		Read/Write
> > 		Streaming
> > 		Extended Pix Format
> > 
> > Required ioctls:
> > 	test VIDIOC_QUERYCAP: OK
> > 
> > Allow for multiple opens:
> > 	test second /dev/video0 open: OK
> > 	test VIDIOC_QUERYCAP: OK
> > 	test VIDIOC_G/S_PRIORITY: OK
> > 	test for unlimited opens: OK
> > 
> > Debug ioctls:
> > 	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> > 	test VIDIOC_LOG_STATUS: OK
> > 
> > Input ioctls:
> > 	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> > 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> > 	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> > 	test VIDIOC_ENUMAUDIO: OK (Not Supported)
> > 	test VIDIOC_G/S/ENUMINPUT: OK
> > 	test VIDIOC_G/S_AUDIO: OK (Not Supported)
> > 	Inputs: 1 Audio Inputs: 0 Tuners: 0
> > 
> > Output ioctls:
> > 	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> > 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> > 	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> > 	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> > 	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> > 	Outputs: 0 Audio Outputs: 0 Modulators: 0
> > 
> > Input/Output configuration ioctls:
> > 	test VIDIOC_ENUM/G/S/QUERY_STD: OK
> > 	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> > 	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> > 	test VIDIOC_G/S_EDID: OK (Not Supported)
> > 
> > Control ioctls (Input 0):
> > 	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> > 	test VIDIOC_QUERYCTRL: OK
> > 	test VIDIOC_G/S_CTRL: OK
> > 	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> > 	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> > 	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> > 	Standard Controls: 3 Private Controls: 0
> > 
> > Format ioctls (Input 0):
> > 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> > 	test VIDIOC_G/S_PARM: OK
> > 	test VIDIOC_G_FBUF: OK (Not Supported)
> > 	test VIDIOC_G_FMT: OK
> > 	test VIDIOC_TRY_FMT: OK
> > 	test VIDIOC_S_FMT: OK
> > 	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> > 		fail: v4l2-test-formats.cpp(1419): node->frmsizes_count[pixfmt] > 1
> > 	test Cropping: FAIL
> > 	test Composing: OK (Not Supported)
> > 	test Scaling: OK (Not Supported)
> > 
> > Codec ioctls (Input 0):
> > 	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> > 	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> > 	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> > 
> > Buffer ioctls (Input 0):
> > 	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> > 	test VIDIOC_EXPBUF: OK
> > 	test Requests: OK (Not Supported)
> > 
> > Test input 0:
> > 
> > Streaming ioctls:
> > 	test read/write: OK
> > 	test blocking wait: OK
> > 	test MMAP (no poll): OK                           
> > 	test MMAP (select): OK                            
> > 	test MMAP (epoll): OK                             
> > 	test USERPTR (no poll): OK (Not Supported)
> > 	test USERPTR (select): OK (Not Supported)
> > 	test DMABUF: Cannot test, specify --expbuf-device
> > 
> > Total for vpfe device /dev/video0: 51, Succeeded: 50, Failed: 1, Warnings: 0
> > dd
> > ============================
> > 
> > Benoit Parrot (12):
> >   media: am437x-vpfe: Fix missing first line
> >   media: am437x-vpfe: Rework ISR routine for clarity
> >   media: am437x-vpfe: Wait for end of frame before tear-down
> >   media: am437x-vpfe: fix start streaming error path
> >   media: am437x-vpfe: Streamlined vb2 buffer cleanup
> >   media: am437x-vpfe: Setting STD to current value is not an error
> >   media: am437x-vpfe: Use a per instance format array instead of a
> >     static one
> >   media: am437x-vpfe: fix function trace debug log
> >   media: am437x-vpfe: Remove print_fourcc helper
> >   media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
> >   media: am437x-vpfe: Remove per bus width static data
> >   media: am437x-vpfe: Switch to SPDX Licensing
> > 
> > Dave Gerlach (1):
> >   media: am437x-vpfe: Fix suspend path to always handle pinctrl config
> > 
> >  drivers/media/platform/am437x/am437x-vpfe.c   | 878 ++++++++----------
> >  drivers/media/platform/am437x/am437x-vpfe.h   |  45 +-
> >  .../media/platform/am437x/am437x-vpfe_regs.h  |  10 +-
> >  3 files changed, 406 insertions(+), 527 deletions(-)
> > 
> 

[Patch 00/13] media: am437x-vpfe: overdue maintenance

[Benoit Parrot]

This patch series is a collection of patches we have been carrying for a
while.

A few patches do fix actual bug and v4l2-compliance errors/warnings.
Other are drivers re-work to simplify/clarify the code for easier
maintenance.

We also include the SPDX Licensing update which seemed to have been
missed by the global script thus far.

Benoit Parrot (12):
  media: am437x-vpfe: Fix missing first line
  media: am437x-vpfe: Rework ISR routine for clarity
  media: am437x-vpfe: Wait for end of frame before tear-down
  media: am437x-vpfe: Streamlined vb2 buffer cleanup
  media: am437x-vpfe: Setting STD to current value is not an error
  media: am437x-vpfe: Use a per instance format array instead of a
    static one
  media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
  media: am437x-vpfe: fix function trace debug log
  media: am437x-vpfe: Remove print_fourcc helper
  media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
  media: am437x-vpfe: Remove per bus width static data
  media: am437x-vpfe: Switch to SPDX Licensing

Dave Gerlach (1):
  media: am437x-vpfe: Fix suspend path to always handle pinctrl config

 drivers/media/platform/am437x/am437x-vpfe.c   | 906 ++++++++----------
 drivers/media/platform/am437x/am437x-vpfe.h   |  44 +-
 .../media/platform/am437x/am437x-vpfe_regs.h  |  10 +-
 3 files changed, 438 insertions(+), 522 deletions(-)

-- 
2.17.1

Re: [Patch 00/13] media: am437x-vpfe: overdue maintenance

[Hans Verkuil]

Hi Benoit,

On 9/9/19 6:27 PM, Benoit Parrot wrote:
> This patch series is a collection of patches we have been carrying for a
> while.
> 
> A few patches do fix actual bug and v4l2-compliance errors/warnings.
> Other are drivers re-work to simplify/clarify the code for easier
> maintenance.

Can you post the output of the latest version of v4l2-compliance? Use
the '-s' option so streaming is tested as well.

Thanks!

	Hans

> 
> We also include the SPDX Licensing update which seemed to have been
> missed by the global script thus far.
> 
> Benoit Parrot (12):
>   media: am437x-vpfe: Fix missing first line
>   media: am437x-vpfe: Rework ISR routine for clarity
>   media: am437x-vpfe: Wait for end of frame before tear-down
>   media: am437x-vpfe: Streamlined vb2 buffer cleanup
>   media: am437x-vpfe: Setting STD to current value is not an error
>   media: am437x-vpfe: Use a per instance format array instead of a
>     static one
>   media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
>   media: am437x-vpfe: fix function trace debug log
>   media: am437x-vpfe: Remove print_fourcc helper
>   media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
>   media: am437x-vpfe: Remove per bus width static data
>   media: am437x-vpfe: Switch to SPDX Licensing
> 
> Dave Gerlach (1):
>   media: am437x-vpfe: Fix suspend path to always handle pinctrl config
> 
>  drivers/media/platform/am437x/am437x-vpfe.c   | 906 ++++++++----------
>  drivers/media/platform/am437x/am437x-vpfe.h   |  44 +-
>  .../media/platform/am437x/am437x-vpfe_regs.h  |  10 +-
>  3 files changed, 438 insertions(+), 522 deletions(-)
> 

Re: [Patch 00/13] media: am437x-vpfe: overdue maintenance

[Benoit Parrot]

Hans Verkuil <hverkuil@xs4all.nl> wrote on Tue [2019-Sep-10 12:42:51 +0200]:
> Hi Benoit,
> 
> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> > This patch series is a collection of patches we have been carrying for a
> > while.
> > 
> > A few patches do fix actual bug and v4l2-compliance errors/warnings.
> > Other are drivers re-work to simplify/clarify the code for easier
> > maintenance.
> 
> Can you post the output of the latest version of v4l2-compliance? Use
> the '-s' option so streaming is tested as well.
> 
> Thanks!
> 
> 	Hans

As already discussed, I'll paste the result here and I'll make sure to
include it in the next revision.

./v4l2-compliance_5b168dc84739 -d 0 -s   
v4l2-compliance SHA: 5b168dc8473911227890526bad26553d9e8ff81b, 32 bits

Compliance test for vpfe device /dev/video0:

Driver Info:
	Driver name      : vpfe
	Card type        : TI AM437x VPFE
	Bus info         : platform:vpfe 48326000.vpfe
	Driver v ersion   : 5.3.0
	Capabilities     : 0x85200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x05200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second /dev/video0 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 3 Private Controls: 0

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
		fail: v4l2-test-formats.cpp(1419): node->frmsizes_count[pixfmt] > 1
	test Cropping: FAIL
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK (Not Supported)

Test input 0:

Streaming ioctls:
	test read/write: OK
	test blocking wait: OK
	test MMAP (no poll): OK                           
	test MMAP (select): OK                            
	test MMAP (epoll): OK                             
	test USERPTR (no poll): OK (Not Supported)
	test USERPTR (select): OK (Not Supported)
	test DMABUF: Cannot test, specify --expbuf-device

Total for vpfe device /dev/video0: 51, Succeeded: 50, Failed: 1, Warnings: 0

Benoit

> 
> > 
> > We also include the SPDX Licensing update which seemed to have been
> > missed by the global script thus far.
> > 
> > Benoit Parrot (12):
> >   media: am437x-vpfe: Fix missing first line
> >   media: am437x-vpfe: Rework ISR routine for clarity
> >   media: am437x-vpfe: Wait for end of frame before tear-down
> >   media: am437x-vpfe: Streamlined vb2 buffer cleanup
> >   media: am437x-vpfe: Setting STD to current value is not an error
> >   media: am437x-vpfe: Use a per instance format array instead of a
> >     static one
> >   media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
> >   media: am437x-vpfe: fix function trace debug log
> >   media: am437x-vpfe: Remove print_fourcc helper
> >   media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
> >   media: am437x-vpfe: Remove per bus width static data
> >   media: am437x-vpfe: Switch to SPDX Licensing
> > 
> > Dave Gerlach (1):
> >   media: am437x-vpfe: Fix suspend path to always handle pinctrl config
> > 
> >  drivers/media/platform/am437x/am437x-vpfe.c   | 906 ++++++++----------
> >  drivers/media/platform/am437x/am437x-vpfe.h   |  44 +-
> >  .../media/platform/am437x/am437x-vpfe_regs.h  |  10 +-
> >  3 files changed, 438 insertions(+), 522 deletions(-)
> > 
>