linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch 00/13] media: am437x-vpfe: overdue maintenance
@ 2019-09-09 16:27 Benoit Parrot
  2019-09-09 16:27 ` [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config Benoit Parrot
                   ` (13 more replies)
  0 siblings, 14 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-13 12:59   ` Hans Verkuil
  2019-09-09 16:27 ` [Patch 02/13] media: am437x-vpfe: Fix missing first line Benoit Parrot
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel,
	Dave Gerlach, 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: Benoit Parrot <bparrot@ti.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c | 44 ++++++++++-----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 2b42ba1f5949..ab959d61f9c9 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2653,22 +2653,22 @@ 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;
+	/* 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);
+		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 +2710,19 @@ 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;
+	/* 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);
+		/* 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 02/13] media: am437x-vpfe: Fix missing first line
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
  2019-09-09 16:27 ` [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-13 13:00   ` Hans Verkuil
  2019-09-09 16:27 ` [Patch 03/13] media: am437x-vpfe: Rework ISR routine for clarity Benoit Parrot
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, Benoit Parrot

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

Signed-off-by: Benoit Parrot <bparrot@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 ab959d61f9c9..0ecb75bf5abd 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 03/13] media: am437x-vpfe: Rework ISR routine for clarity
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
  2019-09-09 16:27 ` [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config Benoit Parrot
  2019-09-09 16:27 ` [Patch 02/13] media: am437x-vpfe: Fix missing first line Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-09 16:27 ` [Patch 04/13] media: am437x-vpfe: Wait for end of frame before tear-down Benoit Parrot
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 0ecb75bf5abd..7c5b734f7143 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 04/13] media: am437x-vpfe: Wait for end of frame before tear-down
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (2 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 03/13] media: am437x-vpfe: Rework ISR routine for clarity Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-09 16:27 ` [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup Benoit Parrot
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 7c5b734f7143..3a8ad9bdf283 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (3 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 04/13] media: am437x-vpfe: Wait for end of frame before tear-down Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-16  8:00   ` Lad, Prabhakar
  2019-09-09 16:27 ` [Patch 06/13] media: am437x-vpfe: Setting STD to current value is not an error Benoit Parrot
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 | 54 ++++++++++-----------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 3a8ad9bdf283..52f7fc6e11dd 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,8 @@ 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;
 }
 
@@ -2022,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);
@@ -2040,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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 06/13] media: am437x-vpfe: Setting STD to current value is not an error
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (4 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-09 16:27 ` [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one Benoit Parrot
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 52f7fc6e11dd..ac759c066d00 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (5 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 06/13] media: am437x-vpfe: Setting STD to current value is not an error Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-13 13:07   ` Hans Verkuil
  2019-09-09 16:27 ` [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt Benoit Parrot
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 | 108 ++++++++------------
 drivers/media/platform/am437x/am437x-vpfe.h |  34 ++++++
 2 files changed, 74 insertions(+), 68 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index ac759c066d00..e76dc2b3b7b8 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -69,30 +69,6 @@ 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[] = {
 	{
 		.fourcc		= V4L2_PIX_FMT_YUYV,
@@ -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..6f25750f84e4 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -215,6 +215,37 @@ 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.
+ */
+#define VPFE_MAX_ACTIVE_FMT	10
+
 struct vpfe_device {
 	/* V4l2 specific parameters */
 	/* Identifies video device for this channel */
@@ -252,6 +283,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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (6 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-13 13:14   ` Hans Verkuil
  2019-09-09 16:27 ` [Patch 09/13] media: am437x-vpfe: fix function trace debug log Benoit Parrot
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, Benoit Parrot

Keep a reference to the currently selected struct vpfe_fmt * object.
By doing so we rename the current struct v4l2_format * member from
fmt to v_fmt.
The added struct vpfe_fmt * reference is set to "const" so we also
constify all accesses and related helper functions.

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

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index e76dc2b3b7b8..a8f6cf1d06a0 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -147,8 +147,8 @@ static int
 __vpfe_get_format(struct vpfe_device *vpfe,
 		  struct v4l2_format *format, unsigned int *bpp);
 
-static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
-					    unsigned int code)
+static const struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
+						  unsigned int code)
 {
 	struct vpfe_fmt *fmt;
 	unsigned int k;
@@ -162,8 +162,8 @@ static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
 	return NULL;
 }
 
-static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
-					   unsigned int pixelformat)
+static const struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
+						 unsigned int pixelformat)
 {
 	struct vpfe_fmt *fmt;
 	unsigned int k;
@@ -184,7 +184,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
 {
 	struct vpfe_subdev_info *sdinfo = vpfe->current_subdev;
 	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
-	struct vpfe_fmt *fmt;
+	const struct vpfe_fmt *fmt;
 
 	fmt = find_format_by_code(vpfe, mbus->code);
 	if (WARN_ON(fmt == NULL)) {
@@ -207,7 +207,7 @@ 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;
+	const struct vpfe_fmt *fmt;
 
 	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
 	if (!fmt) {
@@ -990,10 +990,10 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 	vpfe_dbg(2, vpfe, "vpfe_config_ccdc_image_format\n");
 
 	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
-		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
+		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
 
 	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
-			vpfe->fmt.fmt.pix.pixelformat) < 0) {
+			vpfe->v_fmt.fmt.pix.pixelformat) < 0) {
 		vpfe_err(vpfe, "couldn't set pix format in ccdc\n");
 		return -EINVAL;
 	}
@@ -1001,7 +1001,7 @@ 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);
 
-	switch (vpfe->fmt.fmt.pix.field) {
+	switch (vpfe->v_fmt.fmt.pix.field) {
 	case V4L2_FIELD_INTERLACED:
 		/* do nothing, since it is default */
 		ret = vpfe_ccdc_set_buftype(
@@ -1043,7 +1043,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;
+	const struct vpfe_fmt *fmt;
+	struct v4l2_pix_format *pix = &vpfe->v_fmt.fmt.pix;
 	int i, ret;
 
 	for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) {
@@ -1078,10 +1079,18 @@ static int vpfe_config_image_format(struct vpfe_device *vpfe,
 	else
 		pix->field = V4L2_FIELD_NONE;
 
-	ret = __vpfe_get_format(vpfe, &vpfe->fmt, &vpfe->bpp);
+	ret = __vpfe_get_format(vpfe, &vpfe->v_fmt, &vpfe->bpp);
 	if (ret)
 		return ret;
 
+	fmt = find_format_by_pix(vpfe, pix->pixelformat);
+	if (!fmt) {
+		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s\n",
+			 (char *)&pix->pixelformat);
+		return -EINVAL;
+	}
+	vpfe->fmt = fmt;
+
 	/* Update the crop window based on found values */
 	vpfe->crop.width = pix->width;
 	vpfe->crop.height = pix->height;
@@ -1227,7 +1236,7 @@ static inline void vpfe_schedule_bottom_field(struct vpfe_device *vpfe)
 static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe)
 {
 	vpfe->cur_frm->vb.vb2_buf.timestamp = ktime_get_ns();
-	vpfe->cur_frm->vb.field = vpfe->fmt.fmt.pix.field;
+	vpfe->cur_frm->vb.field = vpfe->v_fmt.fmt.pix.field;
 	vpfe->cur_frm->vb.sequence = vpfe->sequence++;
 	vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf, VB2_BUF_STATE_DONE);
 	vpfe->cur_frm = vpfe->next_frm;
@@ -1296,7 +1305,7 @@ static void vpfe_handle_interlaced_irq(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 = vpfe->fmt.fmt.pix.field;
+	enum v4l2_field field = vpfe->v_fmt.fmt.pix.field;
 	int intr_status, stopping = vpfe->stopping;
 
 	intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS);
@@ -1397,7 +1406,7 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
 		mbus_to_pix(vpfe, &mbus_fmt, &format->fmt.pix, bpp);
 	}
 
-	format->type = vpfe->fmt.type;
+	format->type = vpfe->v_fmt.type;
 
 	vpfe_dbg(1, vpfe,
 		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
@@ -1434,7 +1443,7 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
 	v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
 	mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
 
-	format->type = vpfe->fmt.type;
+	format->type = vpfe->v_fmt.type;
 
 	vpfe_dbg(1, vpfe,
 		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
@@ -1452,7 +1461,7 @@ static int vpfe_g_fmt(struct file *file, void *priv,
 
 	vpfe_dbg(2, vpfe, "vpfe_g_fmt\n");
 
-	*fmt = vpfe->fmt;
+	*fmt = vpfe->v_fmt;
 
 	return 0;
 }
@@ -1496,9 +1505,10 @@ static int vpfe_try_fmt(struct file *file, void *priv,
 }
 
 static int vpfe_s_fmt(struct file *file, void *priv,
-		      struct v4l2_format *fmt)
+		      struct v4l2_format *f)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
+	const struct vpfe_fmt *fmt;
 	struct v4l2_format format;
 	unsigned int bpp;
 	int ret;
@@ -1515,25 +1525,32 @@ static int vpfe_s_fmt(struct file *file, void *priv,
 	if (ret)
 		return ret;
 
-
-	if (!cmp_v4l2_format(fmt, &format)) {
+	if (!cmp_v4l2_format(f, &format)) {
 		/* Sensor format is different from the requested format
 		 * so we need to change it
 		 */
-		ret = __vpfe_set_format(vpfe, fmt, &bpp);
+		ret = __vpfe_set_format(vpfe, f, &bpp);
 		if (ret)
 			return ret;
 	} else /* Just make sure all of the fields are consistent */
-		*fmt = format;
+		*f = format;
+
+	fmt = find_format_by_pix(vpfe, f->fmt.pix.pixelformat);
+	if (!fmt) {
+		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s, This should not happen!!\n",
+			 (char *)&f->fmt.pix.pixelformat);
+		return -EINVAL;
+	}
 
 	/* First detach any IRQ if currently attached */
 	vpfe_detach_irq(vpfe);
-	vpfe->fmt = *fmt;
+	vpfe->v_fmt = *f;
 	vpfe->bpp = bpp;
+	vpfe->fmt = fmt;
 
 	/* Update the crop window based on found values */
-	vpfe->crop.width = fmt->fmt.pix.width;
-	vpfe->crop.height = fmt->fmt.pix.height;
+	vpfe->crop.width = f->fmt.pix.width;
+	vpfe->crop.height = f->fmt.pix.height;
 
 	/* set image capture parameters in the ccdc */
 	return vpfe_config_ccdc_image_format(vpfe);
@@ -1547,7 +1564,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	struct vpfe_subdev_info *sdinfo;
 	struct v4l2_mbus_framefmt mbus;
 	struct v4l2_pix_format pix;
-	struct vpfe_fmt *fmt;
+	const struct vpfe_fmt *fmt;
 	int ret;
 
 	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
@@ -1850,7 +1867,7 @@ static int vpfe_queue_setup(struct vb2_queue *vq,
 			    unsigned int sizes[], struct device *alloc_devs[])
 {
 	struct vpfe_device *vpfe = vb2_get_drv_priv(vq);
-	unsigned size = vpfe->fmt.fmt.pix.sizeimage;
+	unsigned int size = vpfe->v_fmt.fmt.pix.sizeimage;
 
 	if (vq->num_buffers + *nbuffers < 3)
 		*nbuffers = 3 - vq->num_buffers;
@@ -1886,12 +1903,12 @@ static int vpfe_buffer_prepare(struct vb2_buffer *vb)
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct vpfe_device *vpfe = vb2_get_drv_priv(vb->vb2_queue);
 
-	vb2_set_plane_payload(vb, 0, vpfe->fmt.fmt.pix.sizeimage);
+	vb2_set_plane_payload(vb, 0, vpfe->v_fmt.fmt.pix.sizeimage);
 
 	if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0))
 		return -EINVAL;
 
-	vbuf->field = vpfe->fmt.fmt.pix.field;
+	vbuf->field = vpfe->v_fmt.fmt.pix.field;
 
 	return 0;
 }
@@ -2116,11 +2133,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);
-	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.sizeimage = vpfe->fmt.fmt.pix.bytesperline *
-						vpfe->fmt.fmt.pix.height;
+	vpfe->v_fmt.fmt.pix.width = r.width;
+	vpfe->v_fmt.fmt.pix.height = r.height;
+	vpfe->v_fmt.fmt.pix.bytesperline =
+		vpfe_ccdc_get_line_length(&vpfe->ccdc);
+	vpfe->v_fmt.fmt.pix.sizeimage = vpfe->v_fmt.fmt.pix.bytesperline *
+						vpfe->v_fmt.fmt.pix.height;
 
 	vpfe_dbg(1, vpfe, "cropped (%d,%d)/%dx%d of %dx%d\n",
 		 r.left, r.top, r.width, r.height, cr.width, cr.height);
@@ -2156,7 +2174,7 @@ static long vpfe_ioctl_default(struct file *file, void *priv,
 			return ret;
 		}
 		ret = vpfe_get_ccdc_image_format(vpfe,
-						 &vpfe->fmt);
+						 &vpfe->v_fmt);
 		if (ret < 0) {
 			vpfe_dbg(2, vpfe,
 				"Invalid image format at CCDC\n");
@@ -2309,7 +2327,7 @@ static int vpfe_probe_complete(struct vpfe_device *vpfe)
 	spin_lock_init(&vpfe->dma_queue_lock);
 	mutex_init(&vpfe->lock);
 
-	vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	vpfe->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 
 	/* set first sub device as current one */
 	vpfe->current_subdev = &vpfe->cfg->sub_devs[0];
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index 6f25750f84e4..64a25bf720e4 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -280,7 +280,8 @@ struct vpfe_device {
 	/* Pointer pointing to next v4l2_buffer */
 	struct vpfe_cap_buffer *next_frm;
 	/* Used to store pixel format */
-	struct v4l2_format fmt;
+	const struct vpfe_fmt *fmt;
+	struct v4l2_format v_fmt;
 	/* Used to store current bytes per pixel based on current format */
 	unsigned int bpp;
 	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 09/13] media: am437x-vpfe: fix function trace debug log
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (7 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-09 16:54   ` Joe Perches
  2019-09-09 16:27 ` [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper Benoit Parrot
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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.

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

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index a8f6cf1d06a0..93f92097602c 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;
 	}
 
@@ -466,7 +466,7 @@ static void vpfe_ccdc_config_ycbcr(struct vpfe_ccdc *ccdc)
 	struct ccdc_params_ycbcr *params = &ccdc->ccdc_cfg.ycbcr;
 	u32 syn_mode;
 
-	vpfe_dbg(3, vpfe, "vpfe_ccdc_config_ycbcr:\n");
+	vpfe_dbg(3, vpfe, "%s:\n", __func__);
 	/*
 	 * first restore the CCDC registers to default values
 	 * This is important since we assume default values to be set in
@@ -598,7 +598,7 @@ 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");
+	vpfe_dbg(3, vpfe, "%s:\n", __func__);
 
 	/* Reset CCDC */
 	vpfe_ccdc_restore_defaults(ccdc);
@@ -700,8 +700,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,7 +987,7 @@ 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(2, vpfe, "%s:\n", __func__);
 
 	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
 		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
@@ -1363,7 +1363,7 @@ static int vpfe_querycap(struct file *file, void  *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 
-	vpfe_dbg(2, vpfe, "vpfe_querycap\n");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	strscpy(cap->driver, VPFE_MODULE_NAME, sizeof(cap->driver));
 	strscpy(cap->card, "TI AM437x VPFE", sizeof(cap->card));
@@ -1409,7 +1409,7 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
 	format->type = vpfe->v_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);
@@ -1425,7 +1425,7 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
 	struct v4l2_subdev_format fmt;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "__vpfe_set_format\n");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	sdinfo = vpfe->current_subdev;
 	if (!sdinfo->sd)
@@ -1459,7 +1459,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	*fmt = vpfe->v_fmt;
 
@@ -1473,8 +1473,7 @@ 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);
+	vpfe_dbg(2, vpfe, "%s: index:%d\n", __func__, f->index);
 
 	sdinfo = vpfe->current_subdev;
 	if (!sdinfo->sd)
@@ -1487,8 +1486,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;
 }
@@ -1499,7 +1498,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	return __vpfe_get_format(vpfe, fmt, &bpp);
 }
@@ -1513,7 +1512,7 @@ static int vpfe_s_fmt(struct file *file, void *priv,
 	unsigned int bpp;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_s_fmt\n");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
@@ -1567,7 +1566,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	const struct vpfe_fmt *fmt;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	/* check for valid format */
 	fmt = find_format_by_pix(vpfe, fsize->pixel_format);
@@ -1601,17 +1600,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,
+	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;
 }
@@ -1676,7 +1675,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	if (vpfe_get_subdev_input_index(vpfe, &subdev, &index,
 					inp->index) < 0) {
@@ -1694,7 +1693,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	return vpfe_get_app_input_index(vpfe, index);
 }
@@ -1708,7 +1707,7 @@ 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);
+	vpfe_dbg(2, vpfe, "%s: index: %d\n", __func__, index);
 
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
@@ -1765,8 +1764,7 @@ 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);
+	vpfe_dbg(2, vpfe, "%s: index: %d\n", __func__, index);
 
 	return vpfe_set_input(vpfe, index);
 }
@@ -1776,7 +1774,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	sdinfo = vpfe->current_subdev;
 	if (!(sdinfo->inputs[0].capabilities & V4L2_IN_CAP_STD))
@@ -1793,7 +1791,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	sdinfo = vpfe->current_subdev;
 	if (!(sdinfo->inputs[0].capabilities & V4L2_IN_CAP_STD))
@@ -1826,7 +1824,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	sdinfo = vpfe->current_subdev;
 	if (sdinfo->inputs[0].capabilities != V4L2_IN_CAP_STD)
@@ -1845,7 +1843,7 @@ static void vpfe_calculate_offsets(struct vpfe_device *vpfe)
 {
 	struct v4l2_rect image_win;
 
-	vpfe_dbg(2, vpfe, "vpfe_calculate_offsets\n");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	vpfe_ccdc_get_image_window(&vpfe->ccdc, &image_win);
 	vpfe->field_off = image_win.height * image_win.width;
@@ -2047,7 +2045,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
 	    vpfe->std_index >= ARRAY_SIZE(vpfe_standards))
@@ -2152,7 +2150,7 @@ 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");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	if (!valid_prio) {
 		vpfe_err(vpfe, "%s device busy\n", __func__);
@@ -2264,7 +2262,7 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
 	bool found = false;
 	int i, j, k;
 
-	vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
+	vpfe_dbg(1, vpfe, "%s:\n", __func__);
 
 	for (i = 0; i < ARRAY_SIZE(vpfe->cfg->asd); i++) {
 		if (vpfe->cfg->asd[i]->match.fwnode ==
@@ -2597,7 +2595,7 @@ static int vpfe_remove(struct platform_device *pdev)
 {
 	struct vpfe_device *vpfe = platform_get_drvdata(pdev);
 
-	vpfe_dbg(2, vpfe, "vpfe_remove\n");
+	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
 	pm_runtime_disable(&pdev->dev);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (8 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 09/13] media: am437x-vpfe: fix function trace debug log Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-09 16:39   ` Joe Perches
  2019-09-09 16:27 ` [Patch 11/13] media: am437x-vpfe: TRY_FMT ioctl is not really trying anything Benoit Parrot
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, Benoit Parrot

print_fourcc helper function was used for debug log the
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
"%4.4s" instead and casting the pointer to the fourcc code
into a (char *).

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

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 93f92097602c..4fb9c8ed7e92 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)
 {
@@ -700,8 +686,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:%4.4s\n",
+		 __func__, ccdc->ccdc_cfg.if_type, (char *)&pixfmt);
 
 	if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) {
 		ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW;
@@ -989,8 +975,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
 
 	vpfe_dbg(2, vpfe, "%s:\n", __func__);
 
-	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
-		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
+	vpfe_dbg(1, vpfe, "pixelformat: %4.4s\n",
+		 (char *)&vpfe->v_fmt.fmt.pix.pixelformat);
 
 	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
 			vpfe->v_fmt.fmt.pix.pixelformat) < 0) {
@@ -1409,9 +1395,9 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
 	format->type = vpfe->v_fmt.type;
 
 	vpfe_dbg(1, vpfe,
-		 "%s: size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
+		 "%s: size %dx%d (%4.4s) bytesperline = %d, size = %d, bpp = %d\n",
 		 __func__, format->fmt.pix.width, format->fmt.pix.height,
-		 print_fourcc(format->fmt.pix.pixelformat),
+		 (char *)&format->fmt.pix.pixelformat,
 		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
 
 	return 0;
@@ -1446,9 +1432,9 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
 	format->type = vpfe->v_fmt.type;
 
 	vpfe_dbg(1, vpfe,
-		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
+		 "%s: size %dx%d (%4.4s) bytesperline = %d, size = %d, bpp = %d\n",
 		 __func__,  format->fmt.pix.width, format->fmt.pix.height,
-		 print_fourcc(format->fmt.pix.pixelformat),
+		 (char *)&format->fmt.pix.pixelformat,
 		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
 
 	return 0;
@@ -1486,8 +1472,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: %4.4s\n",
+		 __func__, f->index, fmt->code, (char *)&fmt->fourcc);
 
 	return 0;
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 11/13] media: am437x-vpfe: TRY_FMT ioctl is not really trying anything
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (9 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-09 16:27 ` [Patch 12/13] media: am437x-vpfe: Remove per bus width static data Benoit Parrot
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 | 334 +++++++++-----------
 drivers/media/platform/am437x/am437x-vpfe.h |   2 -
 2 files changed, 155 insertions(+), 181 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 4fb9c8ed7e92..9759ed398943 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[] = {
 	},
 };
 
-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 const struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
 						  unsigned int code)
@@ -177,62 +179,16 @@ static const 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;
-	const 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)
-{
-	const 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)
@@ -971,6 +927,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(2, vpfe, "%s:\n", __func__);
@@ -985,7 +942,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->fmt);
+	vpfe_ccdc_set_image_window(&vpfe->ccdc, &vpfe->crop, bpp);
 
 	switch (vpfe->v_fmt.fmt.pix.field) {
 	case V4L2_FIELD_INTERLACED:
@@ -1030,7 +988,7 @@ static int vpfe_config_image_format(struct vpfe_device *vpfe,
 				    v4l2_std_id std_id)
 {
 	const struct vpfe_fmt *fmt;
-	struct v4l2_pix_format *pix = &vpfe->v_fmt.fmt.pix;
+	struct v4l2_mbus_framefmt mbus_fmt;
 	int i, ret;
 
 	for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) {
@@ -1052,34 +1010,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->v_fmt, &vpfe->bpp);
+	ret = __subdev_get_format(vpfe, &mbus_fmt);
 	if (ret)
 		return ret;
 
-	fmt = find_format_by_pix(vpfe, pix->pixelformat);
+	fmt = find_format_by_code(vpfe, mbus_fmt.code);
 	if (!fmt) {
-		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s\n",
-			 (char *)&pix->pixelformat);
+		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->v_fmt.fmt.pix, &mbus_fmt);
+	vpfe->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	vpfe->v_fmt.fmt.pix.pixelformat = fmt->fourcc;
+	vpfe_calc_format_size(vpfe, fmt, &vpfe->v_fmt);
 	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);
 }
@@ -1359,83 +1312,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;
+	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sd_fmt.pad = 0;
 
-	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	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);
-	}
+	*fmt = *mbus_fmt;
 
-	format->type = vpfe->v_fmt.type;
-
-	vpfe_dbg(1, vpfe,
-		 "%s: size %dx%d (%4.4s) bytesperline = %d, size = %d, bpp = %d\n",
-		 __func__, format->fmt.pix.width, format->fmt.pix.height,
-		 (char *)&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;
 
-	vpfe_dbg(2, vpfe, "%s:\n", __func__);
+	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sd_fmt.pad = 0;
+	*mbus_fmt = *fmt;
 
-	sdinfo = vpfe->current_subdev;
-	if (!sdinfo->sd)
-		return -EINVAL;
+	ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sd_fmt);
+	if (ret)
+		return ret;
 
-	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt.pad = 0;
+	vpfe_dbg(1, vpfe, "%s %dx%d code:%04X\n", __func__,
+		 fmt->width, fmt->height, fmt->code);
 
-	pix_to_mbus(vpfe, &format->fmt.pix, &fmt.format);
+	return 0;
+}
 
-	ret = v4l2_subdev_call(sdinfo->sd, pad, set_fmt, NULL, &fmt);
-	if (ret)
-		return ret;
+static int vpfe_calc_format_size(struct vpfe_device *vpfe,
+				 const struct vpfe_fmt *fmt,
+				 struct v4l2_format *f)
+{
+	u32 bpp;
 
-	v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
-	mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
+	if (!fmt) {
+		vpfe_dbg(3, vpfe, "No vpfe_fmt provided!\n");
+		return -EINVAL;
+	}
 
-	format->type = vpfe->v_fmt.type;
+	bpp = __get_bytesperpixel(vpfe, fmt);
 
-	vpfe_dbg(1, vpfe,
-		 "%s: size %dx%d (%4.4s) bytesperline = %d, size = %d, bpp = %d\n",
-		 __func__,  format->fmt.pix.width, format->fmt.pix.height,
-		 (char *)&format->fmt.pix.pixelformat,
-		 format->fmt.pix.bytesperline, format->fmt.pix.sizeimage, *bpp);
+	/* 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: %4.4s size: %dx%d bpl:%d img_size:%d\n",
+		 __func__, (char *)&f->fmt.pix.pixelformat,
+		 f->fmt.pix.width, f->fmt.pix.height,
+		 f->fmt.pix.bytesperline, f->fmt.pix.sizeimage);
 
 	return 0;
 }
@@ -1479,14 +1423,62 @@ 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;
 
-	vpfe_dbg(2, vpfe, "%s:\n", __func__);
+	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;
+	}
+
+	f->fmt.pix.field = vpfe->v_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->v_fmt.fmt.pix.width;
+		f->fmt.pix.height =  vpfe->v_fmt.fmt.pix.height;
+	}
 
-	return __vpfe_get_format(vpfe, fmt, &bpp);
+	/*
+	 * Use current colorspace for now, it will get
+	 * updated properly during s_fmt
+	 */
+	f->fmt.pix.colorspace = vpfe->v_fmt.fmt.pix.colorspace;
+	return vpfe_calc_format_size(vpfe, fmt, f);
 }
 
 static int vpfe_s_fmt(struct file *file, void *priv,
@@ -1494,43 +1486,40 @@ static int vpfe_s_fmt(struct file *file, void *priv,
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 	const struct vpfe_fmt *fmt;
-	struct v4l2_format format;
-	unsigned int bpp;
+	struct v4l2_mbus_framefmt mbus_fmt;
 	int ret;
 
-	vpfe_dbg(2, vpfe, "%s:\n", __func__);
-
 	/* If streaming is started, return error */
 	if (vb2_is_busy(&vpfe->buffer_queue)) {
 		vpfe_err(vpfe, "%s device busy\n", __func__);
 		return -EBUSY;
 	}
 
-	ret = __vpfe_get_format(vpfe, &format, &bpp);
-	if (ret)
+	ret = vpfe_try_fmt(file, priv, f);
+	if (ret < 0)
 		return ret;
 
-	if (!cmp_v4l2_format(f, &format)) {
-		/* Sensor format is different from the requested format
-		 * so we need to change it
-		 */
-		ret = __vpfe_set_format(vpfe, f, &bpp);
-		if (ret)
-			return ret;
-	} else /* Just make sure all of the fields are consistent */
-		*f = format;
-
 	fmt = find_format_by_pix(vpfe, f->fmt.pix.pixelformat);
-	if (!fmt) {
-		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s, This should not happen!!\n",
-			 (char *)&f->fmt.pix.pixelformat);
+
+	v4l2_fill_mbus_format(&mbus_fmt, &f->fmt.pix, fmt->code);
+
+	ret = __subdev_set_format(vpfe, &mbus_fmt);
+	if (ret)
+		return ret;
+
+	/* Just double check nothing has gone wrong */
+	if (mbus_fmt.code != fmt->code) {
+		vpfe_dbg(3, vpfe,
+			 "%s subdev changed format on us, this should not happen\n",
+			 __func__);
 		return -EINVAL;
 	}
 
-	/* First detach any IRQ if currently attached */
-	vpfe_detach_irq(vpfe);
-	vpfe->v_fmt = *f;
-	vpfe->bpp = bpp;
+	v4l2_fill_pix_format(&vpfe->v_fmt.fmt.pix, &mbus_fmt);
+	vpfe->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	vpfe->v_fmt.fmt.pix.pixelformat  = fmt->fourcc;
+	vpfe_calc_format_size(vpfe, fmt, &vpfe->v_fmt);
+	*f = vpfe->v_fmt;
 	vpfe->fmt = fmt;
 
 	/* Update the crop window based on found values */
@@ -1546,9 +1535,7 @@ 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;
 	const struct vpfe_fmt *fmt;
 	int ret;
 
@@ -1557,34 +1544,21 @@ static int vpfe_enum_size(struct file *file, void  *priv,
 	/* 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,
@@ -2091,6 +2065,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)) {
@@ -2116,7 +2091,8 @@ 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->fmt);
+	vpfe_ccdc_set_image_window(&vpfe->ccdc, &r, bpp);
 	vpfe->v_fmt.fmt.pix.width = r.width;
 	vpfe->v_fmt.fmt.pix.height = r.height;
 	vpfe->v_fmt.fmt.pix.bytesperline =
diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
index 64a25bf720e4..0d10d2b4d7a2 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.h
+++ b/drivers/media/platform/am437x/am437x-vpfe.h
@@ -282,8 +282,6 @@ struct vpfe_device {
 	/* Used to store pixel format */
 	const struct vpfe_fmt *fmt;
 	struct v4l2_format v_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;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 12/13] media: am437x-vpfe: Remove per bus width static data
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (10 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 11/13] media: am437x-vpfe: TRY_FMT ioctl is not really trying anything Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-13 13:19   ` Hans Verkuil
  2019-09-09 16:27 ` [Patch 13/13] media: am437x-vpfe: Switch to SPDX Licensing Benoit Parrot
  2019-09-10 10:42 ` [Patch 00/13] media: am437x-vpfe: overdue maintenance Hans Verkuil
  13 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, Benoit Parrot

The bus related static data include 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 9759ed398943..9855d4cb1d13 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[] = {
 	{
 		.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 0d10d2b4d7a2..2c9e89395bea 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [Patch 13/13] media: am437x-vpfe: Switch to SPDX Licensing
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (11 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 12/13] media: am437x-vpfe: Remove per bus width static data Benoit Parrot
@ 2019-09-09 16:27 ` Benoit Parrot
  2019-09-10 10:42 ` [Patch 00/13] media: am437x-vpfe: overdue maintenance Hans Verkuil
  13 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 16:27 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, 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 9855d4cb1d13..c3f69566423b 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 2c9e89395bea..2a63e378451f 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


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper
  2019-09-09 16:27 ` [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper Benoit Parrot
@ 2019-09-09 16:39   ` Joe Perches
  2019-09-09 17:12     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Joe Perches @ 2019-09-09 16:39 UTC (permalink / raw)
  To: Benoit Parrot, Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On Mon, 2019-09-09 at 11:27 -0500, Benoit Parrot wrote:
> print_fourcc helper function was used for debug log the
> 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
> "%4.4s" instead and casting the pointer to the fourcc code
> into a (char *).
[]
> diff --git 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,
[]
> @@ -700,8 +686,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:%4.4s\n",
> +		 __func__, ccdc->ccdc_cfg.if_type, (char *)&pixfmt);


To avoid any possible defect in the content of pixfmt, it's
probably better to use vsprintf extension "%4pE", &pixfmt
see: Documentation/core-api/printk-formats.rst

	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;
> @@ -989,8 +975,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
>  
>  	vpfe_dbg(2, vpfe, "%s:\n", __func__);
>  
> -	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
> -		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
> +	vpfe_dbg(1, vpfe, "pixelformat: %4.4s\n",
> +		 (char *)&vpfe->v_fmt.fmt.pix.pixelformat);

	vpfe_dbg(1, vpfe, "pixelformat: %4pE\n",
		 &vpfe->v_fmt.fmt.pix.pixelformat);

etc...



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 09/13] media: am437x-vpfe: fix function trace debug log
  2019-09-09 16:27 ` [Patch 09/13] media: am437x-vpfe: fix function trace debug log Benoit Parrot
@ 2019-09-09 16:54   ` Joe Perches
  2019-09-09 17:14     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Joe Perches @ 2019-09-09 16:54 UTC (permalink / raw)
  To: Benoit Parrot, Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On Mon, 2019-09-09 at 11:27 -0500, Benoit Parrot wrote:
> 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.

Most of these seem to be function tracing comments
that should probably be removed instead.

The generic kernel facility ftrace works well.

> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
[]
> @@ -466,7 +466,7 @@ static void vpfe_ccdc_config_ycbcr(struct vpfe_ccdc *ccdc)
>  	struct ccdc_params_ycbcr *params = &ccdc->ccdc_cfg.ycbcr;
>  	u32 syn_mode;
>  
> -	vpfe_dbg(3, vpfe, "vpfe_ccdc_config_ycbcr:\n");
> +	vpfe_dbg(3, vpfe, "%s:\n", __func__);

Remove this instead

>  	/*
>  	 * first restore the CCDC registers to default values
>  	 * This is important since we assume default values to be set in
> @@ -598,7 +598,7 @@ 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");
> +	vpfe_dbg(3, vpfe, "%s:\n", __func__);

here too, etc...



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper
  2019-09-09 16:39   ` Joe Perches
@ 2019-09-09 17:12     ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 17:12 UTC (permalink / raw)
  To: Joe Perches
  Cc: Hans Verkuil, Prabhakar Lad, linux-media, devicetree, linux-kernel

Joe Perches <joe@perches.com> wrote on Mon [2019-Sep-09 09:39:37 -0700]:
> On Mon, 2019-09-09 at 11:27 -0500, Benoit Parrot wrote:
> > print_fourcc helper function was used for debug log the
> > 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
> > "%4.4s" instead and casting the pointer to the fourcc code
> > into a (char *).
> []
> > diff --git 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,
> []
> > @@ -700,8 +686,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:%4.4s\n",
> > +		 __func__, ccdc->ccdc_cfg.if_type, (char *)&pixfmt);
> 
> 
> To avoid any possible defect in the content of pixfmt, it's
> probably better to use vsprintf extension "%4pE", &pixfmt
> see: Documentation/core-api/printk-formats.rst
> 
> 	vpfe_dbg(1, vpfe, "%s: if_type: %d, pixfmt:%4pE\n",
> 		 __func__, ccdc->ccdc_cfg.if_type, &pixfmt);
> 

Thanks Joe, I was not aware of this feature.
I'll update this patch.

Benoit

> >  
> >  	if (ccdc->ccdc_cfg.if_type == VPFE_RAW_BAYER) {
> >  		ccdc->ccdc_cfg.bayer.pix_fmt = CCDC_PIXFMT_RAW;
> > @@ -989,8 +975,8 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
> >  
> >  	vpfe_dbg(2, vpfe, "%s:\n", __func__);
> >  
> > -	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
> > -		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
> > +	vpfe_dbg(1, vpfe, "pixelformat: %4.4s\n",
> > +		 (char *)&vpfe->v_fmt.fmt.pix.pixelformat);
> 
> 	vpfe_dbg(1, vpfe, "pixelformat: %4pE\n",
> 		 &vpfe->v_fmt.fmt.pix.pixelformat);
> 
> etc...
> 
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 09/13] media: am437x-vpfe: fix function trace debug log
  2019-09-09 16:54   ` Joe Perches
@ 2019-09-09 17:14     ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-09 17:14 UTC (permalink / raw)
  To: Joe Perches
  Cc: Hans Verkuil, Prabhakar Lad, linux-media, devicetree, linux-kernel

Joe Perches <joe@perches.com> wrote on Mon [2019-Sep-09 09:54:56 -0700]:
> On Mon, 2019-09-09 at 11:27 -0500, Benoit Parrot wrote:
> > 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.
> 
> Most of these seem to be function tracing comments
> that should probably be removed instead.
> 
> The generic kernel facility ftrace works well.

Yeah you are probably right, I should just remove them.
My own laziness prevented me earlier... it's always easier to just enable
debug dynamically in the driver then trying to remember how to use ftrace :)
I obviously don't use often enough.

Benoit

> 
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> []
> > @@ -466,7 +466,7 @@ static void vpfe_ccdc_config_ycbcr(struct vpfe_ccdc *ccdc)
> >  	struct ccdc_params_ycbcr *params = &ccdc->ccdc_cfg.ycbcr;
> >  	u32 syn_mode;
> >  
> > -	vpfe_dbg(3, vpfe, "vpfe_ccdc_config_ycbcr:\n");
> > +	vpfe_dbg(3, vpfe, "%s:\n", __func__);
> 
> Remove this instead
> 
> >  	/*
> >  	 * first restore the CCDC registers to default values
> >  	 * This is important since we assume default values to be set in
> > @@ -598,7 +598,7 @@ 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");
> > +	vpfe_dbg(3, vpfe, "%s:\n", __func__);
> 
> here too, etc...
> 
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 00/13] media: am437x-vpfe: overdue maintenance
  2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
                   ` (12 preceding siblings ...)
  2019-09-09 16:27 ` [Patch 13/13] media: am437x-vpfe: Switch to SPDX Licensing Benoit Parrot
@ 2019-09-10 10:42 ` Hans Verkuil
  2019-09-10 16:20   ` Benoit Parrot
  13 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2019-09-10 10:42 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

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(-)
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 00/13] media: am437x-vpfe: overdue maintenance
  2019-09-10 10:42 ` [Patch 00/13] media: am437x-vpfe: overdue maintenance Hans Verkuil
@ 2019-09-10 16:20   ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-10 16:20 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

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(-)
> > 
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config
  2019-09-09 16:27 ` [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config Benoit Parrot
@ 2019-09-13 12:59   ` Hans Verkuil
  2019-09-13 13:24     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2019-09-13 12:59 UTC (permalink / raw)
  To: Benoit Parrot
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, Dave Gerlach

On 9/9/19 6:27 PM, Benoit Parrot wrote:
> 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: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c | 44 ++++++++++-----------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 2b42ba1f5949..ab959d61f9c9 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2653,22 +2653,22 @@ 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;
> +	/* only do full suspend if streaming has started */
> +	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
>  

It's a bit ugly to start a block with an empty line. Can you remove it?

> -	pm_runtime_get_sync(dev);
> -	vpfe_config_enable(ccdc, 1);
> +		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 +2710,19 @@ 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;
> +	/* only do full resume if streaming has started */
> +	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
>  

Ditto.

> -	/* Enable both master and slave clock */
> -	pm_runtime_get_sync(dev);
> -	vpfe_config_enable(ccdc, 1);
> +		/* 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);
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 02/13] media: am437x-vpfe: Fix missing first line
  2019-09-09 16:27 ` [Patch 02/13] media: am437x-vpfe: Fix missing first line Benoit Parrot
@ 2019-09-13 13:00   ` Hans Verkuil
  2019-09-13 13:25     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2019-09-13 13:00 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On 9/9/19 6:27 PM, Benoit Parrot wrote:
> Previous generation of this driver were hard coded to handle
> encoder/decoder were the first line never contains any data and

were -> where

> was therefore always skipped, however when dealing with actual
> camera sensor the first line is always present.

sensor -> sensors

Regards,

	Hans

> 
> Signed-off-by: Benoit Parrot <bparrot@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 ab959d61f9c9..0ecb75bf5abd 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
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one
  2019-09-09 16:27 ` [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one Benoit Parrot
@ 2019-09-13 13:07   ` Hans Verkuil
  2019-09-13 13:29     ` Benoit Parrot
  2019-09-17 16:19     ` Benoit Parrot
  0 siblings, 2 replies; 36+ messages in thread
From: Hans Verkuil @ 2019-09-13 13:07 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On 9/9/19 6:27 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 | 108 ++++++++------------
>  drivers/media/platform/am437x/am437x-vpfe.h |  34 ++++++
>  2 files changed, 74 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index ac759c066d00..e76dc2b3b7b8 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -69,30 +69,6 @@ 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[] = {
>  	{
>  		.fourcc		= V4L2_PIX_FMT_YUYV,
> @@ -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..6f25750f84e4 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.h
> +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> @@ -215,6 +215,37 @@ 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)

Slightly confused: the '@' indicates docbook format, but the '/*'
indicates a regular comment. Either choose '/**' or drop the @.

> + */
> +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.
> + */
> +#define VPFE_MAX_ACTIVE_FMT	10

I recommend adding something like:

#if ARRAY_SIZE(formats) > VPFE_MAX_ACTIVE_FMT
	#error must update VPFE_MAX_ACTIVE_FMT
#endif

to am437x-vpfe.c.

Or something along those lines. Don't rely on just the comment :-)

Regards,

	Hans

> +
>  struct vpfe_device {
>  	/* V4l2 specific parameters */
>  	/* Identifies video device for this channel */
> @@ -252,6 +283,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
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
  2019-09-09 16:27 ` [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt Benoit Parrot
@ 2019-09-13 13:14   ` Hans Verkuil
  2019-09-13 13:32     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2019-09-13 13:14 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On 9/9/19 6:27 PM, Benoit Parrot wrote:
> Keep a reference to the currently selected struct vpfe_fmt * object.
> By doing so we rename the current struct v4l2_format * member from
> fmt to v_fmt.
> The added struct vpfe_fmt * reference is set to "const" so we also
> constify all accesses and related helper functions.

This explains what you do, but not why you do it.

And I think fmt vs v_fmt is *very* confusing naming.

I'd keep fmt as-is, and come up with a different name for the vpfe_fmt
pointer. ref_vpfe_fmt?

Regards,

	Hans

> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c | 88 +++++++++++++--------
>  drivers/media/platform/am437x/am437x-vpfe.h |  3 +-
>  2 files changed, 55 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index e76dc2b3b7b8..a8f6cf1d06a0 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -147,8 +147,8 @@ static int
>  __vpfe_get_format(struct vpfe_device *vpfe,
>  		  struct v4l2_format *format, unsigned int *bpp);
>  
> -static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> -					    unsigned int code)
> +static const struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> +						  unsigned int code)
>  {
>  	struct vpfe_fmt *fmt;
>  	unsigned int k;
> @@ -162,8 +162,8 @@ static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
>  	return NULL;
>  }
>  
> -static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> -					   unsigned int pixelformat)
> +static const struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> +						 unsigned int pixelformat)
>  {
>  	struct vpfe_fmt *fmt;
>  	unsigned int k;
> @@ -184,7 +184,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
>  {
>  	struct vpfe_subdev_info *sdinfo = vpfe->current_subdev;
>  	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
> -	struct vpfe_fmt *fmt;
> +	const struct vpfe_fmt *fmt;
>  
>  	fmt = find_format_by_code(vpfe, mbus->code);
>  	if (WARN_ON(fmt == NULL)) {
> @@ -207,7 +207,7 @@ 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;
> +	const struct vpfe_fmt *fmt;
>  
>  	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
>  	if (!fmt) {
> @@ -990,10 +990,10 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
>  	vpfe_dbg(2, vpfe, "vpfe_config_ccdc_image_format\n");
>  
>  	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
> -		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
> +		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
>  
>  	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
> -			vpfe->fmt.fmt.pix.pixelformat) < 0) {
> +			vpfe->v_fmt.fmt.pix.pixelformat) < 0) {
>  		vpfe_err(vpfe, "couldn't set pix format in ccdc\n");
>  		return -EINVAL;
>  	}
> @@ -1001,7 +1001,7 @@ 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);
>  
> -	switch (vpfe->fmt.fmt.pix.field) {
> +	switch (vpfe->v_fmt.fmt.pix.field) {
>  	case V4L2_FIELD_INTERLACED:
>  		/* do nothing, since it is default */
>  		ret = vpfe_ccdc_set_buftype(
> @@ -1043,7 +1043,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;
> +	const struct vpfe_fmt *fmt;
> +	struct v4l2_pix_format *pix = &vpfe->v_fmt.fmt.pix;
>  	int i, ret;
>  
>  	for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) {
> @@ -1078,10 +1079,18 @@ static int vpfe_config_image_format(struct vpfe_device *vpfe,
>  	else
>  		pix->field = V4L2_FIELD_NONE;
>  
> -	ret = __vpfe_get_format(vpfe, &vpfe->fmt, &vpfe->bpp);
> +	ret = __vpfe_get_format(vpfe, &vpfe->v_fmt, &vpfe->bpp);
>  	if (ret)
>  		return ret;
>  
> +	fmt = find_format_by_pix(vpfe, pix->pixelformat);
> +	if (!fmt) {
> +		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s\n",
> +			 (char *)&pix->pixelformat);
> +		return -EINVAL;
> +	}
> +	vpfe->fmt = fmt;
> +
>  	/* Update the crop window based on found values */
>  	vpfe->crop.width = pix->width;
>  	vpfe->crop.height = pix->height;
> @@ -1227,7 +1236,7 @@ static inline void vpfe_schedule_bottom_field(struct vpfe_device *vpfe)
>  static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe)
>  {
>  	vpfe->cur_frm->vb.vb2_buf.timestamp = ktime_get_ns();
> -	vpfe->cur_frm->vb.field = vpfe->fmt.fmt.pix.field;
> +	vpfe->cur_frm->vb.field = vpfe->v_fmt.fmt.pix.field;
>  	vpfe->cur_frm->vb.sequence = vpfe->sequence++;
>  	vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  	vpfe->cur_frm = vpfe->next_frm;
> @@ -1296,7 +1305,7 @@ static void vpfe_handle_interlaced_irq(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 = vpfe->fmt.fmt.pix.field;
> +	enum v4l2_field field = vpfe->v_fmt.fmt.pix.field;
>  	int intr_status, stopping = vpfe->stopping;
>  
>  	intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS);
> @@ -1397,7 +1406,7 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
>  		mbus_to_pix(vpfe, &mbus_fmt, &format->fmt.pix, bpp);
>  	}
>  
> -	format->type = vpfe->fmt.type;
> +	format->type = vpfe->v_fmt.type;
>  
>  	vpfe_dbg(1, vpfe,
>  		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
> @@ -1434,7 +1443,7 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
>  	v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
>  	mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
>  
> -	format->type = vpfe->fmt.type;
> +	format->type = vpfe->v_fmt.type;
>  
>  	vpfe_dbg(1, vpfe,
>  		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
> @@ -1452,7 +1461,7 @@ static int vpfe_g_fmt(struct file *file, void *priv,
>  
>  	vpfe_dbg(2, vpfe, "vpfe_g_fmt\n");
>  
> -	*fmt = vpfe->fmt;
> +	*fmt = vpfe->v_fmt;
>  
>  	return 0;
>  }
> @@ -1496,9 +1505,10 @@ static int vpfe_try_fmt(struct file *file, void *priv,
>  }
>  
>  static int vpfe_s_fmt(struct file *file, void *priv,
> -		      struct v4l2_format *fmt)
> +		      struct v4l2_format *f)
>  {
>  	struct vpfe_device *vpfe = video_drvdata(file);
> +	const struct vpfe_fmt *fmt;
>  	struct v4l2_format format;
>  	unsigned int bpp;
>  	int ret;
> @@ -1515,25 +1525,32 @@ static int vpfe_s_fmt(struct file *file, void *priv,
>  	if (ret)
>  		return ret;
>  
> -
> -	if (!cmp_v4l2_format(fmt, &format)) {
> +	if (!cmp_v4l2_format(f, &format)) {
>  		/* Sensor format is different from the requested format
>  		 * so we need to change it
>  		 */
> -		ret = __vpfe_set_format(vpfe, fmt, &bpp);
> +		ret = __vpfe_set_format(vpfe, f, &bpp);
>  		if (ret)
>  			return ret;
>  	} else /* Just make sure all of the fields are consistent */
> -		*fmt = format;
> +		*f = format;
> +
> +	fmt = find_format_by_pix(vpfe, f->fmt.pix.pixelformat);
> +	if (!fmt) {
> +		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s, This should not happen!!\n",
> +			 (char *)&f->fmt.pix.pixelformat);
> +		return -EINVAL;
> +	}
>  
>  	/* First detach any IRQ if currently attached */
>  	vpfe_detach_irq(vpfe);
> -	vpfe->fmt = *fmt;
> +	vpfe->v_fmt = *f;
>  	vpfe->bpp = bpp;
> +	vpfe->fmt = fmt;
>  
>  	/* Update the crop window based on found values */
> -	vpfe->crop.width = fmt->fmt.pix.width;
> -	vpfe->crop.height = fmt->fmt.pix.height;
> +	vpfe->crop.width = f->fmt.pix.width;
> +	vpfe->crop.height = f->fmt.pix.height;
>  
>  	/* set image capture parameters in the ccdc */
>  	return vpfe_config_ccdc_image_format(vpfe);
> @@ -1547,7 +1564,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
>  	struct vpfe_subdev_info *sdinfo;
>  	struct v4l2_mbus_framefmt mbus;
>  	struct v4l2_pix_format pix;
> -	struct vpfe_fmt *fmt;
> +	const struct vpfe_fmt *fmt;
>  	int ret;
>  
>  	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
> @@ -1850,7 +1867,7 @@ static int vpfe_queue_setup(struct vb2_queue *vq,
>  			    unsigned int sizes[], struct device *alloc_devs[])
>  {
>  	struct vpfe_device *vpfe = vb2_get_drv_priv(vq);
> -	unsigned size = vpfe->fmt.fmt.pix.sizeimage;
> +	unsigned int size = vpfe->v_fmt.fmt.pix.sizeimage;
>  
>  	if (vq->num_buffers + *nbuffers < 3)
>  		*nbuffers = 3 - vq->num_buffers;
> @@ -1886,12 +1903,12 @@ static int vpfe_buffer_prepare(struct vb2_buffer *vb)
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct vpfe_device *vpfe = vb2_get_drv_priv(vb->vb2_queue);
>  
> -	vb2_set_plane_payload(vb, 0, vpfe->fmt.fmt.pix.sizeimage);
> +	vb2_set_plane_payload(vb, 0, vpfe->v_fmt.fmt.pix.sizeimage);
>  
>  	if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0))
>  		return -EINVAL;
>  
> -	vbuf->field = vpfe->fmt.fmt.pix.field;
> +	vbuf->field = vpfe->v_fmt.fmt.pix.field;
>  
>  	return 0;
>  }
> @@ -2116,11 +2133,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);
> -	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.sizeimage = vpfe->fmt.fmt.pix.bytesperline *
> -						vpfe->fmt.fmt.pix.height;
> +	vpfe->v_fmt.fmt.pix.width = r.width;
> +	vpfe->v_fmt.fmt.pix.height = r.height;
> +	vpfe->v_fmt.fmt.pix.bytesperline =
> +		vpfe_ccdc_get_line_length(&vpfe->ccdc);
> +	vpfe->v_fmt.fmt.pix.sizeimage = vpfe->v_fmt.fmt.pix.bytesperline *
> +						vpfe->v_fmt.fmt.pix.height;
>  
>  	vpfe_dbg(1, vpfe, "cropped (%d,%d)/%dx%d of %dx%d\n",
>  		 r.left, r.top, r.width, r.height, cr.width, cr.height);
> @@ -2156,7 +2174,7 @@ static long vpfe_ioctl_default(struct file *file, void *priv,
>  			return ret;
>  		}
>  		ret = vpfe_get_ccdc_image_format(vpfe,
> -						 &vpfe->fmt);
> +						 &vpfe->v_fmt);
>  		if (ret < 0) {
>  			vpfe_dbg(2, vpfe,
>  				"Invalid image format at CCDC\n");
> @@ -2309,7 +2327,7 @@ static int vpfe_probe_complete(struct vpfe_device *vpfe)
>  	spin_lock_init(&vpfe->dma_queue_lock);
>  	mutex_init(&vpfe->lock);
>  
> -	vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	vpfe->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  
>  	/* set first sub device as current one */
>  	vpfe->current_subdev = &vpfe->cfg->sub_devs[0];
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
> index 6f25750f84e4..64a25bf720e4 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.h
> +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> @@ -280,7 +280,8 @@ struct vpfe_device {
>  	/* Pointer pointing to next v4l2_buffer */
>  	struct vpfe_cap_buffer *next_frm;
>  	/* Used to store pixel format */
> -	struct v4l2_format fmt;
> +	const struct vpfe_fmt *fmt;
> +	struct v4l2_format v_fmt;
>  	/* Used to store current bytes per pixel based on current format */
>  	unsigned int bpp;
>  	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 12/13] media: am437x-vpfe: Remove per bus width static data
  2019-09-09 16:27 ` [Patch 12/13] media: am437x-vpfe: Remove per bus width static data Benoit Parrot
@ 2019-09-13 13:19   ` Hans Verkuil
  2019-09-13 13:34     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2019-09-13 13:19 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On 9/9/19 6:27 PM, Benoit Parrot wrote:
> The bus related static data include in the vpfe_fmt
> static table can be derived dynamically instead.
> This simplify the table and it's use.

simplify -> simplifies
it's -> its

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

bit -> bits

> 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 9759ed398943..9855d4cb1d13 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[] = {
>  	{
>  		.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 0d10d2b4d7a2..2c9e89395bea 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;
>  };
>  
>  /*
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config
  2019-09-13 12:59   ` Hans Verkuil
@ 2019-09-13 13:24     ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-13 13:24 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel, Dave Gerlach

Thanks for the reviews.

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 14:59:28 +0200]:
> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> > 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: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/platform/am437x/am437x-vpfe.c | 44 ++++++++++-----------
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index 2b42ba1f5949..ab959d61f9c9 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -2653,22 +2653,22 @@ 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;
> > +	/* only do full suspend if streaming has started */
> > +	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
> >  
> 
> It's a bit ugly to start a block with an empty line. Can you remove it?

Yep no problem.

> 
> > -	pm_runtime_get_sync(dev);
> > -	vpfe_config_enable(ccdc, 1);
> > +		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 +2710,19 @@ 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;
> > +	/* only do full resume if streaming has started */
> > +	if (vb2_start_streaming_called(&vpfe->buffer_queue)) {
> >  
> 
> Ditto.

Yep no problem.

> 
> > -	/* Enable both master and slave clock */
> > -	pm_runtime_get_sync(dev);
> > -	vpfe_config_enable(ccdc, 1);
> > +		/* 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);
> > 
> 
> Regards,
> 
> 	Hans

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 02/13] media: am437x-vpfe: Fix missing first line
  2019-09-13 13:00   ` Hans Verkuil
@ 2019-09-13 13:25     ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-13 13:25 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:00:08 +0200]:
> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> > Previous generation of this driver were hard coded to handle
> > encoder/decoder were the first line never contains any data and
> 
> were -> where
> 
> > was therefore always skipped, however when dealing with actual
> > camera sensor the first line is always present.
> 
> sensor -> sensors

I'll fix those.

Thanks

Benoit

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Benoit Parrot <bparrot@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 ab959d61f9c9..0ecb75bf5abd 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
> > 
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one
  2019-09-13 13:07   ` Hans Verkuil
@ 2019-09-13 13:29     ` Benoit Parrot
  2019-09-17 16:19     ` Benoit Parrot
  1 sibling, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-13 13:29 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:07:29 +0200]:
> On 9/9/19 6:27 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 | 108 ++++++++------------
> >  drivers/media/platform/am437x/am437x-vpfe.h |  34 ++++++
> >  2 files changed, 74 insertions(+), 68 deletions(-)
> > 
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index ac759c066d00..e76dc2b3b7b8 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -69,30 +69,6 @@ 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[] = {
> >  	{
> >  		.fourcc		= V4L2_PIX_FMT_YUYV,
> > @@ -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..6f25750f84e4 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.h
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> > @@ -215,6 +215,37 @@ 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)
> 
> Slightly confused: the '@' indicates docbook format, but the '/*'
> indicates a regular comment. Either choose '/**' or drop the @.

Yeah I think this driver pre-date all of that.

Now did you mean to fix just the above comments or all of the other ones
also.

Frankly, I do not have a clear idea either way.
I guess i'll just remove the '@' tag then.

> 
> > + */
> > +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.
> > + */
> > +#define VPFE_MAX_ACTIVE_FMT	10
> 
> I recommend adding something like:
> 
> #if ARRAY_SIZE(formats) > VPFE_MAX_ACTIVE_FMT
> 	#error must update VPFE_MAX_ACTIVE_FMT
> #endif
> 
> to am437x-vpfe.c.
> 
> Or something along those lines. Don't rely on just the comment :-)

Ah yes very good.

> 
> Regards,
> 
> 	Hans
> 
> > +
> >  struct vpfe_device {
> >  	/* V4l2 specific parameters */
> >  	/* Identifies video device for this channel */
> > @@ -252,6 +283,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
> > 
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
  2019-09-13 13:14   ` Hans Verkuil
@ 2019-09-13 13:32     ` Benoit Parrot
  2019-09-13 13:34       ` Hans Verkuil
  0 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-13 13:32 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:14:45 +0200]:
> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> > Keep a reference to the currently selected struct vpfe_fmt * object.
> > By doing so we rename the current struct v4l2_format * member from
> > fmt to v_fmt.
> > The added struct vpfe_fmt * reference is set to "const" so we also
> > constify all accesses and related helper functions.
> 
> This explains what you do, but not why you do it.

Hmm ok I'll rework this a bit.

> 
> And I think fmt vs v_fmt is *very* confusing naming.

Well in this case v_fmt stands for "v4l2_fmt" and depending on the function
I was using fmt to either mean a vpfe_fmt pointer or a v4l2_format pointer
so tried (and apparently failed) to make it clearer which was which.

> 
> I'd keep fmt as-is, and come up with a different name for the vpfe_fmt
> pointer. ref_vpfe_fmt?
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/media/platform/am437x/am437x-vpfe.c | 88 +++++++++++++--------
> >  drivers/media/platform/am437x/am437x-vpfe.h |  3 +-
> >  2 files changed, 55 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index e76dc2b3b7b8..a8f6cf1d06a0 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -147,8 +147,8 @@ static int
> >  __vpfe_get_format(struct vpfe_device *vpfe,
> >  		  struct v4l2_format *format, unsigned int *bpp);
> >  
> > -static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> > -					    unsigned int code)
> > +static const struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> > +						  unsigned int code)
> >  {
> >  	struct vpfe_fmt *fmt;
> >  	unsigned int k;
> > @@ -162,8 +162,8 @@ static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
> >  	return NULL;
> >  }
> >  
> > -static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> > -					   unsigned int pixelformat)
> > +static const struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
> > +						 unsigned int pixelformat)
> >  {
> >  	struct vpfe_fmt *fmt;
> >  	unsigned int k;
> > @@ -184,7 +184,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
> >  {
> >  	struct vpfe_subdev_info *sdinfo = vpfe->current_subdev;
> >  	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
> > -	struct vpfe_fmt *fmt;
> > +	const struct vpfe_fmt *fmt;
> >  
> >  	fmt = find_format_by_code(vpfe, mbus->code);
> >  	if (WARN_ON(fmt == NULL)) {
> > @@ -207,7 +207,7 @@ 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;
> > +	const struct vpfe_fmt *fmt;
> >  
> >  	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
> >  	if (!fmt) {
> > @@ -990,10 +990,10 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
> >  	vpfe_dbg(2, vpfe, "vpfe_config_ccdc_image_format\n");
> >  
> >  	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
> > -		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
> > +		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
> >  
> >  	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
> > -			vpfe->fmt.fmt.pix.pixelformat) < 0) {
> > +			vpfe->v_fmt.fmt.pix.pixelformat) < 0) {
> >  		vpfe_err(vpfe, "couldn't set pix format in ccdc\n");
> >  		return -EINVAL;
> >  	}
> > @@ -1001,7 +1001,7 @@ 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);
> >  
> > -	switch (vpfe->fmt.fmt.pix.field) {
> > +	switch (vpfe->v_fmt.fmt.pix.field) {
> >  	case V4L2_FIELD_INTERLACED:
> >  		/* do nothing, since it is default */
> >  		ret = vpfe_ccdc_set_buftype(
> > @@ -1043,7 +1043,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;
> > +	const struct vpfe_fmt *fmt;
> > +	struct v4l2_pix_format *pix = &vpfe->v_fmt.fmt.pix;
> >  	int i, ret;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) {
> > @@ -1078,10 +1079,18 @@ static int vpfe_config_image_format(struct vpfe_device *vpfe,
> >  	else
> >  		pix->field = V4L2_FIELD_NONE;
> >  
> > -	ret = __vpfe_get_format(vpfe, &vpfe->fmt, &vpfe->bpp);
> > +	ret = __vpfe_get_format(vpfe, &vpfe->v_fmt, &vpfe->bpp);
> >  	if (ret)
> >  		return ret;
> >  
> > +	fmt = find_format_by_pix(vpfe, pix->pixelformat);
> > +	if (!fmt) {
> > +		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s\n",
> > +			 (char *)&pix->pixelformat);
> > +		return -EINVAL;
> > +	}
> > +	vpfe->fmt = fmt;
> > +
> >  	/* Update the crop window based on found values */
> >  	vpfe->crop.width = pix->width;
> >  	vpfe->crop.height = pix->height;
> > @@ -1227,7 +1236,7 @@ static inline void vpfe_schedule_bottom_field(struct vpfe_device *vpfe)
> >  static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe)
> >  {
> >  	vpfe->cur_frm->vb.vb2_buf.timestamp = ktime_get_ns();
> > -	vpfe->cur_frm->vb.field = vpfe->fmt.fmt.pix.field;
> > +	vpfe->cur_frm->vb.field = vpfe->v_fmt.fmt.pix.field;
> >  	vpfe->cur_frm->vb.sequence = vpfe->sequence++;
> >  	vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf, VB2_BUF_STATE_DONE);
> >  	vpfe->cur_frm = vpfe->next_frm;
> > @@ -1296,7 +1305,7 @@ static void vpfe_handle_interlaced_irq(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 = vpfe->fmt.fmt.pix.field;
> > +	enum v4l2_field field = vpfe->v_fmt.fmt.pix.field;
> >  	int intr_status, stopping = vpfe->stopping;
> >  
> >  	intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS);
> > @@ -1397,7 +1406,7 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
> >  		mbus_to_pix(vpfe, &mbus_fmt, &format->fmt.pix, bpp);
> >  	}
> >  
> > -	format->type = vpfe->fmt.type;
> > +	format->type = vpfe->v_fmt.type;
> >  
> >  	vpfe_dbg(1, vpfe,
> >  		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
> > @@ -1434,7 +1443,7 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
> >  	v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
> >  	mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
> >  
> > -	format->type = vpfe->fmt.type;
> > +	format->type = vpfe->v_fmt.type;
> >  
> >  	vpfe_dbg(1, vpfe,
> >  		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
> > @@ -1452,7 +1461,7 @@ static int vpfe_g_fmt(struct file *file, void *priv,
> >  
> >  	vpfe_dbg(2, vpfe, "vpfe_g_fmt\n");
> >  
> > -	*fmt = vpfe->fmt;
> > +	*fmt = vpfe->v_fmt;
> >  
> >  	return 0;
> >  }
> > @@ -1496,9 +1505,10 @@ static int vpfe_try_fmt(struct file *file, void *priv,
> >  }
> >  
> >  static int vpfe_s_fmt(struct file *file, void *priv,
> > -		      struct v4l2_format *fmt)
> > +		      struct v4l2_format *f)
> >  {
> >  	struct vpfe_device *vpfe = video_drvdata(file);
> > +	const struct vpfe_fmt *fmt;
> >  	struct v4l2_format format;
> >  	unsigned int bpp;
> >  	int ret;
> > @@ -1515,25 +1525,32 @@ static int vpfe_s_fmt(struct file *file, void *priv,
> >  	if (ret)
> >  		return ret;
> >  
> > -
> > -	if (!cmp_v4l2_format(fmt, &format)) {
> > +	if (!cmp_v4l2_format(f, &format)) {
> >  		/* Sensor format is different from the requested format
> >  		 * so we need to change it
> >  		 */
> > -		ret = __vpfe_set_format(vpfe, fmt, &bpp);
> > +		ret = __vpfe_set_format(vpfe, f, &bpp);
> >  		if (ret)
> >  			return ret;
> >  	} else /* Just make sure all of the fields are consistent */
> > -		*fmt = format;
> > +		*f = format;
> > +
> > +	fmt = find_format_by_pix(vpfe, f->fmt.pix.pixelformat);
> > +	if (!fmt) {
> > +		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s, This should not happen!!\n",
> > +			 (char *)&f->fmt.pix.pixelformat);
> > +		return -EINVAL;
> > +	}
> >  
> >  	/* First detach any IRQ if currently attached */
> >  	vpfe_detach_irq(vpfe);
> > -	vpfe->fmt = *fmt;
> > +	vpfe->v_fmt = *f;
> >  	vpfe->bpp = bpp;
> > +	vpfe->fmt = fmt;
> >  
> >  	/* Update the crop window based on found values */
> > -	vpfe->crop.width = fmt->fmt.pix.width;
> > -	vpfe->crop.height = fmt->fmt.pix.height;
> > +	vpfe->crop.width = f->fmt.pix.width;
> > +	vpfe->crop.height = f->fmt.pix.height;
> >  
> >  	/* set image capture parameters in the ccdc */
> >  	return vpfe_config_ccdc_image_format(vpfe);
> > @@ -1547,7 +1564,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
> >  	struct vpfe_subdev_info *sdinfo;
> >  	struct v4l2_mbus_framefmt mbus;
> >  	struct v4l2_pix_format pix;
> > -	struct vpfe_fmt *fmt;
> > +	const struct vpfe_fmt *fmt;
> >  	int ret;
> >  
> >  	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
> > @@ -1850,7 +1867,7 @@ static int vpfe_queue_setup(struct vb2_queue *vq,
> >  			    unsigned int sizes[], struct device *alloc_devs[])
> >  {
> >  	struct vpfe_device *vpfe = vb2_get_drv_priv(vq);
> > -	unsigned size = vpfe->fmt.fmt.pix.sizeimage;
> > +	unsigned int size = vpfe->v_fmt.fmt.pix.sizeimage;
> >  
> >  	if (vq->num_buffers + *nbuffers < 3)
> >  		*nbuffers = 3 - vq->num_buffers;
> > @@ -1886,12 +1903,12 @@ static int vpfe_buffer_prepare(struct vb2_buffer *vb)
> >  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >  	struct vpfe_device *vpfe = vb2_get_drv_priv(vb->vb2_queue);
> >  
> > -	vb2_set_plane_payload(vb, 0, vpfe->fmt.fmt.pix.sizeimage);
> > +	vb2_set_plane_payload(vb, 0, vpfe->v_fmt.fmt.pix.sizeimage);
> >  
> >  	if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0))
> >  		return -EINVAL;
> >  
> > -	vbuf->field = vpfe->fmt.fmt.pix.field;
> > +	vbuf->field = vpfe->v_fmt.fmt.pix.field;
> >  
> >  	return 0;
> >  }
> > @@ -2116,11 +2133,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);
> > -	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.sizeimage = vpfe->fmt.fmt.pix.bytesperline *
> > -						vpfe->fmt.fmt.pix.height;
> > +	vpfe->v_fmt.fmt.pix.width = r.width;
> > +	vpfe->v_fmt.fmt.pix.height = r.height;
> > +	vpfe->v_fmt.fmt.pix.bytesperline =
> > +		vpfe_ccdc_get_line_length(&vpfe->ccdc);
> > +	vpfe->v_fmt.fmt.pix.sizeimage = vpfe->v_fmt.fmt.pix.bytesperline *
> > +						vpfe->v_fmt.fmt.pix.height;
> >  
> >  	vpfe_dbg(1, vpfe, "cropped (%d,%d)/%dx%d of %dx%d\n",
> >  		 r.left, r.top, r.width, r.height, cr.width, cr.height);
> > @@ -2156,7 +2174,7 @@ static long vpfe_ioctl_default(struct file *file, void *priv,
> >  			return ret;
> >  		}
> >  		ret = vpfe_get_ccdc_image_format(vpfe,
> > -						 &vpfe->fmt);
> > +						 &vpfe->v_fmt);
> >  		if (ret < 0) {
> >  			vpfe_dbg(2, vpfe,
> >  				"Invalid image format at CCDC\n");
> > @@ -2309,7 +2327,7 @@ static int vpfe_probe_complete(struct vpfe_device *vpfe)
> >  	spin_lock_init(&vpfe->dma_queue_lock);
> >  	mutex_init(&vpfe->lock);
> >  
> > -	vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +	vpfe->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >  
> >  	/* set first sub device as current one */
> >  	vpfe->current_subdev = &vpfe->cfg->sub_devs[0];
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
> > index 6f25750f84e4..64a25bf720e4 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.h
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.h
> > @@ -280,7 +280,8 @@ struct vpfe_device {
> >  	/* Pointer pointing to next v4l2_buffer */
> >  	struct vpfe_cap_buffer *next_frm;
> >  	/* Used to store pixel format */
> > -	struct v4l2_format fmt;
> > +	const struct vpfe_fmt *fmt;
> > +	struct v4l2_format v_fmt;
> >  	/* Used to store current bytes per pixel based on current format */
> >  	unsigned int bpp;
> >  	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
> > 
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 12/13] media: am437x-vpfe: Remove per bus width static data
  2019-09-13 13:19   ` Hans Verkuil
@ 2019-09-13 13:34     ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-13 13:34 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:19:52 +0200]:
> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> > The bus related static data include in the vpfe_fmt
> > static table can be derived dynamically instead.
> > This simplify the table and it's use.
> 
> simplify -> simplifies
> it's -> its
> 
> > 
> > We instead replace the per bus data info with just
> > the usual bit per pixel value for each supported
> 
> bit -> bits

I'll fix all of those, thanks...
It's a shame vi ":set spell" does not help trying to figure out what we
meant to say :)

> 
> > 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 9759ed398943..9855d4cb1d13 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[] = {
> >  	{
> >  		.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 0d10d2b4d7a2..2c9e89395bea 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;
> >  };
> >  
> >  /*
> > 
> 
> Regards,
> 
> 	Hans

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
  2019-09-13 13:32     ` Benoit Parrot
@ 2019-09-13 13:34       ` Hans Verkuil
  2019-09-13 13:43         ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2019-09-13 13:34 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

On 9/13/19 3:32 PM, Benoit Parrot wrote:
> Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:14:45 +0200]:
>> On 9/9/19 6:27 PM, Benoit Parrot wrote:
>>> Keep a reference to the currently selected struct vpfe_fmt * object.
>>> By doing so we rename the current struct v4l2_format * member from
>>> fmt to v_fmt.
>>> The added struct vpfe_fmt * reference is set to "const" so we also
>>> constify all accesses and related helper functions.
>>
>> This explains what you do, but not why you do it.
> 
> Hmm ok I'll rework this a bit.
> 
>>
>> And I think fmt vs v_fmt is *very* confusing naming.
> 
> Well in this case v_fmt stands for "v4l2_fmt" and depending on the function
> I was using fmt to either mean a vpfe_fmt pointer or a v4l2_format pointer
> so tried (and apparently failed) to make it clearer which was which.

Well, v_fmt could refer to either v4l2_format or vpfe_fmt, so that prefix
doesn't help me :-)

Since 'fmt' was already defined in struct vpfe_device as v4l2_format, I'd
just keep that rather than causing code churn by changing it. And come up
with a better name for when you refer to a vpfe_fmt struct.

Regards,

	Hans

> 
>>
>> I'd keep fmt as-is, and come up with a different name for the vpfe_fmt
>> pointer. ref_vpfe_fmt?
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>>> ---
>>>  drivers/media/platform/am437x/am437x-vpfe.c | 88 +++++++++++++--------
>>>  drivers/media/platform/am437x/am437x-vpfe.h |  3 +-
>>>  2 files changed, 55 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
>>> index e76dc2b3b7b8..a8f6cf1d06a0 100644
>>> --- a/drivers/media/platform/am437x/am437x-vpfe.c
>>> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
>>> @@ -147,8 +147,8 @@ static int
>>>  __vpfe_get_format(struct vpfe_device *vpfe,
>>>  		  struct v4l2_format *format, unsigned int *bpp);
>>>  
>>> -static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
>>> -					    unsigned int code)
>>> +static const struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
>>> +						  unsigned int code)
>>>  {
>>>  	struct vpfe_fmt *fmt;
>>>  	unsigned int k;
>>> @@ -162,8 +162,8 @@ static struct vpfe_fmt *find_format_by_code(struct vpfe_device *vpfe,
>>>  	return NULL;
>>>  }
>>>  
>>> -static struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
>>> -					   unsigned int pixelformat)
>>> +static const struct vpfe_fmt *find_format_by_pix(struct vpfe_device *vpfe,
>>> +						 unsigned int pixelformat)
>>>  {
>>>  	struct vpfe_fmt *fmt;
>>>  	unsigned int k;
>>> @@ -184,7 +184,7 @@ mbus_to_pix(struct vpfe_device *vpfe,
>>>  {
>>>  	struct vpfe_subdev_info *sdinfo = vpfe->current_subdev;
>>>  	unsigned int bus_width = sdinfo->vpfe_param.bus_width;
>>> -	struct vpfe_fmt *fmt;
>>> +	const struct vpfe_fmt *fmt;
>>>  
>>>  	fmt = find_format_by_code(vpfe, mbus->code);
>>>  	if (WARN_ON(fmt == NULL)) {
>>> @@ -207,7 +207,7 @@ 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;
>>> +	const struct vpfe_fmt *fmt;
>>>  
>>>  	fmt = find_format_by_pix(vpfe, pix_fmt->pixelformat);
>>>  	if (!fmt) {
>>> @@ -990,10 +990,10 @@ static int vpfe_config_ccdc_image_format(struct vpfe_device *vpfe)
>>>  	vpfe_dbg(2, vpfe, "vpfe_config_ccdc_image_format\n");
>>>  
>>>  	vpfe_dbg(1, vpfe, "pixelformat: %s\n",
>>> -		print_fourcc(vpfe->fmt.fmt.pix.pixelformat));
>>> +		print_fourcc(vpfe->v_fmt.fmt.pix.pixelformat));
>>>  
>>>  	if (vpfe_ccdc_set_pixel_format(&vpfe->ccdc,
>>> -			vpfe->fmt.fmt.pix.pixelformat) < 0) {
>>> +			vpfe->v_fmt.fmt.pix.pixelformat) < 0) {
>>>  		vpfe_err(vpfe, "couldn't set pix format in ccdc\n");
>>>  		return -EINVAL;
>>>  	}
>>> @@ -1001,7 +1001,7 @@ 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);
>>>  
>>> -	switch (vpfe->fmt.fmt.pix.field) {
>>> +	switch (vpfe->v_fmt.fmt.pix.field) {
>>>  	case V4L2_FIELD_INTERLACED:
>>>  		/* do nothing, since it is default */
>>>  		ret = vpfe_ccdc_set_buftype(
>>> @@ -1043,7 +1043,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;
>>> +	const struct vpfe_fmt *fmt;
>>> +	struct v4l2_pix_format *pix = &vpfe->v_fmt.fmt.pix;
>>>  	int i, ret;
>>>  
>>>  	for (i = 0; i < ARRAY_SIZE(vpfe_standards); i++) {
>>> @@ -1078,10 +1079,18 @@ static int vpfe_config_image_format(struct vpfe_device *vpfe,
>>>  	else
>>>  		pix->field = V4L2_FIELD_NONE;
>>>  
>>> -	ret = __vpfe_get_format(vpfe, &vpfe->fmt, &vpfe->bpp);
>>> +	ret = __vpfe_get_format(vpfe, &vpfe->v_fmt, &vpfe->bpp);
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	fmt = find_format_by_pix(vpfe, pix->pixelformat);
>>> +	if (!fmt) {
>>> +		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s\n",
>>> +			 (char *)&pix->pixelformat);
>>> +		return -EINVAL;
>>> +	}
>>> +	vpfe->fmt = fmt;
>>> +
>>>  	/* Update the crop window based on found values */
>>>  	vpfe->crop.width = pix->width;
>>>  	vpfe->crop.height = pix->height;
>>> @@ -1227,7 +1236,7 @@ static inline void vpfe_schedule_bottom_field(struct vpfe_device *vpfe)
>>>  static inline void vpfe_process_buffer_complete(struct vpfe_device *vpfe)
>>>  {
>>>  	vpfe->cur_frm->vb.vb2_buf.timestamp = ktime_get_ns();
>>> -	vpfe->cur_frm->vb.field = vpfe->fmt.fmt.pix.field;
>>> +	vpfe->cur_frm->vb.field = vpfe->v_fmt.fmt.pix.field;
>>>  	vpfe->cur_frm->vb.sequence = vpfe->sequence++;
>>>  	vb2_buffer_done(&vpfe->cur_frm->vb.vb2_buf, VB2_BUF_STATE_DONE);
>>>  	vpfe->cur_frm = vpfe->next_frm;
>>> @@ -1296,7 +1305,7 @@ static void vpfe_handle_interlaced_irq(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 = vpfe->fmt.fmt.pix.field;
>>> +	enum v4l2_field field = vpfe->v_fmt.fmt.pix.field;
>>>  	int intr_status, stopping = vpfe->stopping;
>>>  
>>>  	intr_status = vpfe_reg_read(&vpfe->ccdc, VPFE_IRQ_STS);
>>> @@ -1397,7 +1406,7 @@ static int __vpfe_get_format(struct vpfe_device *vpfe,
>>>  		mbus_to_pix(vpfe, &mbus_fmt, &format->fmt.pix, bpp);
>>>  	}
>>>  
>>> -	format->type = vpfe->fmt.type;
>>> +	format->type = vpfe->v_fmt.type;
>>>  
>>>  	vpfe_dbg(1, vpfe,
>>>  		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
>>> @@ -1434,7 +1443,7 @@ static int __vpfe_set_format(struct vpfe_device *vpfe,
>>>  	v4l2_fill_pix_format(&format->fmt.pix, &fmt.format);
>>>  	mbus_to_pix(vpfe, &fmt.format, &format->fmt.pix, bpp);
>>>  
>>> -	format->type = vpfe->fmt.type;
>>> +	format->type = vpfe->v_fmt.type;
>>>  
>>>  	vpfe_dbg(1, vpfe,
>>>  		 "%s size %dx%d (%s) bytesperline = %d, size = %d, bpp = %d\n",
>>> @@ -1452,7 +1461,7 @@ static int vpfe_g_fmt(struct file *file, void *priv,
>>>  
>>>  	vpfe_dbg(2, vpfe, "vpfe_g_fmt\n");
>>>  
>>> -	*fmt = vpfe->fmt;
>>> +	*fmt = vpfe->v_fmt;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -1496,9 +1505,10 @@ static int vpfe_try_fmt(struct file *file, void *priv,
>>>  }
>>>  
>>>  static int vpfe_s_fmt(struct file *file, void *priv,
>>> -		      struct v4l2_format *fmt)
>>> +		      struct v4l2_format *f)
>>>  {
>>>  	struct vpfe_device *vpfe = video_drvdata(file);
>>> +	const struct vpfe_fmt *fmt;
>>>  	struct v4l2_format format;
>>>  	unsigned int bpp;
>>>  	int ret;
>>> @@ -1515,25 +1525,32 @@ static int vpfe_s_fmt(struct file *file, void *priv,
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> -
>>> -	if (!cmp_v4l2_format(fmt, &format)) {
>>> +	if (!cmp_v4l2_format(f, &format)) {
>>>  		/* Sensor format is different from the requested format
>>>  		 * so we need to change it
>>>  		 */
>>> -		ret = __vpfe_set_format(vpfe, fmt, &bpp);
>>> +		ret = __vpfe_set_format(vpfe, f, &bpp);
>>>  		if (ret)
>>>  			return ret;
>>>  	} else /* Just make sure all of the fields are consistent */
>>> -		*fmt = format;
>>> +		*f = format;
>>> +
>>> +	fmt = find_format_by_pix(vpfe, f->fmt.pix.pixelformat);
>>> +	if (!fmt) {
>>> +		vpfe_dbg(3, vpfe, "Invalid pixel code: %4.4s, This should not happen!!\n",
>>> +			 (char *)&f->fmt.pix.pixelformat);
>>> +		return -EINVAL;
>>> +	}
>>>  
>>>  	/* First detach any IRQ if currently attached */
>>>  	vpfe_detach_irq(vpfe);
>>> -	vpfe->fmt = *fmt;
>>> +	vpfe->v_fmt = *f;
>>>  	vpfe->bpp = bpp;
>>> +	vpfe->fmt = fmt;
>>>  
>>>  	/* Update the crop window based on found values */
>>> -	vpfe->crop.width = fmt->fmt.pix.width;
>>> -	vpfe->crop.height = fmt->fmt.pix.height;
>>> +	vpfe->crop.width = f->fmt.pix.width;
>>> +	vpfe->crop.height = f->fmt.pix.height;
>>>  
>>>  	/* set image capture parameters in the ccdc */
>>>  	return vpfe_config_ccdc_image_format(vpfe);
>>> @@ -1547,7 +1564,7 @@ static int vpfe_enum_size(struct file *file, void  *priv,
>>>  	struct vpfe_subdev_info *sdinfo;
>>>  	struct v4l2_mbus_framefmt mbus;
>>>  	struct v4l2_pix_format pix;
>>> -	struct vpfe_fmt *fmt;
>>> +	const struct vpfe_fmt *fmt;
>>>  	int ret;
>>>  
>>>  	vpfe_dbg(2, vpfe, "vpfe_enum_size\n");
>>> @@ -1850,7 +1867,7 @@ static int vpfe_queue_setup(struct vb2_queue *vq,
>>>  			    unsigned int sizes[], struct device *alloc_devs[])
>>>  {
>>>  	struct vpfe_device *vpfe = vb2_get_drv_priv(vq);
>>> -	unsigned size = vpfe->fmt.fmt.pix.sizeimage;
>>> +	unsigned int size = vpfe->v_fmt.fmt.pix.sizeimage;
>>>  
>>>  	if (vq->num_buffers + *nbuffers < 3)
>>>  		*nbuffers = 3 - vq->num_buffers;
>>> @@ -1886,12 +1903,12 @@ static int vpfe_buffer_prepare(struct vb2_buffer *vb)
>>>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>>  	struct vpfe_device *vpfe = vb2_get_drv_priv(vb->vb2_queue);
>>>  
>>> -	vb2_set_plane_payload(vb, 0, vpfe->fmt.fmt.pix.sizeimage);
>>> +	vb2_set_plane_payload(vb, 0, vpfe->v_fmt.fmt.pix.sizeimage);
>>>  
>>>  	if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0))
>>>  		return -EINVAL;
>>>  
>>> -	vbuf->field = vpfe->fmt.fmt.pix.field;
>>> +	vbuf->field = vpfe->v_fmt.fmt.pix.field;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -2116,11 +2133,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);
>>> -	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.sizeimage = vpfe->fmt.fmt.pix.bytesperline *
>>> -						vpfe->fmt.fmt.pix.height;
>>> +	vpfe->v_fmt.fmt.pix.width = r.width;
>>> +	vpfe->v_fmt.fmt.pix.height = r.height;
>>> +	vpfe->v_fmt.fmt.pix.bytesperline =
>>> +		vpfe_ccdc_get_line_length(&vpfe->ccdc);
>>> +	vpfe->v_fmt.fmt.pix.sizeimage = vpfe->v_fmt.fmt.pix.bytesperline *
>>> +						vpfe->v_fmt.fmt.pix.height;
>>>  
>>>  	vpfe_dbg(1, vpfe, "cropped (%d,%d)/%dx%d of %dx%d\n",
>>>  		 r.left, r.top, r.width, r.height, cr.width, cr.height);
>>> @@ -2156,7 +2174,7 @@ static long vpfe_ioctl_default(struct file *file, void *priv,
>>>  			return ret;
>>>  		}
>>>  		ret = vpfe_get_ccdc_image_format(vpfe,
>>> -						 &vpfe->fmt);
>>> +						 &vpfe->v_fmt);
>>>  		if (ret < 0) {
>>>  			vpfe_dbg(2, vpfe,
>>>  				"Invalid image format at CCDC\n");
>>> @@ -2309,7 +2327,7 @@ static int vpfe_probe_complete(struct vpfe_device *vpfe)
>>>  	spin_lock_init(&vpfe->dma_queue_lock);
>>>  	mutex_init(&vpfe->lock);
>>>  
>>> -	vpfe->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +	vpfe->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>>  
>>>  	/* set first sub device as current one */
>>>  	vpfe->current_subdev = &vpfe->cfg->sub_devs[0];
>>> diff --git a/drivers/media/platform/am437x/am437x-vpfe.h b/drivers/media/platform/am437x/am437x-vpfe.h
>>> index 6f25750f84e4..64a25bf720e4 100644
>>> --- a/drivers/media/platform/am437x/am437x-vpfe.h
>>> +++ b/drivers/media/platform/am437x/am437x-vpfe.h
>>> @@ -280,7 +280,8 @@ struct vpfe_device {
>>>  	/* Pointer pointing to next v4l2_buffer */
>>>  	struct vpfe_cap_buffer *next_frm;
>>>  	/* Used to store pixel format */
>>> -	struct v4l2_format fmt;
>>> +	const struct vpfe_fmt *fmt;
>>> +	struct v4l2_format v_fmt;
>>>  	/* Used to store current bytes per pixel based on current format */
>>>  	unsigned int bpp;
>>>  	struct vpfe_fmt	*active_fmt[VPFE_MAX_ACTIVE_FMT];
>>>
>>


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt
  2019-09-13 13:34       ` Hans Verkuil
@ 2019-09-13 13:43         ` Benoit Parrot
  0 siblings, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-13 13:43 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:34:28 +0200]:
> On 9/13/19 3:32 PM, Benoit Parrot wrote:
> > Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:14:45 +0200]:
> >> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> >>> Keep a reference to the currently selected struct vpfe_fmt * object.
> >>> By doing so we rename the current struct v4l2_format * member from
> >>> fmt to v_fmt.
> >>> The added struct vpfe_fmt * reference is set to "const" so we also
> >>> constify all accesses and related helper functions.
> >>
> >> This explains what you do, but not why you do it.
> > 
> > Hmm ok I'll rework this a bit.
> > 
> >>
> >> And I think fmt vs v_fmt is *very* confusing naming.
> > 
> > Well in this case v_fmt stands for "v4l2_fmt" and depending on the function
> > I was using fmt to either mean a vpfe_fmt pointer or a v4l2_format pointer
> > so tried (and apparently failed) to make it clearer which was which.
> 
> Well, v_fmt could refer to either v4l2_format or vpfe_fmt, so that prefix
> doesn't help me :-)
> 
> Since 'fmt' was already defined in struct vpfe_device as v4l2_format, I'd
> just keep that rather than causing code churn by changing it. And come up
> with a better name for when you refer to a vpfe_fmt struct.

Fair enough.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >>
> >> I'd keep fmt as-is, and come up with a different name for the vpfe_fmt
> >> pointer. ref_vpfe_fmt?
> >>
> >> Regards,
> >>
> >> 	Hans
> >>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup
  2019-09-09 16:27 ` [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup Benoit Parrot
@ 2019-09-16  8:00   ` Lad, Prabhakar
  2019-09-16 14:53     ` Benoit Parrot
  0 siblings, 1 reply; 36+ messages in thread
From: Lad, Prabhakar @ 2019-09-16  8:00 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Hans Verkuil, linux-media, devicetree, LKML

Hi Benoit,

Thank you for the patch.

On Mon, Sep 9, 2019 at 5:26 PM Benoit Parrot <bparrot@ti.com> wrote:
>
> 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 | 54 ++++++++++-----------
>  1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 3a8ad9bdf283..52f7fc6e11dd 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,8 @@ 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);

please create a seperate patch for the above change.

Cheers,
--Prabhakar Lad

>         return ret;
>  }
>
> @@ -2022,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);
> @@ -2040,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
>

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup
  2019-09-16  8:00   ` Lad, Prabhakar
@ 2019-09-16 14:53     ` Benoit Parrot
  2019-09-16 16:02       ` Lad, Prabhakar
  0 siblings, 1 reply; 36+ messages in thread
From: Benoit Parrot @ 2019-09-16 14:53 UTC (permalink / raw)
  To: Lad, Prabhakar; +Cc: Hans Verkuil, linux-media, devicetree, LKML

Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote on Mon [2019-Sep-16 09:00:03 +0100]:
> Hi Benoit,
> 
> Thank you for the patch.
> 
> On Mon, Sep 9, 2019 at 5:26 PM Benoit Parrot <bparrot@ti.com> wrote:
> >
> > 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 | 54 ++++++++++-----------
> >  1 file changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index 3a8ad9bdf283..52f7fc6e11dd 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,8 @@ 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);
> 
> please create a seperate patch for the above change.

You mean a separate patch just for the vpfe_pcr_enable() call?

Benoit

> 
> Cheers,
> --Prabhakar Lad
> 
> >         return ret;
> >  }
> >
> > @@ -2022,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);
> > @@ -2040,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
> >

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup
  2019-09-16 14:53     ` Benoit Parrot
@ 2019-09-16 16:02       ` Lad, Prabhakar
  0 siblings, 0 replies; 36+ messages in thread
From: Lad, Prabhakar @ 2019-09-16 16:02 UTC (permalink / raw)
  To: Benoit Parrot; +Cc: Hans Verkuil, linux-media, devicetree, LKML

On Mon, Sep 16, 2019 at 3:51 PM Benoit Parrot <bparrot@ti.com> wrote:
>
> Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote on Mon [2019-Sep-16 09:00:03 +0100]:
> > Hi Benoit,
> >
> > Thank you for the patch.
> >
> > On Mon, Sep 9, 2019 at 5:26 PM Benoit Parrot <bparrot@ti.com> wrote:
> > >
> > > 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 | 54 ++++++++++-----------
> > >  1 file changed, 26 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > > index 3a8ad9bdf283..52f7fc6e11dd 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,8 @@ 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);
> >
> > please create a seperate patch for the above change.
>
> You mean a separate patch just for the vpfe_pcr_enable() call?
>
yes, as the call to vpfe_pcr_enable() is to disable the CCDC and it
doesn't match the patch
description.

Cheers,
--Prabhakar Lad

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one
  2019-09-13 13:07   ` Hans Verkuil
  2019-09-13 13:29     ` Benoit Parrot
@ 2019-09-17 16:19     ` Benoit Parrot
  1 sibling, 0 replies; 36+ messages in thread
From: Benoit Parrot @ 2019-09-17 16:19 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Prabhakar Lad, linux-media, devicetree, linux-kernel

Hans Verkuil <hverkuil@xs4all.nl> wrote on Fri [2019-Sep-13 15:07:29 +0200]:
> On 9/9/19 6:27 PM, Benoit Parrot wrote:
> > +/*
> > + * 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.
> > + */
> > +#define VPFE_MAX_ACTIVE_FMT	10
> 
> I recommend adding something like:
> 
> #if ARRAY_SIZE(formats) > VPFE_MAX_ACTIVE_FMT
> 	#error must update VPFE_MAX_ACTIVE_FMT
> #endif
> 
> to am437x-vpfe.c.
> 
> Or something along those lines. Don't rely on just the comment :-)

I remeber doing this a while back for another driver.
Not sure if you ever treid this or not but "#if ARRAY_SIZE()" construct
does not work because the ARRAY_SIZE() macro which needs to evaluate
sizeof() generates the following compiler error:

In file included from ../include/linux/delay.h:22,
                 from ../drivers/media/platform/am437x/am437x-vpfe.c:23:
../include/linux/kernel.h:47:26: warning: "sizeof" is not defined,
evaluates to 0 [-Wundef]
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
                          __must_be_array(arr))

So no luck there. But I remembered also how I previously fixed it.
In this case if instead of leaving the formats[] definition with empty
brackets you actually used the same defined value like
formats[VPFE_MAX_ACTIVE_FMT] then if you inadvertantly add more enties in
the table then the value of VPFE_MAX_ACTIVE_FMT then you'll get series of
compile time warnings like this:

drivers/media/platform/am437x/am437x-vpfe.c:108:5: warning: excess elements
in array initializer
  }, {
     ^
drivers/media/platform/am437x/am437x-vpfe.c:108:5: note: (near initialization
for ‘formats’)
drivers/media/platform/am437x/am437x-vpfe.c:115:5: warning: excess elements
in array initializer
  }, {

etc...

So this is how I will address this.

Benoit

> 
> Regards,
> 
> 	Hans
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2019-09-17 16:17 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-09 16:27 [Patch 00/13] media: am437x-vpfe: overdue maintenance Benoit Parrot
2019-09-09 16:27 ` [Patch 01/13] media: am437x-vpfe: Fix suspend path to always handle pinctrl config Benoit Parrot
2019-09-13 12:59   ` Hans Verkuil
2019-09-13 13:24     ` Benoit Parrot
2019-09-09 16:27 ` [Patch 02/13] media: am437x-vpfe: Fix missing first line Benoit Parrot
2019-09-13 13:00   ` Hans Verkuil
2019-09-13 13:25     ` Benoit Parrot
2019-09-09 16:27 ` [Patch 03/13] media: am437x-vpfe: Rework ISR routine for clarity Benoit Parrot
2019-09-09 16:27 ` [Patch 04/13] media: am437x-vpfe: Wait for end of frame before tear-down Benoit Parrot
2019-09-09 16:27 ` [Patch 05/13] media: am437x-vpfe: Streamlined vb2 buffer cleanup Benoit Parrot
2019-09-16  8:00   ` Lad, Prabhakar
2019-09-16 14:53     ` Benoit Parrot
2019-09-16 16:02       ` Lad, Prabhakar
2019-09-09 16:27 ` [Patch 06/13] media: am437x-vpfe: Setting STD to current value is not an error Benoit Parrot
2019-09-09 16:27 ` [Patch 07/13] media: am437x-vpfe: Use a per instance format array instead of a static one Benoit Parrot
2019-09-13 13:07   ` Hans Verkuil
2019-09-13 13:29     ` Benoit Parrot
2019-09-17 16:19     ` Benoit Parrot
2019-09-09 16:27 ` [Patch 08/13] media: am437x-vpfe: Maintain a reference to the current vpfe_fmt Benoit Parrot
2019-09-13 13:14   ` Hans Verkuil
2019-09-13 13:32     ` Benoit Parrot
2019-09-13 13:34       ` Hans Verkuil
2019-09-13 13:43         ` Benoit Parrot
2019-09-09 16:27 ` [Patch 09/13] media: am437x-vpfe: fix function trace debug log Benoit Parrot
2019-09-09 16:54   ` Joe Perches
2019-09-09 17:14     ` Benoit Parrot
2019-09-09 16:27 ` [Patch 10/13] media: am437x-vpfe: Remove print_fourcc helper Benoit Parrot
2019-09-09 16:39   ` Joe Perches
2019-09-09 17:12     ` Benoit Parrot
2019-09-09 16:27 ` [Patch 11/13] media: am437x-vpfe: TRY_FMT ioctl is not really trying anything Benoit Parrot
2019-09-09 16:27 ` [Patch 12/13] media: am437x-vpfe: Remove per bus width static data Benoit Parrot
2019-09-13 13:19   ` Hans Verkuil
2019-09-13 13:34     ` Benoit Parrot
2019-09-09 16:27 ` [Patch 13/13] media: am437x-vpfe: Switch to SPDX Licensing Benoit Parrot
2019-09-10 10:42 ` [Patch 00/13] media: am437x-vpfe: overdue maintenance Hans Verkuil
2019-09-10 16:20   ` Benoit Parrot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).