Date: Mon, 11 Mar 2024 13:10:24 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Sebastian Wick <sebastian.wick@...hat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Jonathan Corbet <corbet@....net>, 
	dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: Document requirements for driver-specific KMS props
 in new drivers

Hi Sebastian,

On Wed, Mar 06, 2024 at 05:50:09PM +0100, Sebastian Wick wrote:
> On Wed, Mar 06, 2024 at 03:14:15PM +0100, Maxime Ripard wrote:
> > On Thu, Feb 29, 2024 at 09:28:31PM +0100, Sebastian Wick wrote:
> > > When extending support for a driver-specific KMS property to additional
> > > drivers, we should apply all the requirements for new properties and
> > > make sure the semantics are the same and documented.
> > > 
> > > Signed-off-by: Sebastian Wick <sebastian.wick@...hat.com>
> > > ---
> > >  Documentation/gpu/drm-kms.rst | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > > index 13d3627d8bc0..afa10a28035f 100644
> > > --- a/Documentation/gpu/drm-kms.rst
> > > +++ b/Documentation/gpu/drm-kms.rst
> > > @@ -496,6 +496,11 @@ addition to the one mentioned above:
> > >  
> > >  * An IGT test must be submitted where reasonable.
> > >  
> > > +For historical reasons, non-standard, driver-specific properties exist. If a KMS
> > > +driver wants to add support for one of those properties, the requirements for
> > > +new properties apply where possible. Additionally, the documented behavior must
> > > +match the de facto semantics of the existing property to ensure compatibility.
> > > +
> > 
> > I'm conflicted about this one, really.
> > 
> > On one hand, yeah, it's definitely reasonable and something we would
> > want on the long run.
> > 
> > But on the other hand, a driver getting its technical debt worked on for
> > free by anyone but its developpers doesn't seem fair to me.
> 
> Most of the work would have to be done for a new property as well. The
> only additional work is then documenting the de-facto semantics and
> moving the existing driver-specific code to the core.

Sure, I think part of the problem with the Broadcast RGB property was
also that it hasn't been reviewed by anyone when it was submitted for
vc4.

> Would it help if we spell out that the developers of the driver-specific
> property shall help with both tasks?

Yes, that's a good idea, and we should probably require that the
maintainers of the driver that first added that property ack the
"standardization" work too.

> > Also, I assume this is in reaction to the discussion we had on the
> > Broadcast RGB property. We used in vc4 precisely because there was some
> > userspace code to deal with it and we could just reuse it, and it was
> > documented. So the requirements were met already.
> 
> It was not in drm core and the behavior was not documented properly at
> least.
> 
> Either way, with Broadcast RGB we were already in a bad situation
> because it was implemented by 2 drivers independently. This is what I
> want to avoid in the first place. The cleanup afterwards (thank you!)
> just exposed the problem.

Actually, I just found out there's three, the third one not being
compatible at all with the other two:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/gma500/cdv_device.c#L471

I'll send a patch to figure that one out once the rest will be merged.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists