mbox series

[v5,00/10] add aspeed-jpeg support for aspeed-video

Message ID 20211118074030.685-1-jammy_huang@aspeedtech.com (mailing list archive)
Headers show
Series add aspeed-jpeg support for aspeed-video | expand

Message

Jammy Huang Nov. 18, 2021, 7:40 a.m. UTC
The aim of this series is to add aspeed-jpeg support for aspeed-video
driver.

To achieve this major goal some refactors are included.

In the last, debugfs information is also updated per this change.

Changes in v5:
 - Use model data to tell different soc

Changes in v4:
 - Add definition for the Aspeed JPEG format
 - Reserve controls for ASPEED
 - Use s_fmt to update format rather than new control
 - Update aspeed hq quality range, 1 ~ 12


Jammy Huang (10):
  media: aspeed: move err-handling together to the bottom
  media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
  media: aspeed: add more debug log messages
  media: aspeed: refactor to gather format/compress settings
  media: v4l: Add definition for the Aspeed JPEG format
  media: v4l2-ctrls: Reserve controls for ASPEED
  media: aspeed: use model-data
  media: aspeed: Support aspeed mode to reduce compressed data
  media: aspeed: add comments and macro
  media: aspeed: Extend debug message

 .../media/uapi/v4l/pixfmt-reserved.rst        |  12 +
 drivers/media/platform/aspeed-video.c         | 534 ++++++++++++++----
 drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
 include/uapi/linux/aspeed-video.h             |  15 +
 include/uapi/linux/v4l2-controls.h            |   5 +
 include/uapi/linux/videodev2.h                |   1 +
 6 files changed, 467 insertions(+), 101 deletions(-)
 create mode 100644 include/uapi/linux/aspeed-video.h

Comments

Hans Verkuil Nov. 25, 2021, 2:31 p.m. UTC | #1
Hi Jammy,

I suggest posting a v3 that moves the patches that deal with the
Aspeed JPEG format to the end of the series. That way I can easily
merge the patches up to that point, and the Aspeed JPEG support can
be handled separately.

I'm not sure if it can be merged without that format being documented,
either in pixfmt-reserved.rst, by implementing support for it in
libv4lconvert from v4l-utils, or by providing a URL to some other
publicly available source code. We really don't like adding pixel
formats that cannot be interpreted by someone without access to the
datasheets.

Regards,

	Hans

On 18/11/2021 08:40, Jammy Huang wrote:
> The aim of this series is to add aspeed-jpeg support for aspeed-video
> driver.
> 
> To achieve this major goal some refactors are included.
> 
> In the last, debugfs information is also updated per this change.
> 
> Changes in v5:
>  - Use model data to tell different soc
> 
> Changes in v4:
>  - Add definition for the Aspeed JPEG format
>  - Reserve controls for ASPEED
>  - Use s_fmt to update format rather than new control
>  - Update aspeed hq quality range, 1 ~ 12
> 
> 
> Jammy Huang (10):
>   media: aspeed: move err-handling together to the bottom
>   media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
>   media: aspeed: add more debug log messages
>   media: aspeed: refactor to gather format/compress settings
>   media: v4l: Add definition for the Aspeed JPEG format
>   media: v4l2-ctrls: Reserve controls for ASPEED
>   media: aspeed: use model-data
>   media: aspeed: Support aspeed mode to reduce compressed data
>   media: aspeed: add comments and macro
>   media: aspeed: Extend debug message
> 
>  .../media/uapi/v4l/pixfmt-reserved.rst        |  12 +
>  drivers/media/platform/aspeed-video.c         | 534 ++++++++++++++----
>  drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
>  include/uapi/linux/aspeed-video.h             |  15 +
>  include/uapi/linux/v4l2-controls.h            |   5 +
>  include/uapi/linux/videodev2.h                |   1 +
>  6 files changed, 467 insertions(+), 101 deletions(-)
>  create mode 100644 include/uapi/linux/aspeed-video.h
>
Jammy Huang Nov. 26, 2021, 8:17 a.m. UTC | #2
Hi Hans,

Please refer to Page 132, 'Video stream data format – ASPEED mode compression' of
https://github.com/AspeedTech-BMC/openbmc/releases/download/v07.02/SDK_User_Guide_v07.02.pdf

Is above information enough??
Or maybe we can postpone this series until the source code for this format ready.
I think I can accomplish it by the end of this year.

On 2021/11/25 下午 10:31, Hans Verkuil wrote:
> Hi Jammy,
>
> I suggest posting a v3 that moves the patches that deal with the
> Aspeed JPEG format to the end of the series. That way I can easily
> merge the patches up to that point, and the Aspeed JPEG support can
> be handled separately.
>
> I'm not sure if it can be merged without that format being documented,
> either in pixfmt-reserved.rst, by implementing support for it in
> libv4lconvert from v4l-utils, or by providing a URL to some other
> publicly available source code. We really don't like adding pixel
> formats that cannot be interpreted by someone without access to the
> datasheets.
>
> Regards,
>
> 	Hans
>
> On 18/11/2021 08:40, Jammy Huang wrote:
>> The aim of this series is to add aspeed-jpeg support for aspeed-video
>> driver.
>>
>> To achieve this major goal some refactors are included.
>>
>> In the last, debugfs information is also updated per this change.
>>
>> Changes in v5:
>>   - Use model data to tell different soc
>>
>> Changes in v4:
>>   - Add definition for the Aspeed JPEG format
>>   - Reserve controls for ASPEED
>>   - Use s_fmt to update format rather than new control
>>   - Update aspeed hq quality range, 1 ~ 12
>>
>>
>> Jammy Huang (10):
>>    media: aspeed: move err-handling together to the bottom
>>    media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
>>    media: aspeed: add more debug log messages
>>    media: aspeed: refactor to gather format/compress settings
>>    media: v4l: Add definition for the Aspeed JPEG format
>>    media: v4l2-ctrls: Reserve controls for ASPEED
>>    media: aspeed: use model-data
>>    media: aspeed: Support aspeed mode to reduce compressed data
>>    media: aspeed: add comments and macro
>>    media: aspeed: Extend debug message
>>
>>   .../media/uapi/v4l/pixfmt-reserved.rst        |  12 +
>>   drivers/media/platform/aspeed-video.c         | 534 ++++++++++++++----
>>   drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
>>   include/uapi/linux/aspeed-video.h             |  15 +
>>   include/uapi/linux/v4l2-controls.h            |   5 +
>>   include/uapi/linux/videodev2.h                |   1 +
>>   6 files changed, 467 insertions(+), 101 deletions(-)
>>   create mode 100644 include/uapi/linux/aspeed-video.h
>>
Hans Verkuil Nov. 26, 2021, 9:51 a.m. UTC | #3
On 26/11/2021 09:17, Jammy Huang wrote:
> Hi Hans,
> 
> Please refer to Page 132, 'Video stream data format – ASPEED mode compression' of
> https://github.com/AspeedTech-BMC/openbmc/releases/download/v07.02/SDK_User_Guide_v07.02.pdf
> 
> Is above information enough??

Hmm, close. It would need some additional information, possibly documented in the source
code:

- What is the Frame Header? (the doc just says that it is different, but not what it actually is)
- What is the EOI code? Is it equal to 'FEND'? (not quite clear to me)

I think the documentation looks sufficient once those two items are addressed.

> Or maybe we can postpone this series until the source code for this format ready.
> I think I can accomplish it by the end of this year.

That's quite soon, so perhaps that would be the best approach. That way you also
know exactly what information is missing from the SDK User Guide, and if that is
documented, then this format can be used in the kernel.

But several of the patches in the series are independent of the new jpeg format,
so it might be useful to get those in first.

Regards,

	Hans

> 
> On 2021/11/25 下午 10:31, Hans Verkuil wrote:
>> Hi Jammy,
>>
>> I suggest posting a v3 that moves the patches that deal with the
>> Aspeed JPEG format to the end of the series. That way I can easily
>> merge the patches up to that point, and the Aspeed JPEG support can
>> be handled separately.
>>
>> I'm not sure if it can be merged without that format being documented,
>> either in pixfmt-reserved.rst, by implementing support for it in
>> libv4lconvert from v4l-utils, or by providing a URL to some other
>> publicly available source code. We really don't like adding pixel
>> formats that cannot be interpreted by someone without access to the
>> datasheets.
>>
>> Regards,
>>
>> 	Hans
>>
>> On 18/11/2021 08:40, Jammy Huang wrote:
>>> The aim of this series is to add aspeed-jpeg support for aspeed-video
>>> driver.
>>>
>>> To achieve this major goal some refactors are included.
>>>
>>> In the last, debugfs information is also updated per this change.
>>>
>>> Changes in v5:
>>>   - Use model data to tell different soc
>>>
>>> Changes in v4:
>>>   - Add definition for the Aspeed JPEG format
>>>   - Reserve controls for ASPEED
>>>   - Use s_fmt to update format rather than new control
>>>   - Update aspeed hq quality range, 1 ~ 12
>>>
>>>
>>> Jammy Huang (10):
>>>    media: aspeed: move err-handling together to the bottom
>>>    media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
>>>    media: aspeed: add more debug log messages
>>>    media: aspeed: refactor to gather format/compress settings
>>>    media: v4l: Add definition for the Aspeed JPEG format
>>>    media: v4l2-ctrls: Reserve controls for ASPEED
>>>    media: aspeed: use model-data
>>>    media: aspeed: Support aspeed mode to reduce compressed data
>>>    media: aspeed: add comments and macro
>>>    media: aspeed: Extend debug message
>>>
>>>   .../media/uapi/v4l/pixfmt-reserved.rst        |  12 +
>>>   drivers/media/platform/aspeed-video.c         | 534 ++++++++++++++----
>>>   drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
>>>   include/uapi/linux/aspeed-video.h             |  15 +
>>>   include/uapi/linux/v4l2-controls.h            |   5 +
>>>   include/uapi/linux/videodev2.h                |   1 +
>>>   6 files changed, 467 insertions(+), 101 deletions(-)
>>>   create mode 100644 include/uapi/linux/aspeed-video.h
>>>
Jammy Huang Dec. 2, 2021, 1:29 a.m. UTC | #4
Hi Hans,

Thanks for your review and suggestions.

On 2021/11/26 下午 05:51, Hans Verkuil wrote:
> On 26/11/2021 09:17, Jammy Huang wrote:
>> Hi Hans,
>>
>> Please refer to Page 132, 'Video stream data format – ASPEED mode compression' of
>> https://github.com/AspeedTech-BMC/openbmc/releases/download/v07.02/SDK_User_Guide_v07.02.pdf
>>
>> Is above information enough??
> Hmm, close. It would need some additional information, possibly documented in the source
> code:
>
> - What is the Frame Header? (the doc just says that it is different, but not what it actually is)
> - What is the EOI code? Is it equal to 'FEND'? (not quite clear to me)
>
> I think the documentation looks sufficient once those two items are addressed.
Information you mentioned will be included in next revision, 
"SDK_User_Guide_v07.03.pdf".
The releases will be updated to 
https://github.com/AspeedTech-BMC/openbmc/releases
>> Or maybe we can postpone this series until the source code for this format ready.
>> I think I can accomplish it by the end of this year.
> That's quite soon, so perhaps that would be the best approach. That way you also
> know exactly what information is missing from the SDK User Guide, and if that is
> documented, then this format can be used in the kernel.
>
> But several of the patches in the series are independent of the new jpeg format,
> so it might be useful to get those in first.

Understood. For the patches below, I will resend except for patch 
5/6/8/10 which are closely
relative to aspeed jpeg format.

    1. media: aspeed: move err-handling together to the bottom
    2. media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
    3. media: aspeed: add more debug log messages
    4. media: aspeed: refactor to gather format/compress settings
    5. media: v4l: Add definition for the Aspeed JPEG format
    6. media: v4l2-ctrls: Reserve controls for ASPEED
    7. media: aspeed: use model-data
    8. media: aspeed: Support aspeed mode to reduce compressed data
    9. media: aspeed: add comments and macro
    10.media: aspeed: Extend debug message



>
> Regards,
>
> 	Hans
>
>> On 2021/11/25 下午 10:31, Hans Verkuil wrote:
>>> Hi Jammy,
>>>
>>> I suggest posting a v3 that moves the patches that deal with the
>>> Aspeed JPEG format to the end of the series. That way I can easily
>>> merge the patches up to that point, and the Aspeed JPEG support can
>>> be handled separately.
>>>
>>> I'm not sure if it can be merged without that format being documented,
>>> either in pixfmt-reserved.rst, by implementing support for it in
>>> libv4lconvert from v4l-utils, or by providing a URL to some other
>>> publicly available source code. We really don't like adding pixel
>>> formats that cannot be interpreted by someone without access to the
>>> datasheets.
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>> On 18/11/2021 08:40, Jammy Huang wrote:
>>>> The aim of this series is to add aspeed-jpeg support for aspeed-video
>>>> driver.
>>>>
>>>> To achieve this major goal some refactors are included.
>>>>
>>>> In the last, debugfs information is also updated per this change.
>>>>
>>>> Changes in v5:
>>>>    - Use model data to tell different soc
>>>>
>>>> Changes in v4:
>>>>    - Add definition for the Aspeed JPEG format
>>>>    - Reserve controls for ASPEED
>>>>    - Use s_fmt to update format rather than new control
>>>>    - Update aspeed hq quality range, 1 ~ 12
>>>>
>>>>
>>>> Jammy Huang (10):
>>>>     media: aspeed: move err-handling together to the bottom
>>>>     media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
>>>>     media: aspeed: add more debug log messages
>>>>     media: aspeed: refactor to gather format/compress settings
>>>>     media: v4l: Add definition for the Aspeed JPEG format
>>>>     media: v4l2-ctrls: Reserve controls for ASPEED
>>>>     media: aspeed: use model-data
>>>>     media: aspeed: Support aspeed mode to reduce compressed data
>>>>     media: aspeed: add comments and macro
>>>>     media: aspeed: Extend debug message
>>>>
>>>>    .../media/uapi/v4l/pixfmt-reserved.rst        |  12 +
>>>>    drivers/media/platform/aspeed-video.c         | 534 ++++++++++++++----
>>>>    drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
>>>>    include/uapi/linux/aspeed-video.h             |  15 +
>>>>    include/uapi/linux/v4l2-controls.h            |   5 +
>>>>    include/uapi/linux/videodev2.h                |   1 +
>>>>    6 files changed, 467 insertions(+), 101 deletions(-)
>>>>    create mode 100644 include/uapi/linux/aspeed-video.h
>>>>
Jammy Huang Dec. 7, 2021, 3:03 a.m. UTC | #5
Hi Hans,

The implementation of decoder for this format as been available here.
https://github.com/AspeedTech-BMC/aspeed_codec

On 2021/12/2 上午 09:29, Jammy Huang wrote:
> Hi Hans,
>
> Thanks for your review and suggestions.
>
> On 2021/11/26 下午 05:51, Hans Verkuil wrote:
>> On 26/11/2021 09:17, Jammy Huang wrote:
>>> Hi Hans,
>>>
>>> Please refer to Page 132, 'Video stream data format – ASPEED mode compression' of
>>> https://github.com/AspeedTech-BMC/openbmc/releases/download/v07.02/SDK_User_Guide_v07.02.pdf
>>>
>>> Is above information enough??
>> Hmm, close. It would need some additional information, possibly documented in the source
>> code:
>>
>> - What is the Frame Header? (the doc just says that it is different, but not what it actually is)
>> - What is the EOI code? Is it equal to 'FEND'? (not quite clear to me)
>>
>> I think the documentation looks sufficient once those two items are addressed.
> Information you mentioned will be included in next revision,
> "SDK_User_Guide_v07.03.pdf".
> The releases will be updated to
> https://github.com/AspeedTech-BMC/openbmc/releases
>>> Or maybe we can postpone this series until the source code for this format ready.
>>> I think I can accomplish it by the end of this year.
>> That's quite soon, so perhaps that would be the best approach. That way you also
>> know exactly what information is missing from the SDK User Guide, and if that is
>> documented, then this format can be used in the kernel.
>>
>> But several of the patches in the series are independent of the new jpeg format,
>> so it might be useful to get those in first.
> Understood. For the patches below, I will resend except for patch
> 5/6/8/10 which are closely
> relative to aspeed jpeg format.
>
>      1. media: aspeed: move err-handling together to the bottom
>      2. media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
>      3. media: aspeed: add more debug log messages
>      4. media: aspeed: refactor to gather format/compress settings
>      5. media: v4l: Add definition for the Aspeed JPEG format
>      6. media: v4l2-ctrls: Reserve controls for ASPEED
>      7. media: aspeed: use model-data
>      8. media: aspeed: Support aspeed mode to reduce compressed data
>      9. media: aspeed: add comments and macro
>      10.media: aspeed: Extend debug message
>
>
>
>> Regards,
>>
>> 	Hans
>>
>>> On 2021/11/25 下午 10:31, Hans Verkuil wrote:
>>>> Hi Jammy,
>>>>
>>>> I suggest posting a v3 that moves the patches that deal with the
>>>> Aspeed JPEG format to the end of the series. That way I can easily
>>>> merge the patches up to that point, and the Aspeed JPEG support can
>>>> be handled separately.
>>>>
>>>> I'm not sure if it can be merged without that format being documented,
>>>> either in pixfmt-reserved.rst, by implementing support for it in
>>>> libv4lconvert from v4l-utils, or by providing a URL to some other
>>>> publicly available source code. We really don't like adding pixel
>>>> formats that cannot be interpreted by someone without access to the
>>>> datasheets.
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>> On 18/11/2021 08:40, Jammy Huang wrote:
>>>>> The aim of this series is to add aspeed-jpeg support for aspeed-video
>>>>> driver.
>>>>>
>>>>> To achieve this major goal some refactors are included.
>>>>>
>>>>> In the last, debugfs information is also updated per this change.
>>>>>
>>>>> Changes in v5:
>>>>>     - Use model data to tell different soc
>>>>>
>>>>> Changes in v4:
>>>>>     - Add definition for the Aspeed JPEG format
>>>>>     - Reserve controls for ASPEED
>>>>>     - Use s_fmt to update format rather than new control
>>>>>     - Update aspeed hq quality range, 1 ~ 12
>>>>>
>>>>>
>>>>> Jammy Huang (10):
>>>>>      media: aspeed: move err-handling together to the bottom
>>>>>      media: aspeed: use v4l2_info/v4l2_warn/v4l2_dbg for log
>>>>>      media: aspeed: add more debug log messages
>>>>>      media: aspeed: refactor to gather format/compress settings
>>>>>      media: v4l: Add definition for the Aspeed JPEG format
>>>>>      media: v4l2-ctrls: Reserve controls for ASPEED
>>>>>      media: aspeed: use model-data
>>>>>      media: aspeed: Support aspeed mode to reduce compressed data
>>>>>      media: aspeed: add comments and macro
>>>>>      media: aspeed: Extend debug message
>>>>>
>>>>>     .../media/uapi/v4l/pixfmt-reserved.rst        |  12 +
>>>>>     drivers/media/platform/aspeed-video.c         | 534 ++++++++++++++----
>>>>>     drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
>>>>>     include/uapi/linux/aspeed-video.h             |  15 +
>>>>>     include/uapi/linux/v4l2-controls.h            |   5 +
>>>>>     include/uapi/linux/videodev2.h                |   1 +
>>>>>     6 files changed, 467 insertions(+), 101 deletions(-)
>>>>>     create mode 100644 include/uapi/linux/aspeed-video.h
>>>>>
Sakari Ailus Dec. 20, 2021, 7:55 p.m. UTC | #6
Hi Jammy,

On Tue, Dec 07, 2021 at 11:03:00AM +0800, Jammy Huang wrote:
> Hi Hans,
> 
> The implementation of decoder for this format as been available here.
> https://github.com/AspeedTech-BMC/aspeed_codec

The format documentation should point to this, as well as other
documentation there is.

I'm not sure there have been specific requirements of license, but it's the
first time I see MPL 2.0 being used in such context. It's eventually
convertible to GPL (or LGPL) although the route is inconvenient at best.
Or would people be happy with MPL code in v4l-utils?

I wonder what others think.
Jammy Huang Dec. 21, 2021, 2:13 a.m. UTC | #7
Hi Sakari,

On 2021/12/21 上午 03:55, sakari.ailus@linux.intel.com wrote:
> Hi Jammy,
>
> On Tue, Dec 07, 2021 at 11:03:00AM +0800, Jammy Huang wrote:
>> Hi Hans,
>>
>> The implementation of decoder for this format as been available here.
>> https://github.com/AspeedTech-BMC/aspeed_codec
> The format documentation should point to this, as well as other
> documentation there is.
OK, I will add this into format documentation as well.
>
> I'm not sure there have been specific requirements of license, but it's the
> first time I see MPL 2.0 being used in such context. It's eventually
> convertible to GPL (or LGPL) although the route is inconvenient at best.
> Or would people be happy with MPL code in v4l-utils?

In order to have aspeed-jpeg format to work on openbmc's KVM, I was 
working on noVNC.
And this codec library is part of the code to make it work.

Before I put this source on github, I am not sure which kind of license 
is suitable. So I check
what noVNC used, https://github.com/novnc/noVNC/blob/master/LICENSE.txt. 
That's why I
adapted MPL 2.0.

If you think GPL(or LGPL) is more suitable, I can make a switch.


>
> I wonder what others think.
>