On 1/10/20 6:26 PM, Helen Koike wrote:
>
>
> On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
>> Hello!
>>
>> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
>> <dafna.hirschfeld@xxxxxxxxxxxxx> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 30.12.19 14:59, Helen Koike wrote:
>>>> Hi,
>>>>
>>>> Thanks for the patch, just minor comments below.
>>>>
>>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
>>>>> Modify the scaler subdevice to accept setting the resolution of the source
>>>>> pad (previously the source resolution would always be 3 times the sink for
>>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
>>>>> and the sink video will be scaled to match it.
>>>>>
>>>>> Test example: With the vimc module up (using the default vimc topology)
>>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
>>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
>>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
>>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
>>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
>>>>> --stream-to=test.raw
>>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
>>>>> -video_size "300x700" test.raw
>>>>>
>>>>> Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@xxxxxxxxx>
>>>>> Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@xxxxxxxxx>
>>>>> Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@xxxxxxxxx>
>>>>> Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@xxxxxxxxx>
>>>>> Signed-off-by: Pedro "pirate" Terra <pirate@xxxxxxxxxx>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in V4:
>>>>> * Rebased with media/master
>>>>> * Scaling is now compatible with crop
>>>>> * Updated test example at the commit message
>>>>> * Add vimc prefix to the pad enumeration
>>>>>
>>>>> Changes in V3:
>>>>> * Corrections suggested by Hans:
>>>>> - Default scaling factor is now 1 (we removed the define and
>>>>> set the source format equals the sink).
>>>>> - Removed SCA_COUNT (enum that represents the number of pads)
>>>>> as there always 2
>>>>> - Swapped the per byte pixel copy to memcpy.
>>>>> * Corrections suggested by Dafna:
>>>>> - Removed from the documentation the old scaler parameter which
>>>>> isn't necessary anymore.
>>>>> * Added a thank you note at the end of the email
>>>>>
>>>>> Changes in V2:
>>>>> * Patch was not sent to media list mail for some reason (even though it
>>>>> was on the Cc list), trying again.
>>>>> * Updating documentation.
>>>>>
>>>>> Hello!
>>>>> This code is the result of friends getting together with too much
>>>>> coffee, sugar and beer trying to get started with some kernel coding.
>>>>> Please, don't go easy on us! s2
>>>>>
>>>>> Running
>>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
>>>>> Gave the following result:
>>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
>>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
>>>>> ---
>>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
>>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
>>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
>>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
>>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
>>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
>>>>> @@ -61,9 +61,11 @@ vimc-debayer:
>>>>> * 1 Pad source
>>>>>
>>>>> vimc-scaler:
>>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
>>>>> - 1920x1440 image. (this value can be configured, see at
>>>>> - `Module options`_).
>>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
>>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
>>>>> +to fit the source resolution. Works for any resolution within the vimc
>>>>> +limitations (even shrinking the image if necessary).
>>>>> +
>>>>> Exposes:
>>>>>
>>>>> * 1 Pad sink
>>>>> @@ -76,19 +78,6 @@ vimc-capture:
>>>>> * 1 Pad sink
>>>>> * 1 Pad source
>>>>>
>>>>> -
>>>>> -Module options
>>>>> ---------------
>>>>> -
>>>>> -Vimc has a module parameter to configure the driver.
>>>>> -
>>>>> -* ``sca_mult=<unsigned int>``
>>>>> -
>>>>> - Image size multiplier factor to be used to multiply both width and
>>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
>>>>> - original one. Currently, only supports scaling up (the default value
>>>>> - is 3).
>>>>> -
>>>>> Source code documentation
>>>>> -------------------------
>>>>>
>>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>>>>> index e2e551bc3ded..785009b7ac9e 100644
>>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>>>>> @@ -6,6 +6,7 @@
>>>>> */
>>>>>
>>>>> #include <linux/moduleparam.h>
>>>>> +#include <linux/string.h>
>>>>> #include <linux/vmalloc.h>
>>>>> #include <linux/v4l2-mediabus.h>
>>>>> #include <media/v4l2-rect.h>
>>>>> @@ -13,11 +14,11 @@
>>>>>
>>>>> #include "vimc-common.h"
>>>>>
>>>>> -static unsigned int sca_mult = 3;
>>>>> -module_param(sca_mult, uint, 0000);
>>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>>> -
>>>>> -#define MAX_ZOOM 8
>>>>> +/* Pad identifier */
>>>>> +enum vimc_sca_pad {
>>>>> + VIMC_SCA_SINK = 0,
>>>>> + VIMC_SCA_SRC = 1,
>>>>> +};
>>>>>
>>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
>>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
>>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>>>>> struct vimc_sca_device {
>>>>> struct vimc_ent_device ved;
>>>>> struct v4l2_subdev sd;
>>>>> - /* NOTE: the source fmt is the same as the sink
>>>>> - * with the width and hight multiplied by mult
>>>>> - */
>>>>> - struct v4l2_mbus_framefmt sink_fmt;
>>>>> struct v4l2_rect crop_rect;
>>>>> + /* Frame format for both sink and src pad */
>>>>> + struct v4l2_mbus_framefmt fmt[2];
>>>>> /* Values calculated when the stream starts */
>>>>> u8 *src_frame;
>>>>> - unsigned int src_line_size;
>>>>> unsigned int bpp;
>>>>> struct media_pad pads[2];
>>>>> };
>>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
>>>>> struct v4l2_rect *r;
>>>>> unsigned int i;
>>>>>
>>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
>>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
>>>>> *mf = sink_fmt_default;
>>>>>
>>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
>>>>> *r = crop_rect_default;
>>>>>
>>>>> for (i = 1; i < sd->entity.num_pads; i++) {
>>>>> mf = v4l2_subdev_get_try_format(sd, cfg, i);
>>>>> *mf = sink_fmt_default;
>>>>> - mf->width = mf->width * sca_mult;
>>>>> - mf->height = mf->height * sca_mult;
>>>>> }
>>>>>
>>>>> return 0;
>>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>>>>>
>>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
>>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
>>>>> -
>>>>> - if (VIMC_IS_SINK(fse->pad)) {
>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>> - } else {
>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
>>>>> - }
>>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
>>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>>>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -154,95 +144,73 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
>>>>> struct v4l2_subdev_format *format)
>>>>> {
>>>>> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
>>>>> - struct v4l2_rect *crop_rect;
>>>>>
>>>>> - /* Get the current sink format */
>>>>> - if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>>> - format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
>>>>> - crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
>>>>> - } else {
>>>>> - format->format = vsca->sink_fmt;
>>>>> - crop_rect = &vsca->crop_rect;
>>>>> - }
>>>>> -
>>>>> - /* Scale the frame size for the source pad */
>>>>> - if (VIMC_IS_SRC(format->pad)) {
>>>>> - format->format.width = crop_rect->width * sca_mult;
>>>>> - format->format.height = crop_rect->height * sca_mult;
>>>>> - }
>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
>>>>> + format->pad);
>>>>> + else
>>>>> + format->format = vsca->fmt[format->pad];
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
>>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
>>>>> {
>>>>> - const struct vimc_pix_map *vpix;
>>>>> + if (pad == VIMC_SCA_SINK) {
>>>>> + const struct vimc_pix_map *vpix;
>>>>>
>>>>> - /* Only accept code in the pix map table in non bayer format */
>>>>> - vpix = vimc_pix_map_by_code(fmt->code);
>>>>> - if (!vpix || vpix->bayer)
>>>>> - fmt->code = sink_fmt_default.code;
>>>>> + /* Only accept code in the pix map table in non bayer format */
>>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
>>>>> + if (!vpix || vpix->bayer)
>>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
>>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
>>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
>>>>>
>>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
>>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
>>>>> + }
>>>>> +
>>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
>>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
>>>>
>>>> Could you fix the alignment here?
>>>> For some reason checkpatch doesn't catch this :(
>>>>
>>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
>>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
>>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
>>>>
>>>> Also here.
>>>>
>>>>>
>>>>> - if (fmt->field == V4L2_FIELD_ANY)
>>>>> - fmt->field = sink_fmt_default.field;
>>>>> -
>>>>> - vimc_colorimetry_clamp(fmt);
>>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
>>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
>>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
>>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
>>>>
>>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
>>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
>>>>
>>> Acctually according to the subdevices documentation, when changing the
>>> sink format, the width and height of the src format should reset to the
>>> same values:
>>>
>>> ""
>>> - Sub-devices that scale frames using variable scaling factors should
>>> reset the scale factors to default values when sink pads formats are
>>> modified. If the 1:1 scaling ratio is supported, this means that
>>> source pads formats should be reset to the sink pads formats.
>>> ""
>>
>> I have a small question: Should I worry about the crop? I believe that
>> in the current
>> implementation setting the sink does not necessarily reset the crop zone.
>> Should we reset to the sink resolution or to the one determined by the crop?
>> With that said, the way we implemented the scaller, setting a crop
>> does not affect the
>> source resolution (it retains the sink dimensions), should we change this too?
>
> From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
> have crop in the sink, then the source pad should have dimentions of the crop.
Correct.
>
> At least that is my understanding, and the docs should be updated to make it more
> clear.
Patches are welcome :-)
>
> I would like to hear other people's opinions on this.
I agree with you, this is my understanding as well.
Regards,
Hans
