by Eugen Hristev

[permalink] [raw]

Subject: Re: [PATCH v3 23/23] media: atmel: atmel-isc: change format propagation to subdev into only verification

On 1/20/22 10:43 AM, Eugen Hristev - M18282 wrote:
> On 1/12/22 11:21 AM, Jacopo Mondi wrote:
>> Hi Eugen
>
>
> Hi Jacopo,
>
>>
>> On Mon, Dec 13, 2021 at 03:49:40PM +0200, Eugen Hristev wrote:
>>> As a top MC video driver, the atmel-isc should not propagate the format to the
>>> subdevice.
>>> It should rather check at streamon() time if the subdev is properly configured
>>> with a compatible format.
>>> Removed the whole format finding logic, and reworked the format verification
>>> at streamon time, such that the ISC will return an error if the subdevice
>>> is not properly configured.
>>> With this being done, the module parameter 'sensor_prefered' makes no sense
>>> anymore. The ISC should not decide which format the sensor is using. The
>>> ISC should only cope with the situation and inform userspace if the streaming
>>> is possible in the current configuration.
>>
>> Sounds great!
>>
>>>
>>> Signed-off-by: Eugen Hristev <[email protected]>
>>> ---
>>> Changes in v3:
>>> - clamp to maximum resolution once the frame size from the subdev is found
>>>
>>> drivers/media/platform/atmel/atmel-isc-base.c | 271 ++++++++----------
>>> drivers/media/platform/atmel/atmel-isc.h | 1 +
>>> 2 files changed, 126 insertions(+), 146 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>> index 31c8e3029eee..00c8c9588a78 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>> @@ -36,11 +36,6 @@ static unsigned int debug;
>>> module_param(debug, int, 0644);
>>> MODULE_PARM_DESC(debug, "debug level (0-2)");
>>>
>>> -static unsigned int sensor_preferred = 1;
>>> -module_param(sensor_preferred, uint, 0644);
>>> -MODULE_PARM_DESC(sensor_preferred,
>>> - "Sensor is preferred to output the specified format (1-on 0-off), default 1");
>>> -
>>> #define ISC_IS_FORMAT_RAW(mbus_code) \
>>> (((mbus_code) & 0xf000) == 0x3000)
>>>
>>> @@ -532,7 +527,7 @@ static int isc_enum_fmt_vid_cap(struct file *file, void *priv,
>>> * convert it to any of the formats that we usually can with a
>>> * RAW sensor. Thus, do not advertise them.
>>> */
>>> - if (!isc->config.sd_format ||
>>> + if (isc->config.sd_format &&
>>
>> Is this change intentional ?
>>
>>> !ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
>>> return -EINVAL;
>>>
>>> @@ -621,20 +616,30 @@ static int isc_try_validate_formats(struct isc_device *isc)
>>> break;
>>> default:
>>> /* any other different formats are not supported */
>>> + v4l2_err(&isc->v4l2_dev, "Requested unsupported format.\n");
>>> ret = -EINVAL;
>>> }
>>> v4l2_dbg(1, debug, &isc->v4l2_dev,
>>> "Format validation, requested rgb=%u, yuv=%u, grey=%u, bayer=%u\n",
>>> rgb, yuv, grey, bayer);
>>
>> Would it make sense to move this before the switch so that the
>> error messages, if any, appear later ?
>
> Actually, no, because the variables rgb, yuv, grey, bayer, are set
> according to what happens in the switch
>>
>>>
>>> - /* we cannot output RAW if we do not receive RAW */
>>> - if ((bayer) && !ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code))
>>> + if ((bayer) &&
>>> + !ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code)) {
>>> + v4l2_err(&isc->v4l2_dev, "Cannot output RAW if we do not receive RAW.\n");
>>> return -EINVAL;
>>> + }
>>>
>>> - /* we cannot output GREY if we do not receive RAW/GREY */
>>> if (grey && !ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code) &&
>>> - !ISC_IS_FORMAT_GREY(isc->try_config.sd_format->mbus_code))
>>> + !ISC_IS_FORMAT_GREY(isc->try_config.sd_format->mbus_code)) {
>>> + v4l2_err(&isc->v4l2_dev, "Cannot output GREY if we do not receive RAW/GREY.\n");
>>> return -EINVAL;
>>> + }
>>> +
>>> + if ((rgb || bayer || yuv) &&
>>> + ISC_IS_FORMAT_GREY(isc->try_config.sd_format->mbus_code)) {
>>> + v4l2_err(&isc->v4l2_dev, "Cannot convert GREY to another format.\n");
>>> + return -EINVAL;
>>> + }
>>>
>>> return ret;
>>> }
>>> @@ -862,7 +867,7 @@ static void isc_try_fse(struct isc_device *isc,
>>> * If we do not know yet which format the subdev is using, we cannot
>>> * do anything.
>>> */
>>> - if (!isc->try_config.sd_format)
>>> + if (!isc->config.sd_format)
>>> return;
>>>
>>> fse.code = isc->try_config.sd_format->mbus_code;
>>> @@ -883,180 +888,141 @@ static void isc_try_fse(struct isc_device *isc,
>>> }
>>> }
>>>
>>> -static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
>>> - u32 *code)
>>> +static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f)
>>> {
>>> - int i;
>>> - struct isc_format *sd_fmt = NULL, *direct_fmt = NULL;
>>> struct v4l2_pix_format *pixfmt = &f->fmt.pix;
>>> - struct v4l2_subdev_pad_config pad_cfg = {};
>>> - struct v4l2_subdev_state pad_state = {
>>> - .pads = &pad_cfg
>>> - };
>>> - struct v4l2_subdev_format format = {
>>> - .which = V4L2_SUBDEV_FORMAT_TRY,
>>> - };
>>> - u32 mbus_code;
>>> - int ret;
>>> - bool rlp_dma_direct_dump = false;
>>> + unsigned int i;
>>>
>>> if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>> return -EINVAL;
>>>
>>> - /* Step 1: find a RAW format that is supported */
>>> - for (i = 0; i < isc->num_user_formats; i++) {
>>> - if (ISC_IS_FORMAT_RAW(isc->user_formats[i]->mbus_code)) {
>>> - sd_fmt = isc->user_formats[i];
>>> + isc->try_config.fourcc = isc->user_formats[0]->fourcc;
>>> +
>>> + /* find if the format requested is supported */
>>> + for (i = 0; i < isc->controller_formats_size; i++)
>>> + if (isc->controller_formats[i].fourcc == pixfmt->pixelformat) {
>>> + isc->try_config.fourcc = pixfmt->pixelformat;
>>> break;
>>> }
>>> - }
>>> - /* Step 2: We can continue with this RAW format, or we can look
>>> - * for better: maybe sensor supports directly what we need.
>>> - */
>>> - direct_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
>>> -
>>> - /* Step 3: We have both. We decide given the module parameter which
>>> - * one to use.
>>> - */
>>> - if (direct_fmt && sd_fmt && sensor_preferred)
>>> - sd_fmt = direct_fmt;
>>> -
>>> - /* Step 4: we do not have RAW but we have a direct format. Use it. */
>>> - if (direct_fmt && !sd_fmt)
>>> - sd_fmt = direct_fmt;
>>> -
>>> - /* Step 5: if we are using a direct format, we need to package
>>> - * everything as 8 bit data and just dump it
>>> - */
>>> - if (sd_fmt == direct_fmt)
>>> - rlp_dma_direct_dump = true;
>>> -
>>> - /* Step 6: We have no format. This can happen if the userspace
>>> - * requests some weird/invalid format.
>>> - * In this case, default to whatever we have
>>> - */
>>> - if (!sd_fmt && !direct_fmt) {
>>> - sd_fmt = isc->user_formats[isc->num_user_formats - 1];
>>> - v4l2_dbg(1, debug, &isc->v4l2_dev,
>>> - "Sensor not supporting %.4s, using %.4s\n",
>>> - (char *)&pixfmt->pixelformat, (char *)&sd_fmt->fourcc);
>>> - }
>>> -
>>> - if (!sd_fmt) {
>>> - ret = -EINVAL;
>>> - goto isc_try_fmt_err;
>>> - }
>>> -
>>> - /* Step 7: Print out what we decided for debugging */
>>> - v4l2_dbg(1, debug, &isc->v4l2_dev,
>>> - "Preferring to have sensor using format %.4s\n",
>>> - (char *)&sd_fmt->fourcc);
>>> -
>>> - /* Step 8: at this moment we decided which format the subdev will use */
>>> - isc->try_config.sd_format = sd_fmt;
>>> -
>>> - /* Limit to Atmel ISC hardware capabilities */
>>> - if (pixfmt->width > isc->max_width)
>>> - pixfmt->width = isc->max_width;
>>> - if (pixfmt->height > isc->max_height)
>>> - pixfmt->height = isc->max_height;
>>> -
>>> - /*
>>> - * The mbus format is the one the subdev outputs.
>>> - * The pixels will be transferred in this format Sensor -> ISC
>>> - */
>>> - mbus_code = sd_fmt->mbus_code;
>>> -
>>> - /*
>>> - * Validate formats. If the required format is not OK, default to raw.
>>> - */
>>> -
>>> - isc->try_config.fourcc = pixfmt->pixelformat;
>>> -
>>> - if (isc_try_validate_formats(isc)) {
>>> - pixfmt->pixelformat = isc->try_config.fourcc = sd_fmt->fourcc;
>>> - /* Re-try to validate the new format */
>>> - ret = isc_try_validate_formats(isc);
>>> - if (ret)
>>> - goto isc_try_fmt_err;
>>> - }
>>> -
>>> - ret = isc_try_configure_rlp_dma(isc, rlp_dma_direct_dump);
>>> - if (ret)
>>> - goto isc_try_fmt_err;
>>> -
>>> - ret = isc_try_configure_pipeline(isc);
>>> - if (ret)
>>> - goto isc_try_fmt_err;
>>>
>>> - /* Obtain frame sizes if possible to have crop requirements ready */
>>> - isc_try_fse(isc, &pad_state);
>>> -
>>> - v4l2_fill_mbus_format(&format.format, pixfmt, mbus_code);
>>> - ret = v4l2_subdev_call(isc->current_subdev->sd, pad, set_fmt,
>>> - &pad_state, &format);
>>> - if (ret < 0)
>>> - goto isc_try_fmt_subdev_err;
>>> + /* If we did not find the requested format, we will fallback here */
>>> + pixfmt->pixelformat = isc->try_config.fourcc;
>>> + pixfmt->colorspace = V4L2_COLORSPACE_SRGB;
>>> + pixfmt->field = V4L2_FIELD_NONE;
>>>
>>> - v4l2_fill_pix_format(pixfmt, &format.format);
>>> + isc_try_configure_rlp_dma(isc, false);
>>>
>>> /* Limit to Atmel ISC hardware capabilities */
>>> - if (pixfmt->width > isc->max_width)
>>> - pixfmt->width = isc->max_width;
>>> - if (pixfmt->height > isc->max_height)
>>> - pixfmt->height = isc->max_height;
>>> + v4l_bound_align_image(&pixfmt->width, 16, isc->max_width, 0,
>>> + &pixfmt->height, 16, isc->max_height, 0, 0);
>>>
>>> pixfmt->field = V4L2_FIELD_NONE;
>>> pixfmt->bytesperline = (pixfmt->width * isc->try_config.bpp_v4l2) >> 3;
>>> pixfmt->sizeimage = ((pixfmt->width * isc->try_config.bpp) >> 3) *
>>> pixfmt->height;
>>>
>>> - if (code)
>>> - *code = mbus_code;
>>> + isc->try_fmt = *f;
>>>
>>> return 0;
>>> +}
>>>
>>> -isc_try_fmt_err:
>>> - v4l2_err(&isc->v4l2_dev, "Could not find any possible format for a working pipeline\n");
>>> -isc_try_fmt_subdev_err:
>>> - memset(&isc->try_config, 0, sizeof(isc->try_config));
>>> +static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>>> +{
>>> + isc_try_fmt(isc, f);
>>>
>>> - return ret;
>>> + /* make the try configuration active */
>>> + isc->config = isc->try_config;
>>> + isc->fmt = isc->try_fmt;
>>> +
>>> + v4l2_dbg(1, debug, &isc->v4l2_dev, "ISC set_fmt to %.4s @%dx%d\n",
>>> + (char *)&f->fmt.pix.pixelformat,
>>> + f->fmt.pix.width, f->fmt.pix.height);
>>> +
>>> + return 0;
>>> }
>>>
>>> -static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>>> +static int isc_validate(struct isc_device *isc)
>>> {
>>> + int ret;
>>> + int i;
>>> + struct isc_format *sd_fmt = NULL;
>>> + struct v4l2_pix_format *pixfmt = &isc->fmt.fmt.pix;
>>> struct v4l2_subdev_format format = {
>>> .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>> + .pad = isc->remote_pad,
>>> + };
>>> + struct v4l2_subdev_pad_config pad_cfg = {};
>>> + struct v4l2_subdev_state pad_state = {
>>> + .pads = &pad_cfg,
>>> };
>>> - u32 mbus_code = 0;
>>> - int ret;
>>>
>>> - ret = isc_try_fmt(isc, f, &mbus_code);
>>> + /* Get current format from subdev */
>>> + ret = v4l2_subdev_call(isc->current_subdev->sd, pad, get_fmt, NULL,
>>> + &format);
>>
>> Ah! Haven't you just said we don't care anymore about the subdev
>> format ? :)
>
> We don't ! but we can't stream anything if the sensor streams a format
> that we don't understand do we ?

Actually, thinking about this, we really need to know what format is at
the input for the ISC. According to this format, we need to understand
how to process it, since the ISC could have at input either Raw bayer
and we need to know the rotation, or directly a non-raw format like yuv422 .

Rather than asking the subdev for the streaming format, we could obtain
this information from the pad link ?

>
>>
>> Kidding, I might got a bit lost in the logic, but if I look at your
>> above isc_try_validate_formats() it seems like an ideal candidate for
>> .link_validate() media_entity operation.
>>
>> Just to make sure we're on the same page, here's how it should ideally
>> look like:
>> - set format does care about subdev format. It only checks that the
>> format required from the user is one of the ISC supported one. Ie.
>> no v4l2_subdev_call()
>>
>> - as s_stream time your top driver calls media_pipeline_start()
>>
>> - media_pipeline_start() walks all the entities in the pipeline and
>> validates the format of connected pads. To validate formats the
>> __media_pipeline_start() functions calls link_validate() on each
>> entity. You should in your driver set
>
> Is this done automatically ? if this would be the case, where all links
> are correctly set, then, in theory, it would not be possible that some
> entities have a format set that is not supported by the other side
>
>>
>> static const struct media_entity_operations your_media_entity_ops = {
>> .link_validate = v4l2_subdev_link_validate,
>> };
>>
>> if you want to use the default link validation procedure, or set the
>> callback to your custom validation function, which can behave more
>> or less like isc_try_validate_formats()
>
> Does this mean that I can ask the subdev for the format ?
> Or how can I obtain the format on the other side of the link ?
> link validate can do this for me ?
>
>
> Thanks for reviewing,
> Eugen
>>
>> Does it match your understanding too ?
>>
>> Thanks
>> j
>>
>>
>>
>>> if (ret)
>>> return ret;
>>>
>>> - v4l2_fill_mbus_format(&format.format, &f->fmt.pix, mbus_code);
>>> - ret = v4l2_subdev_call(isc->current_subdev->sd, pad,
>>> - set_fmt, NULL, &format);
>>> - if (ret < 0)
>>> - return ret;
>>> + /* Identify the subdev's format configuration */
>>> + for (i = 0; i < isc->num_user_formats; i++)
>>> + if (isc->user_formats[i]->mbus_code == format.format.code) {
>>> + sd_fmt = isc->user_formats[i];
>>> + break;
>>> + }
>>> +
>>> + /* Check if the format is not supported */
>>> + if (!sd_fmt) {
>>> + v4l2_err(&isc->v4l2_dev,
>>> + "Current subdevice is streaming a media bus code that is not supported 0x%x\n",
>>> + format.format.code);
>>> + return -EPIPE;
>>> + }
>>> +
>>> + /* At this moment we know which format the subdev will use */
>>> + isc->try_config.sd_format = sd_fmt;
>>> +
>>> + /* If the sensor is not RAW, we can only do a direct dump */
>>> + if (!ISC_IS_FORMAT_RAW(isc->try_config.sd_format->mbus_code))
>>> + isc_try_configure_rlp_dma(isc, true);
>>>
>>> /* Limit to Atmel ISC hardware capabilities */
>>> - if (f->fmt.pix.width > isc->max_width)
>>> - f->fmt.pix.width = isc->max_width;
>>> - if (f->fmt.pix.height > isc->max_height)
>>> - f->fmt.pix.height = isc->max_height;
>>> + v4l_bound_align_image(&format.format.width, 16, isc->max_width, 0,
>>> + &format.format.height, 16, isc->max_height, 0, 0);
>>>
>>> - isc->fmt = *f;
>>> + /* Check if the frame size is the same. Otherwise we may overflow */
>>> + if (pixfmt->height != format.format.height ||
>>> + pixfmt->width != format.format.width) {
>>> + v4l2_err(&isc->v4l2_dev,
>>> + "ISC not configured with the proper frame size: %dx%d\n",
>>> + format.format.width, format.format.height);
>>> + return -EPIPE;
>>> + }
>>>
>>> + v4l2_dbg(1, debug, &isc->v4l2_dev,
>>> + "Identified subdev using format %.4s with %dx%d %d bpp\n",
>>> + (char *)&sd_fmt->fourcc, pixfmt->width, pixfmt->height,
>>> + isc->try_config.bpp);
>>> +
>>> + /* Reset and restart AWB if the subdevice changed the format */
>>> if (isc->try_config.sd_format && isc->config.sd_format &&
>>> isc->try_config.sd_format != isc->config.sd_format) {
>>> isc->ctrls.hist_stat = HIST_INIT;
>>> isc_reset_awb_ctrls(isc);
>>> isc_update_v4l2_ctrls(isc);
>>> }
>>> - /* make the try configuration active */
>>> +
>>> + /* Validate formats */
>>> + ret = isc_try_validate_formats(isc);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* Obtain frame sizes if possible to have crop requirements ready */
>>> + isc_try_fse(isc, &pad_state);
>>> +
>>> + /* Configure ISC pipeline for the config */
>>> + ret = isc_try_configure_pipeline(isc);
>>> + if (ret)
>>> + return ret;
>>> +
>>> isc->config = isc->try_config;
>>>
>>> v4l2_dbg(1, debug, &isc->v4l2_dev, "New ISC configuration in place\n");
>>> @@ -1064,6 +1030,19 @@ static int isc_set_fmt(struct isc_device *isc, struct v4l2_format *f)
>>> return 0;
>>> }
>>>
>>> +static int isc_streamon(struct file *file, void *priv, enum v4l2_buf_type bt)
>>> +{
>>> + struct isc_device *isc = video_drvdata(file);
>>> + int ret;
>>> +
>>> + ret = isc_validate(isc);
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return vb2_ioctl_streamon(file, priv, bt);
>>> +}
>>> +
>>> static int isc_s_fmt_vid_cap(struct file *file, void *priv,
>>> struct v4l2_format *f)
>>> {
>>> @@ -1080,7 +1059,7 @@ static int isc_try_fmt_vid_cap(struct file *file, void *priv,
>>> {
>>> struct isc_device *isc = video_drvdata(file);
>>>
>>> - return isc_try_fmt(isc, f, NULL);
>>> + return isc_try_fmt(isc, f);
>>> }
>>>
>>> static int isc_enum_input(struct file *file, void *priv,
>>> @@ -1176,7 +1155,7 @@ static const struct v4l2_ioctl_ops isc_ioctl_ops = {
>>> .vidioc_dqbuf = vb2_ioctl_dqbuf,
>>> .vidioc_create_bufs = vb2_ioctl_create_bufs,
>>> .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>>> - .vidioc_streamon = vb2_ioctl_streamon,
>>> + .vidioc_streamon = isc_streamon,
>>> .vidioc_streamoff = vb2_ioctl_streamoff,
>>>
>>> .vidioc_g_parm = isc_g_parm,
>>> @@ -1879,7 +1858,7 @@ static int isc_set_default_fmt(struct isc_device *isc)
>>> };
>>> int ret;
>>>
>>> - ret = isc_try_fmt(isc, &f, NULL);
>>> + ret = isc_try_fmt(isc, &f);
>>> if (ret)
>>> return ret;
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
>>> index c2cb805faff3..7081698adddd 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc.h
>>> +++ b/drivers/media/platform/atmel/atmel-isc.h
>>> @@ -297,6 +297,7 @@ struct isc_device {
>>> struct completion comp;
>>>
>>> struct v4l2_format fmt;
>>> + struct v4l2_format try_fmt;
>>> struct isc_format **user_formats;
>>> unsigned int num_user_formats;
>>>
>>> --
>>> 2.25.1
>>>
>