Hi Kieran,
On Wednesday, 4 April 2018 19:15:19 EEST Kieran Bingham wrote:
> On 02/04/18 13:35, Laurent Pinchart wrote:
>
> <snip>
>
> >>> +/* Setup the output side of the pipeline (WPF and LIF). */
> >>> +static int vsp1_du_pipeline_setup_output(struct vsp1_device *vsp1,
> >>> + struct vsp1_pipeline *pipe)
> >>> +{
> >>> + struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> >>> + struct v4l2_subdev_format format = {
> >>> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>
> >> Why do you initialise this .which here, but all the other member
> >> variables below.
> >>
> >> Wouldn't it make more sense to group all of this initialisation together?
> >> or is there a distinction in keeping the .which separate.
> >>
> >> (Perhaps this is just a way to initialise the rest of the structure to 0,
> >> without using the memset?)
> >
> > The initialization of the .which field is indeed there to avoid the
> > memset, but other than that there's no particular reason. I find it
> > clearer to keep the initialization of the structure close to the code that
> > makes use of it (the next v4l2_subdev_call in this case).
> >
> > As initializing all members when declaring the variable doesn't make a
> > change in code size (gcc 6.4.0) but increases .rodata by 18 bytes and
> > decreases __modver by the same amount, I'm tempted to leave it as-is
> > unless you think it should be changed.
>
> I'm happy to leave it as is - the query was as much to understand why the
> change was the way it was :D
>
> But on that logic (reducing .rodata, or rather not increasing it) what's the
> benefit of initialising with one (random/psuedo random) member variable
> over initialising to all zero, then initialising the .which alongside the
> rest of them? Wouldn't the compiler just use the zero page or such to
> initialise then?
I've just tested that, and it seems to generate the exact same code. I'll
initialize the structure to 0 when declaring it and move the which field
initialization with the other fields.
> This way is fine if you are happy with how it reads :D
>
> >>> + };
> >>> + int ret;
> >>> +
> >>> + format.pad = RWPF_PAD_SINK;
> >>> + format.format.width = drm_pipe->width;
> >>> + format.format.height = drm_pipe->height;
> >>> + format.format.code = MEDIA_BUS_FMT_ARGB8888_1X32;
> >>> + format.format.field = V4L2_FIELD_NONE;
> >>> +
> >>> + ret = v4l2_subdev_call(&pipe->output->entity.subdev, pad, set_fmt,
> >
> > NULL,
> >
> >>> + &format);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
--
Regards,
Laurent Pinchart
