Re: [PATCH v3 19/37] drm/i915: stop using kernel-doc markups for something else
On Mon, Sep 12, 2022 at 06:47:56PM +0200, Mauro Carvalho Chehab wrote:
> Hi Matt,
>
> Em Mon, 12 Sep 2022 08:09:57 -0700
> Matt Roper escreveu:
>
> > > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> >
> > Several of the comments in this file do appear to be kerneldoc (in fact
> > kerneldoc that was specifically requested during code review at
> > https://patchwork.freedesktop.org/patch/448456/#comment_804252) and this
> > file is included from Documentation/gpu/i915.rst, so I think some of
> > these changes might be moving in the wrong direction. Should we instead
> > focus on fixing up the comments that aren't quite formatted properly?
>
> Those *appear* to be kernel-doc markups, but they aren't, because
> the structs themselves are not properly marked. See, for instance
> struct intel_context.
>
> scripts/kerneldoc will *only* consider what's there as a proper
> comment if you add:
>
> /**
>* struct intel_context - describes an i915 context
>*
>*/
> struct intel_context {
> union {
> /** @ref: a kernel object reference */
> struct kref ref; /* no kref_get_unless_zero()! */
> /** @rcu: a rcu header */
> struct rcu_head rcu;
> };
> ...
> }
>
> Describing all fields inside the struct. Just placing
> /** something */
> on some random places in the middle doesn't make it a kernel-doc.
Right, what we have today is incomplete/incorrect. But I think we
should be fixing that by adding the missing documentation on the
structure, rather than giving up and removing the kerneldoc. The end
goal should be to have proper generated documentation, not just silence
the warnings while leaving the actual output incomplete.
Matt
>
> If you actually run kernel-doc in Werror mode:
>
> ./scripts/kernel-doc -Werror -sphinx-version 2.4.4
> drivers/gpu/drm/i915/gt/intel_context_types.h | echo "ERROR!"
> ERROR!
> drivers/gpu/drm/i915/gt/intel_context_types.h:1: warning: no structured
> comments found
> 1 warnings as Errors
>
> you'll see that this is currently broken.
>
> Thanks,
> Mauro
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
Re: [PATCH v3 19/37] drm/i915: stop using kernel-doc markups for something else
Hi Matt,
Em Mon, 12 Sep 2022 08:09:57 -0700
Matt Roper escreveu:
> > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
>
> Several of the comments in this file do appear to be kerneldoc (in fact
> kerneldoc that was specifically requested during code review at
> https://patchwork.freedesktop.org/patch/448456/#comment_804252) and this
> file is included from Documentation/gpu/i915.rst, so I think some of
> these changes might be moving in the wrong direction. Should we instead
> focus on fixing up the comments that aren't quite formatted properly?
Those *appear* to be kernel-doc markups, but they aren't, because
the structs themselves are not properly marked. See, for instance
struct intel_context.
scripts/kerneldoc will *only* consider what's there as a proper
comment if you add:
/**
* struct intel_context - describes an i915 context
*
*/
struct intel_context {
union {
/** @ref: a kernel object reference */
struct kref ref; /* no kref_get_unless_zero()! */
/** @rcu: a rcu header */
struct rcu_head rcu;
};
...
}
Describing all fields inside the struct. Just placing
/** something */
on some random places in the middle doesn't make it a kernel-doc.
If you actually run kernel-doc in Werror mode:
./scripts/kernel-doc -Werror -sphinx-version 2.4.4
drivers/gpu/drm/i915/gt/intel_context_types.h | echo "ERROR!"
ERROR!
drivers/gpu/drm/i915/gt/intel_context_types.h:1: warning: no structured
comments found
1 warnings as Errors
you'll see that this is currently broken.
Thanks,
Mauro
Re: [PATCH v3 19/37] drm/i915: stop using kernel-doc markups for something else
On Fri, Sep 09, 2022 at 09:34:26AM +0200, Mauro Carvalho Chehab wrote: > There are some occurrences of "/**" that aren't actually part of > a kernel-doc markup. Replace them by "/*", in order to make easier > to identify what i915 files contain kernel-doc markups. > > Reviewed-by: Rodrigo Vivi > Signed-off-by: Mauro Carvalho Chehab > --- > > To avoid mailbombing on a large number of people, only mailing lists were C/C > on the cover. > See [PATCH v3 00/37] at: > https://lore.kernel.org/all/cover.1662708705.git.mche...@kernel.org/ > > drivers/gpu/drm/i915/display/dvo_ch7017.c | 26 +++ > drivers/gpu/drm/i915/display/dvo_ch7xxx.c | 6 +- > .../drm/i915/display/intel_display_types.h| 2 +- > drivers/gpu/drm/i915/display/intel_dvo_dev.h | 6 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 4 +- > drivers/gpu/drm/i915/display/intel_tv.c | 2 +- > drivers/gpu/drm/i915/gt/intel_context_types.h | 69 +-- > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.h | 2 +- > drivers/gpu/drm/i915/gt/intel_gt_types.h | 12 ++-- > drivers/gpu/drm/i915/gt/intel_reset_types.h | 4 +- > .../gpu/drm/i915/gt/intel_timeline_types.h| 6 +- > .../drm/i915/gt/shaders/clear_kernel/hsw.asm | 4 +- > .../drm/i915/gt/shaders/clear_kernel/ivb.asm | 4 +- > drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h | 10 +-- > drivers/gpu/drm/i915/i915_drm_client.h| 2 +- > drivers/gpu/drm/i915/i915_drv.h | 24 +++ > drivers/gpu/drm/i915/i915_file_private.h | 8 +-- > drivers/gpu/drm/i915/i915_gpu_error.h | 4 +- > drivers/gpu/drm/i915/i915_pmu.h | 32 - > drivers/gpu/drm/i915/intel_uncore.h | 4 +- > 20 files changed, 115 insertions(+), 116 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/dvo_ch7017.c > b/drivers/gpu/drm/i915/display/dvo_ch7017.c > index 0589994dde11..581e29ab77e4 100644 > --- a/drivers/gpu/drm/i915/display/dvo_ch7017.c > +++ b/drivers/gpu/drm/i915/display/dvo_ch7017.c > @@ -55,13 +55,13 @@ > #define CH7017_TEST_PATTERN 0x48 > > #define CH7017_POWER_MANAGEMENT 0x49 > -/** Enables the TV output path. */ > +/* Enables the TV output path. */ > #define CH7017_TV_EN (1 << 0) > #define CH7017_DAC0_POWER_DOWN (1 << 1) > #define CH7017_DAC1_POWER_DOWN (1 << 2) > #define CH7017_DAC2_POWER_DOWN (1 << 3) > #define CH7017_DAC3_POWER_DOWN (1 << 4) > -/** Powers down the TV out block, and DAC0-3 */ > +/* Powers down the TV out block, and DAC0-3 */ > #define CH7017_TV_POWER_DOWN_EN (1 << 5) > > #define CH7017_VERSION_ID0x4a > @@ -84,26 +84,26 @@ > #define CH7017_UP_SCALER_HORIZONTAL_INC_10x5e > > #define CH7017_HORIZONTAL_ACTIVE_PIXEL_INPUT 0x5f > -/**< Low bits of horizontal active pixel input */ > +/* Low bits of horizontal active pixel input */ > > #define CH7017_ACTIVE_INPUT_LINE_OUTPUT 0x60 > -/** High bits of horizontal active pixel input */ > +/* High bits of horizontal active pixel input */ > #define CH7017_LVDS_HAP_INPUT_MASK (0x7 << 0) > -/** High bits of vertical active line output */ > +/* High bits of vertical active line output */ > #define CH7017_LVDS_VAL_HIGH_MASK(0x7 << 3) > > #define CH7017_VERTICAL_ACTIVE_LINE_OUTPUT 0x61 > -/**< Low bits of vertical active line output */ > +/* Low bits of vertical active line output */ > > #define CH7017_HORIZONTAL_ACTIVE_PIXEL_OUTPUT0x62 > -/**< Low bits of horizontal active pixel output */ > +/* Low bits of horizontal active pixel output */ > > #define CH7017_LVDS_POWER_DOWN 0x63 > -/** High bits of horizontal active pixel output */ > +/* High bits of horizontal active pixel output */ > #define CH7017_LVDS_HAP_HIGH_MASK(0x7 << 0) > -/** Enables the LVDS power down state transition */ > +/* Enables the LVDS power down state transition */ > #define CH7017_LVDS_POWER_DOWN_EN(1 << 6) > -/** Enables the LVDS upscaler */ > +/* Enables the LVDS upscaler */ > #define CH7017_LVDS_UPSCALER_EN (1 << 7) > #define CH7017_LVDS_POWER_DOWN_DEFAULT_RESERVED 0x08 > > @@ -116,9 +116,9 @@ > #define CH7017_LVDS_ENCODING_2 0x65 > > #define CH7017_LVDS_PLL_CONTROL 0x66 > -/** Enables the LVDS panel output path */ > +/* Enables the LVDS panel output path */ > #define CH7017_LVDS_PANEN(1 << 0) > -/** Enables the LVDS panel backlight */ > +/* Enables the LVDS panel backlight */ > #define CH7017_LVDS_BKLEN(1 << 3) > > #define CH7017_POWER_SEQUENCING_T1 0x67 > @@ -197,7 +197,7 @@ static bool ch7017_write(struct intel_dvo_device *dvo, u8 > addr, u8 val) > return i2c_transfer(dvo->i2c_bus, , 1) == 1; > } > > -/** Probes for a CH7017 on the given bus and slave address. */ > +/* Probes for a CH7017 on the given bus and slave address. */ > static bool ch7017_init(struct intel_dvo_device
Re: [PATCH v3 19/37] drm/i915: stop using kernel-doc markups for something else
Hi Mauro, On Fri, Sep 09, 2022 at 09:34:26AM +0200, Mauro Carvalho Chehab wrote: > There are some occurrences of "/**" that aren't actually part of > a kernel-doc markup. Replace them by "/*", in order to make easier > to identify what i915 files contain kernel-doc markups. > > Reviewed-by: Rodrigo Vivi > Signed-off-by: Mauro Carvalho Chehab nice cleanup! Reviewed-by: Andi Shyti Thanks, Andi > --- > > To avoid mailbombing on a large number of people, only mailing lists were C/C > on the cover. > See [PATCH v3 00/37] at: > https://lore.kernel.org/all/cover.1662708705.git.mche...@kernel.org/ > > drivers/gpu/drm/i915/display/dvo_ch7017.c | 26 +++ > drivers/gpu/drm/i915/display/dvo_ch7xxx.c | 6 +- > .../drm/i915/display/intel_display_types.h| 2 +- > drivers/gpu/drm/i915/display/intel_dvo_dev.h | 6 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 4 +- > drivers/gpu/drm/i915/display/intel_tv.c | 2 +- > drivers/gpu/drm/i915/gt/intel_context_types.h | 69 +-- > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.h | 2 +- > drivers/gpu/drm/i915/gt/intel_gt_types.h | 12 ++-- > drivers/gpu/drm/i915/gt/intel_reset_types.h | 4 +- > .../gpu/drm/i915/gt/intel_timeline_types.h| 6 +- > .../drm/i915/gt/shaders/clear_kernel/hsw.asm | 4 +- > .../drm/i915/gt/shaders/clear_kernel/ivb.asm | 4 +- > drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h | 10 +-- > drivers/gpu/drm/i915/i915_drm_client.h| 2 +- > drivers/gpu/drm/i915/i915_drv.h | 24 +++ > drivers/gpu/drm/i915/i915_file_private.h | 8 +-- > drivers/gpu/drm/i915/i915_gpu_error.h | 4 +- > drivers/gpu/drm/i915/i915_pmu.h | 32 - > drivers/gpu/drm/i915/intel_uncore.h | 4 +- > 20 files changed, 115 insertions(+), 116 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/dvo_ch7017.c > b/drivers/gpu/drm/i915/display/dvo_ch7017.c > index 0589994dde11..581e29ab77e4 100644 > --- a/drivers/gpu/drm/i915/display/dvo_ch7017.c > +++ b/drivers/gpu/drm/i915/display/dvo_ch7017.c > @@ -55,13 +55,13 @@ > #define CH7017_TEST_PATTERN 0x48 > > #define CH7017_POWER_MANAGEMENT 0x49 > -/** Enables the TV output path. */ > +/* Enables the TV output path. */ > #define CH7017_TV_EN (1 << 0) > #define CH7017_DAC0_POWER_DOWN (1 << 1) > #define CH7017_DAC1_POWER_DOWN (1 << 2) > #define CH7017_DAC2_POWER_DOWN (1 << 3) > #define CH7017_DAC3_POWER_DOWN (1 << 4) > -/** Powers down the TV out block, and DAC0-3 */ > +/* Powers down the TV out block, and DAC0-3 */ > #define CH7017_TV_POWER_DOWN_EN (1 << 5) > > #define CH7017_VERSION_ID0x4a > @@ -84,26 +84,26 @@ > #define CH7017_UP_SCALER_HORIZONTAL_INC_10x5e > > #define CH7017_HORIZONTAL_ACTIVE_PIXEL_INPUT 0x5f > -/**< Low bits of horizontal active pixel input */ > +/* Low bits of horizontal active pixel input */ > > #define CH7017_ACTIVE_INPUT_LINE_OUTPUT 0x60 > -/** High bits of horizontal active pixel input */ > +/* High bits of horizontal active pixel input */ > #define CH7017_LVDS_HAP_INPUT_MASK (0x7 << 0) > -/** High bits of vertical active line output */ > +/* High bits of vertical active line output */ > #define CH7017_LVDS_VAL_HIGH_MASK(0x7 << 3) > > #define CH7017_VERTICAL_ACTIVE_LINE_OUTPUT 0x61 > -/**< Low bits of vertical active line output */ > +/* Low bits of vertical active line output */ > > #define CH7017_HORIZONTAL_ACTIVE_PIXEL_OUTPUT0x62 > -/**< Low bits of horizontal active pixel output */ > +/* Low bits of horizontal active pixel output */ > > #define CH7017_LVDS_POWER_DOWN 0x63 > -/** High bits of horizontal active pixel output */ > +/* High bits of horizontal active pixel output */ > #define CH7017_LVDS_HAP_HIGH_MASK(0x7 << 0) > -/** Enables the LVDS power down state transition */ > +/* Enables the LVDS power down state transition */ > #define CH7017_LVDS_POWER_DOWN_EN(1 << 6) > -/** Enables the LVDS upscaler */ > +/* Enables the LVDS upscaler */ > #define CH7017_LVDS_UPSCALER_EN (1 << 7) > #define CH7017_LVDS_POWER_DOWN_DEFAULT_RESERVED 0x08 > > @@ -116,9 +116,9 @@ > #define CH7017_LVDS_ENCODING_2 0x65 > > #define CH7017_LVDS_PLL_CONTROL 0x66 > -/** Enables the LVDS panel output path */ > +/* Enables the LVDS panel output path */ > #define CH7017_LVDS_PANEN(1 << 0) > -/** Enables the LVDS panel backlight */ > +/* Enables the LVDS panel backlight */ > #define CH7017_LVDS_BKLEN(1 << 3) > > #define CH7017_POWER_SEQUENCING_T1 0x67 > @@ -197,7 +197,7 @@ static bool ch7017_write(struct intel_dvo_device *dvo, u8 > addr, u8 val) > return i2c_transfer(dvo->i2c_bus, , 1) == 1; > } > > -/** Probes for a CH7017 on the given bus and slave address. */ > +/* Probes for a CH7017 on the given bus and
