lkml.org 
[lkml]   [2017]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 12/21] camss: vfe: Format conversion support using PIX interface
Hi Todor,

On Mon, Sep 11, 2017 at 8:56 AM, Todor Tomov <todor.tomov@linaro.org> wrote:
> On 10.09.2017 12:58, Geert Uytterhoeven wrote:
>> 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.

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

\
 
 \ /
  Last update: 2017-09-11 09:45    [W:0.063 / U:0.752 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site