[PATCH 1/2] drm/doc: Allow new UAPI to be used once it's in the driver's -next.

[PATCH 1/2] drm/doc: Allow new UAPI to be used once it's in the driver's -next.

[Eric Anholt]

I was trying to figure out if it was permissible to merge the Mesa
side of V3D's CSD support yet while it's in drm-misc-next but not
drm-next, and developers on #dri-devel IRC had differing opinions of
what the requirement was.  Propose a clarification here to see if Dave
Airlie agrees.

Signed-off-by: Eric Anholt <eric@anholt.net>
---

Personally, I thought the rule was "has to be in drm-next", but
assuming our review processes aren't totally broken, this should be
enough.

 Documentation/gpu/drm-uapi.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index c9fd23efd957..8e5545dfbf82 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -92,8 +92,9 @@ leads to a few additional requirements:
   requirements by doing a quick fork.
 
 - The kernel patch can only be merged after all the above requirements are met,
-  but it **must** be merged **before** the userspace patches land. uAPI always flows
-  from the kernel, doing things the other way round risks divergence of the uAPI
+  but it **must** be merged to the driver's -next tree (as documented in
+  MAINTAINERS) **before** the userspace patches land. uAPI always flows from
+  the kernel, doing things the other way round risks divergence of the uAPI
   definitions and header files.
 
 These are fairly steep requirements, but have grown out from years of shared
-- 
2.20.1

[PATCH 2/2] drm/doc: Document expectation that userspace review looks at kernel uAPI.

[Eric Anholt]

The point of this review process is that userspace using the new uAPI
can actually live with the uAPI being provided, and it's hard to know
that without having actually looked into a kernel patch yourself.

Signed-off-by: Eric Anholt <eric@anholt.net>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/gpu/drm-uapi.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 8e5545dfbf82..298424b98d99 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -85,7 +85,9 @@ leads to a few additional requirements:
 - The userspace side must be fully reviewed and tested to the standards of that
   userspace project. For e.g. mesa this means piglit testcases and review on the
   mailing list. This is again to ensure that the new interface actually gets the
-  job done.
+  job done.  The userspace-side reviewer should also provide at least an
+  Acked-by on the kernel uAPI patch indicating that they've looked at how the
+  kernel side is implementing the new feature being used.
 
 - The userspace patches must be against the canonical upstream, not some vendor
   fork. This is to make sure that no one cheats on the review and testing
-- 
2.20.1

Re: [PATCH 2/2] drm/doc: Document expectation that userspace review looks at kernel uAPI.

[Daniel Vetter]

On Wed, Apr 24, 2019 at 11:56:17AM -0700, Eric Anholt wrote:
> The point of this review process is that userspace using the new uAPI
> can actually live with the uAPI being provided, and it's hard to know
> that without having actually looked into a kernel patch yourself.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/gpu/drm-uapi.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 8e5545dfbf82..298424b98d99 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -85,7 +85,9 @@ leads to a few additional requirements:
>  - The userspace side must be fully reviewed and tested to the standards of that
>    userspace project. For e.g. mesa this means piglit testcases and review on the
>    mailing list. This is again to ensure that the new interface actually gets the
> -  job done.
> +  job done.  The userspace-side reviewer should also provide at least an
> +  Acked-by on the kernel uAPI patch indicating that they've looked at how the
> +  kernel side is implementing the new feature being used.

Answers a question that just recently came up on merging new kms
properties.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  - The userspace patches must be against the canonical upstream, not some vendor
>    fork. This is to make sure that no one cheats on the review and testing
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/doc: Document expectation that userspace review looks at kernel uAPI.

[Daniel Vetter]

On Wed, Apr 24, 2019 at 11:56:17AM -0700, Eric Anholt wrote:
> The point of this review process is that userspace using the new uAPI
> can actually live with the uAPI being provided, and it's hard to know
> that without having actually looked into a kernel patch yourself.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/gpu/drm-uapi.rst | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 8e5545dfbf82..298424b98d99 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -85,7 +85,9 @@ leads to a few additional requirements:
>  - The userspace side must be fully reviewed and tested to the standards of that
>    userspace project. For e.g. mesa this means piglit testcases and review on the
>    mailing list. This is again to ensure that the new interface actually gets the
> -  job done.
> +  job done.  The userspace-side reviewer should also provide at least an
> +  Acked-by on the kernel uAPI patch indicating that they've looked at how the
> +  kernel side is implementing the new feature being used.

Answers a question that just recently came up on merging new kms
properties.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  - The userspace patches must be against the canonical upstream, not some vendor
>    fork. This is to make sure that no one cheats on the review and testing
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/doc: Document expectation that userspace review looks at kernel uAPI.

[Pekka Paalanen]

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

On Wed, 24 Apr 2019 21:36:36 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Apr 24, 2019 at 11:56:17AM -0700, Eric Anholt wrote:
> > The point of this review process is that userspace using the new uAPI
> > can actually live with the uAPI being provided, and it's hard to know
> > that without having actually looked into a kernel patch yourself.
> > 
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/gpu/drm-uapi.rst | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 8e5545dfbf82..298424b98d99 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -85,7 +85,9 @@ leads to a few additional requirements:
> >  - The userspace side must be fully reviewed and tested to the standards of that
> >    userspace project. For e.g. mesa this means piglit testcases and review on the
> >    mailing list. This is again to ensure that the new interface actually gets the
> > -  job done.
> > +  job done.  The userspace-side reviewer should also provide at least an
> > +  Acked-by on the kernel uAPI patch indicating that they've looked at how the
> > +  kernel side is implementing the new feature being used.  
> 
> Answers a question that just recently came up on merging new kms
> properties.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Hi,

for the record, I personally will not be able to provide such Acked-by
tag according to kernel review rules, because I am completely unfamiliar
with kernel DRM internals and cannot review kernel code at all. This
might make people expecting Weston to prove their uAPI disappointed,
since there are very few Weston reviewers available.

If you meant something else, please word it to that you actually meant.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm/doc: Document expectation that userspace review looks at kernel uAPI.

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 1959 bytes --]

On Wed, 24 Apr 2019 21:36:36 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Apr 24, 2019 at 11:56:17AM -0700, Eric Anholt wrote:
> > The point of this review process is that userspace using the new uAPI
> > can actually live with the uAPI being provided, and it's hard to know
> > that without having actually looked into a kernel patch yourself.
> > 
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/gpu/drm-uapi.rst | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 8e5545dfbf82..298424b98d99 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -85,7 +85,9 @@ leads to a few additional requirements:
> >  - The userspace side must be fully reviewed and tested to the standards of that
> >    userspace project. For e.g. mesa this means piglit testcases and review on the
> >    mailing list. This is again to ensure that the new interface actually gets the
> > -  job done.
> > +  job done.  The userspace-side reviewer should also provide at least an
> > +  Acked-by on the kernel uAPI patch indicating that they've looked at how the
> > +  kernel side is implementing the new feature being used.  
> 
> Answers a question that just recently came up on merging new kms
> properties.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Hi,

for the record, I personally will not be able to provide such Acked-by
tag according to kernel review rules, because I am completely unfamiliar
with kernel DRM internals and cannot review kernel code at all. This
might make people expecting Weston to prove their uAPI disappointed,
since there are very few Weston reviewers available.

If you meant something else, please word it to that you actually meant.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/doc: Document expectation that userspace review looks at kernel uAPI.

[Daniel Vetter]

On Tue, May 21, 2019 at 10:47:34AM +0300, Pekka Paalanen wrote:
> On Wed, 24 Apr 2019 21:36:36 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Wed, Apr 24, 2019 at 11:56:17AM -0700, Eric Anholt wrote:
> > > The point of this review process is that userspace using the new uAPI
> > > can actually live with the uAPI being provided, and it's hard to know
> > > that without having actually looked into a kernel patch yourself.
> > > 
> > > Signed-off-by: Eric Anholt <eric@anholt.net>
> > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  Documentation/gpu/drm-uapi.rst | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > index 8e5545dfbf82..298424b98d99 100644
> > > --- a/Documentation/gpu/drm-uapi.rst
> > > +++ b/Documentation/gpu/drm-uapi.rst
> > > @@ -85,7 +85,9 @@ leads to a few additional requirements:
> > >  - The userspace side must be fully reviewed and tested to the standards of that
> > >    userspace project. For e.g. mesa this means piglit testcases and review on the
> > >    mailing list. This is again to ensure that the new interface actually gets the
> > > -  job done.
> > > +  job done.  The userspace-side reviewer should also provide at least an
> > > +  Acked-by on the kernel uAPI patch indicating that they've looked at how the
> > > +  kernel side is implementing the new feature being used.  
> > 
> > Answers a question that just recently came up on merging new kms
> > properties.
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Hi,
> 
> for the record, I personally will not be able to provide such Acked-by
> tag according to kernel review rules, because I am completely unfamiliar
> with kernel DRM internals and cannot review kernel code at all. This
> might make people expecting Weston to prove their uAPI disappointed,
> since there are very few Weston reviewers available.
> 
> If you meant something else, please word it to that you actually meant.

Hm right, that wording is putting a bit too high a bar. We want the
userspace view point here, not force userspace people to review kernel
code. I'll try to clarify this a bit better.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm/doc: Allow new UAPI to be used once it's in the driver's -next.

[Daniel Vetter]

On Wed, Apr 24, 2019 at 11:56:16AM -0700, Eric Anholt wrote:
> I was trying to figure out if it was permissible to merge the Mesa
> side of V3D's CSD support yet while it's in drm-misc-next but not
> drm-next, and developers on #dri-devel IRC had differing opinions of
> what the requirement was.  Propose a clarification here to see if Dave
> Airlie agrees.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> 
> Personally, I thought the rule was "has to be in drm-next", but
> assuming our review processes aren't totally broken, this should be
> enough.

Yeah if you end up with a revert on your hands the process failed much
harder and you get to keep the pieces no matter what. Not sure we should
clarify whether you need a stable sha1 or not (helps with cross
referencing uapi header updates), but imo good as is. And matches what
I've been doing/recommending past few years.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
>  Documentation/gpu/drm-uapi.rst | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index c9fd23efd957..8e5545dfbf82 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -92,8 +92,9 @@ leads to a few additional requirements:
>    requirements by doing a quick fork.
>  
>  - The kernel patch can only be merged after all the above requirements are met,
> -  but it **must** be merged **before** the userspace patches land. uAPI always flows
> -  from the kernel, doing things the other way round risks divergence of the uAPI
> +  but it **must** be merged to the driver's -next tree (as documented in
> +  MAINTAINERS) **before** the userspace patches land. uAPI always flows from
> +  the kernel, doing things the other way round risks divergence of the uAPI
>    definitions and header files.
>  
>  These are fairly steep requirements, but have grown out from years of shared
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm/doc: Allow new UAPI to be used once it's in the driver's -next.

[Dave Airlie]

On Thu, 25 Apr 2019 at 05:35, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Apr 24, 2019 at 11:56:16AM -0700, Eric Anholt wrote:
> > I was trying to figure out if it was permissible to merge the Mesa
> > side of V3D's CSD support yet while it's in drm-misc-next but not
> > drm-next, and developers on #dri-devel IRC had differing opinions of
> > what the requirement was.  Propose a clarification here to see if Dave
> > Airlie agrees.
> >
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > ---
> >
> > Personally, I thought the rule was "has to be in drm-next", but
> > assuming our review processes aren't totally broken, this should be
> > enough.
>
> Yeah if you end up with a revert on your hands the process failed much
> harder and you get to keep the pieces no matter what. Not sure we should
> clarify whether you need a stable sha1 or not (helps with cross
> referencing uapi header updates), but imo good as is. And matches what
> I've been doing/recommending past few years.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> >
> >  Documentation/gpu/drm-uapi.rst | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index c9fd23efd957..8e5545dfbf82 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -92,8 +92,9 @@ leads to a few additional requirements:
> >    requirements by doing a quick fork.
> >
> >  - The kernel patch can only be merged after all the above requirements are met,
> > -  but it **must** be merged **before** the userspace patches land. uAPI always flows
> > -  from the kernel, doing things the other way round risks divergence of the uAPI
> > +  but it **must** be merged to the driver's -next tree (as documented in
> > +  MAINTAINERS) **before** the userspace patches land. uAPI always flows from
> > +  the kernel, doing things the other way round risks divergence of the uAPI
> >    definitions and header files.

I'd rather restrict this to drm-next and drm-misc-next, I frankly
don't trust driver trees here to have the review practices in place.

I trust drm-misc-next to have at least had someone unrelated look over
the new api.

Dave.

[PATCH 1/2 v2] drm/doc: Allow new UAPI to be used once it's in drm-next/drm-misc-next.

[Eric Anholt]

I was trying to figure out if it was permissible to merge the Mesa
side of V3D's CSD support yet while it's in drm-misc-next but not
drm-next, and developers on #dri-devel IRC had differing opinions of
what the requirement was.

v2: Restrict to just drm-next or drm-misc-next on airlied's request.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 Documentation/gpu/drm-uapi.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index c9fd23efd957..b7a96dc02d21 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -92,9 +92,9 @@ leads to a few additional requirements:
   requirements by doing a quick fork.
 
 - The kernel patch can only be merged after all the above requirements are met,
-  but it **must** be merged **before** the userspace patches land. uAPI always flows
-  from the kernel, doing things the other way round risks divergence of the uAPI
-  definitions and header files.
+  but it **must** be merged to either drm-next or drm-misc-next **before** the
+  userspace patches land. uAPI always flows from the kernel, doing things the
+  other way round risks divergence of the uAPI definitions and header files.
 
 These are fairly steep requirements, but have grown out from years of shared
 pain and experience with uAPI added hastily, and almost always regretted about
-- 
2.20.1

Re: [PATCH 1/2 v2] drm/doc: Allow new UAPI to be used once it's in drm-next/drm-misc-next.

[Daniel Vetter]

On Wed, Apr 24, 2019 at 03:06:38PM -0700, Eric Anholt wrote:
> I was trying to figure out if it was permissible to merge the Mesa
> side of V3D's CSD support yet while it's in drm-misc-next but not
> drm-next, and developers on #dri-devel IRC had differing opinions of
> what the requirement was.
> 
> v2: Restrict to just drm-next or drm-misc-next on airlied's request.

Personally I think that's a bit too strict (if people want to screw up,
they will be able to anyway). But since I'm all for clearer rules where
possible, this has my support too.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  Documentation/gpu/drm-uapi.rst | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index c9fd23efd957..b7a96dc02d21 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -92,9 +92,9 @@ leads to a few additional requirements:
>    requirements by doing a quick fork.
>  
>  - The kernel patch can only be merged after all the above requirements are met,
> -  but it **must** be merged **before** the userspace patches land. uAPI always flows
> -  from the kernel, doing things the other way round risks divergence of the uAPI
> -  definitions and header files.
> +  but it **must** be merged to either drm-next or drm-misc-next **before** the
> +  userspace patches land. uAPI always flows from the kernel, doing things the
> +  other way round risks divergence of the uAPI definitions and header files.
>  
>  These are fairly steep requirements, but have grown out from years of shared
>  pain and experience with uAPI added hastily, and almost always regretted about
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2 v2] drm/doc: Allow new UAPI to be used once it's in drm-next/drm-misc-next.

[Eric Anholt]

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

Daniel Vetter <daniel@ffwll.ch> writes:

> On Wed, Apr 24, 2019 at 03:06:38PM -0700, Eric Anholt wrote:
>> I was trying to figure out if it was permissible to merge the Mesa
>> side of V3D's CSD support yet while it's in drm-misc-next but not
>> drm-next, and developers on #dri-devel IRC had differing opinions of
>> what the requirement was.
>> 
>> v2: Restrict to just drm-next or drm-misc-next on airlied's request.
>
> Personally I think that's a bit too strict (if people want to screw up,
> they will be able to anyway). But since I'm all for clearer rules where
> possible, this has my support too.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed to drm-misc-next now.

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