[PATCH] drm: document TV margin properties

[PATCH] drm: document TV margin properties

[Simon Ser]

Add docs for "{left,right,top,bottom} margin" properties.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
---
 drivers/gpu/drm/drm_connector.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 9d0250c28e9b..65a586680940 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1590,10 +1590,6 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
 
 /*
  * TODO: Document the properties:
- *   - left margin
- *   - right margin
- *   - top margin
- *   - bottom margin
  *   - brightness
  *   - contrast
  *   - flicker reduction
@@ -1651,6 +1647,16 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
  *
  *	Drivers can set up this property by calling
  *	drm_mode_create_tv_properties().
+ *
+ * left margin, right margin, top margin, bottom margin:
+ *	Add margins to the connector's viewport.
+ *
+ *	The value is the size in pixels of the black border which will be
+ *	added. The attached CRTC's content will be scaled to fill the whole
+ *	area inside the margin.
+ *
+ *	Drivers can set up these properties by calling
+ *	drm_mode_create_tv_margin_properties().
  */
 
 /**
-- 
2.39.2

Re: [PATCH] drm: document TV margin properties

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Mon, Feb 27, 2023 at 12:21:16PM +0000, Simon Ser wrote:
> Add docs for "{left,right,top,bottom} margin" properties.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm: document TV margin properties

[Dave Stevenson]

On Mon, 27 Feb 2023 at 12:21, Simon Ser <contact@emersion.fr> wrote:
>
> Add docs for "{left,right,top,bottom} margin" properties.
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>

This certainly matches my understanding of the properties. Thanks.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/drm_connector.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 9d0250c28e9b..65a586680940 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1590,10 +1590,6 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>
>  /*
>   * TODO: Document the properties:
> - *   - left margin
> - *   - right margin
> - *   - top margin
> - *   - bottom margin
>   *   - brightness
>   *   - contrast
>   *   - flicker reduction
> @@ -1651,6 +1647,16 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>   *
>   *     Drivers can set up this property by calling
>   *     drm_mode_create_tv_properties().
> + *
> + * left margin, right margin, top margin, bottom margin:
> + *     Add margins to the connector's viewport.
> + *
> + *     The value is the size in pixels of the black border which will be
> + *     added. The attached CRTC's content will be scaled to fill the whole
> + *     area inside the margin.
> + *
> + *     Drivers can set up these properties by calling
> + *     drm_mode_create_tv_margin_properties().
>   */
>
>  /**
> --
> 2.39.2
>
>

Re: [PATCH] drm: document TV margin properties

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]

On Mon, 27 Feb 2023 12:21:16 +0000
Simon Ser <contact@emersion.fr> wrote:

> Add docs for "{left,right,top,bottom} margin" properties.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 9d0250c28e9b..65a586680940 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1590,10 +1590,6 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>  
>  /*
>   * TODO: Document the properties:
> - *   - left margin
> - *   - right margin
> - *   - top margin
> - *   - bottom margin
>   *   - brightness
>   *   - contrast
>   *   - flicker reduction
> @@ -1651,6 +1647,16 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>   *
>   *	Drivers can set up this property by calling
>   *	drm_mode_create_tv_properties().
> + *
> + * left margin, right margin, top margin, bottom margin:
> + *	Add margins to the connector's viewport.
> + *
> + *	The value is the size in pixels of the black border which will be
> + *	added. The attached CRTC's content will be scaled to fill the whole
> + *	area inside the margin.
> + *
> + *	Drivers can set up these properties by calling
> + *	drm_mode_create_tv_margin_properties().
>   */
>  
>  /**

Hi Simon,

can these be negative as well, to achieve overscan and not just
underscan? Did I get overscan and underscan right... these are related
to under/overscan, aren't they?

Hmm, no, I guess that doesn't make sense, there is no room in the
signal to have negative margins, it would result in clipping the
framebuffer while scaling up. So this can only be used to scale
framebuffer *down*, add borders, and the TV then scales it back up
again?

Looks like neither my Intel nor AMD cards support these, I don't see
the properties. More things to the list of KMS properties Weston needs
to explicitly control. Oh, it seems vc4 exclusive for now.

Where does this text appear in the HTML kernel docs? I tried to look at
drm_connector.c but I cannot find the spot where this patch applies.

Is this actually a connector property? How does that work, should this
not be a CRTC property?

Is this instead not scaling anything but simply sending metadata
through the connector?

Or are there underlying requirements that this connector property is
actually affecting the CRTC, which means that it is fundamentally
impossible to use multiple connectors with different values on the same
CRTC? And drivers will reject any attempt, so there is no need to
define what conflicting settings will do?

IOW, does simply the existence of these properties on a connector
guarantee that the connector must be the only one on a CRTC?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document TV margin properties

[Simon Ser]

On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> can these be negative as well, to achieve overscan and not just
> underscan? Did I get overscan and underscan right... these are related
> to under/overscan, aren't they?
> 
> Hmm, no, I guess that doesn't make sense, there is no room in the
> signal to have negative margins, it would result in clipping the
> framebuffer while scaling up. So this can only be used to scale
> framebuffer down, add borders, and the TV then scales it back up
> again?

Correct.

> Looks like neither my Intel nor AMD cards support these, I don't see
> the properties. More things to the list of KMS properties Weston needs
> to explicitly control. Oh, it seems vc4 exclusive for now.

i915 does support it but for TV connectors only (i915/display/intel_tv.c).
gud also supports it.

> Where does this text appear in the HTML kernel docs? I tried to look at
> drm_connector.c but I cannot find the spot where this patch applies.

Here:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#analog-tv-specific-connector-properties

> Is this actually a connector property? How does that work, should this
> not be a CRTC property?

Yeah, it's a connector property for some reason.

> Is this instead not scaling anything but simply sending metadata
> through the connector?

No metadata is sent. This is purely equivalent to setting up CRTC_*
properties to scale the planes.

> Or are there underlying requirements that this connector property is
> actually affecting the CRTC, which means that it is fundamentally
> impossible to use multiple connectors with different values on the same
> CRTC? And drivers will reject any attempt, so there is no need to
> define what conflicting settings will do?

I don't think any driver above supports cloning CRTCs for these
connector types. i915 sets clonable = false for these connectors.
vc4 picks the first connector's TV margins, always.

> IOW, does simply the existence of these properties on a connector
> guarantee that the connector must be the only one on a CRTC?

I suppose that there could exist some hardware capable of applying
margins post-CRTC? Such driver could support cloning CRTCs and still
applying the connector margins correctly.

Re: [PATCH] drm: document TV margin properties

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On Tue, 28 Feb 2023 09:53:47 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > can these be negative as well, to achieve overscan and not just
> > underscan? Did I get overscan and underscan right... these are related
> > to under/overscan, aren't they?
> > 
> > Hmm, no, I guess that doesn't make sense, there is no room in the
> > signal to have negative margins, it would result in clipping the
> > framebuffer while scaling up. So this can only be used to scale
> > framebuffer down, add borders, and the TV then scales it back up
> > again?  
> 
> Correct.
> 
> > Looks like neither my Intel nor AMD cards support these, I don't see
> > the properties. More things to the list of KMS properties Weston needs
> > to explicitly control. Oh, it seems vc4 exclusive for now.  
> 
> i915 does support it but for TV connectors only (i915/display/intel_tv.c).
> gud also supports it.
> 
> > Where does this text appear in the HTML kernel docs? I tried to look at
> > drm_connector.c but I cannot find the spot where this patch applies.  
> 
> Here:
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#analog-tv-specific-connector-properties

Analog TV properties? So this does not apply to e.g. HDMI?

I believe HDMI TVs do have the problems that margins could mitigate.

> > Is this actually a connector property? How does that work, should this
> > not be a CRTC property?  
> 
> Yeah, it's a connector property for some reason.
> 
> > Is this instead not scaling anything but simply sending metadata
> > through the connector?  
> 
> No metadata is sent. This is purely equivalent to setting up CRTC_*
> properties to scale the planes.

Oh! That would be really good to mention in the doc. Maybe even prefer
plane props over this? Or is this for analog TV, and plane props for
digital TV?


The above are the important comments. All below is just musings you can
ignore if you wish.

> > Or are there underlying requirements that this connector property is
> > actually affecting the CRTC, which means that it is fundamentally
> > impossible to use multiple connectors with different values on the same
> > CRTC? And drivers will reject any attempt, so there is no need to
> > define what conflicting settings will do?  
> 
> I don't think any driver above supports cloning CRTCs for these
> connector types. i915 sets clonable = false for these connectors.
> vc4 picks the first connector's TV margins, always.

Sounds like i915 does it right, and vc4 does not, assuming vc4 does not
prevent cloning.

> 
> > IOW, does simply the existence of these properties on a connector
> > guarantee that the connector must be the only one on a CRTC?  
> 
> I suppose that there could exist some hardware capable of applying
> margins post-CRTC? Such driver could support cloning CRTCs and still
> applying the connector margins correctly.

Yeah, theoretically. But in the KMS object model, is there the idea
that connectors do not do image manipulation, they can only convert the
signal type at most and send metadata?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document TV margin properties

[Ville Syrjälä]

On Tue, Feb 28, 2023 at 12:12:22PM +0200, Pekka Paalanen wrote:
> On Tue, 28 Feb 2023 09:53:47 +0000
> Simon Ser <contact@emersion.fr> wrote:
> 
> > On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > 
> > > can these be negative as well, to achieve overscan and not just
> > > underscan? Did I get overscan and underscan right... these are related
> > > to under/overscan, aren't they?
> > > 
> > > Hmm, no, I guess that doesn't make sense, there is no room in the
> > > signal to have negative margins, it would result in clipping the
> > > framebuffer while scaling up. So this can only be used to scale
> > > framebuffer down, add borders, and the TV then scales it back up
> > > again?  
> > 
> > Correct.
> > 
> > > Looks like neither my Intel nor AMD cards support these, I don't see
> > > the properties. More things to the list of KMS properties Weston needs
> > > to explicitly control. Oh, it seems vc4 exclusive for now.  
> > 
> > i915 does support it but for TV connectors only (i915/display/intel_tv.c).

I also have some patches to add it for HDMI and DP on i915.
But those are a bit stalled due to more important stuff
taking up my time.

Some fairly old version here:
https://github.com/vsyrjala/linux.git hdmi_margins_3

> > > Is this instead not scaling anything but simply sending metadata
> > > through the connector?  
> > 
> > No metadata is sent. This is purely equivalent to setting up CRTC_*
> > properties to scale the planes.

My wip HDMI/DP patches do set the AVI inforame "bars" based on
this. I think vc4 is already doing that as well.

> 
> Oh! That would be really good to mention in the doc. Maybe even prefer
> plane props over this? Or is this for analog TV, and plane props for
> digital TV?

Plane properties would be pointless for this. CRTC properties might
make sense. But what is more accurate kinda depends on the hardware
design.

> The above are the important comments. All below is just musings you can
> ignore if you wish.
> 
> > > Or are there underlying requirements that this connector property is
> > > actually affecting the CRTC, which means that it is fundamentally
> > > impossible to use multiple connectors with different values on the same
> > > CRTC? And drivers will reject any attempt, so there is no need to
> > > define what conflicting settings will do?  
> > 
> > I don't think any driver above supports cloning CRTCs for these
> > connector types. i915 sets clonable = false for these connectors.
> > vc4 picks the first connector's TV margins, always.
> 
> Sounds like i915 does it right, and vc4 does not, assuming vc4 does not
> prevent cloning.

For i915 my plan was to reject even HDMI+HDMI/VGA cloning (which
otherwise is allowed) when these properties are set. And that's
because they are connector properties and thus could disagree
between the cloned connectors. If they were CRTC properties
that would work fine.

The main reason i915 rejects cloning with many output types
is that generating the correct clock for each output becomes
difficult/impossible. And on HSW+ cloning is no longer
supported by the hardware at all.

> 
> > 
> > > IOW, does simply the existence of these properties on a connector
> > > guarantee that the connector must be the only one on a CRTC?  
> > 
> > I suppose that there could exist some hardware capable of applying
> > margins post-CRTC? Such driver could support cloning CRTCs and still
> > applying the connector margins correctly.
> 
> Yeah, theoretically. But in the KMS object model, is there the idea
> that connectors do not do image manipulation, they can only convert the
> signal type at most and send metadata?

No such rule.

Some hardware has scalers and all kinds of fancy stuff in the 
encoder essentially. Quite common in old TV encoder chips.
That's pretty much where these properties came from I think.

And eDP/LVDS/etc. also do scaling in the connector in the
current model since that's where the 'scaling mode' property
lives.

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: document TV margin properties

[Dave Stevenson]

Hi Pekka

On Tue, 28 Feb 2023 at 10:12, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Tue, 28 Feb 2023 09:53:47 +0000
> Simon Ser <contact@emersion.fr> wrote:
>
> > On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > > can these be negative as well, to achieve overscan and not just
> > > underscan? Did I get overscan and underscan right... these are related
> > > to under/overscan, aren't they?
> > >
> > > Hmm, no, I guess that doesn't make sense, there is no room in the
> > > signal to have negative margins, it would result in clipping the
> > > framebuffer while scaling up. So this can only be used to scale
> > > framebuffer down, add borders, and the TV then scales it back up
> > > again?
> >
> > Correct.
> >
> > > Looks like neither my Intel nor AMD cards support these, I don't see
> > > the properties. More things to the list of KMS properties Weston needs
> > > to explicitly control. Oh, it seems vc4 exclusive for now.

I've started a discussion with Simon with regard overscan handling [1].
There would be nothing stopping Weston ignoring the DRM properties if
Weston/userspace provides a way to reduce the destintation rectangle
on the display device. Using that would also be renderer agnostic.

[1] https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3597

> > i915 does support it but for TV connectors only (i915/display/intel_tv.c).
> > gud also supports it.
> >
> > > Where does this text appear in the HTML kernel docs? I tried to look at
> > > drm_connector.c but I cannot find the spot where this patch applies.
> >
> > Here:
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#analog-tv-specific-connector-properties
>
> Analog TV properties? So this does not apply to e.g. HDMI?
>
> I believe HDMI TVs do have the problems that margins could mitigate.

Correct. Particularly on TVs instead of monitors, it's not uncommon to
find the HDMI output gets overscanned.

> > > Is this actually a connector property? How does that work, should this
> > > not be a CRTC property?
> >
> > Yeah, it's a connector property for some reason.
> >
> > > Is this instead not scaling anything but simply sending metadata
> > > through the connector?
> >
> > No metadata is sent. This is purely equivalent to setting up CRTC_*
> > properties to scale the planes.
>
> Oh! That would be really good to mention in the doc. Maybe even prefer
> plane props over this? Or is this for analog TV, and plane props for
> digital TV?
>
>
> The above are the important comments. All below is just musings you can
> ignore if you wish.
>
> > > Or are there underlying requirements that this connector property is
> > > actually affecting the CRTC, which means that it is fundamentally
> > > impossible to use multiple connectors with different values on the same
> > > CRTC? And drivers will reject any attempt, so there is no need to
> > > define what conflicting settings will do?
> >
> > I don't think any driver above supports cloning CRTCs for these
> > connector types. i915 sets clonable = false for these connectors.
> > vc4 picks the first connector's TV margins, always.
>
> Sounds like i915 does it right, and vc4 does not, assuming vc4 does not
> prevent cloning.

The cloneable flag is in struct intel_encoder, not core.

vc4 doesn't support cloning of a single CRTC to multiple connectors at
all, and I believe sets up the possible_crtc bitmasks correctly to
denote that.

  Dave

> >
> > > IOW, does simply the existence of these properties on a connector
> > > guarantee that the connector must be the only one on a CRTC?
> >
> > I suppose that there could exist some hardware capable of applying
> > margins post-CRTC? Such driver could support cloning CRTCs and still
> > applying the connector margins correctly.
>
> Yeah, theoretically. But in the KMS object model, is there the idea
> that connectors do not do image manipulation, they can only convert the
> signal type at most and send metadata?
>
>
> Thanks,
> pq

Re: [PATCH] drm: document TV margin properties

[Ville Syrjälä]

On Tue, Feb 28, 2023 at 11:37:38AM +0000, Dave Stevenson wrote:
> Hi Pekka
> 
> On Tue, 28 Feb 2023 at 10:12, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Tue, 28 Feb 2023 09:53:47 +0000
> > Simon Ser <contact@emersion.fr> wrote:
> >
> > > On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >
> > > > can these be negative as well, to achieve overscan and not just
> > > > underscan? Did I get overscan and underscan right... these are related
> > > > to under/overscan, aren't they?
> > > >
> > > > Hmm, no, I guess that doesn't make sense, there is no room in the
> > > > signal to have negative margins, it would result in clipping the
> > > > framebuffer while scaling up. So this can only be used to scale
> > > > framebuffer down, add borders, and the TV then scales it back up
> > > > again?
> > >
> > > Correct.
> > >
> > > > Looks like neither my Intel nor AMD cards support these, I don't see
> > > > the properties. More things to the list of KMS properties Weston needs
> > > > to explicitly control. Oh, it seems vc4 exclusive for now.
> 
> I've started a discussion with Simon with regard overscan handling [1].
> There would be nothing stopping Weston ignoring the DRM properties if
> Weston/userspace provides a way to reduce the destintation rectangle
> on the display device. Using that would also be renderer agnostic.
> 
> [1] https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3597
> 
> > > i915 does support it but for TV connectors only (i915/display/intel_tv.c).
> > > gud also supports it.
> > >
> > > > Where does this text appear in the HTML kernel docs? I tried to look at
> > > > drm_connector.c but I cannot find the spot where this patch applies.
> > >
> > > Here:
> > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#analog-tv-specific-connector-properties
> >
> > Analog TV properties? So this does not apply to e.g. HDMI?
> >
> > I believe HDMI TVs do have the problems that margins could mitigate.
> 
> Correct. Particularly on TVs instead of monitors, it's not uncommon to
> find the HDMI output gets overscanned.
> 
> > > > Is this actually a connector property? How does that work, should this
> > > > not be a CRTC property?
> > >
> > > Yeah, it's a connector property for some reason.
> > >
> > > > Is this instead not scaling anything but simply sending metadata
> > > > through the connector?
> > >
> > > No metadata is sent. This is purely equivalent to setting up CRTC_*
> > > properties to scale the planes.
> >
> > Oh! That would be really good to mention in the doc. Maybe even prefer
> > plane props over this? Or is this for analog TV, and plane props for
> > digital TV?
> >
> >
> > The above are the important comments. All below is just musings you can
> > ignore if you wish.
> >
> > > > Or are there underlying requirements that this connector property is
> > > > actually affecting the CRTC, which means that it is fundamentally
> > > > impossible to use multiple connectors with different values on the same
> > > > CRTC? And drivers will reject any attempt, so there is no need to
> > > > define what conflicting settings will do?
> > >
> > > I don't think any driver above supports cloning CRTCs for these
> > > connector types. i915 sets clonable = false for these connectors.
> > > vc4 picks the first connector's TV margins, always.
> >
> > Sounds like i915 does it right, and vc4 does not, assuming vc4 does not
> > prevent cloning.
> 
> The cloneable flag is in struct intel_encoder, not core.

It gets converted into the core thing by intel_encoder_possible_clones().

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: document TV margin properties

[Dave Stevenson]

On Tue, 28 Feb 2023 at 11:45, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Feb 28, 2023 at 11:37:38AM +0000, Dave Stevenson wrote:
> > Hi Pekka
> >
> > On Tue, 28 Feb 2023 at 10:12, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >
> > > On Tue, 28 Feb 2023 09:53:47 +0000
> > > Simon Ser <contact@emersion.fr> wrote:
> > >
> > > > On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > >
> > > > > can these be negative as well, to achieve overscan and not just
> > > > > underscan? Did I get overscan and underscan right... these are related
> > > > > to under/overscan, aren't they?
> > > > >
> > > > > Hmm, no, I guess that doesn't make sense, there is no room in the
> > > > > signal to have negative margins, it would result in clipping the
> > > > > framebuffer while scaling up. So this can only be used to scale
> > > > > framebuffer down, add borders, and the TV then scales it back up
> > > > > again?
> > > >
> > > > Correct.
> > > >
> > > > > Looks like neither my Intel nor AMD cards support these, I don't see
> > > > > the properties. More things to the list of KMS properties Weston needs
> > > > > to explicitly control. Oh, it seems vc4 exclusive for now.
> >
> > I've started a discussion with Simon with regard overscan handling [1].
> > There would be nothing stopping Weston ignoring the DRM properties if
> > Weston/userspace provides a way to reduce the destintation rectangle
> > on the display device. Using that would also be renderer agnostic.
> >
> > [1] https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3597
> >
> > > > i915 does support it but for TV connectors only (i915/display/intel_tv.c).
> > > > gud also supports it.
> > > >
> > > > > Where does this text appear in the HTML kernel docs? I tried to look at
> > > > > drm_connector.c but I cannot find the spot where this patch applies.
> > > >
> > > > Here:
> > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#analog-tv-specific-connector-properties
> > >
> > > Analog TV properties? So this does not apply to e.g. HDMI?
> > >
> > > I believe HDMI TVs do have the problems that margins could mitigate.
> >
> > Correct. Particularly on TVs instead of monitors, it's not uncommon to
> > find the HDMI output gets overscanned.
> >
> > > > > Is this actually a connector property? How does that work, should this
> > > > > not be a CRTC property?
> > > >
> > > > Yeah, it's a connector property for some reason.
> > > >
> > > > > Is this instead not scaling anything but simply sending metadata
> > > > > through the connector?
> > > >
> > > > No metadata is sent. This is purely equivalent to setting up CRTC_*
> > > > properties to scale the planes.
> > >
> > > Oh! That would be really good to mention in the doc. Maybe even prefer
> > > plane props over this? Or is this for analog TV, and plane props for
> > > digital TV?
> > >
> > >
> > > The above are the important comments. All below is just musings you can
> > > ignore if you wish.
> > >
> > > > > Or are there underlying requirements that this connector property is
> > > > > actually affecting the CRTC, which means that it is fundamentally
> > > > > impossible to use multiple connectors with different values on the same
> > > > > CRTC? And drivers will reject any attempt, so there is no need to
> > > > > define what conflicting settings will do?
> > > >
> > > > I don't think any driver above supports cloning CRTCs for these
> > > > connector types. i915 sets clonable = false for these connectors.
> > > > vc4 picks the first connector's TV margins, always.
> > >
> > > Sounds like i915 does it right, and vc4 does not, assuming vc4 does not
> > > prevent cloning.
> >
> > The cloneable flag is in struct intel_encoder, not core.
>
> It gets converted into the core thing by intel_encoder_possible_clones().

Thanks Ville.
vc4 never adds additional possible_clones, therefore I believe it is
still doing the right thing.

  Dave

Re: [PATCH] drm: document TV margin properties

[Ville Syrjälä]

On Tue, Feb 28, 2023 at 11:52:54AM +0000, Dave Stevenson wrote:
> On Tue, 28 Feb 2023 at 11:45, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Tue, Feb 28, 2023 at 11:37:38AM +0000, Dave Stevenson wrote:
> > > Hi Pekka
> > >
> > > On Tue, 28 Feb 2023 at 10:12, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > >
> > > > On Tue, 28 Feb 2023 09:53:47 +0000
> > > > Simon Ser <contact@emersion.fr> wrote:
> > > >
> > > > > On Tuesday, February 28th, 2023 at 09:46, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > > >
> > > > > > can these be negative as well, to achieve overscan and not just
> > > > > > underscan? Did I get overscan and underscan right... these are related
> > > > > > to under/overscan, aren't they?
> > > > > >
> > > > > > Hmm, no, I guess that doesn't make sense, there is no room in the
> > > > > > signal to have negative margins, it would result in clipping the
> > > > > > framebuffer while scaling up. So this can only be used to scale
> > > > > > framebuffer down, add borders, and the TV then scales it back up
> > > > > > again?
> > > > >
> > > > > Correct.
> > > > >
> > > > > > Looks like neither my Intel nor AMD cards support these, I don't see
> > > > > > the properties. More things to the list of KMS properties Weston needs
> > > > > > to explicitly control. Oh, it seems vc4 exclusive for now.
> > >
> > > I've started a discussion with Simon with regard overscan handling [1].
> > > There would be nothing stopping Weston ignoring the DRM properties if
> > > Weston/userspace provides a way to reduce the destintation rectangle
> > > on the display device. Using that would also be renderer agnostic.
> > >
> > > [1] https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3597
> > >
> > > > > i915 does support it but for TV connectors only (i915/display/intel_tv.c).
> > > > > gud also supports it.
> > > > >
> > > > > > Where does this text appear in the HTML kernel docs? I tried to look at
> > > > > > drm_connector.c but I cannot find the spot where this patch applies.
> > > > >
> > > > > Here:
> > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#analog-tv-specific-connector-properties
> > > >
> > > > Analog TV properties? So this does not apply to e.g. HDMI?
> > > >
> > > > I believe HDMI TVs do have the problems that margins could mitigate.
> > >
> > > Correct. Particularly on TVs instead of monitors, it's not uncommon to
> > > find the HDMI output gets overscanned.
> > >
> > > > > > Is this actually a connector property? How does that work, should this
> > > > > > not be a CRTC property?
> > > > >
> > > > > Yeah, it's a connector property for some reason.
> > > > >
> > > > > > Is this instead not scaling anything but simply sending metadata
> > > > > > through the connector?
> > > > >
> > > > > No metadata is sent. This is purely equivalent to setting up CRTC_*
> > > > > properties to scale the planes.
> > > >
> > > > Oh! That would be really good to mention in the doc. Maybe even prefer
> > > > plane props over this? Or is this for analog TV, and plane props for
> > > > digital TV?
> > > >
> > > >
> > > > The above are the important comments. All below is just musings you can
> > > > ignore if you wish.
> > > >
> > > > > > Or are there underlying requirements that this connector property is
> > > > > > actually affecting the CRTC, which means that it is fundamentally
> > > > > > impossible to use multiple connectors with different values on the same
> > > > > > CRTC? And drivers will reject any attempt, so there is no need to
> > > > > > define what conflicting settings will do?
> > > > >
> > > > > I don't think any driver above supports cloning CRTCs for these
> > > > > connector types. i915 sets clonable = false for these connectors.
> > > > > vc4 picks the first connector's TV margins, always.
> > > >
> > > > Sounds like i915 does it right, and vc4 does not, assuming vc4 does not
> > > > prevent cloning.
> > >
> > > The cloneable flag is in struct intel_encoder, not core.
> >
> > It gets converted into the core thing by intel_encoder_possible_clones().
> 
> Thanks Ville.
> vc4 never adds additional possible_clones, therefore I believe it is
> still doing the right thing.

Yeah, should be fine.

These days drivers can just leave/set possible_clones=0 if they
don't support cloning, and fixup_encoder_possible_clones() will
fix it up later to follow the rule that the encoder itself must
be included in the bitmask.

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: document TV margin properties

[Simon Ser]

On Tuesday, February 28th, 2023 at 12:28, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> > > > Is this instead not scaling anything but simply sending metadata
> > > > through the connector?
> > > 
> > > No metadata is sent. This is purely equivalent to setting up CRTC_*
> > > properties to scale the planes.
> 
> My wip HDMI/DP patches do set the AVI inforame "bars" based on
> this. I think vc4 is already doing that as well.

Eh. Indeed it does, via drm_hdmi_avi_infoframe_bars().

Re: [PATCH] drm: document TV margin properties

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On Tue, 28 Feb 2023 13:28:48 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Tue, Feb 28, 2023 at 12:12:22PM +0200, Pekka Paalanen wrote:

> > Oh! That would be really good to mention in the doc. Maybe even prefer
> > plane props over this? Or is this for analog TV, and plane props for
> > digital TV?  
> 
> Plane properties would be pointless for this. CRTC properties might
> make sense. But what is more accurate kinda depends on the hardware
> design.

I meant the existing plane properties CRTC_X,Y,W,H. They can already
describe e.g. a primary plane that does not cover the whole CRTC area,
which is essentially the same as margins, scaling included even.

> Some hardware has scalers and all kinds of fancy stuff in the 
> encoder essentially. Quite common in old TV encoder chips.
> That's pretty much where these properties came from I think.
> 
> And eDP/LVDS/etc. also do scaling in the connector in the
> current model since that's where the 'scaling mode' property
> lives.

Ok, that makes sense.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: document TV margin properties

[Ville Syrjälä]

On Tue, Feb 28, 2023 at 02:24:23PM +0200, Pekka Paalanen wrote:
> On Tue, 28 Feb 2023 13:28:48 +0200
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Tue, Feb 28, 2023 at 12:12:22PM +0200, Pekka Paalanen wrote:
> 
> > > Oh! That would be really good to mention in the doc. Maybe even prefer
> > > plane props over this? Or is this for analog TV, and plane props for
> > > digital TV?  
> > 
> > Plane properties would be pointless for this. CRTC properties might
> > make sense. But what is more accurate kinda depends on the hardware
> > design.
> 
> I meant the existing plane properties CRTC_X,Y,W,H. They can already
> describe e.g. a primary plane that does not cover the whole CRTC area,
> which is essentially the same as margins, scaling included even.

Yeah that can work, assuming you have hardware that supports it.

-- 
Ville Syrjälä
Intel