* [PATCH 0/2] media: staging: rkisp1: Fix formats for metadata pads @ 2020-03-25 21:27 Dafna Hirschfeld 2020-03-25 21:27 ` [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA Dafna Hirschfeld 2020-03-25 21:27 ` [PATCH 2/2] media: staging: rkisp1: add the flag MEDIA_PAD_FL_METADATA to the metadata pads Dafna Hirschfeld 0 siblings, 2 replies; 9+ messages in thread From: Dafna Hirschfeld @ 2020-03-25 21:27 UTC (permalink / raw) To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart There are 4 pads between the entities rkisp1_isp, rkisp1_params, rkisp1_stats that transmit metadata. Since metadata has no dimensions, it makes sense to set the width and height of these pads to 0. The problem is that it makes the v4l-compliance tests fail. Currently, in order to silent the tests, the width and height are set to RKISP1_DEFAULT_* This patchset sets the dimensions to 0 and solves the compliance error by introducing a new flag MEDIA_PAD_FL_METADATA in media.h and setting this flag to those pads. Then the v4l2-compliance tests can skip the dimensions test if this flag is set. This is just a suggested implementation from Laurent, a better documentation is probably needed. Another possible solution suggested by Verkuil is to add a new media bus format: MEDIA_BUS_FMT_FIXED_METADATA. - patch 1: introduces the the flag - patch 2: set the flag to the pads and removes a related TODO item A corresponding patch to v4l-utils will be sent. Dafna Hirschfeld (2): media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA media: staging: rkisp1: add the flag MEDIA_PAD_FL_METADATA to the metadata pads Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ drivers/staging/media/rkisp1/TODO | 1 - drivers/staging/media/rkisp1/rkisp1-isp.c | 10 ++++++---- drivers/staging/media/rkisp1/rkisp1-params.c | 2 +- drivers/staging/media/rkisp1/rkisp1-stats.c | 2 +- include/uapi/linux/media.h | 1 + 6 files changed, 13 insertions(+), 7 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-03-25 21:27 [PATCH 0/2] media: staging: rkisp1: Fix formats for metadata pads Dafna Hirschfeld @ 2020-03-25 21:27 ` Dafna Hirschfeld 2020-03-25 22:26 ` Laurent Pinchart 2020-03-25 21:27 ` [PATCH 2/2] media: staging: rkisp1: add the flag MEDIA_PAD_FL_METADATA to the metadata pads Dafna Hirschfeld 1 sibling, 1 reply; 9+ messages in thread From: Dafna Hirschfeld @ 2020-03-25 21:27 UTC (permalink / raw) To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart Add a flag to the flags field of 'struct media_pad_desc' that indicates that the data transmitted by the pad is metadata. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ include/uapi/linux/media.h | 1 + 2 files changed, 5 insertions(+) diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst index 3af6a414b501..4ca902478971 100644 --- a/Documentation/media/uapi/mediactl/media-types.rst +++ b/Documentation/media/uapi/mediactl/media-types.rst @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements .. _MEDIA-PAD-FL-SINK: .. _MEDIA-PAD-FL-SOURCE: .. _MEDIA-PAD-FL-MUST-CONNECT: +.. _MEDIA-PAD-FL-METADATA: .. flat-table:: Media pad flags :header-rows: 0 @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements configuration dependent) for the pad to need enabled links even when this flag isn't set; the absence of the flag doesn't imply there is none. + * - ``MEDIA_PAD_FL_METADATA`` + - This flag indicates that the data transmitted by the pad is of + type metadata. One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index 383ac7b7d8f0..ae37226eb5c9 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -210,6 +210,7 @@ struct media_entity_desc { #define MEDIA_PAD_FL_SINK (1 << 0) #define MEDIA_PAD_FL_SOURCE (1 << 1) #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) +#define MEDIA_PAD_FL_METADATA (1 << 3) struct media_pad_desc { __u32 entity; /* entity ID */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-03-25 21:27 ` [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA Dafna Hirschfeld @ 2020-03-25 22:26 ` Laurent Pinchart 2020-03-26 7:59 ` Dafna Hirschfeld 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2020-03-25 22:26 UTC (permalink / raw) To: Dafna Hirschfeld Cc: linux-media, helen.koike, ezequiel, hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab Hi Dafna, Thank you for the patch. On Wed, Mar 25, 2020 at 10:27:03PM +0100, Dafna Hirschfeld wrote: > Add a flag to the flags field of 'struct media_pad_desc' > that indicates that the data transmitted by the pad is > metadata. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ > include/uapi/linux/media.h | 1 + > 2 files changed, 5 insertions(+) > > diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst > index 3af6a414b501..4ca902478971 100644 > --- a/Documentation/media/uapi/mediactl/media-types.rst > +++ b/Documentation/media/uapi/mediactl/media-types.rst > @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > .. _MEDIA-PAD-FL-SINK: > .. _MEDIA-PAD-FL-SOURCE: > .. _MEDIA-PAD-FL-MUST-CONNECT: > +.. _MEDIA-PAD-FL-METADATA: > > .. flat-table:: Media pad flags > :header-rows: 0 > @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements > configuration dependent) for the pad to need enabled links even > when this flag isn't set; the absence of the flag doesn't imply > there is none. > + * - ``MEDIA_PAD_FL_METADATA`` > + - This flag indicates that the data transmitted by the pad is of > + type metadata. > > > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h > index 383ac7b7d8f0..ae37226eb5c9 100644 > --- a/include/uapi/linux/media.h > +++ b/include/uapi/linux/media.h > @@ -210,6 +210,7 @@ struct media_entity_desc { > #define MEDIA_PAD_FL_SINK (1 << 0) > #define MEDIA_PAD_FL_SOURCE (1 << 1) > #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) > +#define MEDIA_PAD_FL_METADATA (1 << 3) I think we need to reserve a few bits here. We'll have more than metadata. Audio comes to mind, there will likely be more. Having independent flags would not only waste a bit of space in the bitfield (not that we're about to run out of bits, but still), but would make it possible to specify invalid configurations such as MEDIA_PAD_FL_METADATA | MEDIA_PAD_FL_AUDIO. And now that I've written this, I realize that audio metadata could be a thing, so maybe a metadata flag is actually the best option :-) There are pros and cons for both options, so I won't recommend one. > struct media_pad_desc { > __u32 entity; /* entity ID */ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-03-25 22:26 ` Laurent Pinchart @ 2020-03-26 7:59 ` Dafna Hirschfeld 2020-03-27 8:08 ` Sakari Ailus 0 siblings, 1 reply; 9+ messages in thread From: Dafna Hirschfeld @ 2020-03-26 7:59 UTC (permalink / raw) To: Laurent Pinchart Cc: Dafna Hirschfeld, Linux Media Mailing List, Helen Koike, Ezequiel Garcia, Hans Verkuil, kernel, sakari.ailus, linux-rockchip, mchehab On Wed, Mar 25, 2020 at 11:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dafna, > > Thank you for the patch. > > On Wed, Mar 25, 2020 at 10:27:03PM +0100, Dafna Hirschfeld wrote: > > Add a flag to the flags field of 'struct media_pad_desc' > > that indicates that the data transmitted by the pad is > > metadata. > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > --- > > Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ > > include/uapi/linux/media.h | 1 + > > 2 files changed, 5 insertions(+) > > > > diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst > > index 3af6a414b501..4ca902478971 100644 > > --- a/Documentation/media/uapi/mediactl/media-types.rst > > +++ b/Documentation/media/uapi/mediactl/media-types.rst > > @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > > .. _MEDIA-PAD-FL-SINK: > > .. _MEDIA-PAD-FL-SOURCE: > > .. _MEDIA-PAD-FL-MUST-CONNECT: > > +.. _MEDIA-PAD-FL-METADATA: > > > > .. flat-table:: Media pad flags > > :header-rows: 0 > > @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements > > configuration dependent) for the pad to need enabled links even > > when this flag isn't set; the absence of the flag doesn't imply > > there is none. > > + * - ``MEDIA_PAD_FL_METADATA`` > > + - This flag indicates that the data transmitted by the pad is of > > + type metadata. > > > > > > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` > > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h > > index 383ac7b7d8f0..ae37226eb5c9 100644 > > --- a/include/uapi/linux/media.h > > +++ b/include/uapi/linux/media.h > > @@ -210,6 +210,7 @@ struct media_entity_desc { > > #define MEDIA_PAD_FL_SINK (1 << 0) > > #define MEDIA_PAD_FL_SOURCE (1 << 1) > > #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) > > +#define MEDIA_PAD_FL_METADATA (1 << 3) > > I think we need to reserve a few bits here. We'll have more than > metadata. Audio comes to mind, there will likely be more. Having > independent flags would not only waste a bit of space in the bitfield > (not that we're about to run out of bits, but still), but would make it > possible to specify invalid configurations such as MEDIA_PAD_FL_METADATA > | MEDIA_PAD_FL_AUDIO. And now that I've written this, I realize that > audio metadata could be a thing, so maybe a metadata flag is actually > the best option :-) hehe, ok, but drivers that set the METADATA flag should also set the media bus code to MEDIA_BUS_FMT_FIXED ? If yes then setting the METADATA flag with a different media bus is also an invalid configuration. > > There are pros and cons for both options, so I won't recommend one. > > > struct media_pad_desc { > > __u32 entity; /* entity ID */ > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-03-26 7:59 ` Dafna Hirschfeld @ 2020-03-27 8:08 ` Sakari Ailus 2020-03-30 12:42 ` Dafna Hirschfeld 0 siblings, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2020-03-27 8:08 UTC (permalink / raw) To: Dafna Hirschfeld Cc: Laurent Pinchart, Dafna Hirschfeld, Linux Media Mailing List, Helen Koike, Ezequiel Garcia, Hans Verkuil, kernel, linux-rockchip, mchehab Dafna, Laurent, On Thu, Mar 26, 2020 at 08:59:04AM +0100, Dafna Hirschfeld wrote: > On Wed, Mar 25, 2020 at 11:26 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Dafna, > > > > Thank you for the patch. > > > > On Wed, Mar 25, 2020 at 10:27:03PM +0100, Dafna Hirschfeld wrote: > > > Add a flag to the flags field of 'struct media_pad_desc' > > > that indicates that the data transmitted by the pad is > > > metadata. > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > > --- > > > Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ > > > include/uapi/linux/media.h | 1 + > > > 2 files changed, 5 insertions(+) > > > > > > diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst > > > index 3af6a414b501..4ca902478971 100644 > > > --- a/Documentation/media/uapi/mediactl/media-types.rst > > > +++ b/Documentation/media/uapi/mediactl/media-types.rst > > > @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > > > .. _MEDIA-PAD-FL-SINK: > > > .. _MEDIA-PAD-FL-SOURCE: > > > .. _MEDIA-PAD-FL-MUST-CONNECT: > > > +.. _MEDIA-PAD-FL-METADATA: > > > > > > .. flat-table:: Media pad flags > > > :header-rows: 0 > > > @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements > > > configuration dependent) for the pad to need enabled links even > > > when this flag isn't set; the absence of the flag doesn't imply > > > there is none. > > > + * - ``MEDIA_PAD_FL_METADATA`` > > > + - This flag indicates that the data transmitted by the pad is of > > > + type metadata. > > > > > > > > > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` > > > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h > > > index 383ac7b7d8f0..ae37226eb5c9 100644 > > > --- a/include/uapi/linux/media.h > > > +++ b/include/uapi/linux/media.h > > > @@ -210,6 +210,7 @@ struct media_entity_desc { > > > #define MEDIA_PAD_FL_SINK (1 << 0) > > > #define MEDIA_PAD_FL_SOURCE (1 << 1) > > > #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) > > > +#define MEDIA_PAD_FL_METADATA (1 << 3) > > > > I think we need to reserve a few bits here. We'll have more than > > metadata. Audio comes to mind, there will likely be more. Having > > independent flags would not only waste a bit of space in the bitfield > > (not that we're about to run out of bits, but still), but would make it > > possible to specify invalid configurations such as MEDIA_PAD_FL_METADATA > > | MEDIA_PAD_FL_AUDIO. And now that I've written this, I realize that > > audio metadata could be a thing, so maybe a metadata flag is actually > > the best option :-) > hehe, ok, but drivers that set the METADATA flag should also set the media > bus code to MEDIA_BUS_FMT_FIXED ? If yes then setting > the METADATA flag with a different media bus is also an invalid configuration. That may be currently the case, but not all non-image data (metadata in practice) will be using MEDIA_BUS_FMT_FIXED, for instance sensor embedded data when we support that in upstream. Note that whether metadata flows over a pad is dynamic configuration. I wonder if it is useful to tell this to the user, as the user, in many cases, will be making the configuration affecting this. There definitely are devices where this configuration would be static, but many of those cases (ISPs in particular) have DMAs (i.e. video nodes) directly connected over a link, where you'll find this information on the video node. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-03-27 8:08 ` Sakari Ailus @ 2020-03-30 12:42 ` Dafna Hirschfeld 2020-10-30 4:56 ` Helen Koike 0 siblings, 1 reply; 9+ messages in thread From: Dafna Hirschfeld @ 2020-03-30 12:42 UTC (permalink / raw) To: Sakari Ailus Cc: Laurent Pinchart, Dafna Hirschfeld, Linux Media Mailing List, Helen Koike, Ezequiel Garcia, Hans Verkuil, kernel, linux-rockchip, mchehab On Fri, Mar 27, 2020 at 9:09 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Dafna, Laurent, > > On Thu, Mar 26, 2020 at 08:59:04AM +0100, Dafna Hirschfeld wrote: > > On Wed, Mar 25, 2020 at 11:26 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Dafna, > > > > > > Thank you for the patch. > > > > > > On Wed, Mar 25, 2020 at 10:27:03PM +0100, Dafna Hirschfeld wrote: > > > > Add a flag to the flags field of 'struct media_pad_desc' > > > > that indicates that the data transmitted by the pad is > > > > metadata. > > > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > > > --- > > > > Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ > > > > include/uapi/linux/media.h | 1 + > > > > 2 files changed, 5 insertions(+) > > > > > > > > diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst > > > > index 3af6a414b501..4ca902478971 100644 > > > > --- a/Documentation/media/uapi/mediactl/media-types.rst > > > > +++ b/Documentation/media/uapi/mediactl/media-types.rst > > > > @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements > > > > .. _MEDIA-PAD-FL-SINK: > > > > .. _MEDIA-PAD-FL-SOURCE: > > > > .. _MEDIA-PAD-FL-MUST-CONNECT: > > > > +.. _MEDIA-PAD-FL-METADATA: > > > > > > > > .. flat-table:: Media pad flags > > > > :header-rows: 0 > > > > @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements > > > > configuration dependent) for the pad to need enabled links even > > > > when this flag isn't set; the absence of the flag doesn't imply > > > > there is none. > > > > + * - ``MEDIA_PAD_FL_METADATA`` > > > > + - This flag indicates that the data transmitted by the pad is of > > > > + type metadata. > > > > > > > > > > > > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` > > > > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h > > > > index 383ac7b7d8f0..ae37226eb5c9 100644 > > > > --- a/include/uapi/linux/media.h > > > > +++ b/include/uapi/linux/media.h > > > > @@ -210,6 +210,7 @@ struct media_entity_desc { > > > > #define MEDIA_PAD_FL_SINK (1 << 0) > > > > #define MEDIA_PAD_FL_SOURCE (1 << 1) > > > > #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) > > > > +#define MEDIA_PAD_FL_METADATA (1 << 3) > > > > > > I think we need to reserve a few bits here. We'll have more than > > > metadata. Audio comes to mind, there will likely be more. Having > > > independent flags would not only waste a bit of space in the bitfield > > > (not that we're about to run out of bits, but still), but would make it > > > possible to specify invalid configurations such as MEDIA_PAD_FL_METADATA > > > | MEDIA_PAD_FL_AUDIO. And now that I've written this, I realize that > > > audio metadata could be a thing, so maybe a metadata flag is actually > > > the best option :-) > > hehe, ok, but drivers that set the METADATA flag should also set the media > > bus code to MEDIA_BUS_FMT_FIXED ? If yes then setting > > the METADATA flag with a different media bus is also an invalid configuration. > > That may be currently the case, but not all non-image data (metadata in > practice) will be using MEDIA_BUS_FMT_FIXED, for instance sensor embedded > data when we support that in upstream. > > Note that whether metadata flows over a pad is dynamic configuration. I > wonder if it is useful to tell this to the user, as the user, in many > cases, will be making the configuration affecting this. There definitely Hi, you mean that there might be pads that can either support metadata or non-metadata? Currently there is no media bus for METDATA so with the flag userspace knows it is metadata. > are devices where this configuration would be static, but many of those > cases (ISPs in particular) have DMAs (i.e. video nodes) directly connected > over a link, where you'll find this information on the video node. > > -- > Regards, > > Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-03-30 12:42 ` Dafna Hirschfeld @ 2020-10-30 4:56 ` Helen Koike 2020-10-30 10:07 ` Dafna Hirschfeld 0 siblings, 1 reply; 9+ messages in thread From: Helen Koike @ 2020-10-30 4:56 UTC (permalink / raw) To: Dafna Hirschfeld, Sakari Ailus Cc: Laurent Pinchart, Dafna Hirschfeld, Linux Media Mailing List, Ezequiel Garcia, Hans Verkuil, kernel, linux-rockchip, mchehab Hi, On 3/30/20 9:42 AM, Dafna Hirschfeld wrote: > On Fri, Mar 27, 2020 at 9:09 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: >> >> Dafna, Laurent, >> >> On Thu, Mar 26, 2020 at 08:59:04AM +0100, Dafna Hirschfeld wrote: >>> On Wed, Mar 25, 2020 at 11:26 PM Laurent Pinchart >>> <laurent.pinchart@ideasonboard.com> wrote: >>>> >>>> Hi Dafna, >>>> >>>> Thank you for the patch. >>>> >>>> On Wed, Mar 25, 2020 at 10:27:03PM +0100, Dafna Hirschfeld wrote: >>>>> Add a flag to the flags field of 'struct media_pad_desc' >>>>> that indicates that the data transmitted by the pad is >>>>> metadata. >>>>> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>> --- >>>>> Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ >>>>> include/uapi/linux/media.h | 1 + >>>>> 2 files changed, 5 insertions(+) >>>>> >>>>> diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst >>>>> index 3af6a414b501..4ca902478971 100644 >>>>> --- a/Documentation/media/uapi/mediactl/media-types.rst >>>>> +++ b/Documentation/media/uapi/mediactl/media-types.rst >>>>> @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements >>>>> .. _MEDIA-PAD-FL-SINK: >>>>> .. _MEDIA-PAD-FL-SOURCE: >>>>> .. _MEDIA-PAD-FL-MUST-CONNECT: >>>>> +.. _MEDIA-PAD-FL-METADATA: >>>>> >>>>> .. flat-table:: Media pad flags >>>>> :header-rows: 0 >>>>> @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements >>>>> configuration dependent) for the pad to need enabled links even >>>>> when this flag isn't set; the absence of the flag doesn't imply >>>>> there is none. >>>>> + * - ``MEDIA_PAD_FL_METADATA`` >>>>> + - This flag indicates that the data transmitted by the pad is of >>>>> + type metadata. >>>>> >>>>> >>>>> One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` >>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h >>>>> index 383ac7b7d8f0..ae37226eb5c9 100644 >>>>> --- a/include/uapi/linux/media.h >>>>> +++ b/include/uapi/linux/media.h >>>>> @@ -210,6 +210,7 @@ struct media_entity_desc { >>>>> #define MEDIA_PAD_FL_SINK (1 << 0) >>>>> #define MEDIA_PAD_FL_SOURCE (1 << 1) >>>>> #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) >>>>> +#define MEDIA_PAD_FL_METADATA (1 << 3) >>>> >>>> I think we need to reserve a few bits here. We'll have more than >>>> metadata. Audio comes to mind, there will likely be more. Having >>>> independent flags would not only waste a bit of space in the bitfield >>>> (not that we're about to run out of bits, but still), but would make it >>>> possible to specify invalid configurations such as MEDIA_PAD_FL_METADATA >>>> | MEDIA_PAD_FL_AUDIO. And now that I've written this, I realize that >>>> audio metadata could be a thing, so maybe a metadata flag is actually >>>> the best option :-) >>> hehe, ok, but drivers that set the METADATA flag should also set the media >>> bus code to MEDIA_BUS_FMT_FIXED ? If yes then setting >>> the METADATA flag with a different media bus is also an invalid configuration. >> >> That may be currently the case, but not all non-image data (metadata in >> practice) will be using MEDIA_BUS_FMT_FIXED, for instance sensor embedded >> data when we support that in upstream. >> >> Note that whether metadata flows over a pad is dynamic configuration. I >> wonder if it is useful to tell this to the user, as the user, in many >> cases, will be making the configuration affecting this. There definitely > > Hi, you mean that there might be pads that can either support metadata > or non-metadata? > Currently there is no media bus for METDATA so with the flag userspace > knows it is metadata. Maybe this is a silly question, but why do we need a flag in the pads to indicate metadata if we have mbus code MEDIA_BUS_FMT_METADATA_FIXED for this? Aren't we adding redundant information? Regards, Helen > >> are devices where this configuration would be static, but many of those >> cases (ISPs in particular) have DMAs (i.e. video nodes) directly connected >> over a link, where you'll find this information on the video node. > >> >> -- >> Regards, >> >> Sakari Ailus > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA 2020-10-30 4:56 ` Helen Koike @ 2020-10-30 10:07 ` Dafna Hirschfeld 0 siblings, 0 replies; 9+ messages in thread From: Dafna Hirschfeld @ 2020-10-30 10:07 UTC (permalink / raw) To: Helen Koike, Dafna Hirschfeld, Sakari Ailus Cc: Laurent Pinchart, Linux Media Mailing List, Ezequiel Garcia, Hans Verkuil, kernel, linux-rockchip, mchehab Am 30.10.20 um 05:56 schrieb Helen Koike: > Hi, > > On 3/30/20 9:42 AM, Dafna Hirschfeld wrote: >> On Fri, Mar 27, 2020 at 9:09 AM Sakari Ailus >> <sakari.ailus@linux.intel.com> wrote: >>> >>> Dafna, Laurent, >>> >>> On Thu, Mar 26, 2020 at 08:59:04AM +0100, Dafna Hirschfeld wrote: >>>> On Wed, Mar 25, 2020 at 11:26 PM Laurent Pinchart >>>> <laurent.pinchart@ideasonboard.com> wrote: >>>>> >>>>> Hi Dafna, >>>>> >>>>> Thank you for the patch. >>>>> >>>>> On Wed, Mar 25, 2020 at 10:27:03PM +0100, Dafna Hirschfeld wrote: >>>>>> Add a flag to the flags field of 'struct media_pad_desc' >>>>>> that indicates that the data transmitted by the pad is >>>>>> metadata. >>>>>> >>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>>> --- >>>>>> Documentation/media/uapi/mediactl/media-types.rst | 4 ++++ >>>>>> include/uapi/linux/media.h | 1 + >>>>>> 2 files changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/media/uapi/mediactl/media-types.rst b/Documentation/media/uapi/mediactl/media-types.rst >>>>>> index 3af6a414b501..4ca902478971 100644 >>>>>> --- a/Documentation/media/uapi/mediactl/media-types.rst >>>>>> +++ b/Documentation/media/uapi/mediactl/media-types.rst >>>>>> @@ -361,6 +361,7 @@ Types and flags used to represent the media graph elements >>>>>> .. _MEDIA-PAD-FL-SINK: >>>>>> .. _MEDIA-PAD-FL-SOURCE: >>>>>> .. _MEDIA-PAD-FL-MUST-CONNECT: >>>>>> +.. _MEDIA-PAD-FL-METADATA: >>>>>> >>>>>> .. flat-table:: Media pad flags >>>>>> :header-rows: 0 >>>>>> @@ -381,6 +382,9 @@ Types and flags used to represent the media graph elements >>>>>> configuration dependent) for the pad to need enabled links even >>>>>> when this flag isn't set; the absence of the flag doesn't imply >>>>>> there is none. >>>>>> + * - ``MEDIA_PAD_FL_METADATA`` >>>>>> + - This flag indicates that the data transmitted by the pad is of >>>>>> + type metadata. >>>>>> >>>>>> >>>>>> One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE`` >>>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h >>>>>> index 383ac7b7d8f0..ae37226eb5c9 100644 >>>>>> --- a/include/uapi/linux/media.h >>>>>> +++ b/include/uapi/linux/media.h >>>>>> @@ -210,6 +210,7 @@ struct media_entity_desc { >>>>>> #define MEDIA_PAD_FL_SINK (1 << 0) >>>>>> #define MEDIA_PAD_FL_SOURCE (1 << 1) >>>>>> #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2) >>>>>> +#define MEDIA_PAD_FL_METADATA (1 << 3) >>>>> >>>>> I think we need to reserve a few bits here. We'll have more than >>>>> metadata. Audio comes to mind, there will likely be more. Having >>>>> independent flags would not only waste a bit of space in the bitfield >>>>> (not that we're about to run out of bits, but still), but would make it >>>>> possible to specify invalid configurations such as MEDIA_PAD_FL_METADATA >>>>> | MEDIA_PAD_FL_AUDIO. And now that I've written this, I realize that >>>>> audio metadata could be a thing, so maybe a metadata flag is actually >>>>> the best option :-) >>>> hehe, ok, but drivers that set the METADATA flag should also set the media >>>> bus code to MEDIA_BUS_FMT_FIXED ? If yes then setting >>>> the METADATA flag with a different media bus is also an invalid configuration. >>> >>> That may be currently the case, but not all non-image data (metadata in >>> practice) will be using MEDIA_BUS_FMT_FIXED, for instance sensor embedded >>> data when we support that in upstream. >>> >>> Note that whether metadata flows over a pad is dynamic configuration. I >>> wonder if it is useful to tell this to the user, as the user, in many >>> cases, will be making the configuration affecting this. There definitely >> >> Hi, you mean that there might be pads that can either support metadata >> or non-metadata? >> Currently there is no media bus for METDATA so with the flag userspace >> knows it is metadata. > > Maybe this is a silly question, but why do we need a flag in the pads to > indicate metadata if we have mbus code MEDIA_BUS_FMT_METADATA_FIXED for this? > Aren't we adding redundant information? Hi, This patch was abandoned in favor for the MEDIA_BUS_FMT_METADATA_FIXED solution. I should have send the MEDIA_BUS_FMT_METADATA_FIXED patch as a new version of this patch since it replaces it. Sorry for that. Thanks, Dafna > > Regards, > Helen > >> >>> are devices where this configuration would be static, but many of those >>> cases (ISPs in particular) have DMAs (i.e. video nodes) directly connected >>> over a link, where you'll find this information on the video node. >> >>> >>> -- >>> Regards, >>> >>> Sakari Ailus >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] media: staging: rkisp1: add the flag MEDIA_PAD_FL_METADATA to the metadata pads 2020-03-25 21:27 [PATCH 0/2] media: staging: rkisp1: Fix formats for metadata pads Dafna Hirschfeld 2020-03-25 21:27 ` [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA Dafna Hirschfeld @ 2020-03-25 21:27 ` Dafna Hirschfeld 1 sibling, 0 replies; 9+ messages in thread From: Dafna Hirschfeld @ 2020-03-25 21:27 UTC (permalink / raw) To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart The 4 pads of the links between kisp1_isp and the params and stats entities transmit metadata. This patch adds the flag MEDIA_PAD_FL_METADATA to those pads. In addition it initializes the width and height of the pads formats to 0 since metadata format has no dimensions. This fixes the TODO item: "Fix pad format size for statistics and parameters entities." So the patch also removes this item. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/staging/media/rkisp1/TODO | 1 - drivers/staging/media/rkisp1/rkisp1-isp.c | 10 ++++++---- drivers/staging/media/rkisp1/rkisp1-params.c | 2 +- drivers/staging/media/rkisp1/rkisp1-stats.c | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO index 0aa9877dd64a..c10e8cb4cc5a 100644 --- a/drivers/staging/media/rkisp1/TODO +++ b/drivers/staging/media/rkisp1/TODO @@ -1,7 +1,6 @@ * Don't use v4l2_async_notifier_parse_fwnode_endpoints_by_port(). e.g. isp_parse_of_endpoints in drivers/media/platform/omap3isp/isp.c cio2_parse_firmware in drivers/media/pci/intel/ipu3/ipu3-cio2.c. -* Fix pad format size for statistics and parameters entities. * Use threaded interrupt for rkisp1_stats_isr(), remove work queue. * Fix checkpatch errors. * Make sure uapi structs have the same size and layout in 32 and 62 bits, diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c index a41c6ff14009..bca9e3d1d94b 100644 --- a/drivers/staging/media/rkisp1/rkisp1-isp.c +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c @@ -654,8 +654,8 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd, RKISP1_ISP_PAD_SINK_PARAMS); src_fmt = v4l2_subdev_get_try_format(sd, cfg, RKISP1_ISP_PAD_SOURCE_STATS); - sink_fmt->width = RKISP1_DEFAULT_WIDTH; - sink_fmt->height = RKISP1_DEFAULT_HEIGHT; + sink_fmt->width = 0; + sink_fmt->height = 0; sink_fmt->field = V4L2_FIELD_NONE; sink_fmt->code = MEDIA_BUS_FMT_FIXED; *src_fmt = *sink_fmt; @@ -1041,9 +1041,11 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1, pads[RKISP1_ISP_PAD_SINK_VIDEO].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT; - pads[RKISP1_ISP_PAD_SINK_PARAMS].flags = MEDIA_PAD_FL_SINK; + pads[RKISP1_ISP_PAD_SINK_PARAMS].flags = + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_METADATA; pads[RKISP1_ISP_PAD_SOURCE_VIDEO].flags = MEDIA_PAD_FL_SOURCE; - pads[RKISP1_ISP_PAD_SOURCE_STATS].flags = MEDIA_PAD_FL_SOURCE; + pads[RKISP1_ISP_PAD_SOURCE_STATS].flags = + MEDIA_PAD_FL_SOURCE | MEDIA_PAD_FL_METADATA; isp->sink_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SINK_PAD_FMT); isp->src_fmt = rkisp1_isp_mbus_info_get(RKISP1_DEF_SRC_PAD_FMT); diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c index 44d542caf32b..e934fab50262 100644 --- a/drivers/staging/media/rkisp1/rkisp1-params.c +++ b/drivers/staging/media/rkisp1/rkisp1-params.c @@ -1601,7 +1601,7 @@ int rkisp1_params_register(struct rkisp1_params *params, rkisp1_init_params(params); video_set_drvdata(vdev, params); - node->pad.flags = MEDIA_PAD_FL_SOURCE; + node->pad.flags = MEDIA_PAD_FL_SOURCE | MEDIA_PAD_FL_METADATA; ret = media_entity_pads_init(&vdev->entity, 1, &node->pad); if (ret) goto err_release_queue; diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c index 6dfcbdc3deb8..c54d69aea7de 100644 --- a/drivers/staging/media/rkisp1/rkisp1-stats.c +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c @@ -481,7 +481,7 @@ int rkisp1_stats_register(struct rkisp1_stats *stats, rkisp1_init_stats(stats); video_set_drvdata(vdev, stats); - node->pad.flags = MEDIA_PAD_FL_SINK; + node->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_METADATA; ret = media_entity_pads_init(&vdev->entity, 1, &node->pad); if (ret) goto err_release_queue; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-30 10:08 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-25 21:27 [PATCH 0/2] media: staging: rkisp1: Fix formats for metadata pads Dafna Hirschfeld 2020-03-25 21:27 ` [PATCH 1/2] media: media.h: Add a pad flag MEDIA_PAD_FL_METADATA Dafna Hirschfeld 2020-03-25 22:26 ` Laurent Pinchart 2020-03-26 7:59 ` Dafna Hirschfeld 2020-03-27 8:08 ` Sakari Ailus 2020-03-30 12:42 ` Dafna Hirschfeld 2020-10-30 4:56 ` Helen Koike 2020-10-30 10:07 ` Dafna Hirschfeld 2020-03-25 21:27 ` [PATCH 2/2] media: staging: rkisp1: add the flag MEDIA_PAD_FL_METADATA to the metadata pads Dafna Hirschfeld
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).