| Message ID | 20250901092320.44615-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-09-01 10:23:20) > First there is a single big endian format defined in `formats.yaml`: RGB565_BE. > However, while the yaml file specifies "big_endian: true", the python script > looks for a key named "big-endian". Causing `RGB565{,_BE}` both to be the same. Eeek. Good find. > Second, the python script simply appends " | DRM_FORMAT_BIG_ENDIAN" to the > fourcc of the format. However, there is no definition of that macro is > available in the only user, `formats.h.in`. aha, and this would then require users of formats.h to include the drm headers. > Fix the first one by checking for "big_endian" in the script as well, and > fix the second one by defining a constant with the same value and using that. I think that is indeed more preferable in this instance than requiring inclusion of the drm headers for a single value. > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Fixes: 7c496f1c54dd58 ("utils: gen-formats: Support big-endian DRM formats") > --- > include/libcamera/formats.h.in | 4 +++- > utils/codegen/gen-formats.py | 3 +-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/formats.h.in b/include/libcamera/formats.h.in > index 5ff9c3bf4..c821ffbb1 100644 > --- a/include/libcamera/formats.h.in > +++ b/include/libcamera/formats.h.in > @@ -33,10 +33,12 @@ constexpr uint64_t __mod(unsigned int vendor, unsigned int mod) > (static_cast<uint64_t>(mod) << 0); > } > > +constexpr uint32_t kDrmFormatBigEndian = uint32_t(1) << 31; /* DRM_FORMAT_BIG_ENDIAN */ > + > } /* namespace */ > > {% for f in formats %} > -constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}), __mod({{f.mod}})); > +constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}){{ ' | kDrmFormatBigEndian' if f.big_endian }}, __mod({{f.mod}})); > {%- endfor %} > > } /* namespace formats */ > diff --git a/utils/codegen/gen-formats.py b/utils/codegen/gen-formats.py > index 872f3fe34..01adf5e1f 100755 > --- a/utils/codegen/gen-formats.py > +++ b/utils/codegen/gen-formats.py > @@ -59,13 +59,12 @@ def generate_formats(formats, drm_fourcc): > for format in formats: > name, format = format.popitem() > fourcc = drm_fourcc.fourcc(format['fourcc']) > - if format.get('big-endian'): > - fourcc += '| DRM_FORMAT_BIG_ENDIAN' > > data = { > 'name': name, > 'fourcc': fourcc, > 'mod': '0, 0', > + 'big_endian': format.get('big_endian') == True, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > mod = format.get('mod') > -- > 2.51.0 >
2025. 09. 01. 15:31 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-09-01 10:23:20) >> First there is a single big endian format defined in `formats.yaml`: RGB565_BE. >> However, while the yaml file specifies "big_endian: true", the python script >> looks for a key named "big-endian". Causing `RGB565{,_BE}` both to be the same. > > Eeek. Good find. > > >> Second, the python script simply appends " | DRM_FORMAT_BIG_ENDIAN" to the >> fourcc of the format. However, there is no definition of that macro is >> available in the only user, `formats.h.in`. > > aha, and this would then require users of formats.h to include the drm > headers. > >> Fix the first one by checking for "big_endian" in the script as well, and >> fix the second one by defining a constant with the same value and using that. > > I think that is indeed more preferable in this instance than requiring > inclusion of the drm headers for a single value. Based on e8dc74317afa43ffa4096e7e7e0c25b5c5189682 the explicit goal appears to be to avoid using drm_fourcc.h. Regards, Barnabás Pőcze > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> Fixes: 7c496f1c54dd58 ("utils: gen-formats: Support big-endian DRM formats") >> --- >> include/libcamera/formats.h.in | 4 +++- >> utils/codegen/gen-formats.py | 3 +-- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/include/libcamera/formats.h.in b/include/libcamera/formats.h.in >> index 5ff9c3bf4..c821ffbb1 100644 >> --- a/include/libcamera/formats.h.in >> +++ b/include/libcamera/formats.h.in >> @@ -33,10 +33,12 @@ constexpr uint64_t __mod(unsigned int vendor, unsigned int mod) >> (static_cast<uint64_t>(mod) << 0); >> } >> >> +constexpr uint32_t kDrmFormatBigEndian = uint32_t(1) << 31; /* DRM_FORMAT_BIG_ENDIAN */ >> + >> } /* namespace */ >> >> {% for f in formats %} >> -constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}), __mod({{f.mod}})); >> +constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}){{ ' | kDrmFormatBigEndian' if f.big_endian }}, __mod({{f.mod}})); >> {%- endfor %} >> >> } /* namespace formats */ >> diff --git a/utils/codegen/gen-formats.py b/utils/codegen/gen-formats.py >> index 872f3fe34..01adf5e1f 100755 >> --- a/utils/codegen/gen-formats.py >> +++ b/utils/codegen/gen-formats.py >> @@ -59,13 +59,12 @@ def generate_formats(formats, drm_fourcc): >> for format in formats: >> name, format = format.popitem() >> fourcc = drm_fourcc.fourcc(format['fourcc']) >> - if format.get('big-endian'): >> - fourcc += '| DRM_FORMAT_BIG_ENDIAN' >> >> data = { >> 'name': name, >> 'fourcc': fourcc, >> 'mod': '0, 0', >> + 'big_endian': format.get('big_endian') == True, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> } >> >> mod = format.get('mod') >> -- >> 2.51.0 >>
On Mon, Sep 01, 2025 at 03:37:57PM +0200, Barnabás Pőcze wrote: > 2025. 09. 01. 15:31 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-09-01 10:23:20) > >> First there is a single big endian format defined in `formats.yaml`: RGB565_BE. > >> However, while the yaml file specifies "big_endian: true", the python script > >> looks for a key named "big-endian". Causing `RGB565{,_BE}` both to be the same. > > > > Eeek. Good find. > > > >> Second, the python script simply appends " | DRM_FORMAT_BIG_ENDIAN" to the > >> fourcc of the format. However, there is no definition of that macro is > >> available in the only user, `formats.h.in`. > > > > aha, and this would then require users of formats.h to include the drm > > headers. > > > >> Fix the first one by checking for "big_endian" in the script as well, and > >> fix the second one by defining a constant with the same value and using that. > > > > I think that is indeed more preferable in this instance than requiring > > inclusion of the drm headers for a single value. > > Based on e8dc74317afa43ffa4096e7e7e0c25b5c5189682 the explicit goal appears to > be to avoid using drm_fourcc.h. > > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> Fixes: 7c496f1c54dd58 ("utils: gen-formats: Support big-endian DRM formats") > >> --- > >> include/libcamera/formats.h.in | 4 +++- > >> utils/codegen/gen-formats.py | 3 +-- > >> 2 files changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/libcamera/formats.h.in b/include/libcamera/formats.h.in > >> index 5ff9c3bf4..c821ffbb1 100644 > >> --- a/include/libcamera/formats.h.in > >> +++ b/include/libcamera/formats.h.in > >> @@ -33,10 +33,12 @@ constexpr uint64_t __mod(unsigned int vendor, unsigned int mod) > >> (static_cast<uint64_t>(mod) << 0); > >> } > >> > >> +constexpr uint32_t kDrmFormatBigEndian = uint32_t(1) << 31; /* DRM_FORMAT_BIG_ENDIAN */ I would have written constexpr uint32_t kDrmFormatBigEndian = 1U << 31; /* DRM_FORMAT_BIG_ENDIAN */ > >> + > >> } /* namespace */ > >> > >> {% for f in formats %} > >> -constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}), __mod({{f.mod}})); > >> +constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}){{ ' | kDrmFormatBigEndian' if f.big_endian }}, __mod({{f.mod}})); > >> {%- endfor %} > >> > >> } /* namespace formats */ > >> diff --git a/utils/codegen/gen-formats.py b/utils/codegen/gen-formats.py > >> index 872f3fe34..01adf5e1f 100755 > >> --- a/utils/codegen/gen-formats.py > >> +++ b/utils/codegen/gen-formats.py > >> @@ -59,13 +59,12 @@ def generate_formats(formats, drm_fourcc): > >> for format in formats: > >> name, format = format.popitem() > >> fourcc = drm_fourcc.fourcc(format['fourcc']) > >> - if format.get('big-endian'): > >> - fourcc += '| DRM_FORMAT_BIG_ENDIAN' > >> > >> data = { > >> 'name': name, > >> 'fourcc': fourcc, > >> 'mod': '0, 0', > >> + 'big_endian': format.get('big_endian') == True, Do you need == True ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> } > >> > >> mod = format.get('mod')
2025. 09. 01. 16:43 keltezéssel, Laurent Pinchart írta: > On Mon, Sep 01, 2025 at 03:37:57PM +0200, Barnabás Pőcze wrote: >> 2025. 09. 01. 15:31 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-09-01 10:23:20) >>>> First there is a single big endian format defined in `formats.yaml`: RGB565_BE. >>>> However, while the yaml file specifies "big_endian: true", the python script >>>> looks for a key named "big-endian". Causing `RGB565{,_BE}` both to be the same. >>> >>> Eeek. Good find. >>> >>>> Second, the python script simply appends " | DRM_FORMAT_BIG_ENDIAN" to the >>>> fourcc of the format. However, there is no definition of that macro is >>>> available in the only user, `formats.h.in`. >>> >>> aha, and this would then require users of formats.h to include the drm >>> headers. >>> >>>> Fix the first one by checking for "big_endian" in the script as well, and >>>> fix the second one by defining a constant with the same value and using that. >>> >>> I think that is indeed more preferable in this instance than requiring >>> inclusion of the drm headers for a single value. >> >> Based on e8dc74317afa43ffa4096e7e7e0c25b5c5189682 the explicit goal appears to >> be to avoid using drm_fourcc.h. >> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> Fixes: 7c496f1c54dd58 ("utils: gen-formats: Support big-endian DRM formats") >>>> --- >>>> include/libcamera/formats.h.in | 4 +++- >>>> utils/codegen/gen-formats.py | 3 +-- >>>> 2 files changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/libcamera/formats.h.in b/include/libcamera/formats.h.in >>>> index 5ff9c3bf4..c821ffbb1 100644 >>>> --- a/include/libcamera/formats.h.in >>>> +++ b/include/libcamera/formats.h.in >>>> @@ -33,10 +33,12 @@ constexpr uint64_t __mod(unsigned int vendor, unsigned int mod) >>>> (static_cast<uint64_t>(mod) << 0); >>>> } >>>> >>>> +constexpr uint32_t kDrmFormatBigEndian = uint32_t(1) << 31; /* DRM_FORMAT_BIG_ENDIAN */ > > I would have written > > constexpr uint32_t kDrmFormatBigEndian = 1U << 31; /* DRM_FORMAT_BIG_ENDIAN */ Right. I generally like to avoid integer suffixes. I was also considering UINT32_C(1), which expands to the type appropriate suffix. > >>>> + >>>> } /* namespace */ >>>> >>>> {% for f in formats %} >>>> -constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}), __mod({{f.mod}})); >>>> +constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}){{ ' | kDrmFormatBigEndian' if f.big_endian }}, __mod({{f.mod}})); >>>> {%- endfor %} >>>> >>>> } /* namespace formats */ >>>> diff --git a/utils/codegen/gen-formats.py b/utils/codegen/gen-formats.py >>>> index 872f3fe34..01adf5e1f 100755 >>>> --- a/utils/codegen/gen-formats.py >>>> +++ b/utils/codegen/gen-formats.py >>>> @@ -59,13 +59,12 @@ def generate_formats(formats, drm_fourcc): >>>> for format in formats: >>>> name, format = format.popitem() >>>> fourcc = drm_fourcc.fourcc(format['fourcc']) >>>> - if format.get('big-endian'): >>>> - fourcc += '| DRM_FORMAT_BIG_ENDIAN' >>>> >>>> data = { >>>> 'name': name, >>>> 'fourcc': fourcc, >>>> 'mod': '0, 0', >>>> + 'big_endian': format.get('big_endian') == True, > > Do you need == True ? Hmmm... probably not. Maybe it's better to accept any truth-y value. I will remove it. Regards, Barnabás Pőcze > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> } >>>> >>>> mod = format.get('mod') >
diff --git a/include/libcamera/formats.h.in b/include/libcamera/formats.h.in index 5ff9c3bf4..c821ffbb1 100644 --- a/include/libcamera/formats.h.in +++ b/include/libcamera/formats.h.in @@ -33,10 +33,12 @@ constexpr uint64_t __mod(unsigned int vendor, unsigned int mod) (static_cast<uint64_t>(mod) << 0); } +constexpr uint32_t kDrmFormatBigEndian = uint32_t(1) << 31; /* DRM_FORMAT_BIG_ENDIAN */ + } /* namespace */ {% for f in formats %} -constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}), __mod({{f.mod}})); +constexpr PixelFormat {{f.name}}(__fourcc({{f.fourcc}}){{ ' | kDrmFormatBigEndian' if f.big_endian }}, __mod({{f.mod}})); {%- endfor %} } /* namespace formats */ diff --git a/utils/codegen/gen-formats.py b/utils/codegen/gen-formats.py index 872f3fe34..01adf5e1f 100755 --- a/utils/codegen/gen-formats.py +++ b/utils/codegen/gen-formats.py @@ -59,13 +59,12 @@ def generate_formats(formats, drm_fourcc): for format in formats: name, format = format.popitem() fourcc = drm_fourcc.fourcc(format['fourcc']) - if format.get('big-endian'): - fourcc += '| DRM_FORMAT_BIG_ENDIAN' data = { 'name': name, 'fourcc': fourcc, 'mod': '0, 0', + 'big_endian': format.get('big_endian') == True, } mod = format.get('mod')
First there is a single big endian format defined in `formats.yaml`: RGB565_BE. However, while the yaml file specifies "big_endian: true", the python script looks for a key named "big-endian". Causing `RGB565{,_BE}` both to be the same. Second, the python script simply appends " | DRM_FORMAT_BIG_ENDIAN" to the fourcc of the format. However, there is no definition of that macro is available in the only user, `formats.h.in`. Fix the first one by checking for "big_endian" in the script as well, and fix the second one by defining a constant with the same value and using that. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Fixes: 7c496f1c54dd58 ("utils: gen-formats: Support big-endian DRM formats") --- include/libcamera/formats.h.in | 4 +++- utils/codegen/gen-formats.py | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-)