Re: [PATCH] drm: document TV margin properties

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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