Re: [PATCH v4 12/21] camss: vfe: Format conversion support using PIX interface

[Todor Tomov <todor.tomov@linaro.org>]

Hi Geert,

On 10.09.2017 12:58, Geert Uytterhoeven wrote:
> Hi Todor,
> 
> On Tue, Aug 8, 2017 at 3:30 PM, Todor Tomov <todor.tomov@linaro.org> 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 <todor.tomov@linaro.org>
> 
> 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. 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?

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

-- 
Best regards,
Todor Tomov