[PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input() - Kernel

This is a discussion on [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input() - Kernel ; From: Márton Németh The cx18_get_input() and ivtv_get_input() are called once from the VIDIOC_ENUMINPUT ioctl() and once from the *_log_status() functions. In the first case the struct v4l2_input is already filled with zeros, so doing this again is unnecessary. The *_log_status() ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input()

  1. [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input()

    From: Márton Németh

    The cx18_get_input() and ivtv_get_input() are called
    once from the VIDIOC_ENUMINPUT ioctl() and once from
    the *_log_status() functions. In the first case the
    struct v4l2_input is already filled with zeros,
    so doing this again is unnecessary.

    The *_log_status() functions are called from
    VIDIOC_LOG_STATUS ioctl() which is only used for
    debug purposes, so it is worth to move the filling
    with zeros to a least frequently used function.

    Signed-off-by: Márton Németh
    ---
    diff -upr linux-2.6.27.orig/drivers/media/video/cx18/cx18-cards.c linux-2.6.27/drivers/media/video/cx18/cx18-cards.c
    --- linux-2.6.27.orig/drivers/media/video/cx18/cx18-cards.c 2008-10-10 00:13:53.000000000 +0200
    +++ linux-2.6.27/drivers/media/video/cx18/cx18-cards.c 2008-10-13 21:27:54.000000000 +0200
    @@ -320,10 +320,9 @@ int cx18_get_input(struct cx18 *cx, u16
    "Composite 3"
    };

    - memset(input, 0, sizeof(*input));
    if (index >= cx->nof_inputs)
    return -EINVAL;
    - input->index = index;
    +
    strlcpy(input->name, input_strs[card_input->video_type - 1],
    sizeof(input->name));
    input->type = (card_input->video_type == CX18_CARD_INPUT_VID_TUNER ?
    diff -upr linux-2.6.27.orig/drivers/media/video/cx18/cx18-ioctl.c linux-2.6.27/drivers/media/video/cx18/cx18-ioctl.c
    --- linux-2.6.27.orig/drivers/media/video/cx18/cx18-ioctl.c 2008-10-10 00:13:53.000000000 +0200
    +++ linux-2.6.27/drivers/media/video/cx18/cx18-ioctl.c 2008-10-13 21:28:11.000000000 +0200
    @@ -712,7 +712,11 @@ static int cx18_log_status(struct file *
    cx18_read_eeprom(cx, &tv);
    }
    cx18_call_i2c_clients(cx, VIDIOC_LOG_STATUS, NULL);
    +
    + memset(&vidin, 0, sizeof(vidin));
    + vidin.index = cx->active_input;
    cx18_get_input(cx, cx->active_input, &vidin);
    +
    cx18_get_audio_input(cx, cx->audio_input, &audin);
    CX18_INFO("Video Input: %s\n", vidin.name);
    CX18_INFO("Audio Input: %s\n", audin.name);
    diff -upr linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-cards.c linux-2.6.27/drivers/media/video/ivtv/ivtv-cards.c
    --- linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-cards.c 2008-10-10 00:13:53.000000000 +0200
    +++ linux-2.6.27/drivers/media/video/ivtv/ivtv-cards.c 2008-10-13 21:22:59.000000000 +0200
    @@ -1199,10 +1199,9 @@ int ivtv_get_input(struct ivtv *itv, u16
    "Composite 3"
    };

    - memset(input, 0, sizeof(*input));
    if (index >= itv->nof_inputs)
    return -EINVAL;
    - input->index = index;
    +
    strlcpy(input->name, input_strs[card_input->video_type - 1],
    sizeof(input->name));
    input->type = (card_input->video_type == IVTV_CARD_INPUT_VID_TUNER ?
    diff -upr linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-ioctl.c linux-2.6.27/drivers/media/video/ivtv/ivtv-ioctl.c
    --- linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-ioctl.c 2008-10-10 00:13:53.000000000 +0200
    +++ linux-2.6.27/drivers/media/video/ivtv/ivtv-ioctl.c 2008-10-13 21:21:35.000000000 +0200
    @@ -1446,7 +1446,11 @@ static int ivtv_log_status(struct file *
    ivtv_read_eeprom(itv, &tv);
    }
    ivtv_call_i2c_clients(itv, VIDIOC_LOG_STATUS, NULL);
    +
    + memset(&vidin, 0, sizeof(vidin));
    + vidin.index = itv->active_input;
    ivtv_get_input(itv, itv->active_input, &vidin);
    +
    ivtv_get_audio_input(itv, itv->audio_input, &audin);
    IVTV_INFO("Video Input: %s\n", vidin.name);
    IVTV_INFO("Audio Input: %s%s\n", audin.name,
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input()

    On Monday 13 October 2008 22:54:06 Németh Márton wrote:
    > From: Márton Németh
    >
    > The cx18_get_input() and ivtv_get_input() are called
    > once from the VIDIOC_ENUMINPUT ioctl() and once from
    > the *_log_status() functions. In the first case the
    > struct v4l2_input is already filled with zeros,
    > so doing this again is unnecessary.


    And in the second case no one cares whether the struct is zeroed. And
    the same situation is also true for ivtv_get_output().

    I would suggest just removing the memset() from ivtv_get_input,
    ivtv_get_output and cx18_get_input.

    Good that you spotted that this memset is no longer needed BTW. I hadn't
    realized that.

    For the record, I NACK this patch, but if you can make a new one that
    just removes the memsets then I'll sign off on that.

    Regards,

    Hans

    > The *_log_status() functions are called from
    > VIDIOC_LOG_STATUS ioctl() which is only used for
    > debug purposes, so it is worth to move the filling
    > with zeros to a least frequently used function.
    >
    > Signed-off-by: Márton Németh
    > ---
    > diff -upr linux-2.6.27.orig/drivers/media/video/cx18/cx18-cards.c
    > linux-2.6.27/drivers/media/video/cx18/cx18-cards.c ---
    > linux-2.6.27.orig/drivers/media/video/cx18/cx18-cards.c 2008-10-10
    > 00:13:53.000000000 +0200 +++
    > linux-2.6.27/drivers/media/video/cx18/cx18-cards.c 2008-10-13
    > 21:27:54.000000000 +0200 @@ -320,10 +320,9 @@ int
    > cx18_get_input(struct cx18 *cx, u16 "Composite 3"
    > };
    >
    > - memset(input, 0, sizeof(*input));
    > if (index >= cx->nof_inputs)
    > return -EINVAL;
    > - input->index = index;
    > +
    > strlcpy(input->name, input_strs[card_input->video_type - 1],
    > sizeof(input->name));
    > input->type = (card_input->video_type == CX18_CARD_INPUT_VID_TUNER
    > ? diff -upr linux-2.6.27.orig/drivers/media/video/cx18/cx18-ioctl.c
    > linux-2.6.27/drivers/media/video/cx18/cx18-ioctl.c ---
    > linux-2.6.27.orig/drivers/media/video/cx18/cx18-ioctl.c 2008-10-10
    > 00:13:53.000000000 +0200 +++
    > linux-2.6.27/drivers/media/video/cx18/cx18-ioctl.c 2008-10-13
    > 21:28:11.000000000 +0200 @@ -712,7 +712,11 @@ static int
    > cx18_log_status(struct file * cx18_read_eeprom(cx, &tv);
    > }
    > cx18_call_i2c_clients(cx, VIDIOC_LOG_STATUS, NULL);
    > +
    > + memset(&vidin, 0, sizeof(vidin));
    > + vidin.index = cx->active_input;
    > cx18_get_input(cx, cx->active_input, &vidin);
    > +
    > cx18_get_audio_input(cx, cx->audio_input, &audin);
    > CX18_INFO("Video Input: %s\n", vidin.name);
    > CX18_INFO("Audio Input: %s\n", audin.name);
    > diff -upr linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-cards.c
    > linux-2.6.27/drivers/media/video/ivtv/ivtv-cards.c ---
    > linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-cards.c 2008-10-10
    > 00:13:53.000000000 +0200 +++
    > linux-2.6.27/drivers/media/video/ivtv/ivtv-cards.c 2008-10-13
    > 21:22:59.000000000 +0200 @@ -1199,10 +1199,9 @@ int
    > ivtv_get_input(struct ivtv *itv, u16 "Composite 3"
    > };
    >
    > - memset(input, 0, sizeof(*input));
    > if (index >= itv->nof_inputs)
    > return -EINVAL;
    > - input->index = index;
    > +
    > strlcpy(input->name, input_strs[card_input->video_type - 1],
    > sizeof(input->name));
    > input->type = (card_input->video_type == IVTV_CARD_INPUT_VID_TUNER
    > ? diff -upr linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-ioctl.c
    > linux-2.6.27/drivers/media/video/ivtv/ivtv-ioctl.c ---
    > linux-2.6.27.orig/drivers/media/video/ivtv/ivtv-ioctl.c 2008-10-10
    > 00:13:53.000000000 +0200 +++
    > linux-2.6.27/drivers/media/video/ivtv/ivtv-ioctl.c 2008-10-13
    > 21:21:35.000000000 +0200 @@ -1446,7 +1446,11 @@ static int
    > ivtv_log_status(struct file * ivtv_read_eeprom(itv, &tv);
    > }
    > ivtv_call_i2c_clients(itv, VIDIOC_LOG_STATUS, NULL);
    > +
    > + memset(&vidin, 0, sizeof(vidin));
    > + vidin.index = itv->active_input;
    > ivtv_get_input(itv, itv->active_input, &vidin);
    > +
    > ivtv_get_audio_input(itv, itv->audio_input, &audin);
    > IVTV_INFO("Video Input: %s\n", vidin.name);
    > IVTV_INFO("Audio Input: %s%s\n", audin.name,
    > --
    > To unsubscribe from this list: send the line "unsubscribe
    > linux-kernel" in the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input()

    Hans Verkuil wrote:
    > On Monday 13 October 2008 22:54:06 Németh Márton wrote:
    >> From: Márton Németh
    >>
    >> The cx18_get_input() and ivtv_get_input() are called
    >> once from the VIDIOC_ENUMINPUT ioctl() and once from
    >> the *_log_status() functions. In the first case the
    >> struct v4l2_input is already filled with zeros,
    >> so doing this again is unnecessary.

    >
    > And in the second case no one cares whether the struct is zeroed. And
    > the same situation is also true for ivtv_get_output().


    Yeah, 'cos there's nothing better than uninitialized fields, like the
    recent report of a control that returns minimum and maximum values of
    zero, but a step-size of 9. Why are we optimizing code paths that are
    not performance critical by uninitializing memory?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input()

    On Monday 13 October 2008 23:40:48 Robert William Fuller wrote:
    > Hans Verkuil wrote:
    > > On Monday 13 October 2008 22:54:06 Németh Márton wrote:
    > >> From: Márton Németh
    > >>
    > >> The cx18_get_input() and ivtv_get_input() are called
    > >> once from the VIDIOC_ENUMINPUT ioctl() and once from
    > >> the *_log_status() functions. In the first case the
    > >> struct v4l2_input is already filled with zeros,
    > >> so doing this again is unnecessary.

    > >
    > > And in the second case no one cares whether the struct is zeroed.
    > > And the same situation is also true for ivtv_get_output().

    >
    > Yeah, 'cos there's nothing better than uninitialized fields, like the
    > recent report of a control that returns minimum and maximum values of
    > zero, but a step-size of 9. Why are we optimizing code paths that
    > are not performance critical by uninitializing memory?


    It's already initialized to 0 in v4l2-ioctl.c. No need to do it twice.

    Regards,

    Hans
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 2/2] video: simplify cx18_get_input() and ivtv_get_input()

    From: Márton Németh

    The cx18_get_input() and ivtv_get_input() are called
    once from the VIDIOC_ENUMINPUT ioctl() (see
    drivers/media/video/v4l2-ioctl.c:1165) and once from
    the *_log_status() functions. In the first case the
    struct v4l2_input is already filled with zeros,
    so doing this again is unnecessary.

    The second case is only used for debugging purposes
    from the VIDIOC_LOG_STATUS ioctl(). Currently only
    the "name" field is used in the *_log_status() functions.

    Signed-off-by: Márton Németh
    ---
    diff -upr linux-2.6.28-rc1.orig/drivers/media/video/cx18/cx18-cards.c linux-2.6.28-rc1/drivers/media/video/cx18/cx18-cards.c
    --- linux-2.6.28-rc1.orig/drivers/media/video/cx18/cx18-cards.c 2008-11-01 20:27:58.000000000 +0100
    +++ linux-2.6.28-rc1/drivers/media/video/cx18/cx18-cards.c 2008-11-01 20:36:39.000000000 +0100
    @@ -419,10 +419,8 @@ int cx18_get_input(struct cx18 *cx, u16
    "Composite 3"
    };

    - memset(input, 0, sizeof(*input));
    if (index >= cx->nof_inputs)
    return -EINVAL;
    - input->index = index;
    strlcpy(input->name, input_strs[card_input->video_type - 1],
    sizeof(input->name));
    input->type = (card_input->video_type == CX18_CARD_INPUT_VID_TUNER ?
    diff -upr linux-2.6.28-rc1.orig/drivers/media/video/ivtv/ivtv-cards.c linux-2.6.28-rc1/drivers/media/video/ivtv/ivtv-cards.c
    --- linux-2.6.28-rc1.orig/drivers/media/video/ivtv/ivtv-cards.c 2008-10-10 00:13:53.000000000 +0200
    +++ linux-2.6.28-rc1/drivers/media/video/ivtv/ivtv-cards.c 2008-11-01 20:37:27.000000000 +0100
    @@ -1199,10 +1199,8 @@ int ivtv_get_input(struct ivtv *itv, u16
    "Composite 3"
    };

    - memset(input, 0, sizeof(*input));
    if (index >= itv->nof_inputs)
    return -EINVAL;
    - input->index = index;
    strlcpy(input->name, input_strs[card_input->video_type - 1],
    sizeof(input->name));
    input->type = (card_input->video_type == IVTV_CARD_INPUT_VID_TUNER ?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread