| Message ID | 20240724002044.112544-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] gpu: ipu-v3: vdic: Simplify ipu_vdi_setup() | expand |
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.
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
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 --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);
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(-)