| Message ID | 20240719122051.1507927-1-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/panic: Add missing static inline to drm_panic_is_enabled() | expand |
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@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; } > #endif shouldn't this whole chunk be part of <drm/drm_panic.h> ? other exported drm_panic functions have forward declarations there > > #endif /* __DRM_CRTC_INTERNAL_H__ */ > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c > index 9f1a3568e62d..072752b658f0 100644 > --- a/drivers/gpu/drm/drm_panic.c > +++ b/drivers/gpu/drm/drm_panic.c > @@ -27,6 +27,8 @@ > #include <drm/drm_plane.h> > #include <drm/drm_print.h> > > +#include "drm_crtc_internal.h" then you will not need this include here > + > MODULE_AUTHOR("Jocelyn Falempe"); > MODULE_DESCRIPTION("DRM panic handler"); > MODULE_LICENSE("GPL");
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@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; } >> #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.
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@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 ? >>> #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 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@intel.com>
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@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@intel.com> > Thanks a lot, I will push it asap to unblock the build. Best regards,
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; } #endif #endif /* __DRM_CRTC_INTERNAL_H__ */ diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 9f1a3568e62d..072752b658f0 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -27,6 +27,8 @@ #include <drm/drm_plane.h> #include <drm/drm_print.h> +#include "drm_crtc_internal.h" + MODULE_AUTHOR("Jocelyn Falempe"); MODULE_DESCRIPTION("DRM panic handler"); MODULE_LICENSE("GPL");
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@redhat.com> --- drivers/gpu/drm/drm_crtc_internal.h | 2 +- drivers/gpu/drm/drm_panic.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)