Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6dde7bb4-8931-ccdb-2677-930a1c6d6dab@quicinc.com>
Date:   Wed, 4 May 2022 07:58:38 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH] drm: drm_gem.h: Add explicit includes for
 DEFINE_DRM_GEM_FOPS
Content-Language: en-US
To:     Jani Nikula <jani.nikula@linux.intel.com>,
        <maarten.lankhorst@linux.intel.com>, <mripard@kernel.org>,
        <tzimmermann@suse.de>, <airlied@linux.ie>,
        <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>
References: <1651262112-29664-1-git-send-email-quic_jhugo@quicinc.com>
 <87y1zkq6vg.fsf@intel.com> <YnJJq6UdCVIWcH3G@phenom.ffwll.local>
From:   Jeffrey Hugo <quic_jhugo@quicinc.com>
In-Reply-To: <YnJJq6UdCVIWcH3G@phenom.ffwll.local>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>