Re: [PATCH] drm: Document requirements for driver-specific KMS props in new drivers

[Maxime Ripard <mripard@kernel.org>]

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

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@redhat.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

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