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

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

[Sebastian Wick]

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.
+
 Property Types and Blob Property Support
 ----------------------------------------
 
-- 
2.43.0

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

[Maxime Ripard]

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

Hi,

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.

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.

Maxime

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

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

[Sebastian Wick]

On Wed, Mar 06, 2024 at 03:14:15PM +0100, Maxime Ripard wrote:
> Hi,
> 
> 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.

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

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

> Maxime

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

[Maxime Ripard]

[-- 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 --]