diff mbox series

[v2,1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup()

Message ID 20240724002044.112544-1-marex@denx.de (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup() | expand

Commit Message

Marek Vasut July 24, 2024, 12:19 a.m. UTC
The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
subsampling, turn it into boolean to select exactly that and update
related code accordingly.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Helge Deller <deller@gmx.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Steve Longerbeam <slongerbeam@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-media@vger.kernel.org
Cc: linux-staging@lists.linux.dev
--
V2: No change
---
 drivers/gpu/ipu-v3/ipu-vdi.c               | 14 +++-----------
 drivers/staging/media/imx/imx-media-vdic.c |  3 +--
 include/video/imx-ipu-v3.h                 |  2 +-
 3 files changed, 5 insertions(+), 14 deletions(-)

Comments

Philipp Zabel Sept. 4, 2024, 9:05 a.m. UTC | #1
On Mi, 2024-07-24 at 02:19 +0200, Marek Vasut wrote:
> The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
> subsampling, turn it into boolean to select exactly that and update
> related code accordingly.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

I'd prefer this to be an enum ipu_chroma_subsampling or similar,
instead of a boolean. Otherwise,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
Marek Vasut Sept. 24, 2024, 10:47 a.m. UTC | #2
On 9/4/24 11:05 AM, Philipp Zabel wrote:
> On Mi, 2024-07-24 at 02:19 +0200, Marek Vasut wrote:
>> The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
>> subsampling, turn it into boolean to select exactly that and update
>> related code accordingly.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> I'd prefer this to be an enum ipu_chroma_subsampling or similar,
> instead of a boolean. Otherwise,

I'm afraid this introduces unnecessary back and forth conversions 
between the boolean and either of the two enum ipu_chroma_subsampling 
values in the code.
Philipp Zabel Sept. 25, 2024, 4:43 p.m. UTC | #3
On Di, 2024-09-24 at 12:47 +0200, Marek Vasut wrote:
> On 9/4/24 11:05 AM, Philipp Zabel wrote:
> > On Mi, 2024-07-24 at 02:19 +0200, Marek Vasut wrote:
> > > The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
> > > subsampling, turn it into boolean to select exactly that and update
> > > related code accordingly.
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > 
> > I'd prefer this to be an enum ipu_chroma_subsampling or similar,
> > instead of a boolean. Otherwise,
> 
> I'm afraid this introduces unnecessary back and forth conversions 
> between the boolean and either of the two enum ipu_chroma_subsampling 
> values in the code.

Fair enough. I dislike the opaque usage in vdic_start() a bit, but with
the in422 variable in ipu_mem2mem_vdic_setup_hardware() it is clear
enough.

regards
Philipp
Marek Vasut Sept. 25, 2024, 7:47 p.m. UTC | #4
On 9/25/24 6:43 PM, Philipp Zabel wrote:
> On Di, 2024-09-24 at 12:47 +0200, Marek Vasut wrote:
>> On 9/4/24 11:05 AM, Philipp Zabel wrote:
>>> On Mi, 2024-07-24 at 02:19 +0200, Marek Vasut wrote:
>>>> The 'code' parameter only ever selects between YUV 4:2:0 and 4:2:2
>>>> subsampling, turn it into boolean to select exactly that and update
>>>> related code accordingly.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>
>>> I'd prefer this to be an enum ipu_chroma_subsampling or similar,
>>> instead of a boolean. Otherwise,
>>
>> I'm afraid this introduces unnecessary back and forth conversions
>> between the boolean and either of the two enum ipu_chroma_subsampling
>> values in the code.
> 
> Fair enough. I dislike the opaque usage in vdic_start() a bit, but with
> the in422 variable in ipu_mem2mem_vdic_setup_hardware() it is clear
> enough.
I'll keep the rework for V3 and then we can decide whether or not to 
undo it.
diff mbox series

Patch

diff --git a/drivers/gpu/ipu-v3/ipu-vdi.c b/drivers/gpu/ipu-v3/ipu-vdi.c
index a593b232b6d3e..4df2821977c0c 100644
--- a/drivers/gpu/ipu-v3/ipu-vdi.c
+++ b/drivers/gpu/ipu-v3/ipu-vdi.c
@@ -117,10 +117,10 @@  void ipu_vdi_set_motion(struct ipu_vdi *vdi, enum ipu_motion_sel motion_sel)
 }
 EXPORT_SYMBOL_GPL(ipu_vdi_set_motion);
 
-void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres)
+void ipu_vdi_setup(struct ipu_vdi *vdi, bool yuv422not420, int xres, int yres)
 {
 	unsigned long flags;
-	u32 pixel_fmt, reg;
+	u32 reg;
 
 	spin_lock_irqsave(&vdi->lock, flags);
 
@@ -131,16 +131,8 @@  void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres)
 	 * Full motion, only vertical filter is used.
 	 * Burst size is 4 accesses
 	 */
-	if (code == MEDIA_BUS_FMT_UYVY8_2X8 ||
-	    code == MEDIA_BUS_FMT_UYVY8_1X16 ||
-	    code == MEDIA_BUS_FMT_YUYV8_2X8 ||
-	    code == MEDIA_BUS_FMT_YUYV8_1X16)
-		pixel_fmt = VDI_C_CH_422;
-	else
-		pixel_fmt = VDI_C_CH_420;
-
 	reg = ipu_vdi_read(vdi, VDI_C);
-	reg |= pixel_fmt;
+	reg |= yuv422not420 ? VDI_C_CH_422 : VDI_C_CH_420;
 	reg |= VDI_C_BURST_SIZE2_4;
 	reg |= VDI_C_BURST_SIZE1_4 | VDI_C_VWM1_CLR_2;
 	reg |= VDI_C_BURST_SIZE3_4 | VDI_C_VWM3_CLR_2;
diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
index 09da4103a8dbe..ea5b4ef3573de 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -376,8 +376,7 @@  static int vdic_start(struct vdic_priv *priv)
 	 * only supports 4:2:2 or 4:2:0, and this subdev will only
 	 * negotiate 4:2:2 at its sink pads.
 	 */
-	ipu_vdi_setup(priv->vdi, MEDIA_BUS_FMT_UYVY8_2X8,
-		      infmt->width, infmt->height);
+	ipu_vdi_setup(priv->vdi, true, infmt->width, infmt->height);
 	ipu_vdi_set_field_order(priv->vdi, V4L2_STD_UNKNOWN, infmt->field);
 	ipu_vdi_set_motion(priv->vdi, priv->motion);
 
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index c422a403c0990..75f435d024895 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -466,7 +466,7 @@  void ipu_ic_dump(struct ipu_ic *ic);
 struct ipu_vdi;
 void ipu_vdi_set_field_order(struct ipu_vdi *vdi, v4l2_std_id std, u32 field);
 void ipu_vdi_set_motion(struct ipu_vdi *vdi, enum ipu_motion_sel motion_sel);
-void ipu_vdi_setup(struct ipu_vdi *vdi, u32 code, int xres, int yres);
+void ipu_vdi_setup(struct ipu_vdi *vdi, bool yuv422not420, int xres, int yres);
 void ipu_vdi_unsetup(struct ipu_vdi *vdi);
 int ipu_vdi_enable(struct ipu_vdi *vdi);
 int ipu_vdi_disable(struct ipu_vdi *vdi);