[PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS

[PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS

[Jeffrey Hugo]

DEFINE_DRM_GEM_FOPS() references drm functions from other headers.  For
example drm_open() is defined in drm_file.h and drm_ioctl() is defined
in drm_ioctl.h.  Since drm_gem.h doesn't include these headers, it
relies on an implicit include from the .c file to have included these
required headers before DEFINE_DRM_GEM_FOPS() gets used.  Relying on
these implicit includes can cause build failures for new code that
doesn't know about these requirements, and can lead to future problems
if the headers ever get restructured as there will be a need to update
every downstream file that includes drm_gem.h.

Lets fix this explicitly including the required headers in drm_gem.h so
that code that includes drm_gem.h does not need to worry about these
implicit dependencies.

Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
---
 include/drm/drm_gem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 9d7c61a..1cbe3d8 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -37,6 +37,8 @@
 #include <linux/kref.h>
 #include <linux/dma-resv.h>
 
+#include <drm/drm_file.h>
+#include <drm/drm_ioctl.h>
 #include <drm/drm_vma_manager.h>
 
 struct iosys_map;
-- 
2.7.4

Re: [PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS

[Jani Nikula]

On Fri, 29 Apr 2022, Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
> DEFINE_DRM_GEM_FOPS() references drm functions from other headers.  For
> example drm_open() is defined in drm_file.h and drm_ioctl() is defined
> in drm_ioctl.h.  Since drm_gem.h doesn't include these headers, it
> relies on an implicit include from the .c file to have included these
> required headers before DEFINE_DRM_GEM_FOPS() gets used.  Relying on
> these implicit includes can cause build failures for new code that
> doesn't know about these requirements, and can lead to future problems
> if the headers ever get restructured as there will be a need to update
> every downstream file that includes drm_gem.h.
>
> Lets fix this explicitly including the required headers in drm_gem.h so
> that code that includes drm_gem.h does not need to worry about these
> implicit dependencies.

In the general case, I tend to agree, but in this specific instance I
think I'd err on the side of fewer includes. I think the more likely
outcome here is accumulating implicit dependencies on symbols from
drm_file.h and drm_ioctl.h by including drm_gem.h only!

I do think headers need to be self-contained, and we actually enforce
this in i915 (see HDRTEST in drivers/gpu/drm/i915/Makefile), but not to
the point of macro expansions.

BR,
Jani.



>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> ---
>  include/drm/drm_gem.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 9d7c61a..1cbe3d8 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -37,6 +37,8 @@
>  #include <linux/kref.h>
>  #include <linux/dma-resv.h>
>  
> +#include <drm/drm_file.h>
> +#include <drm/drm_ioctl.h>
>  #include <drm/drm_vma_manager.h>
>  
>  struct iosys_map;

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS

[Daniel Vetter]

On Mon, May 02, 2022 at 06:41:39PM +0300, Jani Nikula wrote:
> On Fri, 29 Apr 2022, Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
> > DEFINE_DRM_GEM_FOPS() references drm functions from other headers.  For
> > example drm_open() is defined in drm_file.h and drm_ioctl() is defined
> > in drm_ioctl.h.  Since drm_gem.h doesn't include these headers, it
> > relies on an implicit include from the .c file to have included these
> > required headers before DEFINE_DRM_GEM_FOPS() gets used.  Relying on
> > these implicit includes can cause build failures for new code that
> > doesn't know about these requirements, and can lead to future problems
> > if the headers ever get restructured as there will be a need to update
> > every downstream file that includes drm_gem.h.
> >
> > Lets fix this explicitly including the required headers in drm_gem.h so
> > that code that includes drm_gem.h does not need to worry about these
> > implicit dependencies.
> 
> In the general case, I tend to agree, but in this specific instance I
> think I'd err on the side of fewer includes. I think the more likely
> outcome here is accumulating implicit dependencies on symbols from
> drm_file.h and drm_ioctl.h by including drm_gem.h only!
> 
> I do think headers need to be self-contained, and we actually enforce
> this in i915 (see HDRTEST in drivers/gpu/drm/i915/Makefile), but not to
> the point of macro expansions.

Yeah we abuse macros in a bunch of places to untangle header dependencies,
so then going back and pulling in all the headers back in feels a bit
silly and defeats the point.

iow, I concur.
-Daniel

> 
> BR,
> Jani.
> 
> 
> 
> >
> > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > ---
> >  include/drm/drm_gem.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 9d7c61a..1cbe3d8 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -37,6 +37,8 @@
> >  #include <linux/kref.h>
> >  #include <linux/dma-resv.h>
> >  
> > +#include <drm/drm_file.h>
> > +#include <drm/drm_ioctl.h>
> >  #include <drm/drm_vma_manager.h>
> >  
> >  struct iosys_map;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

Re: [PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS

[Jeffrey Hugo]

On 5/4/2022 3:38 AM, Daniel Vetter wrote:
> On Mon, May 02, 2022 at 06:41:39PM +0300, Jani Nikula wrote:
>> On Fri, 29 Apr 2022, Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>>> DEFINE_DRM_GEM_FOPS() references drm functions from other headers.  For
>>> example drm_open() is defined in drm_file.h and drm_ioctl() is defined
>>> in drm_ioctl.h.  Since drm_gem.h doesn't include these headers, it
>>> relies on an implicit include from the .c file to have included these
>>> required headers before DEFINE_DRM_GEM_FOPS() gets used.  Relying on
>>> these implicit includes can cause build failures for new code that
>>> doesn't know about these requirements, and can lead to future problems
>>> if the headers ever get restructured as there will be a need to update
>>> every downstream file that includes drm_gem.h.
>>>
>>> Lets fix this explicitly including the required headers in drm_gem.h so
>>> that code that includes drm_gem.h does not need to worry about these
>>> implicit dependencies.
>>
>> In the general case, I tend to agree, but in this specific instance I
>> think I'd err on the side of fewer includes. I think the more likely
>> outcome here is accumulating implicit dependencies on symbols from
>> drm_file.h and drm_ioctl.h by including drm_gem.h only!
>>
>> I do think headers need to be self-contained, and we actually enforce
>> this in i915 (see HDRTEST in drivers/gpu/drm/i915/Makefile), but not to
>> the point of macro expansions.
> 
> Yeah we abuse macros in a bunch of places to untangle header dependencies,
> so then going back and pulling in all the headers back in feels a bit
> silly and defeats the point.

Fair enough.  I'll consider this NAK'd

I've been pondering alternate solutions, but haven't come up with any. 
I guess, for now, the status quo will remain.

> 
> iow, I concur.
> -Daniel
> 
>>
>> BR,
>> Jani.
>>
>>
>>
>>>
>>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>> ---
>>>   include/drm/drm_gem.h | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>> index 9d7c61a..1cbe3d8 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -37,6 +37,8 @@
>>>   #include <linux/kref.h>
>>>   #include <linux/dma-resv.h>
>>>   
>>> +#include <drm/drm_file.h>
>>> +#include <drm/drm_ioctl.h>
>>>   #include <drm/drm_vma_manager.h>
>>>   
>>>   struct iosys_map;
>>
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center
> 

Re: [PATCH] drm: drm_gem.h: Add explicit includes for DEFINE_DRM_GEM_FOPS

[Daniel Vetter]

On Wed, May 04, 2022 at 07:58:38AM -0600, Jeffrey Hugo wrote:
> On 5/4/2022 3:38 AM, Daniel Vetter wrote:
> > On Mon, May 02, 2022 at 06:41:39PM +0300, Jani Nikula wrote:
> > > On Fri, 29 Apr 2022, Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
> > > > DEFINE_DRM_GEM_FOPS() references drm functions from other headers.  For
> > > > example drm_open() is defined in drm_file.h and drm_ioctl() is defined
> > > > in drm_ioctl.h.  Since drm_gem.h doesn't include these headers, it
> > > > relies on an implicit include from the .c file to have included these
> > > > required headers before DEFINE_DRM_GEM_FOPS() gets used.  Relying on
> > > > these implicit includes can cause build failures for new code that
> > > > doesn't know about these requirements, and can lead to future problems
> > > > if the headers ever get restructured as there will be a need to update
> > > > every downstream file that includes drm_gem.h.
> > > > 
> > > > Lets fix this explicitly including the required headers in drm_gem.h so
> > > > that code that includes drm_gem.h does not need to worry about these
> > > > implicit dependencies.
> > > 
> > > In the general case, I tend to agree, but in this specific instance I
> > > think I'd err on the side of fewer includes. I think the more likely
> > > outcome here is accumulating implicit dependencies on symbols from
> > > drm_file.h and drm_ioctl.h by including drm_gem.h only!
> > > 
> > > I do think headers need to be self-contained, and we actually enforce
> > > this in i915 (see HDRTEST in drivers/gpu/drm/i915/Makefile), but not to
> > > the point of macro expansions.
> > 
> > Yeah we abuse macros in a bunch of places to untangle header dependencies,
> > so then going back and pulling in all the headers back in feels a bit
> > silly and defeats the point.
> 
> Fair enough.  I'll consider this NAK'd
> 
> I've been pondering alternate solutions, but haven't come up with any. I
> guess, for now, the status quo will remain.

I think a standalone header for gem fops or so might be an option. We also
have some macro iirc for standard stuff in drm_driver (or at least had in
the past, maybe that's cleaned up now better).

That would allow drivers to include this monster header that pulls in a
lot of things in the one place they define their file ops, and nowhere
else. I think that would cover everything we'd want to achieve? But would
be a bit of churn to roll out everywhere.
-Daniel


> 
> > 
> > iow, I concur.
> > -Daniel
> > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > 
> > > > 
> > > > Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > > > ---
> > > >   include/drm/drm_gem.h | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > > index 9d7c61a..1cbe3d8 100644
> > > > --- a/include/drm/drm_gem.h
> > > > +++ b/include/drm/drm_gem.h
> > > > @@ -37,6 +37,8 @@
> > > >   #include <linux/kref.h>
> > > >   #include <linux/dma-resv.h>
> > > > +#include <drm/drm_file.h>
> > > > +#include <drm/drm_ioctl.h>
> > > >   #include <drm/drm_vma_manager.h>
> > > >   struct iosys_map;
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Graphics Center
> > 
> 

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