Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751018AbdIKHpH (ORCPT ); Mon, 11 Sep 2017 03:45:07 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:38172 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750937AbdIKHpF (ORCPT ); Mon, 11 Sep 2017 03:45:05 -0400 X-Google-Smtp-Source: ADKCNb5mkDy4+5U2SqcgzVDEyXo4EVF0h1ENXajmHRFhAh9OhbCxB9L2bXrcznCcZqtHAO55ZUubWvKBjuPFRy+vAYE= MIME-Version: 1.0 In-Reply-To: <5fdb0554-51b7-29e3-34ee-d79c46194253@linaro.org> References: <1502199018-28250-1-git-send-email-todor.tomov@linaro.org> <1502199018-28250-13-git-send-email-todor.tomov@linaro.org> <5fdb0554-51b7-29e3-34ee-d79c46194253@linaro.org> From: Geert Uytterhoeven Date: Mon, 11 Sep 2017 09:45:04 +0200 X-Google-Sender-Auth: 99JKOWuqfFI0OdD-tOS6U7s0Tdo Message-ID: Subject: Re: [PATCH v4 12/21] camss: vfe: Format conversion support using PIX interface To: Todor Tomov Cc: Mauro Carvalho Chehab , Hans Verkuil , Sylwester Nawrocki , Sakari Ailus , Linux Media Mailing List , "linux-kernel@vger.kernel.org" , "linux-arm-msm@vger.kernel.org" , Arnd Bergmann Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8B7jDJb003677 Content-Length: 4693 Lines: 127 Hi Todor, On Mon, Sep 11, 2017 at 8:56 AM, Todor Tomov wrote: > On 10.09.2017 12:58, Geert Uytterhoeven wrote: >> On Tue, Aug 8, 2017 at 3:30 PM, Todor Tomov wrote: >>> Use VFE PIX input interface and do format conversion in VFE. >>> >>> Supported input format is UYVY (single plane YUV 4:2:2) and >>> its different sample order variations. >>> >>> Supported output formats are: >>> - NV12/NV21 (two plane YUV 4:2:0) >>> - NV16/NV61 (two plane YUV 4:2:2) >>> >>> Signed-off-by: Todor Tomov >> >> This is now commit 9b5833f7b82f1431 upstream. >> >>> @@ -355,6 +471,38 @@ static void vfe_bus_disconnect_wm_from_rdi(struct vfe_device *vfe, u8 wm, >>> vfe_reg_clr(vfe, VFE_0_BUS_XBAR_CFG_x(wm), reg); >>> } >>> >>> +static void vfe_set_xbar_cfg(struct vfe_device *vfe, struct vfe_output *output, >>> + u8 enable) >>> +{ >>> + struct vfe_line *line = container_of(output, struct vfe_line, output); >>> + u32 p = line->video_out.active_fmt.fmt.pix_mp.pixelformat; >>> + u32 reg; >> >> With gcc 4.1.2: >> >> drivers/media/platform/qcom/camss-8x16/camss-vfe.c: In function >> ‘vfe_set_xbar_cfg’: >> drivers/media/platform/qcom/camss-8x16/camss-vfe.c:614: warning: >> ‘reg’ may be used uninitialized in this function >> >> This is a false positive, as output->wm_num is always either 1 or 2, hence the >> index i can never have a value different from 0 or 1, and reg is thus always >> initialized. >> >>> + unsigned int i; >>> + >>> + for (i = 0; i < output->wm_num; i++) { >>> + if (i == 0) { >>> + reg = VFE_0_BUS_XBAR_CFG_x_M_SINGLE_STREAM_SEL_LUMA << >>> + VFE_0_BUS_XBAR_CFG_x_M_SINGLE_STREAM_SEL_SHIFT; >>> + } else if (i == 1) { >>> + reg = VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_EN; >>> + if (p == V4L2_PIX_FMT_NV12 || p == V4L2_PIX_FMT_NV16) >>> + reg |= VFE_0_BUS_XBAR_CFG_x_M_PAIR_STREAM_SWAP_INTER_INTRA; >>> + } >> >>> @@ -458,6 +728,10 @@ static void vfe_init_outputs(struct vfe_device *vfe) >>> output->buf[0] = NULL; >>> output->buf[1] = NULL; >>> INIT_LIST_HEAD(&output->pending_bufs); >>> + >>> + output->wm_num = 1; >>> + if (vfe->line[i].id == VFE_LINE_PIX) >>> + output->wm_num = 2; >>> } >>> } >>> >> >>> --- a/drivers/media/platform/qcom/camss-8x16/camss-vfe.h >>> +++ b/drivers/media/platform/qcom/camss-8x16/camss-vfe.h >>> @@ -30,8 +30,9 @@ >>> #define MSM_VFE_PAD_SRC 1 >>> #define MSM_VFE_PADS_NUM 2 >>> >>> -#define MSM_VFE_LINE_NUM 3 >>> +#define MSM_VFE_LINE_NUM 4 >>> #define MSM_VFE_IMAGE_MASTERS_NUM 7 >>> +#define MSM_VFE_COMPOSITE_IRQ_NUM 4 >>> >>> #define MSM_VFE_VFE0_UB_SIZE 1023 >>> #define MSM_VFE_VFE0_UB_SIZE_RDI (MSM_VFE_VFE0_UB_SIZE / 3) >>> @@ -51,11 +52,13 @@ enum vfe_line_id { >>> VFE_LINE_NONE = -1, >>> VFE_LINE_RDI0 = 0, >>> VFE_LINE_RDI1 = 1, >>> - VFE_LINE_RDI2 = 2 >>> + VFE_LINE_RDI2 = 2, >>> + VFE_LINE_PIX = 3 >>> }; >>> >>> struct vfe_output { >>> - u8 wm_idx; >>> + u8 wm_num; >>> + u8 wm_idx[3]; >> >> However, wm_idx[] reserves space for 3 entries, while currently only 2 are >> needed. Why? >> >> If this is meant to accommodate for a future extension, the false positive >> will become a real issue. > > The third entry will be needed if we add any three planar pixel format support > to the driver. OK. > If this happens this will involve also changes in > vfe_set_xbar_cfg() to support it. It is fine to change wm_idx[3] to wm_idx[2] > until then. However this will not remove the false positive warning. I suppose > it is best to also change vfe_set_xbar_cfg() now so that there is no warning - > init reg to 0 in all cases? Initializing reg to 0 in all cases will kill the warning, but won't prevent future bugs (e.g. someone forgets to update vfe_set_xbar_cfg()). Keeping the warning also doesn't help to protect against that, as I only look at newly introduced warnings (all old unfixed ones are supposed to be false positives). Well, just don't make mistakes when extending the code ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds