On 11/8/21 4:08 PM, Eugen Hristev - M18282 wrote:

On 11/8/21 1:20 PM, Jacopo Mondi wrote:

quoted

Hi Eugen

On Fri, Nov 05, 2021 at 11:03:25AM +0000, Eugen.Hristev@microchip.com wrote:

quoted

On 11/5/21 11:51 AM, Jacopo Mondi wrote:

quoted

Hi Eugen

On Fri, Nov 05, 2021 at 10:25:59AM +0100, Jacopo Mondi wrote:

quoted

Hi Eugen,

On Fri, Oct 22, 2021 at 10:52:37AM +0300, Eugen Hristev wrote:

quoted

If enumfmt is called with an mbus_code, the enumfmt handler should only
return the formats that are supported for this mbus_code.
To make it more easy to understand the formats, changed the report order
to report first the native formats, and after that the formats that the ISC
can convert to.

Signed-off-by: Eugen Hristev <redacted>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Too soon! Sorry...

quoted

Thanks
      j

quoted

---
    drivers/media/platform/atmel/atmel-isc-base.c | 51 ++++++++++++++++---
    1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index 2dd2511c7be1..1f7fbe5e4d79 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c

@@ -499,21 +499,56 @@ static int isc_enum_fmt_vid_cap(struct file *file, void *priv,
       u32 index = f->index;
       u32 i, supported_index;

-   if (index < isc->controller_formats_size) {
-           f->pixelformat = isc->controller_formats[index].fourcc;
-           return 0;
+   supported_index = 0;
+

Hi Jacopo,

This for loop below first iterates through the formats that were
identified as directly supported by the subdevice.
As we are able in the ISC to just dump what the subdevice can stream, we
advertise all these formats here.
(if the userspace requires one specific mbus code, we only advertise the
corresponding format)

quoted

quoted

quoted

+   for (i = 0; i < isc->formats_list_size; i++) {
+           if (!isc->formats_list[i].sd_support)

quoted

quoted

+                   continue;
+           /*
+            * If specific mbus_code is requested, provide only
+            * supported formats with this mbus code
+            */
+           if (f->mbus_code && f->mbus_code !=
+               isc->formats_list[i].mbus_code)
+                   continue;
+           if (supported_index == index) {
+                   f->pixelformat = isc->formats_list[i].fourcc;
+                   return 0;
+           }
+           supported_index++;
       }

-   index -= isc->controller_formats_size;
+   /*
+    * If the sensor does not support this mbus_code whatsoever,
+    * there is no reason to advertise any of our output formats
+    */
+   if (supported_index == 0)
+           return -EINVAL;

Shouldn't you also return -EINVAL if index > supported_index ?

No. If we could not find any format that the sensor can use
(supported_index == 0), and that our ISC can also use, then we don't
support anything, thus, return -EINVAL regardless of the index.

quoted

In that case, I'm not gettng what the rest of the function is for ?

This is the case when we identified one supported format for both the
sensor and the ISC (case where supported_index found earlier is >= 1)

quoted

quoted

quoted

+
+   /*
+    * If the sensor uses a format that is not raw, then we cannot
+    * 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 ||
+       !ISC_IS_FORMAT_RAW(isc->config.sd_format->mbus_code))
+           return -EINVAL;

Next, if the current format from the subdev is not raw, we are done.

Ok, I think here it's were I disconnect.

I don't think you should care about the -current- format on the
subdev, as once you move to MC the ISC formats should be enumerated
regardless of the how the remote subdev is configured. Rather, you
should consider if IS_RAW(f->mbus_code) and from there enumerate the
output formats you can generate from raw bayer (in addition to the
ones that can be produced by dumping the sensor produced formats)

Actually , in some words, this is what I am doing.
I am following Laurent's advice:
enumerate the formats supported for a given mbus code.

In consequence, if the mbus code given is a raw mbus code , I can
advertise all my ISC formats, and if the mbus code is not raw, I don't .

So I am doing what you are saying, namely three cases:

If there is no mbus code given as a parameter to this function, I
advertise all my formats, raw, non-raw, etc.

If there is raw mbus code given, I advertise this specific raw mbus code
if the sensor supports it, and the ISC supports it, and in addition, all
the formats ISC can convert from raw to.

If the mbus code given is not raw, then, I can only advertise this
specific non-raw format, since there is nothing more I can do with it.
It wouldn't make much sense to still advertise my rgb,yuv formats, since
they cannot be used at all, and my later try_validate_formats will bail out

quoted

quoted

But, if it's raw, we can use it to convert to a plethora of other
formats. So we have to enumerate them below :

With the previous checks, I am making sure that the ISC can really
convert to these formats, otherwise they are badly reported

Hope this makes things more clear, but please ask if something looks strange

I think our disconnection comes from the way the supported formats are
defined in ISC and I think their definition could be reworkd to
simplify the format selection logic.

The driver defines three lists of formats:

- isc->controller_formats

         static const struct isc_format sama7g5_controller_formats[] = {
          {
                  .fourcc         = V4L2_PIX_FMT_ARGB444,
          },
          {
                  .fourcc         = V4L2_PIX_FMT_ARGB555,
          },
          ...

          };

   These are listed with the output fourcc only, and if I get
   try_configure_pipeline() right, they can be generated from any Bayer
   RAW format. Is this right ?

- isc->formats_list

          static struct isc_format sama7g5_formats_list[] = {
                  {
                          .fourcc         = V4L2_PIX_FMT_SBGGR8,
                          .mbus_code      = MEDIA_BUS_FMT_SBGGR8_1X8,
                          .pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,
                          .cfa_baycfg     = ISC_BAY_CFG_BGBG,
                  },
                  {
                          .fourcc         = V4L2_PIX_FMT_SGBRG8,
                          .mbus_code      = MEDIA_BUS_FMT_SGBRG8_1X8,
                          .pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,
                          .cfa_baycfg     = ISC_BAY_CFG_GBGB,
                  },

           ...

           };

    These are formats the ISC can output by dumping the sensor output,
    hence they require a specific format to be output from the sensor

- isc->user_formats

          static int isc_formats_init() {

                  ...

                  fmt = &isc->formats_list[0];
                  for (i = 0, j = 0; i < list_size; i++) {
                          if (fmt->sd_support)
                                  isc->user_formats[j++] = fmt;
                          fmt++;
                  }

        }

    this list is obtained at runtime by restricting the formats_list to
    what the sensor can actually output. I think these, if you move to
    MC should be removed but I'm not 100% sure it is possible with the
    current implementation of set_fmt().

Do you think controller_formats and formats_list should be unified ?

If my understanding that all controller_formats can be generated from
RAW Bayer formats you could model struct isc_format as

          struct isc_format {
                  u32     fourcc;
                  bool    processed;
                  u32     mbus_codes
                  u32     cfa_baycfg;
                  u32     pfe_cfg0_bps;
          };

and have in example:

          {
                  .fourcc         = V4L2_PIX_FMT_ARGB444,
                  .processed      = true,
                  .mbus_codes     = nullptr,
                  ....
          },
          {
                  .fourcc         = V4L2_PIX_FMT_SBGGR8,
                  .processed      = false,
                  .mbus_codes     = { MEDIA_BUS_FMT_SBGGR8_1X8 }
                  .pfe_cfg0_bps   = ISC_PFE_CFG0_BPS_EIGHT,
                  .cfa_baycfg     = ISC_BAY_CFG_BGBG,
          },

and when enumerating and configuring formats check that

          if (isc_format[i].processed &&
              (f->mbus_code && IS_RAW(f->mbus)code))

or

          if (!isc_format[i].processed &&
              (f->mbus_code == isc_format[i].mbus_code

if you have other restrictions as in example V4L2_PIX_FMT_YUV420
requiring a packed YUV format, you can implement more complex
validations to match processed formats, like you do in try_validate_formats()

Also, and a bit unrelated to enum_fmt, if I got this right
at format configuration time you match the ISC format with
the sensor format. I think this should be moved to .link_validate() op time.

The MC core calls .link_validate when streaming is started on each
entity part of a pipeline, and there you could make sure that the ISC output format can be produced using
the sensor format (and sizes). This will greatly simplify set_fmt() as
there you will have just to make sure the supplied format is in the
list of the ISC supported ones and that's it. If userspace fails to
configure the pipeline correctly (in example by setting a non RAW
Bayer sensor on the format and requesting a RAW Bayer format from ISC,
you will fail at s_stream() time by returning an EPIPE.

Hi Jacopo,

I tried to look over the link_validate call.
It looks like this is only called by media_pipeline_start, which most 
drivers use in start_streaming() .
However, I need the format validation to be done before that, at 
streamon() time, because after streamon() , the vb2 queue will be filled 
with buffers, and I need to know exactly which format I will be using.
So, do you think it's fine to keep the format validation at streamon() 
time instead of calling media_pipeline_start in start_streaming ?

Currently I am not calling media_pipeline_start at all.
Do you think it's required?

Or maybe I am missing something and it should be done in a different way ?

Thanks !

quoted

Hope all of this makes a bit of sense :)

About this last part you are telling me: I have to tell you what am I
doing right now: with this patch series, including this patch, I am
adding support for mc handling in this driver, but! the driver is still
completely compatible with 'the old way' like setting fmt-video for
/dev/video0 and everything is propagated down the pipeline.

I am doing the conversion to mc-only type of driver in multiple steps.
This series adds the 'new way' while having the 'old way' still there.
At the moment I am working on another patch on top of this series that
will basically remove most of my format propagation logic from
/dev/video0 to the subdevice, and after this patch that is on the way,
the 'old way' is gone. The sensor will *have to* be configured through
userspace because ISC will never call set_fmt on it at all.

So the purpose of the patch you are reviewing now is to have the mbus
code parameter in the enum_fmt_vid_cap in the way the current driver
handles things.

So if you agree, I will let the other patch speak for itself and rework
the logic completely .
I am currently trying to do it at streamon() time like Laurent told me,
but I can try to have it at validate link as well, to see how it works.

I will add that patch to the series when it's ready and I have a v2 of
this series as well. So in baby steps, I am working towards the goal. I
am not sure if this means that you could agree to this patch as-is, or I
have to integrate it completely into a bigger patch that will also fix
everything up including the format logic.

Thanks again for your review and ideas

Eugen

quoted

quoted

quoted

quoted

quoted

+   /*
+    * Iterate again through the formats that we can convert to.
+    * However, to avoid duplicates, skip the formats that
+    * the sensor already supports directly
+    */
+   index -= supported_index;
       supported_index = 0;

-   for (i = 0; i < isc->formats_list_size; i++) {
-           if (!ISC_IS_FORMAT_RAW(isc->formats_list[i].mbus_code) ||
-               !isc->formats_list[i].sd_support)
+   for (i = 0; i < isc->controller_formats_size; i++) {
+           /* if this format is already supported by sensor, skip it */
+           if (find_format_by_fourcc(isc, isc->controller_formats[i].fourcc))
                       continue;
               if (supported_index == index) {
-                   f->pixelformat = isc->formats_list[i].fourcc;
+                   f->pixelformat =
+                           isc->controller_formats[i].fourcc;
                       return 0;
               }
               supported_index++;
--
2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel