[PATCH v2] drm/panic: Add missing static inline to drm_panic_is_enabled()

[Jocelyn Falempe]

On 19/07/2024 20:26, Michal Wajdeczko wrote:
> 
> 
> On 19.07.2024 17:37, Jocelyn Falempe wrote:
>>
>>
>> On 19/07/2024 17:28, Michal Wajdeczko wrote:
>>>
>>>
>>> On 19.07.2024 14:20, Jocelyn Falempe wrote:
>>>> This breaks build if DRM_PANIC is not enabled.
>>>> Also add the missing include drm_crtc_internal.h in drm_panic.c
>>>>
>>>> Fixes: 9f774c42a908 ("drm/panic: Add drm_panic_is_enabled()")
>>>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_crtc_internal.h | 2 +-
>>>>    drivers/gpu/drm/drm_panic.c         | 2 ++
>>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_crtc_internal.h
>>>> b/drivers/gpu/drm/drm_crtc_internal.h
>>>> index c10de39cbe83..bbac5350774e 100644
>>>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>>>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>>>> @@ -321,7 +321,7 @@ drm_edid_load_firmware(struct drm_connector
>>>> *connector)
>>>>    #ifdef CONFIG_DRM_PANIC
>>>>    bool drm_panic_is_enabled(struct drm_device *dev);
>>>>    #else
>>>> -bool drm_panic_is_enabled(struct drm_device *dev) {return false; }
>>>> +static inline bool drm_panic_is_enabled(struct drm_device *dev)
>>>> {return false; }
> 
> btw, missing space after opening {
> 
> did you run checkpatch.pl ?

Yes, but it looks like he doesn't check this case.
I got the same:
./scripts/checkpatch.pl -g HEAD
total: 0 errors, 0 warnings, 16 lines checked

with or without space here.
> 
>>>>    #endif
>>>
>>> shouldn't this whole chunk be part of <drm/drm_panic.h> ?
>>> other exported drm_panic functions have forward declarations there
>>>
>> drm/drm_panic.h is for GPU drivers implementing drm_panic.
>> And they don't need this function.
>>
>> This function is only for drm internal (it's called from drm_fb_helper.c).
>>
>> Sima suggested this change in my previous series:
>> https://lists.freedesktop.org/archives/dri-devel/2024-July/462375.html
>>
>> I will move drm_panic_[un]register() prototypes there for the same
>> reason in follow-up patch.
>>
> 
> hmm, but then IMO there is a little (?) problem with the layering, as
> drm_panic.h no longer matches drm_panic.c and forward decls for
> functions from drm_panic.c are in some random internal header
> 

The problem it want to solve is to avoid external kernel modules to rely 
on internal functions.
Maybe for something cleaner, I could create a drm_panic_internal.h, but 
I find it a bit too much.

I will submit the patch to move drm_panic_[un]register() next week, and 
we will discuss that here.

> but that's probably topic for another discussion, and now we need to fix
> the drm-tip ASAP, so with above nit fixed:
> 
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> 

Thanks a lot, I will push it asap to unblock the build.

Best regards,

-- 

Jocelyn