Re: [PATCH v2 09/41] drm/connector: Add TV standard property
W dniu 9.09.2022 o 11:46, Maxime Ripard pisze:
> On Wed, Sep 07, 2022 at 09:52:09PM +0200, Mateusz Kwiatkowski wrote:
>> W dniu 7.09.2022 o 14:10, Maxime Ripard pisze:
>>> Hi,
>>>
>>> On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> The TV mode property has been around for a while now to select and get the
> current TV mode output on an analog TV connector.
>
> Despite that property name being generic, its content isn't and has been
> driver-specific which makes it hard to build any generic behaviour on top
> of it, both in kernel and user-space.
>
> Let's create a new bitmask tv norm property, that can contain any of the
> analog TV standards currently supported by kernel drivers. Each driver can
> then pass in a bitmask of the modes it supports.
This is not a bitmask property anymore, you've just changed it to an enum.
The commit message is now misleading.
> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> +};
I did not comment on it the last time, but this list looks a little bit
random.
Compared to the standards defined by V4L2, you also define SECAM-60 (a good
thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
SECAM-H, SECAM-LC (whatever that is - probably just another name for
SECAM-L,
see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of
NTSC).
Like I mentioned previously, I'm personally not a fan of including all
those
CCIR/ITU system variants, as they don't mean any difference to the output
unless
there is an RF modulator involved. But I get it that they have already
been used
and regressing probably wouldn't be a very good idea. But in that case
keeping
it consistent with the set of values used by V4L2 would be wise, I think.
>>>
>>> Ack. What would be the list of standards we'd absolutely need? NSTC-M,
>>> NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
>>
>> The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and
>> SECAM.
>> Note that:
>>
>> - I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system
>> designation. If we only consider composite signals, there's no difference
>> between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a
>>generic
>> mode, IMO.
>>
>> * PAL-M and PAL-N are different, because those unique color encodings were
>> only ever used with Systems M and N, respectively.
>>
>> * NTSC-J is also different, because "System J" doesn't exist anywhere in
>>ITU
>> documents. Japan technically uses System M with a non-standard black
>>level.
>> But "NTSC-J" stuck as a universally recognized name for that variant.
>>
>> - I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just
>> regular PAL paired with 480i60 modeline. Most if not all displays that
>> accept PAL-60 input will just label it as "PAL". If we are not introducing
>> strict modeline validation, then maybe separating PAL and PAL-60 isn't
>>really
>> necessary? Same goes for SECAM vs. SECAM-60.
>>
>> ...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic
>> mode, but known to exist at least in the Atari ST world, see also:
>> https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50
>>
>> Combining PAL and PAL-60 into a single setting would complicate the vc4
>> driver
>> a little bit, though, as the registers need to be set up differently for
>> those.
>>
>> My feelings about the PAL-60 issue are not that strong, though. Merging PAL
>> and PAL-60 in this context is just a loose suggestion, I won't even try to
>> argue if you disagree.
>
> Ack
>
> +/**
> + * drm_mode_create_tv_properties - create TV specific connector
> properties
> + * @dev: DRM device
> + * @supported_tv_modes: Bitmask of TV modes supported (See
> DRM_MODE_TV_MODE_*)
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
On Wed, Sep 07, 2022 at 09:52:09PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 7.09.2022 o 14:10, Maxime Ripard pisze:
> > Hi,
> >
> > On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
> >> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> >>> The TV mode property has been around for a while now to select and get the
> >>> current TV mode output on an analog TV connector.
> >>>
> >>> Despite that property name being generic, its content isn't and has been
> >>> driver-specific which makes it hard to build any generic behaviour on top
> >>> of it, both in kernel and user-space.
> >>>
> >>> Let's create a new bitmask tv norm property, that can contain any of the
> >>> analog TV standards currently supported by kernel drivers. Each driver can
> >>> then pass in a bitmask of the modes it supports.
> >>
> >> This is not a bitmask property anymore, you've just changed it to an enum.
> >> The commit message is now misleading.
> >>
> >>> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> >>> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> >>> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> >>> + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> >>> + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> >>> + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> >>> + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> >>> + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> >>> + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> >>> + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> >>> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> >>> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> >>> + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> >>> + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> >>> + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> >>> + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> >>> + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> >>> + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> >>> + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> >>> + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> >>> +};
> >>
> >> I did not comment on it the last time, but this list looks a little bit
> >> random.
> >>
> >> Compared to the standards defined by V4L2, you also define SECAM-60 (a good
> >> thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
> >> SECAM-H, SECAM-LC (whatever that is - probably just another name for
> >> SECAM-L,
> >> see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of
> >> NTSC).
> >>
> >> Like I mentioned previously, I'm personally not a fan of including all
> >> those
> >> CCIR/ITU system variants, as they don't mean any difference to the output
> >> unless
> >> there is an RF modulator involved. But I get it that they have already
> >> been used
> >> and regressing probably wouldn't be a very good idea. But in that case
> >> keeping
> >> it consistent with the set of values used by V4L2 would be wise, I think.
> >
> > Ack. What would be the list of standards we'd absolutely need? NSTC-M,
> > NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
>
> The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and
> SECAM.
> Note that:
>
> - I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system
> designation. If we only consider composite signals, there's no difference
> between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a generic
> mode, IMO.
>
> * PAL-M and PAL-N are different, because those unique color encodings were
> only ever used with Systems M and N, respectively.
>
> * NTSC-J is also different, because "System J" doesn't exist anywhere in ITU
> documents. Japan technically uses System M with a non-standard black
> level.
> But "NTSC-J" stuck as a universally recognized name for that variant.
>
> - I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just
> regular PAL paired with 480i60 modeline. Most if not all displays that
> accept PAL-60 input will just label it as "PAL". If we are not introducing
> strict modeline validation, then maybe separating PAL and PAL-60 isn't
> really
> necessary? Same goes for SECAM vs. SECAM-60.
>
> ...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic
> mode, but known to exist at least in the Atari ST world, see also:
> https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50
>
> Combining PAL and PAL-60 into a single setting would complicate the vc4 driver
> a little bit, though, as the registers need to be set up differently for
> those.
>
> My feelings about the PAL-60 issue are not that strong, though. Merging PAL
> and PAL-60 in this context is just a loose suggestion, I won't even try to
> argue if you disagree.
Ack
> >>> +/**
> >>> + * drm_mode_create_tv_properties - create TV specific connector
> >>> properties
> >>> + * @dev: DRM device
> >>> + * @supported_tv_modes: Bitmask of TV modes supported (See
> >>> DRM_MODE_TV_MODE_*)
> >>> +
> >>> + * Called by a driver's TV initialization routine, this function creates
> >
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Maxime,
W dniu 7.09.2022 o 14:10, Maxime Ripard pisze:
> Hi,
>
> On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
>> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
>>> The TV mode property has been around for a while now to select and get the
>>> current TV mode output on an analog TV connector.
>>>
>>> Despite that property name being generic, its content isn't and has been
>>> driver-specific which makes it hard to build any generic behaviour on top
>>> of it, both in kernel and user-space.
>>>
>>> Let's create a new bitmask tv norm property, that can contain any of the
>>> analog TV standards currently supported by kernel drivers. Each driver can
>>> then pass in a bitmask of the modes it supports.
>>
>> This is not a bitmask property anymore, you've just changed it to an enum.
>> The commit message is now misleading.
>>
>>> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
>>> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
>>> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
>>> + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
>>> + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
>>> + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
>>> + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
>>> + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
>>> + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
>>> + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
>>> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
>>> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
>>> + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
>>> + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
>>> + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
>>> + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
>>> + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
>>> + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
>>> + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
>>> + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
>>> +};
>>
>> I did not comment on it the last time, but this list looks a little bit
>> random.
>>
>> Compared to the standards defined by V4L2, you also define SECAM-60 (a good
>> thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
>> SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L,
>> see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC).
>>
>> Like I mentioned previously, I'm personally not a fan of including all those
>> CCIR/ITU system variants, as they don't mean any difference to the output
>> unless
>> there is an RF modulator involved. But I get it that they have already been
>> used
>> and regressing probably wouldn't be a very good idea. But in that case
>> keeping
>> it consistent with the set of values used by V4L2 would be wise, I think.
>
> Ack. What would be the list of standards we'd absolutely need? NSTC-M,
> NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and SECAM.
Note that:
- I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system
designation. If we only consider composite signals, there's no difference
between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a generic
mode, IMO.
* PAL-M and PAL-N are different, because those unique color encodings were
only ever used with Systems M and N, respectively.
* NTSC-J is also different, because "System J" doesn't exist anywhere in ITU
documents. Japan technically uses System M with a non-standard black level.
But "NTSC-J" stuck as a universally recognized name for that variant.
- I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just
regular PAL paired with 480i60 modeline. Most if not all displays that
accept PAL-60 input will just label it as "PAL". If we are not introducing
strict modeline validation, then maybe separating PAL and PAL-60 isn't really
necessary? Same goes for SECAM vs. SECAM-60.
...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic
mode, but known to exist at least in the Atari ST world, see also:
https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50
Combining PAL and PAL-60 into a single setting would complicate the vc4 driver
a little bit, though, as the registers need to be set up differently for those.
My feelings about the PAL-60 issue are not that strong, though. Merging PAL
and PAL-60 in this context is just a loose suggestion, I won't even try to
argue if you disagree.
>>> +/**
>>> + * drm_mode_create_tv_properties - create TV specific connector properties
>>> + * @dev: DRM device
>>> + * @supported_tv_modes: Bitmask of TV modes supported (See
>>> DRM_MODE_TV_MODE_*)
>>> +
>>> + * Called by a driver's TV initialization routine, this function creates
>>> + * the TV specific connector properties for a given device. Caller is
>>> + * responsible for allocating a list of format names and passing them to
>>> + * this routine.
>>> + *
>>> + * Returns:
>>> + * 0 on success or a negative error code on failure.
>>> + */
>>> +int drm_mode_create_tv_prop
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
On Fri, Sep 02, 2022 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
> On Fri, Sep 2, 2022 at 12:00 AM Mateusz Kwiatkowski wrote:
> > W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> > > The TV mode property has been around for a while now to select and get the
> > > current TV mode output on an analog TV connector.
> > >
> > > Despite that property name being generic, its content isn't and has been
> > > driver-specific which makes it hard to build any generic behaviour on top
> > > of it, both in kernel and user-space.
> > >
> > > Let's create a new bitmask tv norm property, that can contain any of the
> > > analog TV standards currently supported by kernel drivers. Each driver can
> > > then pass in a bitmask of the modes it supports.
> >
> > This is not a bitmask property anymore, you've just changed it to an enum.
> > The commit message is now misleading.
> >
> > > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> > > +{ DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> > > +{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> > > +{ DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> > > +{ DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> > > +{ DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> > > +{ DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> > > +{ DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> > > +{ DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> > > +{ DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> > > +{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> > > +{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> > > +{ DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> > > +{ DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> > > +{ DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> > > +{ DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> > > +{ DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> > > +{ DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> > > +{ DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> > > +{ DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> > > +};
> >
> > I did not comment on it the last time, but this list looks a little bit
> > random.
> >
> > Compared to the standards defined by V4L2, you also define SECAM-60 (a good
> > thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
> > SECAM-H, SECAM-LC (whatever that is - probably just another name for
> > SECAM-L,
> > see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC).
> >
> > Like I mentioned previously, I'm personally not a fan of including all those
> > CCIR/ITU system variants, as they don't mean any difference to the output
> > unless
> > there is an RF modulator involved. But I get it that they have already been
> > used
> > and regressing probably wouldn't be a very good idea. But in that case
> > keeping
> > it consistent with the set of values used by V4L2 would be wise, I think.
>
> Exactly. Anything outputting RGB (e.g. through a SCART or VGA connector)
> doesn't care about the color subcarrier or modulator parts. Likewise,
> anything outputting CVBS doesn't care about the modulator part.
>
> Perhaps "generic" variants of NSTC and PAL/SECAM should be added, which
> would really just mean 525/60 resp. 625/50.
>
> Alternatively, the tv_mode field could be split in two parts (either
> two separate fields, or bitwise), to maintain a clear separation between
> lines/fields versus color encoding and RF modulation (with zero for the
> latter meaning a generic version)? That would also keep the door open
> for TV_MODE_405_50, TV_MODE_819_50, TV_MODE_750_50, TV_MODE_750_60, ...
Again, that property is only about color encoding and RF modulation. The
lines numbers and whether it's interlaced or not is encoded in the mode,
not here. So what you suggest is totally doable today.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Hi,
On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> > The TV mode property has been around for a while now to select and get the
> > current TV mode output on an analog TV connector.
> >
> > Despite that property name being generic, its content isn't and has been
> > driver-specific which makes it hard to build any generic behaviour on top
> > of it, both in kernel and user-space.
> >
> > Let's create a new bitmask tv norm property, that can contain any of the
> > analog TV standards currently supported by kernel drivers. Each driver can
> > then pass in a bitmask of the modes it supports.
>
> This is not a bitmask property anymore, you've just changed it to an enum.
> The commit message is now misleading.
>
> > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> > + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> > + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> > + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> > + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> > + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> > + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> > + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> > + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> > + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> > + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> > + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> > + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> > + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> > + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> > + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> > + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> > + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> > + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> > + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> > +};
>
> I did not comment on it the last time, but this list looks a little bit
> random.
>
> Compared to the standards defined by V4L2, you also define SECAM-60 (a good
> thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
> SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L,
> see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC).
>
> Like I mentioned previously, I'm personally not a fan of including all those
> CCIR/ITU system variants, as they don't mean any difference to the output
> unless
> there is an RF modulator involved. But I get it that they have already been
> used
> and regressing probably wouldn't be a very good idea. But in that case keeping
> it consistent with the set of values used by V4L2 would be wise, I think.
Ack. What would be the list of standards we'd absolutely need? NSTC-M,
NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
> > +/**
> > + * drm_mode_create_tv_properties - create TV specific connector properties
> > + * @dev: DRM device
> > + * @supported_tv_modes: Bitmask of TV modes supported (See
> > DRM_MODE_TV_MODE_*)
> > +
> > + * Called by a driver's TV initialization routine, this function creates
> > + * the TV specific connector properties for a given device. Caller is
> > + * responsible for allocating a list of format names and passing them to
> > + * this routine.
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code on failure.
> > + */
> > +int drm_mode_create_tv_properties(struct drm_device *dev,
> > + unsigned int supported_tv_modes)
>
> supported_tv_modes is supposed to be a bitmask of BIT(DRM_MODE_TV_MODE_*)
> (or (1< is not said explicitly anywhere in this doc comment.
The argument doc mentions that it's a "Bitmask of TV modes supported
(See DRM_MODE_TV_MODE_*)", how would you improve it?
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Den 29.08.2022 15.11, skrev Maxime Ripard:
> The TV mode property has been around for a while now to select and get the
>
> current TV mode output on an analog TV connector.
>
>
>
> Despite that property name being generic, its content isn't and has been
>
> driver-specific which makes it hard to build any generic behaviour on top
>
> of it, both in kernel and user-space.
>
>
>
> Let's create a new bitmask tv norm property, that can contain any of the
>
> analog TV standards currently supported by kernel drivers. Each driver can
>
> then pass in a bitmask of the modes it supports.
>
>
>
> We'll then be able to phase out the older tv mode property.
>
>
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> +/**
>
> + * drm_mode_create_tv_properties - create TV specific connector properties
>
> + * @dev: DRM device
>
> + * @supported_tv_modes: Bitmask of TV modes supported (See
> DRM_MODE_TV_MODE_*)
>
> +
>
> + * Called by a driver's TV initialization routine, this function creates
>
> + * the TV specific connector properties for a given device. Caller is
>
> + * responsible for allocating a list of format names and passing them to
>
> + * this routine.
>
> + *
>
> + * Returns:
>
> + * 0 on success or a negative error code on failure.
>
> + */
>
> +int drm_mode_create_tv_properties(struct drm_device *dev,
>
> + unsigned int supported_tv_modes)
>
> +{
>
> + struct drm_prop_enum_list tv_mode_list[DRM_MODE_TV_MODE_MAX];
>
> + struct drm_property *tv_mode;
>
> + unsigned int i, len = 0;
>
> +
>
Can you add a check here like in the legacy version:
if (dev->mode_config.tv_mode_property)
return 0;
This way it's possible to call this multiple times. Like in drm/gud
during connector init if there are multiple TV connectors or if a device
with multiple IP blocks should show up.
Noralf.
> + for (i = 0; i < DRM_MODE_TV_MODE_MAX; i++) {
>
> + if (!(supported_tv_modes & BIT(i)))
>
> + continue;
>
> +
>
> + tv_mode_list[len].type = i;
>
> + tv_mode_list[len].name = drm_get_tv_mode_name(i);
>
> + len++;
>
> + }
>
> +
>
> + tv_mode = drm_property_create_enum(dev, 0, "TV mode",
>
> +tv_mode_list, len);
>
> + if (!tv_mode)
>
> + return -ENOMEM;
>
> +
>
> + dev->mode_config.tv_mode_property = tv_mode;
>
> +
>
> + return drm_mode_create_tv_properties_legacy(dev, 0, NULL);
>
> +}
>
> +EXPORT_SYMBOL(drm_mode_create_tv_properties);
>
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Mateusz,
On Fri, Sep 2, 2022 at 12:00 AM Mateusz Kwiatkowski wrote:
> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> > The TV mode property has been around for a while now to select and get the
> > current TV mode output on an analog TV connector.
> >
> > Despite that property name being generic, its content isn't and has been
> > driver-specific which makes it hard to build any generic behaviour on top
> > of it, both in kernel and user-space.
> >
> > Let's create a new bitmask tv norm property, that can contain any of the
> > analog TV standards currently supported by kernel drivers. Each driver can
> > then pass in a bitmask of the modes it supports.
>
> This is not a bitmask property anymore, you've just changed it to an enum.
> The commit message is now misleading.
>
> > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> > +{ DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> > +{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> > +{ DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> > +{ DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> > +{ DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> > +{ DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> > +{ DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> > +{ DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> > +{ DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> > +{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> > +{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> > +{ DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> > +{ DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> > +{ DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> > +{ DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> > +{ DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> > +{ DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> > +{ DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> > +{ DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> > +};
>
> I did not comment on it the last time, but this list looks a little bit
> random.
>
> Compared to the standards defined by V4L2, you also define SECAM-60 (a good
> thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
> SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L,
> see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC).
>
> Like I mentioned previously, I'm personally not a fan of including all those
> CCIR/ITU system variants, as they don't mean any difference to the output
> unless
> there is an RF modulator involved. But I get it that they have already been
> used
> and regressing probably wouldn't be a very good idea. But in that case keeping
> it consistent with the set of values used by V4L2 would be wise, I think.
Exactly. Anything outputting RGB (e.g. through a SCART or VGA connector)
doesn't care about the color subcarrier or modulator parts. Likewise,
anything outputting CVBS doesn't care about the modulator part.
Perhaps "generic" variants of NSTC and PAL/SECAM should be added, which
would really just mean 525/60 resp. 625/50.
Alternatively, the tv_mode field could be split in two parts (either
two separate fields, or bitwise), to maintain a clear separation between
lines/fields versus color encoding and RF modulation (with zero for the
latter meaning a generic version)? That would also keep the door open
for TV_MODE_405_50, TV_MODE_819_50, TV_MODE_750_50, TV_MODE_750_60, ...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Maxime,
W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> The TV mode property has been around for a while now to select and get the
> current TV mode output on an analog TV connector.
>
> Despite that property name being generic, its content isn't and has been
> driver-specific which makes it hard to build any generic behaviour on top
> of it, both in kernel and user-space.
>
> Let's create a new bitmask tv norm property, that can contain any of the
> analog TV standards currently supported by kernel drivers. Each driver can
> then pass in a bitmask of the modes it supports.
This is not a bitmask property anymore, you've just changed it to an enum.
The commit message is now misleading.
> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> +};
I did not comment on it the last time, but this list looks a little bit random.
Compared to the standards defined by V4L2, you also define SECAM-60 (a good
thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L,
see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC).
Like I mentioned previously, I'm personally not a fan of including all those
CCIR/ITU system variants, as they don't mean any difference to the output unless
there is an RF modulator involved. But I get it that they have already been used
and regressing probably wouldn't be a very good idea. But in that case keeping
it consistent with the set of values used by V4L2 would be wise, I think.
> +/**
> + * drm_mode_create_tv_properties - create TV specific connector properties
> + * @dev: DRM device
> + * @supported_tv_modes: Bitmask of TV modes supported (See
> DRM_MODE_TV_MODE_*)
> +
> + * Called by a driver's TV initialization routine, this function creates
> + * the TV specific connector properties for a given device. Caller is
> + * responsible for allocating a list of format names and passing them to
> + * this routine.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_mode_create_tv_properties(struct drm_device *dev,
> + unsigned int supported_tv_modes)
supported_tv_modes is supposed to be a bitmask of BIT(DRM_MODE_TV_MODE_*)
(or (1< + /**
> + * @DRM_MODE_TV_MODE_PAL_NC: Seems equivalent to
> + * @DRM_MODE_TV_MODE_PAL_N.
> + */
> + DRM_MODE_TV_MODE_PAL_NC,
AFAIK, the entire reason that "PAL-Nc" is ever mentioned as something separate
from PAL-N is a result of a misunderstanding or misreading of the CCIR/ITU
documents. See also the posting signed as Alchaemist here:
https://en.wikipedia.org/wiki/Talk:PAL#PAL-N_versus_PAL-Nc
That being said, we probably want to keep it if we want to remaing compatible
with the loads of software and drivers which enumerate those as separate
systems. But from a technical standpoint, PAL-N and PAL-Nc (and N/PAL, PAL-CN
etc.) are just different "spellings" referring to exactly the same system.
> + /**
> + * @DRM_MODE_TV_MODE_SECAM_K: CCIR System G together with the
> + * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G but
> + * with different channels.
> + */
> + DRM_MODE_TV_MODE_SECAM_K,
> +
> + /**
> + * @DRM_MODE_TV_MODE_SECAM_K1: CCIR System G together with the
> + * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G and
> + * @DRM_MODE_TV_MODE_SECAM_K but with different channels.
> + */
> + DRM_MODE_TV_MODE_SECAM_K1,
Typos: you meant CCIR Systems K and K1, not System G.
Best regards,
Mateusz Kwiatkowski
[PATCH v2 09/41] drm/connector: Add TV standard property
The TV mode property has been around for a while now to select and get the
current TV mode output on an analog TV connector.
Despite that property name being generic, its content isn't and has been
driver-specific which makes it hard to build any generic behaviour on top
of it, both in kernel and user-space.
Let's create a new bitmask tv norm property, that can contain any of the
analog TV standards currently supported by kernel drivers. Each driver can
then pass in a bitmask of the modes it supports.
We'll then be able to phase out the older tv mode property.
Signed-off-by: Maxime Ripard
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 7f2b9a07fbdf..d867e7f9f2cd 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -700,6 +700,8 @@ static int drm_atomic_connector_set_property(struct
drm_connector *connector,
state->tv.margins.bottom = val;
} else if (property == config->legacy_tv_mode_property) {
state->tv.legacy_mode = val;
+ } else if (property == config->tv_mode_property) {
+ state->tv.mode = val;
} else if (property == config->tv_brightness_property) {
state->tv.brightness = val;
} else if (property == config->tv_contrast_property) {
@@ -810,6 +812,8 @@ drm_atomic_connector_get_property(struct drm_connector
*connector,
*val = state->tv.margins.bottom;
} else if (property == config->legacy_tv_mode_property) {
*val = state->tv.legacy_mode;
+ } else if (property == config->tv_mode_property) {
+ *val = state->tv.mode;
} else if (property == config->tv_brightness_property) {
*val = state->tv.brightness;
} else if (property == config->tv_contrast_property) {
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 4e4fbc9e0049..b1fcacd150e8 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -980,6 +980,29 @@ static const struct drm_prop_enum_list
drm_dvi_i_subconnector_enum_list[] = {
DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name,
drm_dvi_i_subconnector_enum_list)
+static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
+ { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
+ { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
+ { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
+ { DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
+ { DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
+ { DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
+ { DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
+ { DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
+ { DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
+ { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
+ { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
+ { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
+ { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
+ { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
+ { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
+ { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
+ { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
+ { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
+ { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
+};
+DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
+
static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
{ DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
{ DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
@@ -1645,6 +1668,10 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties);
* responsible for allocating a list of format names and passing them to
* this routine.
*
+ * NOTE: This functions registers the deprecated "mode" connector
+ * property to select the analog TV mode (ie, NTSC, PAL, etc.). New
+ * drivers must use drm_mode_create_tv_properties() instead.
+ *
* Returns:
* 0 on success or a negative error code on failure.
*/
@@ -1686,7 +1713,6 @@ int drm_mode_create_tv_properties_legacy(struct
drm_device *dev,
if (drm_mode_create_tv_margin_properties(dev))
goto nomem;
-
if (num_modes) {
dev->mode_config.legacy_tv_mode_property =
drm_property_create(dev, DRM_MODE_PROP_ENUM,
@@ -1735,6 +1761,46 @@ int drm_mode_create_tv_properties_legacy(struct
drm_device *dev,
}
EXPORT_SYMBOL(drm_mode_create_tv_properties_legacy);
+/**
+ * drm_mode_create_tv_properties - create TV specific connector properties
+ * @dev: DRM device
+ * @supported_tv_modes: Bitmask of TV modes supported (See DRM_MODE_TV_MODE_*)
+
+ * Called by a driver's TV initialization routine, this function creates
+ * the TV specific connector properties for a given device. Caller is
+ * responsible for allocating a list of format names and passing them to
+ * this routine.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_mode_create_tv_properties(struct drm_device *dev,
+
