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