On Mon, May 02, 2022 at 06:41:39PM +0300, Jani Nikula wrote: > On Fri, 29 Apr 2022, Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > --- > > 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
