[RFC/PATCH 0/6] Interleaved image data on media bus

[RFC/PATCH 0/6] Interleaved image data on media bus

[Sylwester Nawrocki]

Hello,

This patch series presents a method of capturing interleaved YUYV/JPEG image
frames with the S5P/EXYNOS FIMC and MIPI-CSIS devices I went for to support 
a camera (sensor) that outputs interleaved image data at single User Defined 
MIPI-CSI2 data format. Such data is a combined two frames where each frame's 
resolution is separately configurable, i.e. both frames can have different
resolution. Additionally the sensor generates relatively small amount of 
meta data which is necessary for interpreting the interleaved format.  

I decided to use two-planar buffers for this, rather than using separate
buffer queues for the image and the meta data. Since both data are captured
at different devices and matching those data in user space might be hard to
achieve, and would add to complexity in the applications significantly.

So here is the initial patch series, I'm sending it early to possibly get
some better ideas...or just to have some background for discussion. :)

I suppose the get/set_frame_config callbacks are most open issues. I intended
these callbacks and the associated data structure as helpers for performing
additional configuration for transmission of more complex data than just
single raw image frame on media bus. I'm open to changing it, that's mainly
to indicate we really need such sort of an API.


Thoughts ?

--

Regards,
Sylwester

Sylwester Nawrocki (6):
  V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format
  V4L: Add V4L2_PIX_FMT_JPG_YUV_S5C fourcc definition
  V4L: Add g_embedded_data subdev callback
  V4L: Add get/set_frame_config subdev callbacks
  s5p-fimc: Add support for V4L2_PIX_FMT_JPG_YUYV_S5C fourcc
  s5p-csis: Add support for non-image data packets capture

 Documentation/DocBook/media/v4l/pixfmt.xml  |    8 +
 drivers/media/video/s5p-fimc/fimc-capture.c |  123 ++++++++---
 drivers/media/video/s5p-fimc/fimc-core.c    |   37 +++-
 drivers/media/video/s5p-fimc/fimc-core.h    |   22 ++-
 drivers/media/video/s5p-fimc/fimc-reg.c     |    5 +-
 drivers/media/video/s5p-fimc/mipi-csis.c    |  312 +++++++++++++++++++++++++--
 include/linux/v4l2-mediabus.h               |    3 +
 include/linux/videodev2.h                   |    1 +
 include/media/v4l2-subdev.h                 |   28 +++
 9 files changed, 483 insertions(+), 56 deletions(-)

-- 
1.7.9

[RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sylwester Nawrocki]

This patch adds media bus pixel code for the interleaved JPEG/YUYV image
format used by S5C73MX Samsung cameras. The interleaved image data is
transferred on MIPI-CSI2 bus as User Defined Byte-based Data.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/linux/v4l2-mediabus.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
index 5ea7f75..c2f0e4e 100644
--- a/include/linux/v4l2-mediabus.h
+++ b/include/linux/v4l2-mediabus.h
@@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
 
 	/* JPEG compressed formats - next is 0x4002 */
 	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
+
+	/* Interleaved JPEG and YUV formats - next is 0x4102 */
+	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
 };
 
 /**
-- 
1.7.9

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sakari Ailus]

Hi Sylwester,

On Thu, Feb 16, 2012 at 07:23:54PM +0100, Sylwester Nawrocki wrote:
> This patch adds media bus pixel code for the interleaved JPEG/YUYV image
> format used by S5C73MX Samsung cameras. The interleaved image data is
> transferred on MIPI-CSI2 bus as User Defined Byte-based Data.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  include/linux/v4l2-mediabus.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> index 5ea7f75..c2f0e4e 100644
> --- a/include/linux/v4l2-mediabus.h
> +++ b/include/linux/v4l2-mediabus.h
> @@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
>  
>  	/* JPEG compressed formats - next is 0x4002 */
>  	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
> +
> +	/* Interleaved JPEG and YUV formats - next is 0x4102 */
> +	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
>  };

Thanks for the patch. Just a tiny comment:

I'd go with a new hardware-specific buffer range, e.g. 0x5000.

Guennadi also proposed an interesting idea: a "pass-through" format. Does
your format have dimensions that the driver would use for something or is
that just a blob?

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sylwester Nawrocki]

Hi Sakari,

On 02/16/2012 08:46 PM, Sakari Ailus wrote:
> Hi Sylwester,
> 
> On Thu, Feb 16, 2012 at 07:23:54PM +0100, Sylwester Nawrocki wrote:
>> This patch adds media bus pixel code for the interleaved JPEG/YUYV image
>> format used by S5C73MX Samsung cameras. The interleaved image data is
>> transferred on MIPI-CSI2 bus as User Defined Byte-based Data.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  include/linux/v4l2-mediabus.h |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>> index 5ea7f75..c2f0e4e 100644
>> --- a/include/linux/v4l2-mediabus.h
>> +++ b/include/linux/v4l2-mediabus.h
>> @@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
>>  
>>  	/* JPEG compressed formats - next is 0x4002 */
>>  	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
>> +
>> +	/* Interleaved JPEG and YUV formats - next is 0x4102 */
>> +	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
>>  };
> 
> Thanks for the patch. Just a tiny comment:
> 
> I'd go with a new hardware-specific buffer range, e.g. 0x5000.

Sure, that makes more sense. But I guess you mean "format" not "buffer" range ?

> Guennadi also proposed an interesting idea: a "pass-through" format. Does
> your format have dimensions that the driver would use for something or is
> that just a blob?

It's just a blob for the drivers, dimensions may be needed for subdevs to
compute overall size of data for example. But the host driver, in case of
Samsung devices, basically just needs to know the total size of frame data.

I'm afraid the host would have to additionally configure subdevs in the data
pipeline in case of hardware-specific format, when we have used a single
binary blob media bus format identifier. For example MIPI-CSI2 data format
would have to be set up along the pipeline. There might be more attributes
in the future like this. Not sure if we want to go that path ?

I'll try and see how it would look like with a single "pass-through" format.
Probably using g/s_mbus_config operations ?


Best regards
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sakari Ailus]

Hi Sylwester,

On Fri, Feb 17, 2012 at 03:26:29PM +0100, Sylwester Nawrocki wrote:
> On 02/16/2012 08:46 PM, Sakari Ailus wrote:
> > On Thu, Feb 16, 2012 at 07:23:54PM +0100, Sylwester Nawrocki wrote:
> >> This patch adds media bus pixel code for the interleaved JPEG/YUYV image
> >> format used by S5C73MX Samsung cameras. The interleaved image data is
> >> transferred on MIPI-CSI2 bus as User Defined Byte-based Data.
> >>
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >>  include/linux/v4l2-mediabus.h |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
> >> index 5ea7f75..c2f0e4e 100644
> >> --- a/include/linux/v4l2-mediabus.h
> >> +++ b/include/linux/v4l2-mediabus.h
> >> @@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
> >>  
> >>  	/* JPEG compressed formats - next is 0x4002 */
> >>  	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
> >> +
> >> +	/* Interleaved JPEG and YUV formats - next is 0x4102 */
> >> +	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
> >>  };
> > 
> > Thanks for the patch. Just a tiny comment:
> > 
> > I'd go with a new hardware-specific buffer range, e.g. 0x5000.
> 
> Sure, that makes more sense. But I guess you mean "format" not "buffer" range ?

Yeah, a format that begins a new range.

> > Guennadi also proposed an interesting idea: a "pass-through" format. Does
> > your format have dimensions that the driver would use for something or is
> > that just a blob?
> 
> It's just a blob for the drivers, dimensions may be needed for subdevs to
> compute overall size of data for example. But the host driver, in case of
> Samsung devices, basically just needs to know the total size of frame data.
> 
> I'm afraid the host would have to additionally configure subdevs in the data
> pipeline in case of hardware-specific format, when we have used a single
> binary blob media bus format identifier. For example MIPI-CSI2 data format
> would have to be set up along the pipeline. There might be more attributes
> in the future like this. Not sure if we want to go that path ?
> 
> I'll try and see how it would look like with a single "pass-through" format.
> Probably using g/s_mbus_config operations ?

I think we could use the framesize control to tell the size of the frame, or
however it is done for jpeg blobs.

The issue I see in the pass-through mode is that the user would have no
information whatsoever what he's getting. This would be perhaps fixed by
adding the frame format descriptor: it could contain information how to
handle the data. (Just thinking out loud. :))

I'm fine keeping this approach with sensor specific pixel code for now at
least, but we must mark it experimental IMHO.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sylwester Nawrocki]

Hi Sakari,

On 02/17/2012 07:15 PM, Sakari Ailus wrote:
> On Fri, Feb 17, 2012 at 03:26:29PM +0100, Sylwester Nawrocki wrote:
>> On 02/16/2012 08:46 PM, Sakari Ailus wrote:
>>> On Thu, Feb 16, 2012 at 07:23:54PM +0100, Sylwester Nawrocki wrote:
>>>> This patch adds media bus pixel code for the interleaved JPEG/YUYV image
>>>> format used by S5C73MX Samsung cameras. The interleaved image data is
>>>> transferred on MIPI-CSI2 bus as User Defined Byte-based Data.
>>>>
>>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>> ---
>>>>   include/linux/v4l2-mediabus.h |    3 +++
>>>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>>>> index 5ea7f75..c2f0e4e 100644
>>>> --- a/include/linux/v4l2-mediabus.h
>>>> +++ b/include/linux/v4l2-mediabus.h
>>>> @@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
>>>>
>>>>   	/* JPEG compressed formats - next is 0x4002 */
>>>>   	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
>>>> +
>>>> +	/* Interleaved JPEG and YUV formats - next is 0x4102 */
>>>> +	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
>>>>   };
>>>
>>> Thanks for the patch. Just a tiny comment:
>>>
>>> I'd go with a new hardware-specific buffer range, e.g. 0x5000.
>>
>> Sure, that makes more sense. But I guess you mean "format" not "buffer" range ?
> 
> Yeah, a format that begins a new range.
> 
>>> Guennadi also proposed an interesting idea: a "pass-through" format. Does
>>> your format have dimensions that the driver would use for something or is
>>> that just a blob?
>>
>> It's just a blob for the drivers, dimensions may be needed for subdevs to
>> compute overall size of data for example. But the host driver, in case of
>> Samsung devices, basically just needs to know the total size of frame data.
>>
>> I'm afraid the host would have to additionally configure subdevs in the data
>> pipeline in case of hardware-specific format, when we have used a single
>> binary blob media bus format identifier. For example MIPI-CSI2 data format
>> would have to be set up along the pipeline. There might be more attributes
>> in the future like this. Not sure if we want to go that path ?
>>
>> I'll try and see how it would look like with a single "pass-through" format.
>> Probably using g/s_mbus_config operations ?
> 
> I think we could use the framesize control to tell the size of the frame, or
> however it is done for jpeg blobs.

Yes, we could add a standard framesize control to the Image Source class but it
will solve only part of the problem. Nevertheless it might be worth to have it.
It could be used by applications to configure subdevs directly, while the host 
drivers could use e.g. s/g_frame_config op for that.

> The issue I see in the pass-through mode is that the user would have no
> information whatsoever what he's getting. This would be perhaps fixed by
> adding the frame format descriptor: it could contain information how to
> handle the data. (Just thinking out loud. :))

Do you mean a user space application by "user" ?

I'd like to clearly separate blob media bus pixel codes and hardware-specific
blob fourccs. If we don't want to change fundamental assumptions of V4L2
we likely need separate fourccs for each weird format.

I can imagine "pass-through" media bus pixel code but a transparent fourcc
sounds like a higher abstraction. :)
 
> I'm fine keeping this approach with sensor specific pixel code for now at
> least, but we must mark it experimental IMHO.

Good point, I'm eventually going to add a relevant note in the DocBook.

--

Thanks,
Sylwester

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sakari Ailus]

Hi Sylwester,

Sylwester Nawrocki wrote:
> On 02/17/2012 07:15 PM, Sakari Ailus wrote:
>> On Fri, Feb 17, 2012 at 03:26:29PM +0100, Sylwester Nawrocki wrote:
>>> On 02/16/2012 08:46 PM, Sakari Ailus wrote:
>>>> On Thu, Feb 16, 2012 at 07:23:54PM +0100, Sylwester Nawrocki wrote:
>>>>> This patch adds media bus pixel code for the interleaved JPEG/YUYV image
>>>>> format used by S5C73MX Samsung cameras. The interleaved image data is
>>>>> transferred on MIPI-CSI2 bus as User Defined Byte-based Data.
>>>>>
>>>>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>> ---
>>>>>   include/linux/v4l2-mediabus.h |    3 +++
>>>>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/v4l2-mediabus.h b/include/linux/v4l2-mediabus.h
>>>>> index 5ea7f75..c2f0e4e 100644
>>>>> --- a/include/linux/v4l2-mediabus.h
>>>>> +++ b/include/linux/v4l2-mediabus.h
>>>>> @@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
>>>>>
>>>>>   	/* JPEG compressed formats - next is 0x4002 */
>>>>>   	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
>>>>> +
>>>>> +	/* Interleaved JPEG and YUV formats - next is 0x4102 */
>>>>> +	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
>>>>>   };
>>>>
>>>> Thanks for the patch. Just a tiny comment:
>>>>
>>>> I'd go with a new hardware-specific buffer range, e.g. 0x5000.
>>>
>>> Sure, that makes more sense. But I guess you mean "format" not "buffer" range ?
>>
>> Yeah, a format that begins a new range.
>>
>>>> Guennadi also proposed an interesting idea: a "pass-through" format. Does
>>>> your format have dimensions that the driver would use for something or is
>>>> that just a blob?
>>>
>>> It's just a blob for the drivers, dimensions may be needed for subdevs to
>>> compute overall size of data for example. But the host driver, in case of
>>> Samsung devices, basically just needs to know the total size of frame data.
>>>
>>> I'm afraid the host would have to additionally configure subdevs in the data
>>> pipeline in case of hardware-specific format, when we have used a single
>>> binary blob media bus format identifier. For example MIPI-CSI2 data format
>>> would have to be set up along the pipeline. There might be more attributes
>>> in the future like this. Not sure if we want to go that path ?
>>>
>>> I'll try and see how it would look like with a single "pass-through" format.
>>> Probably using g/s_mbus_config operations ?
>>
>> I think we could use the framesize control to tell the size of the frame, or
>> however it is done for jpeg blobs.
> 
> Yes, we could add a standard framesize control to the Image Source class but it
> will solve only part of the problem. Nevertheless it might be worth to have it.
> It could be used by applications to configure subdevs directly, while the host 
> drivers could use e.g. s/g_frame_config op for that.

(I think we could continue this discussion in the context of the RFC.)

>> The issue I see in the pass-through mode is that the user would have no
>> information whatsoever what he's getting. This would be perhaps fixed by
>> adding the frame format descriptor: it could contain information how to
>> handle the data. (Just thinking out loud. :))
> 
> Do you mean a user space application by "user" ?

Yeah.

> I'd like to clearly separate blob media bus pixel codes and hardware-specific
> blob fourccs. If we don't want to change fundamental assumptions of V4L2
> we likely need separate fourccs for each weird format.
>
> I can imagine "pass-through" media bus pixel code but a transparent fourcc
> sounds like a higher abstraction. :)

I agree... how about this:

We currently provide the information on the media bus pixel code to the
CSI-2 receivers but most of the time it's not necessary for them to know
what the pixel code exactly is: it doesn't do anything with the data but
writes it to memory. Bits uncompressed, compressed and the compression
method are enough --- if uncompression is desired. Even pixel order
isn't always needed.

What might make sense is to provide generic table with pixel code
related information, such as bits compressed and uncompressed, pixel
order, compression method and default 4CC.

Custom formats would only be present in this table without individual
CSI-2 receiver drivers having to know about them. Same goes with 4CC's.

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Sylwester Nawrocki]

Hi Sakari,

On 02/26/2012 11:25 PM, Sakari Ailus wrote:
>>> I think we could use the framesize control to tell the size of the frame, or
>>> however it is done for jpeg blobs.
>>
>> Yes, we could add a standard framesize control to the Image Source class but it
>> will solve only part of the problem. Nevertheless it might be worth to have it.
>> It could be used by applications to configure subdevs directly, while the host
>> drivers could use e.g. s/g_frame_config op for that.
> 
> (I think we could continue this discussion in the context of the RFC.)

Sure, let's continue in your RFC thread.

>>> The issue I see in the pass-through mode is that the user would have no
>>> information whatsoever what he's getting. This would be perhaps fixed by
>>> adding the frame format descriptor: it could contain information how to
>>> handle the data. (Just thinking out loud. :))
>>
>> Do you mean a user space application by "user" ?
> 
> Yeah.
> 
>> I'd like to clearly separate blob media bus pixel codes and hardware-specific
>> blob fourccs. If we don't want to change fundamental assumptions of V4L2
>> we likely need separate fourccs for each weird format.
>>
>> I can imagine "pass-through" media bus pixel code but a transparent fourcc
>> sounds like a higher abstraction. :)
> 
> I agree... how about this:
> 
> We currently provide the information on the media bus pixel code to the
> CSI-2 receivers but most of the time it's not necessary for them to know
> what the pixel code exactly is: it doesn't do anything with the data but
> writes it to memory. Bits uncompressed, compressed and the compression
> method are enough --- if uncompression is desired. Even pixel order
> isn't always needed.

I don't think so. For all image formats defined by MIPI-CSI2 standard a pixel 
code is necessary. Sample compression or bit expansion is most of the time 
related to a specific image format. A MIPI-CSI2 receiver must know an exact 
image format, otherwise it won't be able to decode data from the low level 
protocol.

> What might make sense is to provide generic table with pixel code
> related information, such as bits compressed and uncompressed, pixel
> order, compression method and default 4CC.

This doesn't look like an improvement to me, most of these information we 
now have in single 4-byte media bus pixel code. Do you want the drivers 
to search such tables by comparing all those parameters ?

> Custom formats would only be present in this table without individual
> CSI-2 receiver drivers having to know about them. Same goes with 4CC's.

Media bus/fourcc translation tables will always be driver-specific. There 
have already been discussions about centralizing such tables IIRC. All you
can have is probably just some default ("statistical") fourcc, which is really
useful for nothing. 

Having a bunch of parameters for each custom format could be useful probably
only if we've dropped an assumption that each hardware specific data format 
gets it's own fourcc, and have exposed those parameters to the user space.

The multi-planar formats complicate things further. Now the fourcc determines
whether a v4l2 buffer is has more than one data plane.

--

Regards,
Sylwester

Re: [RFC/PATCH 1/6] V4L: Add V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 media bus format

[Laurent Pinchart]

Hi Sylwester,

On Friday 17 February 2012 15:26:29 Sylwester Nawrocki wrote:
> On 02/16/2012 08:46 PM, Sakari Ailus wrote:
> > On Thu, Feb 16, 2012 at 07:23:54PM +0100, Sylwester Nawrocki wrote:
> >> This patch adds media bus pixel code for the interleaved JPEG/YUYV image
> >> format used by S5C73MX Samsung cameras. The interleaved image data is
> >> transferred on MIPI-CSI2 bus as User Defined Byte-based Data.
> >> 
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >> 
> >>  include/linux/v4l2-mediabus.h |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/include/linux/v4l2-mediabus.h
> >> b/include/linux/v4l2-mediabus.h
> >> index 5ea7f75..c2f0e4e 100644
> >> --- a/include/linux/v4l2-mediabus.h
> >> +++ b/include/linux/v4l2-mediabus.h
> >> @@ -92,6 +92,9 @@ enum v4l2_mbus_pixelcode {
> >> 
> >>  	/* JPEG compressed formats - next is 0x4002 */
> >>  	V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
> >> 
> >> +
> >> +	/* Interleaved JPEG and YUV formats - next is 0x4102 */
> >> +	V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 = 0x4101,
> >> 
> >>  };

Please remember to update Documentation/DocBook/media/v4l/subdev-formats.xml.

> > Thanks for the patch. Just a tiny comment:
> > 
> > I'd go with a new hardware-specific buffer range, e.g. 0x5000.
> 
> Sure, that makes more sense. But I guess you mean "format" not "buffer"
> range ?
>
> > Guennadi also proposed an interesting idea: a "pass-through" format. Does
> > your format have dimensions that the driver would use for something or is
> > that just a blob?
> 
> It's just a blob for the drivers, dimensions may be needed for subdevs to
> compute overall size of data for example. But the host driver, in case of
> Samsung devices, basically just needs to know the total size of frame data.
> 
> I'm afraid the host would have to additionally configure subdevs in the data
> pipeline in case of hardware-specific format, when we have used a single
> binary blob media bus format identifier. For example MIPI-CSI2 data format
> would have to be set up along the pipeline. There might be more attributes
> in the future like this. Not sure if we want to go that path ?
> 
> I'll try and see how it would look like with a single "pass-through" format.
> Probably using g/s_mbus_config operations ?

I'm not sure yet how all this should be handled exactly, but I'm pretty sure I 
don't want the CSI2 receiver drivers to handle all the vendor-specific blob-
like formats explicitly. For instance your sensor could be used with the OMAP3 
ISP, and I don't want to add support for V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 to the 
OMAP3 ISP CSI2 driver. We need a way for CSI2 receiver drivers to map blob 
formats automatically. Enumeration also needs to be handled, which makes the 
problem even more complex.

-- 
Regards,

Laurent Pinchart

[RFC/PATCH 2/6] V4L: Add V4L2_PIX_FMT_JPG_YUV_S5C fourcc definition

[Sylwester Nawrocki]

The V4L2_PIX_FMT_JPG_YUV_S5C is a two-plane image format generated
by S5C73M3 camera. The first plane contains interleaved JPEG and
YUYV data and the second one the meta data containing offsets
(pointers) to the YUYV data blocks. First 4 bytes of the meta
data plane indicate total size of the image data plane, subsequent
4 bytes indicate actual size of the meta data and the remainder
is a list of offsets to YUYV blocks within the first plane.
All numbers are 4 byte unsigned integers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/pixfmt.xml |    8 ++++++++
 include/linux/videodev2.h                  |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml b/Documentation/DocBook/media/v4l/pixfmt.xml
index 31eaae2..0512f2b 100644
--- a/Documentation/DocBook/media/v4l/pixfmt.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt.xml
@@ -999,6 +999,14 @@ the other bits are set to 0.</entry>
 	    <entry>Old 6-bit greyscale format. Only the least significant 6 bits of each byte are used,
 the other bits are set to 0.</entry>
 	  </row>
+	  <row id="V4L2-PIX-FMT-JPG-YUYV-S5C">
+	    <entry><constant>V4L2_PIX_FMT_JPG_YUYV_S5C</constant></entry>
+	    <entry>'S5CJ'</entry>
+	    <entry>Two-planar format used by Samsung S5C73MX cameras.The first plane contains
+interleaved JPEG and YUYV data and the second one the meta data containing a list of offsets
+to the YUYV data blocks within first plane. All numbers in the second plane are 4-byte unsigned
+integers.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 740b35b..4fdba17 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -415,6 +415,7 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_KONICA420  v4l2_fourcc('K', 'O', 'N', 'I') /* YUV420 planar in blocks of 256 pixels */
 #define V4L2_PIX_FMT_JPGL	v4l2_fourcc('J', 'P', 'G', 'L') /* JPEG-Lite */
 #define V4L2_PIX_FMT_SE401      v4l2_fourcc('S', '4', '0', '1') /* se401 janggu compressed rgb */
+#define V4L2_PIX_FMT_JPG_YUYV_S5C v4l2_fourcc('S', '5', 'C', 'J') /* S5C73M3 interleaved JPEG/YUYV */
 
 /*
  *	F O R M A T   E N U M E R A T I O N
-- 
1.7.9

[RFC/PATCH 3/6] V4L: Add g_embedded_data subdev callback

[Sylwester Nawrocki]

The g_embedded_data callback allows the host to retrieve frame embedded
(meta) data from a certain subdev. This callback can be implemented by
an image sensor or a MIPI-CSI receiver, allowing to read embedded frame
data from a subdev or just query it for the data size.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/media/v4l2-subdev.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index f0f3358..be74061 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -274,6 +274,14 @@ struct v4l2_subdev_audio_ops {
    s_mbus_config: set a certain mediabus configuration. This operation is added
 	for compatibility with soc-camera drivers and should not be used by new
 	software.
+
+   g_embedded_data: retrieve the frame embedded data (frame header or footer).
+	After a full frame has been transmitted the host can query a subdev
+	for frame meta data using this operation. Metadata size is returned
+	in @size, and the actual metadata in memory pointed by @data. When
+	@buf is NULL the subdev will return only the metadata size. The
+	subdevs can adjust @size to a lower value but must not write more
+	data than the @size's original value.
  */
 struct v4l2_subdev_video_ops {
 	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);
@@ -321,6 +329,8 @@ struct v4l2_subdev_video_ops {
 			     struct v4l2_mbus_config *cfg);
 	int (*s_mbus_config)(struct v4l2_subdev *sd,
 			     const struct v4l2_mbus_config *cfg);
+	int (*g_embedded_data)(struct v4l2_subdev *sd, unsigned int *size,
+			       void **buf);
 };
 
 /*
-- 
1.7.9

Re: [RFC/PATCH 3/6] V4L: Add g_embedded_data subdev callback

[Laurent Pinchart]

Hi Sylwester,

Thanks for the patch.

On Thursday 16 February 2012 19:23:56 Sylwester Nawrocki wrote:
> The g_embedded_data callback allows the host to retrieve frame embedded
> (meta) data from a certain subdev. This callback can be implemented by
> an image sensor or a MIPI-CSI receiver, allowing to read embedded frame
> data from a subdev or just query it for the data size.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  include/media/v4l2-subdev.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index f0f3358..be74061 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -274,6 +274,14 @@ struct v4l2_subdev_audio_ops {
>     s_mbus_config: set a certain mediabus configuration. This operation is
> added for compatibility with soc-camera drivers and should not be used by
> new software.
> +
> +   g_embedded_data: retrieve the frame embedded data (frame header or
> footer). +	After a full frame has been transmitted the host can query a
> subdev +	for frame meta data using this operation. Metadata size is
> returned +	in @size, and the actual metadata in memory pointed by @data.
> When +	@buf is NULL the subdev will return only the metadata size. The
> +	subdevs can adjust @size to a lower value but must not write more
> +	data than the @size's original value.
>   */
>  struct v4l2_subdev_video_ops {
>  	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32
> config); @@ -321,6 +329,8 @@ struct v4l2_subdev_video_ops {
>  			     struct v4l2_mbus_config *cfg);
>  	int (*s_mbus_config)(struct v4l2_subdev *sd,
>  			     const struct v4l2_mbus_config *cfg);
> +	int (*g_embedded_data)(struct v4l2_subdev *sd, unsigned int *size,
> +			       void **buf);
>  };

How is the embedded data transferred from the sensor to the host in your case 
? Over I2C ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 3/6] V4L: Add g_embedded_data subdev callback

[Sylwester Nawrocki]

Hi Laurent,

On 02/18/2012 12:23 AM, Laurent Pinchart wrote:
>>   struct v4l2_subdev_video_ops {
>>   	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32
>> config); @@ -321,6 +329,8 @@ struct v4l2_subdev_video_ops {
>>   			     struct v4l2_mbus_config *cfg);
>>   	int (*s_mbus_config)(struct v4l2_subdev *sd,
>>   			     const struct v4l2_mbus_config *cfg);
>> +	int (*g_embedded_data)(struct v4l2_subdev *sd, unsigned int *size,
>> +			       void **buf);
>>   };
>
> How is the embedded data transferred from the sensor to the host in your case
> ? Over I2C ?

It's transferred over MIPI-CSI2 bus and is intercepted by the MIPI-CSI2
receiver, before the image data DMA. The MIPI-CSI2 doesn't have its own
DMA engine. More details can be found in patch 6/6.

--

Regards,
Sylwester

Re: [RFC/PATCH 3/6] V4L: Add g_embedded_data subdev callback

[Laurent Pinchart]

Hi Sylwester,

On Saturday 18 February 2012 00:33:23 Sylwester Nawrocki wrote:
> On 02/18/2012 12:23 AM, Laurent Pinchart wrote:
> >>   struct v4l2_subdev_video_ops {
> >>   
> >>   	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32
> >> 
> >> config); @@ -321,6 +329,8 @@ struct v4l2_subdev_video_ops {
> >> 
> >>   			     struct v4l2_mbus_config *cfg);
> >>   	
> >>   	int (*s_mbus_config)(struct v4l2_subdev *sd,
> >>   	
> >>   			     const struct v4l2_mbus_config *cfg);
> >> 
> >> +	int (*g_embedded_data)(struct v4l2_subdev *sd, unsigned int *size,
> >> +			       void **buf);
> >> 
> >>   };
> > 
> > How is the embedded data transferred from the sensor to the host in your
> > case ? Over I2C ?
> 
> It's transferred over MIPI-CSI2 bus and is intercepted by the MIPI-CSI2
> receiver, before the image data DMA. The MIPI-CSI2 doesn't have its own
> DMA engine. More details can be found in patch 6/6.

As the data is transmitted by the device without any polling from the host, 
shouldn't it just go to a metadata plane in the V4L2 buffer ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 3/6] V4L: Add g_embedded_data subdev callback

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> On Saturday 18 February 2012 00:33:23 Sylwester Nawrocki wrote:
>> On 02/18/2012 12:23 AM, Laurent Pinchart wrote:
>>>>   struct v4l2_subdev_video_ops {
>>>>   
>>>>   	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32
>>>>
>>>> config); @@ -321,6 +329,8 @@ struct v4l2_subdev_video_ops {
>>>>
>>>>   			     struct v4l2_mbus_config *cfg);
>>>>   	
>>>>   	int (*s_mbus_config)(struct v4l2_subdev *sd,
>>>>   	
>>>>   			     const struct v4l2_mbus_config *cfg);
>>>>
>>>> +	int (*g_embedded_data)(struct v4l2_subdev *sd, unsigned int *size,
>>>> +			       void **buf);
>>>>
>>>>   };
>>>
>>> How is the embedded data transferred from the sensor to the host in your
>>> case ? Over I2C ?
>>
>> It's transferred over MIPI-CSI2 bus and is intercepted by the MIPI-CSI2
>> receiver, before the image data DMA. The MIPI-CSI2 doesn't have its own
>> DMA engine. More details can be found in patch 6/6.
> 
> As the data is transmitted by the device without any polling from the host, 
> shouldn't it just go to a metadata plane in the V4L2 buffer ?

I'd say that if it can be given to the user space separately, a way
should be provided to do that. It's all about timing: software
controlled digital camera depend on that.

Some receivers are also able to put such metadata into a separate memory
area using DMA, such as the OMAP 3 ISP CCP-2 receiver.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC/PATCH 3/6] V4L: Add g_embedded_data subdev callback

[Sylwester Nawrocki]

Hi Laurent,

On 02/18/2012 02:43 AM, Laurent Pinchart wrote:
> On Saturday 18 February 2012 00:33:23 Sylwester Nawrocki wrote:
>> On 02/18/2012 12:23 AM, Laurent Pinchart wrote:
>>>> config); @@ -321,6 +329,8 @@ struct v4l2_subdev_video_ops {
>>>>
>>>>    			     struct v4l2_mbus_config *cfg);
>>>>    	
>>>>    	int (*s_mbus_config)(struct v4l2_subdev *sd,
>>>>    	
>>>>    			     const struct v4l2_mbus_config *cfg);
>>>>
>>>> +	int (*g_embedded_data)(struct v4l2_subdev *sd, unsigned int *size,
>>>> +			       void **buf);
>>>>
>>>>    };
>>>
>>> How is the embedded data transferred from the sensor to the host in your
>>> case ? Over I2C ?
>>
>> It's transferred over MIPI-CSI2 bus and is intercepted by the MIPI-CSI2
>> receiver, before the image data DMA. The MIPI-CSI2 doesn't have its own
>> DMA engine. More details can be found in patch 6/6.
> 
> As the data is transmitted by the device without any polling from the host,
> shouldn't it just go to a metadata plane in the V4L2 buffer ?

That would be more correct and more optimal from performance perspective.
However I was a bit concerned about synchronization between two interrupt
handlers and over complicating subdev-host interface.

On the other hand even now I do rely on proper interrupts' sequence and
it shouldn't be difficult to make the subdev write directly to the metadata
plane.

As far as the timing is concerned, in my case it was taking about 300 us 
to copy metadata from the MIPI-CSI receiver IO memory to the subdev's 
internal buffer and about 5 us to copy it again from that buffer to V4L2
buffer metadata plane. So I wasn't concerned much about the additional
latency. Nevertheless the numbers may be different in other cases.

As I really don't consider exposing a video node by the MIPI-CSIS driver
I'm wondering if we need some kind of buffer operations at v4l2_subdev 
interface, that would be used only in kernel space ?

For example queue_buffer/dequeue_buffer callbacks, what do you think about
it ?

--

Regards,
Sylwester

[RFC/PATCH 4/6] V4L: Add get/set_frame_config subdev callbacks

[Sylwester Nawrocki]

Add subdev callbacks for setting up parameters of frame on media bus that
are not exposed to user space directly. This is more a stub containing
only parameters needed to setup V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 data
transmision and the associated frame embedded data.

The @length field of struct v4l2_frame_config determines maximum number
of frame samples per frame, excluding embedded non-image data.

@header_length and @footer length determine the size in bytes of data
embedded at frame beginning and end respectively.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/media/v4l2-subdev.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index be74061..bd95f00 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -21,6 +21,7 @@
 #ifndef _V4L2_SUBDEV_H
 #define _V4L2_SUBDEV_H
 
+#include <linux/types.h>
 #include <linux/v4l2-subdev.h>
 #include <media/media-entity.h>
 #include <media/v4l2-common.h>
@@ -45,6 +46,7 @@ struct v4l2_fh;
 struct v4l2_subdev;
 struct v4l2_subdev_fh;
 struct tuner_setup;
+struct v4l2_frame_config;
 
 /* decode_vbi_line */
 struct v4l2_decode_vbi_line {
@@ -476,6 +478,10 @@ struct v4l2_subdev_pad_ops {
 		       struct v4l2_subdev_crop *crop);
 	int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 		       struct v4l2_subdev_crop *crop);
+	int (*set_frame_config)(struct v4l2_subdev *sd, unsigned int pad,
+				struct v4l2_frame_config *fc);
+	int (*get_frame_config)(struct v4l2_subdev *sd, unsigned int pad,
+				struct v4l2_frame_config *fc);
 };
 
 struct v4l2_subdev_ops {
@@ -567,6 +573,18 @@ struct v4l2_subdev_fh {
 #define to_v4l2_subdev_fh(fh)	\
 	container_of(fh, struct v4l2_subdev_fh, vfh)
 
+/**
+ * struct v4l2_frame_config - media bus data frame configuration
+ * @length: maximum number of media bus samples per frame
+ * @header_length: size of embedded data at frame start (header)
+ * @footer_length: size of embedded data at frame end (footer)
+ */
+struct v4l2_frame_config {
+	size_t length;
+	size_t header_length;
+	size_t footer_length;
+};
+
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 static inline struct v4l2_mbus_framefmt *
 v4l2_subdev_get_try_format(struct v4l2_subdev_fh *fh, unsigned int pad)
-- 
1.7.9

Re: [RFC/PATCH 4/6] V4L: Add get/set_frame_config subdev callbacks

[Sakari Ailus]

Hi Sylwester,

Thanks for the patch.

Sylwester Nawrocki wrote:
> Add subdev callbacks for setting up parameters of frame on media bus that
> are not exposed to user space directly. This is more a stub containing
> only parameters needed to setup V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 data
> transmision and the associated frame embedded data.
> 
> The @length field of struct v4l2_frame_config determines maximum number
> of frame samples per frame, excluding embedded non-image data.
> 
> @header_length and @footer length determine the size in bytes of data
> embedded at frame beginning and end respectively.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  include/media/v4l2-subdev.h |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index be74061..bd95f00 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -21,6 +21,7 @@
>  #ifndef _V4L2_SUBDEV_H
>  #define _V4L2_SUBDEV_H
>  
> +#include <linux/types.h>
>  #include <linux/v4l2-subdev.h>
>  #include <media/media-entity.h>
>  #include <media/v4l2-common.h>
> @@ -45,6 +46,7 @@ struct v4l2_fh;
>  struct v4l2_subdev;
>  struct v4l2_subdev_fh;
>  struct tuner_setup;
> +struct v4l2_frame_config;
>  
>  /* decode_vbi_line */
>  struct v4l2_decode_vbi_line {
> @@ -476,6 +478,10 @@ struct v4l2_subdev_pad_ops {
>  		       struct v4l2_subdev_crop *crop);
>  	int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>  		       struct v4l2_subdev_crop *crop);
> +	int (*set_frame_config)(struct v4l2_subdev *sd, unsigned int pad,
> +				struct v4l2_frame_config *fc);
> +	int (*get_frame_config)(struct v4l2_subdev *sd, unsigned int pad,
> +				struct v4l2_frame_config *fc);
>  };
>  
>  struct v4l2_subdev_ops {
> @@ -567,6 +573,18 @@ struct v4l2_subdev_fh {
>  #define to_v4l2_subdev_fh(fh)	\
>  	container_of(fh, struct v4l2_subdev_fh, vfh)
>  
> +/**
> + * struct v4l2_frame_config - media bus data frame configuration
> + * @length: maximum number of media bus samples per frame
> + * @header_length: size of embedded data at frame start (header)
> + * @footer_length: size of embedded data at frame end (footer)
> + */
> +struct v4l2_frame_config {
> +	size_t length;
> +	size_t header_length;
> +	size_t footer_length;
> +};
> +
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  static inline struct v4l2_mbus_framefmt *
>  v4l2_subdev_get_try_format(struct v4l2_subdev_fh *fh, unsigned int pad)

I think we need something a little more expressive to describe the
metadata. Preferrably the structure of the whole frame.

Is the size of your metadata measured in just bytes? If we have a frame
that has width and height the metadata is just spread to a number of
lines. That's the case on the SMIA(++) driver, for example.

Is the length field intended to be what once was planned in
v4l2_mbus_framefmt and later on as a control?

Also, only some receivers will be able to separate the metadata from the
rest of the frame. The above struct doesn't have information on the
format of the metadata either.

I admit that I should have written an RFC on this but it's my general
lack of time that has prevented me from doing that. :-I

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC/PATCH 4/6] V4L: Add get/set_frame_config subdev callbacks

[Sylwester Nawrocki]

Hi Sakari,

thanks for your comments.

On 02/16/2012 11:44 PM, Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
>> Add subdev callbacks for setting up parameters of frame on media bus that
>> are not exposed to user space directly. This is more a stub containing
>> only parameters needed to setup V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8 data
>> transmision and the associated frame embedded data.
>>
>> The @length field of struct v4l2_frame_config determines maximum number
>> of frame samples per frame, excluding embedded non-image data.
>>
>> @header_length and @footer length determine the size in bytes of data
>> embedded at frame beginning and end respectively.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  include/media/v4l2-subdev.h |   18 ++++++++++++++++++
>>  1 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index be74061..bd95f00 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -21,6 +21,7 @@
>>  #ifndef _V4L2_SUBDEV_H
>>  #define _V4L2_SUBDEV_H
>>  
>> +#include <linux/types.h>
>>  #include <linux/v4l2-subdev.h>
>>  #include <media/media-entity.h>
>>  #include <media/v4l2-common.h>
>> @@ -45,6 +46,7 @@ struct v4l2_fh;
>>  struct v4l2_subdev;
>>  struct v4l2_subdev_fh;
>>  struct tuner_setup;
>> +struct v4l2_frame_config;
>>  
>>  /* decode_vbi_line */
>>  struct v4l2_decode_vbi_line {
>> @@ -476,6 +478,10 @@ struct v4l2_subdev_pad_ops {
>>  		       struct v4l2_subdev_crop *crop);
>>  	int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>>  		       struct v4l2_subdev_crop *crop);
>> +	int (*set_frame_config)(struct v4l2_subdev *sd, unsigned int pad,
>> +				struct v4l2_frame_config *fc);
>> +	int (*get_frame_config)(struct v4l2_subdev *sd, unsigned int pad,
>> +				struct v4l2_frame_config *fc);
>>  };
>>  
>>  struct v4l2_subdev_ops {
>> @@ -567,6 +573,18 @@ struct v4l2_subdev_fh {
>>  #define to_v4l2_subdev_fh(fh)	\
>>  	container_of(fh, struct v4l2_subdev_fh, vfh)
>>  
>> +/**
>> + * struct v4l2_frame_config - media bus data frame configuration
>> + * @length: maximum number of media bus samples per frame
>> + * @header_length: size of embedded data at frame start (header)
>> + * @footer_length: size of embedded data at frame end (footer)
>> + */
>> +struct v4l2_frame_config {
>> +	size_t length;
>> +	size_t header_length;
>> +	size_t footer_length;
>> +};
>> +
>>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>>  static inline struct v4l2_mbus_framefmt *
>>  v4l2_subdev_get_try_format(struct v4l2_subdev_fh *fh, unsigned int pad)
>
> I think we need something a little more expressive to describe the
> metadata. Preferrably the structure of the whole frame.

Yes, that was my intention. This patch is really just a starting point.
I wanted this data structure to be describing the frame data organization,
not necessarily containing all the details about each frame's part. Those
would be available through other data structures/callbacks.

> Is the size of your metadata measured in just bytes? If we have a frame
> that has width and height the metadata is just spread to a number of
> lines. That's the case on the SMIA(++) driver, for example.

Yes, it's in bytes. Number of lines is not helpful when you have 2 frames
of distinct resolutions mixed in one data plane.

Then we need to express the number of lines as well. If we need only size
in bytes or size in number of pixel scan lines (without HBLANK ?) the probably
a union would do ? On the other hand, it would be not obvious to the hosts
which union member is used by which sensor. So maybe (just a rough idea):

struct v4l2_frame_config {
	struct {
		unsigned int num_lines;
		size_t length;
	} header;

	struct {
		size_t length;
	} data;

	struct {
		unsigned int num_lines;
		size_t length;
	} header;
};

?
> Is the length field intended to be what once was planned in
> v4l2_mbus_framefmt and later on as a control?

Yes, that's same thing. It seemed more appropriate to me to handle it this
way. If some subdevs need adjusting it from user space probably a private
control is good for that.

> Also, only some receivers will be able to separate the metadata from the
> rest of the frame. The above struct doesn't have information on the
> format of the metadata either.

I skipped it deliberately, given diversity of formats amongs various hardware.
I assumed metadata is passed transparently by the hosts and they don't need
to know all details of the meta data. Obviously that's something still could
be addressed in future, I guess...

> I admit that I should have written an RFC on this but it's my general
> lack of time that has prevented me from doing that. :-I

Yeah, AFAIR you brought up an idea of the frame description during previous
discussions. Still I can see nothing really preventing you from writing
the RFC :-)

-- 

Thanks,
Sylwester

[RFC/PATCH 5/6] s5p-fimc: Add support for V4L2_PIX_FMT_JPG_YUYV_S5C fourcc

[Sylwester Nawrocki]

The V4L2_PIX_FMT_JPG_YUYV_S5C image formats consists of
2 planes, the first containing interleaved JPEG/YUYV data
and the second containing meta data describing the
interleaving method.

The image data is transferred with MIPI-CSI "User Defined
Byte-Based Data 1" type and is captured to memory by FIMC
DMA engine.

The meta data is transferred using MIPI-CSI2 "Embedded 8-bit
non Image Data" and it is captured in the MIPI-CSI slave device
and retrieved by the bridge through a subdev callback.

Copying data from MIPI-CSIS doesn't influence performance a lot
(copying 4KiB buffer takes about 5 us) and simplifies the subdev
- bridge interactions. In opposite to having the bridge driver
allocating memory for meta data and passing it to the subdev.

The interrupt_service_routine subdev callback is used to provide
the MIPI-CSI receiver subdev with frame sequence numbers. This
is needed to discard non-image packets when FIMC DMA engine is
stopped, e.g. because user space is not de-queueing buffers.

To make sure the size of allocated buffers is correct for a
subdev configuration during VIDIOC_STREAMON ioctl the video
pipeline validation has been extended with an additional
check.

Flag FMT_FLAGS_COMPRESSED indicates the buffer size should
be retrieved from a sensor subdev.

For the discussion purposes this patch relies on new vendor
specific media bus pixel code - V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-capture.c |  123 ++++++++++++++++++++------
 drivers/media/video/s5p-fimc/fimc-core.c    |   37 ++++++++-
 drivers/media/video/s5p-fimc/fimc-core.h    |   22 +++++-
 drivers/media/video/s5p-fimc/fimc-reg.c     |    5 +-
 drivers/media/video/s5p-fimc/mipi-csis.c    |    7 +-
 5 files changed, 157 insertions(+), 37 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index b06efd2..fb4eea8 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -246,29 +246,27 @@ int fimc_capture_resume(struct fimc_dev *fimc)
 
 }
 
-static unsigned int get_plane_size(struct fimc_frame *fr, unsigned int plane)
-{
-	if (!fr || plane >= fr->fmt->memplanes)
-		return 0;
-	return fr->f_width * fr->f_height * fr->fmt->depth[plane] / 8;
-}
-
 static int queue_setup(struct vb2_queue *vq,  const struct v4l2_format *pfmt,
 		       unsigned int *num_buffers, unsigned int *num_planes,
 		       unsigned int sizes[], void *allocators[])
 {
 	struct fimc_ctx *ctx = vq->drv_priv;
-	struct fimc_fmt *fmt = ctx->d_frame.fmt;
-	int i;
+	struct fimc_frame *f = &ctx->d_frame;
+	int plane;
 
-	if (!fmt)
+	if (!f->fmt)
 		return -EINVAL;
 
-	*num_planes = fmt->memplanes;
+	*num_planes = f->fmt->memplanes;
+
+	for (plane = 0; plane < f->fmt->memplanes; plane++) {
+		allocators[plane] = ctx->fimc_dev->alloc_ctx;
 
-	for (i = 0; i < fmt->memplanes; i++) {
-		sizes[i] = get_plane_size(&ctx->d_frame, i);
-		allocators[i] = ctx->fimc_dev->alloc_ctx;
+		if (fimc_fmt_is_user_defined(f->fmt->color))
+			sizes[plane] = f->payload[plane];
+		else
+			sizes[plane] = f->f_width * f->f_height *
+				f->fmt->depth[plane] / 8;
 	}
 
 	return 0;
@@ -493,10 +491,10 @@ static struct fimc_fmt *fimc_capture_try_format(struct fimc_ctx *ctx,
 	u32 mask = FMT_FLAGS_CAM;
 	struct fimc_fmt *ffmt;
 
-	/* Color conversion from/to JPEG is not supported */
+	/* Conversion from/to JPEG or User Defined format is not supported */
 	if (code && ctx->s_frame.fmt && pad == FIMC_SD_PAD_SOURCE &&
-	    fimc_fmt_is_jpeg(ctx->s_frame.fmt->color))
-		*code = V4L2_MBUS_FMT_JPEG_1X8;
+	    fimc_fmt_is_user_defined(ctx->s_frame.fmt->color))
+		*code = ctx->s_frame.fmt->mbus_code;
 
 	if (fourcc && *fourcc != V4L2_PIX_FMT_JPEG && pad != FIMC_SD_PAD_SINK)
 		mask |= FMT_FLAGS_M2M;
@@ -510,18 +508,19 @@ static struct fimc_fmt *fimc_capture_try_format(struct fimc_ctx *ctx,
 		*fourcc = ffmt->fourcc;
 
 	if (pad == FIMC_SD_PAD_SINK) {
-		max_w = fimc_fmt_is_jpeg(ffmt->color) ?
+		max_w = fimc_fmt_is_user_defined(ffmt->color) ?
 			pl->scaler_dis_w : pl->scaler_en_w;
 		/* Apply the camera input interface pixel constraints */
 		v4l_bound_align_image(width, max_t(u32, *width, 32), max_w, 4,
 				      height, max_t(u32, *height, 32),
 				      FIMC_CAMIF_MAX_HEIGHT,
-				      fimc_fmt_is_jpeg(ffmt->color) ? 3 : 1,
+				      fimc_fmt_is_user_defined(ffmt->color) ?
+				      3 : 1,
 				      0);
 		return ffmt;
 	}
 	/* Can't scale or crop in transparent (JPEG) transfer mode */
-	if (fimc_fmt_is_jpeg(ffmt->color)) {
+	if (fimc_fmt_is_user_defined(ffmt->color)) {
 		*width  = ctx->s_frame.f_width;
 		*height = ctx->s_frame.f_height;
 		return ffmt;
@@ -560,7 +559,7 @@ static void fimc_capture_try_crop(struct fimc_ctx *ctx, struct v4l2_rect *r,
 	u32 max_sc_h, max_sc_v;
 
 	/* In JPEG transparent transfer mode cropping is not supported */
-	if (fimc_fmt_is_jpeg(ctx->d_frame.fmt->color)) {
+	if (fimc_fmt_is_user_defined(ctx->d_frame.fmt->color)) {
 		r->width  = sink->f_width;
 		r->height = sink->f_height;
 		r->left   = r->top = 0;
@@ -723,6 +722,36 @@ static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
 	return 0;
 }
 
+/* Query the sensor for required buffer size (only for compressed data). */
+static int fimc_sd_get_plane_size(struct fimc_dev *fimc, unsigned int *size,
+				  unsigned int plane)
+{
+	struct v4l2_subdev *sd = fimc->pipeline.sensor;
+	struct v4l2_frame_config fc;
+	int ret;
+
+	fc.length = *size;
+
+	ret = v4l2_subdev_call(sd, pad, set_frame_config, 0, &fc);
+	if (ret < 0)
+		return ret;
+
+	switch (plane) {
+	case 0:
+		if (fc.length > FIMC_MAX_JPEG_BUF_SIZE) {
+			v4l2_err(sd, "Unsupported buffer size\n");
+			return -EINVAL;
+		}
+		*size = fc.length;
+		break;
+	case 1:
+		*size = fc.footer_length;
+		break;
+	}
+
+	return 0;
+}
+
 static int fimc_cap_g_fmt_mplane(struct file *file, void *fh,
 				 struct v4l2_format *f)
 {
@@ -743,11 +772,12 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	struct fimc_ctx *ctx = fimc->vid_cap.ctx;
 	struct v4l2_mbus_framefmt mf;
 	struct fimc_fmt *ffmt = NULL;
+	int i;
 
 	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
 		return -EINVAL;
 
-	if (pix->pixelformat == V4L2_PIX_FMT_JPEG) {
+	if (fimc_jpeg_fourcc(pix->pixelformat)) {
 		fimc_capture_try_format(ctx, &pix->width, &pix->height,
 					NULL, &pix->pixelformat,
 					FIMC_SD_PAD_SINK);
@@ -775,11 +805,26 @@ static int fimc_cap_try_fmt_mplane(struct file *file, void *fh,
 	}
 
 	fimc_adjust_mplane_format(ffmt, pix->width, pix->height, pix);
+
+	if (!(ffmt->flags & FMT_FLAGS_COMPRESSED))
+		return 0;
+
+	for (i = 0; i < ffmt->memplanes; i++) {
+		unsigned int size = 0;
+		int ret = fimc_sd_get_plane_size(fimc, &size, i);
+		if (ret < 0)
+			return ret;
+		pix->plane_fmt[i].sizeimage = size;
+	}
+
 	return 0;
 }
 
-static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx, bool jpeg)
+static void fimc_capture_mark_jpeg_xfer(struct fimc_ctx *ctx,
+					enum fimc_color_fmt color)
 {
+	bool jpeg = fimc_fmt_is_user_defined(color);
+
 	ctx->scaler.enabled = !jpeg;
 	fimc_ctrls_activate(ctx, !jpeg);
 
@@ -804,7 +849,7 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 		return -EBUSY;
 
 	/* Pre-configure format at camera interface input, for JPEG only */
-	if (pix->pixelformat == V4L2_PIX_FMT_JPEG) {
+	if (fimc_jpeg_fourcc(pix->pixelformat)) {
 		fimc_capture_try_format(ctx, &pix->width, &pix->height,
 					NULL, &pix->pixelformat,
 					FIMC_SD_PAD_SINK);
@@ -836,16 +881,28 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 		pix->height = mf->height;
 	}
 	fimc_adjust_mplane_format(ff->fmt, pix->width, pix->height, pix);
-	for (i = 0; i < ff->fmt->colplanes; i++)
-		ff->payload[i] =
-			(pix->width * pix->height * ff->fmt->depth[i]) / 8;
+
+	if (ff->fmt->flags & FMT_FLAGS_COMPRESSED) {
+		for (i = 0; i < ff->fmt->memplanes; i++) {
+			unsigned int size = 0;
+			ret = fimc_sd_get_plane_size(fimc, &size, i);
+			if (ret < 0)
+				return ret;
+			pix->plane_fmt[i].sizeimage = size;
+			ff->payload[i] = size;
+		}
+	} else {
+		for (i = 0; i < ff->fmt->colplanes; i++)
+			ff->payload[i] = pix->width * pix->height *
+					 ff->fmt->depth[i] / 8;
+	}
 
 	set_frame_bounds(ff, pix->width, pix->height);
 	/* Reset the composition rectangle if not yet configured */
 	if (!(ctx->state & FIMC_DST_CROP))
 		set_frame_crop(ff, 0, 0, pix->width, pix->height);
 
-	fimc_capture_mark_jpeg_xfer(ctx, fimc_fmt_is_jpeg(ff->fmt->color));
+	fimc_capture_mark_jpeg_xfer(ctx, ff->fmt->color);
 
 	/* Reset cropping and set format at the camera interface input */
 	if (!fimc->vid_cap.user_subdev_api) {
@@ -947,6 +1004,14 @@ static int fimc_pipeline_validate(struct fimc_dev *fimc)
 		    src_fmt.format.height != sink_fmt.format.height ||
 		    src_fmt.format.code != sink_fmt.format.code)
 			return -EPIPE;
+
+		if (sd == fimc->pipeline.sensor &&
+		    fimc_user_defined_mbus_fmt(src_fmt.format.code)) {
+			unsigned int size = 0;
+			fimc_sd_get_plane_size(fimc, &size, 0);
+			if (vid_cap->ctx->d_frame.payload[0] < size)
+				return -EPIPE;
+		    }
 	}
 	return 0;
 }
@@ -1314,7 +1379,7 @@ static int fimc_subdev_set_fmt(struct v4l2_subdev *sd,
 	/* Update RGB Alpha control state and value range */
 	fimc_alpha_ctrl_update(ctx);
 
-	fimc_capture_mark_jpeg_xfer(ctx, fimc_fmt_is_jpeg(ffmt->color));
+	fimc_capture_mark_jpeg_xfer(ctx, ffmt->color);
 
 	ff = fmt->pad == FIMC_SD_PAD_SINK ?
 		&ctx->s_frame : &ctx->d_frame;
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index e184e65..8e53c9a 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -183,7 +183,17 @@ static struct fimc_fmt fimc_formats[] = {
 		.memplanes	= 1,
 		.colplanes	= 1,
 		.mbus_code	= V4L2_MBUS_FMT_JPEG_1X8,
-		.flags		= FMT_FLAGS_CAM,
+		.flags		= FMT_FLAGS_CAM | FMT_FLAGS_COMPRESSED,
+	}, {
+		.name		= "Interleaved JPEG/YUYV S5C73MX sensor data",
+		.fourcc		= V4L2_PIX_FMT_JPG_YUYV_S5C,
+		.color		= S5P_FIMC_VYUY_JPEG,
+		.depth		= { 8 },
+		.memplanes	= 2,
+		.colplanes	= 1,
+		.mdataplanes	= 0x2, /* plane 1 holds frame meta data */
+		.mbus_code	= V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8,
+		.flags		= FMT_FLAGS_CAM | FMT_FLAGS_COMPRESSED,
 	},
 };
 
@@ -353,6 +363,7 @@ static int stop_streaming(struct vb2_queue *q)
 
 void fimc_capture_irq_handler(struct fimc_dev *fimc, bool final)
 {
+	struct v4l2_subdev *csis = fimc->pipeline.csis;
 	struct fimc_vid_cap *cap = &fimc->vid_cap;
 	struct fimc_vid_buffer *v_buf;
 	struct timeval *tv;
@@ -363,8 +374,12 @@ void fimc_capture_irq_handler(struct fimc_dev *fimc, bool final)
 		return;
 	}
 
+	v4l2_subdev_call(csis, core, interrupt_service_routine, 0, NULL);
+
 	if (!list_empty(&cap->active_buf_q) &&
 	    test_bit(ST_CAPT_RUN, &fimc->state) && final) {
+		struct fimc_frame *f = &cap->ctx->d_frame;
+
 		ktime_get_real_ts(&ts);
 
 		v_buf = fimc_active_queue_pop(cap);
@@ -374,6 +389,19 @@ void fimc_capture_irq_handler(struct fimc_dev *fimc, bool final)
 		tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 		v_buf->vb.v4l2_buf.sequence = cap->frame_count++;
 
+		if (f->fmt->mdataplanes) {
+			unsigned int size = f->payload[1];
+			void *vaddr;
+			dma_addr_t addr;
+
+			addr = vb2_dma_contig_plane_dma_addr(&v_buf->vb,
+					     ffs(f->fmt->mdataplanes) - 1);
+			vaddr = phys_to_virt(addr);
+
+			v4l2_subdev_call(csis, video, g_embedded_data,
+					 &size, &vaddr);
+		}
+
 		vb2_buffer_done(&v_buf->vb, VB2_BUF_STATE_DONE);
 	}
 
@@ -492,7 +520,7 @@ int fimc_prepare_addr(struct fimc_ctx *ctx, struct vb2_buffer *vb,
 		default:
 			return -EINVAL;
 		}
-	} else {
+	} else if (!frame->fmt->mdataplanes) {
 		if (frame->fmt->memplanes >= 2)
 			paddr->cb = vb2_dma_contig_plane_dma_addr(vb, 1);
 
@@ -968,6 +996,11 @@ int fimc_fill_format(struct fimc_frame *frame, struct v4l2_format *f)
 		if (frame->fmt->colplanes == 1) /* packed formats */
 			bpl = (bpl * frame->fmt->depth[0]) / 8;
 		pixm->plane_fmt[i].bytesperline = bpl;
+
+		if (frame->fmt->flags & FMT_FLAGS_COMPRESSED) {
+			pixm->plane_fmt[i].sizeimage = frame->payload[i];
+			continue;
+		}
 		pixm->plane_fmt[i].sizeimage = (frame->o_width *
 			frame->o_height * frame->fmt->depth[i]) / 8;
 	}
diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index a18291e..f42260f 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -17,6 +17,7 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 #include <linux/io.h>
+#include <asm/sizes.h>
 
 #include <media/media-entity.h>
 #include <media/videobuf2-core.h>
@@ -44,6 +45,7 @@
 #define SCALER_MAX_VRATIO	64
 #define DMA_MIN_SIZE		8
 #define FIMC_CAMIF_MAX_HEIGHT	0x2000
+#define FIMC_MAX_JPEG_BUF_SIZE	SZ_8M
 
 /* indices to the clocks array */
 enum {
@@ -98,10 +100,11 @@ enum fimc_color_fmt {
 	S5P_FIMC_CRYCBY422,
 	S5P_FIMC_YCBCR444_LOCAL,
 	S5P_FIMC_JPEG = 0x40,
+	S5P_FIMC_VYUY_JPEG = 0x80,
 };
 
+#define fimc_fmt_is_user_defined(x) (!!((x) & 0xC0))
 #define fimc_fmt_is_rgb(x) (!!((x) & 0x10))
-#define fimc_fmt_is_jpeg(x) (!!((x) & 0x40))
 
 #define IS_M2M(__strt) ((__strt) == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE || \
 			__strt == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
@@ -151,6 +154,7 @@ enum fimc_color_fmt {
  * @memplanes: number of physically non-contiguous data planes
  * @colplanes: number of physically contiguous data planes
  * @depth: per plane driver's private 'number of bits per pixel'
+ * @mdataplanes: bitmask indicating meta data plane(s), (1 << plane_no)
  * @flags: flags indicating which operation mode format applies to
  */
 struct fimc_fmt {
@@ -161,12 +165,14 @@ struct fimc_fmt {
 	u16	memplanes;
 	u16	colplanes;
 	u8	depth[VIDEO_MAX_PLANES];
+	u16	mdataplanes;
 	u16	flags;
 #define FMT_FLAGS_CAM		(1 << 0)
 #define FMT_FLAGS_M2M_IN	(1 << 1)
 #define FMT_FLAGS_M2M_OUT	(1 << 2)
 #define FMT_FLAGS_M2M		(1 << 1 | 1 << 2)
 #define FMT_HAS_ALPHA		(1 << 3)
+#define FMT_FLAGS_COMPRESSED	(1 << 4)
 };
 
 /**
@@ -284,7 +290,7 @@ struct fimc_frame {
 	u32	offs_v;
 	u32	width;
 	u32	height;
-	unsigned long		payload[VIDEO_MAX_PLANES];
+	unsigned int		payload[VIDEO_MAX_PLANES];
 	struct fimc_addr	paddr;
 	struct fimc_dma_offset	dma_offset;
 	struct fimc_fmt		*fmt;
@@ -585,6 +591,18 @@ static inline int tiled_fmt(struct fimc_fmt *fmt)
 	return fmt->fourcc == V4L2_PIX_FMT_NV12MT;
 }
 
+static inline bool fimc_jpeg_fourcc(u32 pixelformat)
+{
+	return (pixelformat == V4L2_PIX_FMT_JPEG ||
+		pixelformat == V4L2_PIX_FMT_JPG_YUYV_S5C);
+}
+
+static inline bool fimc_user_defined_mbus_fmt(u32 code)
+{
+	return (code == V4L2_MBUS_FMT_JPEG_1X8 ||
+		code == V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8);
+}
+
 /* Return the alpha component bit mask */
 static inline int fimc_get_alpha_mask(struct fimc_fmt *fmt)
 {
diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c b/drivers/media/video/s5p-fimc/fimc-reg.c
index 15466d0..2821459 100644
--- a/drivers/media/video/s5p-fimc/fimc-reg.c
+++ b/drivers/media/video/s5p-fimc/fimc-reg.c
@@ -637,7 +637,7 @@ int fimc_hw_set_camera_source(struct fimc_dev *fimc,
 				cfg |= S5P_CISRCFMT_ITU601_16BIT;
 		} /* else defaults to ITU-R BT.656 8-bit */
 	} else if (cam->bus_type == FIMC_MIPI_CSI2) {
-		if (fimc_fmt_is_jpeg(f->fmt->color))
+		if (fimc_fmt_is_user_defined(f->fmt->color))
 			cfg |= S5P_CISRCFMT_ITU601_8BIT;
 	}
 
@@ -694,12 +694,13 @@ int fimc_hw_set_camera_type(struct fimc_dev *fimc,
 			tmp = S5P_CSIIMGFMT_YCBCR422_8BIT;
 			break;
 		case V4L2_MBUS_FMT_JPEG_1X8:
+		case V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8:
 			tmp = S5P_CSIIMGFMT_USER(1);
 			cfg |= S5P_CIGCTRL_CAM_JPEG;
 			break;
 		default:
 			v4l2_err(fimc->vid_cap.vfd,
-				 "Not supported camera pixel format: %d",
+				 "Not supported camera pixel format: %#x",
 				 vid_cap->mf.code);
 			return -EINVAL;
 		}
diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c
index a903138..04f1a0d 100644
--- a/drivers/media/video/s5p-fimc/mipi-csis.c
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -141,7 +141,10 @@ static const struct csis_pix_format s5pcsis_formats[] = {
 	}, {
 		.code = V4L2_MBUS_FMT_JPEG_1X8,
 		.fmt_reg = S5PCSIS_CFG_FMT_USER(1),
-	},
+	}, {
+		.code = V4L2_MBUS_FMT_VYUY_JPEG_I1_1X8,
+		.fmt_reg = S5PCSIS_CFG_FMT_USER(1),
+	}
 };
 
 #define s5pcsis_write(__csis, __r, __v) writel(__v, __csis->regs + __r)
@@ -205,7 +208,7 @@ static void __s5pcsis_set_format(struct csis_state *state)
 	struct v4l2_mbus_framefmt *mf = &state->format;
 	u32 val;
 
-	v4l2_dbg(1, debug, &state->sd, "fmt: %d, %d x %d\n",
+	v4l2_dbg(1, debug, &state->sd, "fmt: %#x, %d x %d\n",
 		 mf->code, mf->width, mf->height);
 
 	/* Color format */
-- 
1.7.9

[RFC/PATCH 6/6] s5p-csis: Add support for non-image data packets capture

[Sylwester Nawrocki]

MIPI-CSI has internal memory mapped buffers for the frame embedded
(non-image) data. There are two buffers, for even and odd frames which
need to be saved after an interrupt is raised. The packet data buffers
size is 4 KiB and there is no status register in the hardware where the
actual non-image data size can be read from. Hence the driver uses
pre-allocated buffers of 4 KiB in size to save the whole PKTDATA memory.
The packet data is copied to the bridge allocated buffers within
g_embedded_data callback. This will form a separate plane in the user
buffer.

When FIMC DMA engine is stopped by the driver due the to user space
not keeping up with buffer de-queuing the MIPI-CSIS will still run,
however it must discard data which is not captured by FIMC. For this
purpose FIMC driver sends VSYNC interrupt notification through
interrupt_service_routine callback which provides MIPI-CSIS with
information the header/footer of which frame is to be captured.

This patch also adds the hardware event/error counters which can be
dumped through VIDIOC_LOG_STATUS ioctl. The counters are reset in each
s_stream(1) call. Any errors are logged after streaming is turned off.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/mipi-csis.c |  305 ++++++++++++++++++++++++++++--
 1 files changed, 286 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c
index 04f1a0d..15a0eb5 100644
--- a/drivers/media/video/s5p-fimc/mipi-csis.c
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -9,6 +9,8 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt) "%s:%d " fmt, __func__, __LINE__
+
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -25,13 +27,14 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/videodev2.h>
+#include <linux/vmalloc.h>
 #include <media/v4l2-subdev.h>
 #include <plat/mipi_csis.h>
 #include "mipi-csis.h"
 
 static int debug;
 module_param(debug, int, 0644);
-MODULE_PARM_DESC(debug, "Debug level (0-1)");
+MODULE_PARM_DESC(debug, "Debug level (0-2)");
 
 /* Register map definition */
 
@@ -60,16 +63,51 @@ MODULE_PARM_DESC(debug, "Debug level (0-1)");
 #define S5PCSIS_CFG_FMT_MASK		(0x3f << 2)
 #define S5PCSIS_CFG_NR_LANE_MASK	3
 
-/* Interrupt mask. */
+/* Interrupt mask */
 #define S5PCSIS_INTMSK			0x10
-#define S5PCSIS_INTMSK_EN_ALL		0xf000003f
+#define S5PCSIS_INTMSK_EN_ALL		0xf000103f
+#define S5PCSIS_INTMSK_EVEN_BEFORE	(1 << 31)
+#define S5PCSIS_INTMSK_EVEN_AFTER	(1 << 30)
+#define S5PCSIS_INTMSK_ODD_BEFORE	(1 << 29)
+#define S5PCSIS_INTMSK_ODD_AFTER	(1 << 28)
+#define S5PCSIS_INTMSK_ERR_SOT_HS	(1 << 12)
+#define S5PCSIS_INTMSK_ERR_LOST_FS	(1 << 5)
+#define S5PCSIS_INTMSK_ERR_LOST_FE	(1 << 4)
+#define S5PCSIS_INTMSK_ERR_OVER		(1 << 3)
+#define S5PCSIS_INTMSK_ERR_ECC		(1 << 2)
+#define S5PCSIS_INTMSK_ERR_CRC		(1 << 1)
+#define S5PCSIS_INTMSK_ERR_UNKNOWN	(1 << 0)
+
+/* Interrupt source */
 #define S5PCSIS_INTSRC			0x14
+#define S5PCSIS_INTSRC_EVEN_BEFORE	(1 << 31)
+#define S5PCSIS_INTSRC_EVEN_AFTER	(1 << 30)
+#define S5PCSIS_INTSRC_EVEN		(0x3 << 30)
+#define S5PCSIS_INTSRC_ODD_BEFORE	(1 << 29)
+#define S5PCSIS_INTSRC_ODD_AFTER	(1 << 28)
+#define S5PCSIS_INTSRC_ODD		(0x3 << 28)
+#define S5PCSIS_INTSRC_NON_IMAGE_DATA	(0xff << 28)
+#define S5PCSIS_INTSRC_ERR_SOT_HS	(0xf << 12)
+#define S5PCSIS_INTSRC_ERR_LOST_FS	(1 << 5)
+#define S5PCSIS_INTSRC_ERR_LOST_FE	(1 << 4)
+#define S5PCSIS_INTSRC_ERR_OVER		(1 << 3)
+#define S5PCSIS_INTSRC_ERR_ECC		(1 << 2)
+#define S5PCSIS_INTSRC_ERR_CRC		(1 << 1)
+#define S5PCSIS_INTSRC_ERR_UNKNOWN	(1 << 0)
+#define S5PCSIS_INTSRC_ERRORS		0xf03f
 
 /* Pixel resolution */
 #define S5PCSIS_RESOL			0x2c
 #define CSIS_MAX_PIX_WIDTH		0xffff
 #define CSIS_MAX_PIX_HEIGHT		0xffff
 
+/* Non-image packet data buffers */
+#define S5PCSIS_PKTDATA_ODD		0x2000
+#define S5PCSIS_PKTDATA_EVEN		0x3000
+#define S5PCSIS_PKTDATA_SIZE		SZ_4K
+/* Number of non-image data buffers */
+#define S5PCSIS_NUM_BUFFERS		4
+
 enum {
 	CSIS_CLK_MUX,
 	CSIS_CLK_GATE,
@@ -93,6 +131,35 @@ enum {
 	ST_SUSPENDED	= 4,
 };
 
+struct s5pcsis_event {
+	u32 mask;
+	const char * const name;
+	unsigned int counter;
+};
+
+static const struct s5pcsis_event s5pcsis_events[] = {
+	/* Errors */
+	{ S5PCSIS_INTSRC_ERR_SOT_HS,	"SOT Error" },
+	{ S5PCSIS_INTSRC_ERR_LOST_FS,	"Lost FS Error" },
+	{ S5PCSIS_INTSRC_ERR_LOST_FE,	"Lost FE Error" },
+	{ S5PCSIS_INTSRC_ERR_OVER,	"OVER Error" },
+	{ S5PCSIS_INTSRC_ERR_ECC,	"ECC Error" },
+	{ S5PCSIS_INTSRC_ERR_CRC,	"CRC Error" },
+	{ S5PCSIS_INTSRC_ERR_UNKNOWN,	"Unknown Error" },
+	/* Non-image data receive events */
+	{ S5PCSIS_INTSRC_EVEN_BEFORE,	"Non-image data before even frame" },
+	{ S5PCSIS_INTSRC_EVEN_AFTER,	"Non-image data after even frame" },
+	{ S5PCSIS_INTSRC_ODD_BEFORE,	"Non-image data before odd frame" },
+	{ S5PCSIS_INTSRC_ODD_AFTER,	"Non-image data after odd frame" },
+};
+#define S5PCSIS_NUM_EVENTS ARRAY_SIZE(s5pcsis_events)
+
+struct csis_pktbuf {
+	u32 *data;
+	unsigned int len;
+	struct list_head list;
+};
+
 /**
  * struct csis_state - the driver's internal state data structure
  * @lock: mutex serializing the subdev and power management operations,
@@ -101,11 +168,19 @@ enum {
  * @sd: v4l2_subdev associated with CSIS device instance
  * @pdev: CSIS platform device
  * @regs: mmaped I/O registers memory
+ * @supplies: CSIS regulator supplies
  * @clock: CSIS clocks
  * @irq: requested s5p-mipi-csis irq number
  * @flags: the state variable for power and streaming control
  * @csis_fmt: current CSIS pixel format
  * @format: common media bus format for the source and sink pad
+ * @slock: spinlock protecting structure members below
+ * @empty_buf_list: list of empty non-image data buffers
+ * @empty_buf_list: list of filled non-image data buffers
+ * @buffers: the frame embedded (non-image) data buffers
+ * @frame_seq: data frame sequence number (updated from FIMC)
+ * @last_frame_seq: sequence number of the last serviced frame
+ * @events: MIPI-CSIS event (error) counters
  */
 struct csis_state {
 	struct mutex lock;
@@ -119,6 +194,14 @@ struct csis_state {
 	u32 flags;
 	const struct csis_pix_format *csis_fmt;
 	struct v4l2_mbus_framefmt format;
+
+	struct spinlock slock;
+	struct list_head empty_buf_list;
+	struct list_head ready_buf_list;
+	struct csis_pktbuf buffers[S5PCSIS_NUM_BUFFERS];
+	unsigned int frame_seq;
+	unsigned int last_frame_seq;
+	struct s5pcsis_event events[S5PCSIS_NUM_EVENTS];
 };
 
 /**
@@ -291,17 +374,6 @@ err:
 	return -ENXIO;
 }
 
-static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
-{
-	struct csis_state *state = sd_to_csis_state(sd);
-	struct device *dev = &state->pdev->dev;
-
-	if (on)
-		return pm_runtime_get_sync(dev);
-
-	return pm_runtime_put_sync(dev);
-}
-
 static void s5pcsis_start_stream(struct csis_state *state)
 {
 	s5pcsis_reset(state);
@@ -316,7 +388,86 @@ static void s5pcsis_stop_stream(struct csis_state *state)
 	s5pcsis_system_enable(state, false);
 }
 
-/* v4l2_subdev operations */
+/*
+ * Non-image data packet queue handling
+ */
+static void s5pcsis_free_packet_queue(struct csis_state *state)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(state->buffers); i++) {
+		vfree(state->buffers[i].data);
+		state->buffers[i].data = NULL;
+		state->buffers[i].len = 0;
+	}
+}
+
+static int s5pcsis_init_packet_queue(struct csis_state *state)
+{
+	struct csis_pktbuf *buf;
+	int i, ret = -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(state->buffers); i++) {
+		buf = &state->buffers[i];
+		buf->data = vmalloc(S5PCSIS_PKTDATA_SIZE);
+		if (buf->data == NULL)
+			goto err;
+		buf->len = S5PCSIS_PKTDATA_SIZE;
+		list_add_tail(&buf->list, &state->empty_buf_list);
+	}
+
+	return 0;
+err:
+	s5pcsis_free_packet_queue(state);
+	v4l2_err(&state->sd, "%s failed\n", __func__);
+	return ret;
+}
+
+static void s5pcsis_clear_counters(struct csis_state *state)
+{
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&state->slock, flags);
+
+	for (i = 0; i < S5PCSIS_NUM_EVENTS; i++)
+		state->events[i].counter = 0;
+	state->last_frame_seq = 0;
+	state->frame_seq = 0;
+	spin_unlock_irqrestore(&state->slock, flags);
+}
+
+static void s5pcsis_log_counters(struct csis_state *state, bool non_errors)
+{
+	unsigned long flags;
+	int i = non_errors ? S5PCSIS_NUM_EVENTS : S5PCSIS_NUM_EVENTS - 4;
+
+	spin_lock_irqsave(&state->slock, flags);
+
+	for (i--; i >= 0; i--)
+		if (state->events[i].counter >= 0)
+			v4l2_info(&state->sd, "%s events: %d\n",
+				  state->events[i].name,
+				  state->events[i].counter);
+
+	v4l2_info(&state->sd, "frame sequence: %d\n", state->frame_seq);
+	spin_unlock_irqrestore(&state->slock, flags);
+}
+
+/*
+ * V4L2 subdev operations
+ */
+static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct csis_state *state = sd_to_csis_state(sd);
+	struct device *dev = &state->pdev->dev;
+
+	if (on)
+		return pm_runtime_get_sync(dev);
+
+	return pm_runtime_put_sync(dev);
+}
+
 static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct csis_state *state = sd_to_csis_state(sd);
@@ -326,10 +477,12 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
 		 __func__, enable, state->flags);
 
 	if (enable) {
+		s5pcsis_clear_counters(state);
 		ret = pm_runtime_get_sync(&state->pdev->dev);
 		if (ret && ret != 1)
 			return ret;
 	}
+
 	mutex_lock(&state->lock);
 	if (enable) {
 		if (state->flags & ST_SUSPENDED) {
@@ -341,6 +494,7 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
 	} else {
 		s5pcsis_stop_stream(state);
 		state->flags &= ~ST_STREAMING;
+		s5pcsis_log_counters(state, true);
 	}
 unlock:
 	mutex_unlock(&state->lock);
@@ -438,6 +592,41 @@ static int s5pcsis_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	return 0;
 }
 
+static int s5pcsis_g_embedded_data(struct v4l2_subdev *sd, unsigned int *size,
+				   void **data)
+{
+	struct csis_state *state = sd_to_csis_state(sd);
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&state->slock, flags);
+
+	if (!list_empty(&state->ready_buf_list)) {
+		struct csis_pktbuf *buf = list_first_entry(
+			   &state->ready_buf_list, struct csis_pktbuf, list);
+
+		*size = min_t(unsigned int, *size, S5PCSIS_PKTDATA_SIZE);
+
+		if (!WARN_ON(buf->data == NULL))
+			memcpy(*data, buf->data, *size);
+		list_move_tail(&buf->list, state->empty_buf_list.next);
+	} else {
+		*size = 0;
+		ret = -ENODATA;
+	}
+	spin_unlock_irqrestore(&state->slock, flags);
+
+	return ret;
+}
+
+static int s5pcsis_log_status(struct v4l2_subdev *sd)
+{
+	struct csis_state *state = sd_to_csis_state(sd);
+
+	s5pcsis_log_counters(state, true);
+	return 0;
+}
+
 static int s5pcsis_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(fh, 0);
@@ -450,13 +639,32 @@ static int s5pcsis_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	return 0;
 }
+static int s5pcsis_ext_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
+{
+	struct csis_state *state = sd_to_csis_state(sd);
+	unsigned long flags;
+
+	spin_lock_irqsave(&state->slock, flags);
+	/*
+	 * Increment the frame_seq counter at each FIMC VSYNC interrupt before
+	 * an image frame is captured. This signals to MIPI-CSIS for which
+	 * frames the embedded non-image data is to be stored into the internal
+	 * buffer queue. The queue is emptied in g_embedded_data callback.
+	 */
+	state->frame_seq++;
+	spin_unlock_irqrestore(&state->slock, flags);
+
+	return 0;
+}
 
 static const struct v4l2_subdev_internal_ops s5pcsis_sd_internal_ops = {
 	.open = s5pcsis_open,
 };
 
 static struct v4l2_subdev_core_ops s5pcsis_core_ops = {
+	.interrupt_service_routine = s5pcsis_ext_isr,
 	.s_power = s5pcsis_s_power,
+	.log_status = s5pcsis_log_status,
 };
 
 static struct v4l2_subdev_pad_ops s5pcsis_pad_ops = {
@@ -466,6 +674,7 @@ static struct v4l2_subdev_pad_ops s5pcsis_pad_ops = {
 };
 
 static struct v4l2_subdev_video_ops s5pcsis_video_ops = {
+	.g_embedded_data = s5pcsis_g_embedded_data,
 	.s_stream = s5pcsis_s_stream,
 };
 
@@ -475,15 +684,63 @@ static struct v4l2_subdev_ops s5pcsis_subdev_ops = {
 	.video = &s5pcsis_video_ops,
 };
 
+static void s5pcsis_pkt_copy(struct csis_state *state, u32 *buf,
+			     u32 offset, u32 size)
+{
+	int i;
+
+	for (i = 0; i < S5PCSIS_PKTDATA_SIZE; i += 4, buf++)
+		*buf = s5pcsis_read(state, offset + i);
+}
+
 static irqreturn_t s5pcsis_irq_handler(int irq, void *dev_id)
 {
 	struct csis_state *state = dev_id;
-	u32 val;
+	unsigned long flags;
+	u32 status;
+
+	status = s5pcsis_read(state, S5PCSIS_INTSRC);
 
-	/* Just clear the interrupt pending bits. */
-	val = s5pcsis_read(state, S5PCSIS_INTSRC);
-	s5pcsis_write(state, S5PCSIS_INTSRC, val);
+	spin_lock_irqsave(&state->slock, flags);
+	WARN_ON(state->frame_seq == 0);
 
+	if ((status & S5PCSIS_INTSRC_NON_IMAGE_DATA) &&
+	    state->frame_seq != state->last_frame_seq &&
+	    !list_empty(&state->empty_buf_list)) {
+		struct csis_pktbuf *buf = list_first_entry(
+			   &state->empty_buf_list, struct csis_pktbuf, list);
+		u32 offset;
+
+		if (status & S5PCSIS_INTSRC_EVEN)
+			offset = S5PCSIS_PKTDATA_EVEN;
+		else
+			offset = S5PCSIS_PKTDATA_ODD;
+
+		s5pcsis_pkt_copy(state, buf->data, offset, 4096);
+		list_move_tail(&buf->list, state->ready_buf_list.next);
+
+		/* Let FIMC driver control which frames are captured */
+		state->last_frame_seq = state->frame_seq;
+	}
+
+	/* Update the event/error counters */
+	if ((status & S5PCSIS_INTSRC_ERRORS) || debug) {
+		int i;
+		for (i = 0; i < S5PCSIS_NUM_EVENTS; i++) {
+			if (!(status & state->events[i].mask))
+				continue;
+			state->events[i].counter++;
+			v4l2_dbg(2, debug, &state->sd, "%s: %d\n",
+				 state->events[i].name,
+				 state->events[i].counter);
+		}
+
+		v4l2_dbg(2, debug, &state->sd, "status: %#08x, frame %d\n",
+			 status, state->frame_seq);
+	}
+	spin_unlock_irqrestore(&state->slock, flags);
+
+	s5pcsis_write(state, S5PCSIS_INTSRC, status);
 	return IRQ_HANDLED;
 }
 
@@ -500,6 +757,10 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mutex_init(&state->lock);
+	INIT_LIST_HEAD(&state->empty_buf_list);
+	INIT_LIST_HEAD(&state->ready_buf_list);
+	spin_lock_init(&state->slock);
+
 	state->pdev = pdev;
 
 	pdata = pdev->dev.platform_data;
@@ -576,6 +837,11 @@ static int __devinit s5pcsis_probe(struct platform_device *pdev)
 	/* .. and a pointer to the subdev. */
 	platform_set_drvdata(pdev, &state->sd);
 
+	ret = s5pcsis_init_packet_queue(state);
+	if (ret < 0)
+		goto e_regput;
+
+	memcpy(state->events, s5pcsis_events, sizeof(state->events));;
 	pm_runtime_enable(&pdev->dev);
 	return 0;
 
@@ -694,6 +960,7 @@ static int __devexit s5pcsis_remove(struct platform_device *pdev)
 	regulator_bulk_free(CSIS_NUM_SUPPLIES, state->supplies);
 
 	media_entity_cleanup(&state->sd.entity);
+	s5pcsis_free_packet_queue(state);
 
 	return 0;
 }
-- 
1.7.9