| Message ID | 20240229202833.198691-1-sebastian.wick@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Document requirements for driver-specific KMS props in new drivers | expand |
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
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
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
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 ----------------------------------------
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(+)