From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from nasmtp01.atmel.com ([192.199.1.245]:2036 "EHLO DVREDG01.corp.atmel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750807AbbJNGqT (ORCPT ); Wed, 14 Oct 2015 02:46:19 -0400 Subject: Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs To: Guennadi Liakhovetski References: <1442898875-7147-1-git-send-email-josh.wu@atmel.com> <1442898875-7147-2-git-send-email-josh.wu@atmel.com> CC: Linux Media Mailing List , , Laurent Pinchart From: Josh Wu Message-ID: <561DFA36.2070705@atmel.com> Date: Wed, 14 Oct 2015 14:46:14 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Dear Gueannadi, On 10/5/2015 1:04 AM, Guennadi Liakhovetski wrote: > Even simpler: > > On Sun, 4 Oct 2015, Guennadi Liakhovetski wrote: > >> Hi Josh, >> >> On Tue, 22 Sep 2015, Josh Wu wrote: >> >>> we need to configure the YCC_SWAP bits in ISI_CFG2 according to current >>> sensor output and Atmel ISI output format. >>> >>> Current there are two cases Atmel ISI supported: >>> 1. Atmel ISI outputs YUYV format. >>> This case we need to setup YCC_SWAP according to sensor output >>> format. >>> 2. Atmel ISI output a pass-through formats, which means no swap. >>> Just setup YCC_SWAP as default with no swap. >>> >>> Signed-off-by: Josh Wu >>> --- >>> >>> drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++------- >>> 1 file changed, 33 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c >>> index 45e304a..df64294 100644 >>> --- a/drivers/media/platform/soc_camera/atmel-isi.c >>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c >>> @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg) >>> return readl(isi->regs + reg); >>> } >>> >>> +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi, >>> + const struct soc_camera_format_xlate *xlate) >>> +{ >> This function will be called only for the four media codes from the >> switch-case statement below, namely for >> >> MEDIA_BUS_FMT_VYUY8_2X8 >> MEDIA_BUS_FMT_UYVY8_2X8 >> MEDIA_BUS_FMT_YVYU8_2X8 >> MEDIA_BUS_FMT_YUYV8_2X8 >> >>> + /* By default, no swap for the codec path of Atmel ISI. So codec >>> + * output is same as sensor's output. >>> + * For instance, if sensor's output is YUYV, then codec outputs YUYV. >>> + * And if sensor's output is UYVY, then codec outputs UYVY. >>> + */ >>> + u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT; >> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for >> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't >> think this initialisation is really justified. >> >>> + >>> + if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) { >>> + /* all convert to YUYV */ >>> + switch (xlate->code) { >>> + case MEDIA_BUS_FMT_VYUY8_2X8: >>> + cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3; >>> + break; > Why don't you just return the result value here directly? Then you don't > need the variable at all yes, This sounds good. I'll take rid of the cfg2_yuv_swap variable. Thanks. Best Regards, Josh Wu > >>> + case MEDIA_BUS_FMT_UYVY8_2X8: >>> + cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2; >>> + break; >>> + case MEDIA_BUS_FMT_YVYU8_2X8: >>> + cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1; >>> + break; >>> + } >>> + } >>> + >>> + return cfg2_yuv_swap; >>> +} >>> + >>> static void configure_geometry(struct atmel_isi *isi, u32 width, >>> - u32 height, u32 code) >>> + u32 height, const struct soc_camera_format_xlate *xlate) >>> { >>> u32 cfg2; >>> >>> /* According to sensor's output format to set cfg2 */ >>> - switch (code) { >>> + switch (xlate->code) { >>> default: >>> /* Grey */ >>> case MEDIA_BUS_FMT_Y8_1X8: >>> @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width, >>> break; >>> /* YUV */ >>> case MEDIA_BUS_FMT_VYUY8_2X8: >>> - cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr; >>> - break; >>> case MEDIA_BUS_FMT_UYVY8_2X8: >>> - cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr; >>> - break; >>> case MEDIA_BUS_FMT_YVYU8_2X8: >>> - cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr; >>> - break; >>> case MEDIA_BUS_FMT_YUYV8_2X8: >>> - cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr; >>> + cfg2 = ISI_CFG2_COL_SPACE_YCbCr | >>> + setup_cfg2_yuv_swap(isi, xlate); >>> break; >>> /* RGB, TODO */ >>> } >> I would move this switch-case completely inside setup_cfg2_yuv_swap(). >> Just do >> >> cfg2 = setup_cfg2_yuv_swap(isi, xlate); >> >> and handle the >> >> case MEDIA_BUS_FMT_Y8_1X8: >> >> in the function too. These two switch-case statements really look >> redundant. >> >>> @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count) >>> isi_writel(isi, ISI_INTDIS, (u32)~0UL); >>> >>> configure_geometry(isi, icd->user_width, icd->user_height, >>> - icd->current_fmt->code); >>> + icd->current_fmt); >>> >>> spin_lock_irq(&isi->lock); >>> /* Clear any pending interrupt */ >>> -- >>> 1.9.1 >>> >> Thanks >> Guennadi >>