Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
On Tue, Dec 21, 2021 at 06:05:34PM +0200, Ville Syrjälä wrote:
> On Sat, Dec 18, 2021 at 06:00:47PM -0700, Nathan Chancellor wrote:
> > Hi Ville,
> >
> > On Mon, Dec 13, 2021 at 05:14:35PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä
> > >
> > > Expose FBC debugfs files for each crtc. These may or may not point
> > > to the same FBC instance depending on the platform.
> > >
> > > We leave the old global debugfs files in place until
> > > igt catches up to the new per-crtc approach.
> > >
> > > v2: Take a trip via intel_crtc_debugfs_add() (Jani)
> > >
> > > Cc: Jani Nikula
> > > Signed-off-by: Ville Syrjälä
> >
> > Apologies if this has already been reported and fixed, I have not seen
> > anything on lore.kernel.org or drm-intel about it.
> >
> > This patch as commit e74c6aa955ca ("drm/i915/fbc: Register per-crtc
> > debugfs files") breaks the build when CONFIG_DEBUG_FS is disabled.
> >
> > drivers/gpu/drm/i915/display/intel_fbc.c: In function
> > ‘intel_fbc_crtc_debugfs_add’:
> > drivers/gpu/drm/i915/display/intel_fbc.c:1817:61: error: ‘struct drm_crtc’
> > has no member named ‘debugfs_entry’
> > 1817 | intel_fbc_debugfs_add(plane->fbc,
> > crtc->base.debugfs_entry);
> > | ^
>
> Doh. I guess I didn't actually build test the final version with
> DEBUGFS=n.
>
> Does this fix it for you?
Yes, it does.
Tested-by: Nathan Chancellor
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 13eeba2a750a..4d01b4d89775 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1135,14 +1135,12 @@ struct drm_crtc {
>*/
> spinlock_t commit_lock;
>
> -#ifdef CONFIG_DEBUG_FS
> /**
>* @debugfs_entry:
>*
>* Debugfs directory for this CRTC.
>*/
> struct dentry *debugfs_entry;
> -#endif
>
> /**
>* @crc:
> --
> 2.32.0
>
> --
> Ville Syrjälä
> Intel
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
Hi Ville,
On Mon, Dec 13, 2021 at 05:14:35PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Expose FBC debugfs files for each crtc. These may or may not point
> to the same FBC instance depending on the platform.
>
> We leave the old global debugfs files in place until
> igt catches up to the new per-crtc approach.
>
> v2: Take a trip via intel_crtc_debugfs_add() (Jani)
>
> Cc: Jani Nikula
> Signed-off-by: Ville Syrjälä
Apologies if this has already been reported and fixed, I have not seen
anything on lore.kernel.org or drm-intel about it.
This patch as commit e74c6aa955ca ("drm/i915/fbc: Register per-crtc
debugfs files") breaks the build when CONFIG_DEBUG_FS is disabled.
drivers/gpu/drm/i915/display/intel_fbc.c: In function
‘intel_fbc_crtc_debugfs_add’:
drivers/gpu/drm/i915/display/intel_fbc.c:1817:61: error: ‘struct drm_crtc’ has
no member named ‘debugfs_entry’
1817 | intel_fbc_debugfs_add(plane->fbc,
crtc->base.debugfs_entry);
| ^
Cheers,
Nathan
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
On Sat, Dec 18, 2021 at 06:00:47PM -0700, Nathan Chancellor wrote:
> Hi Ville,
>
> On Mon, Dec 13, 2021 at 05:14:35PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Expose FBC debugfs files for each crtc. These may or may not point
> > to the same FBC instance depending on the platform.
> >
> > We leave the old global debugfs files in place until
> > igt catches up to the new per-crtc approach.
> >
> > v2: Take a trip via intel_crtc_debugfs_add() (Jani)
> >
> > Cc: Jani Nikula
> > Signed-off-by: Ville Syrjälä
>
> Apologies if this has already been reported and fixed, I have not seen
> anything on lore.kernel.org or drm-intel about it.
>
> This patch as commit e74c6aa955ca ("drm/i915/fbc: Register per-crtc
> debugfs files") breaks the build when CONFIG_DEBUG_FS is disabled.
>
> drivers/gpu/drm/i915/display/intel_fbc.c: In function
> ‘intel_fbc_crtc_debugfs_add’:
> drivers/gpu/drm/i915/display/intel_fbc.c:1817:61: error: ‘struct drm_crtc’
> has no member named ‘debugfs_entry’
> 1817 | intel_fbc_debugfs_add(plane->fbc,
> crtc->base.debugfs_entry);
> | ^
Doh. I guess I didn't actually build test the final version with
DEBUGFS=n.
Does this fix it for you?
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 13eeba2a750a..4d01b4d89775 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1135,14 +1135,12 @@ struct drm_crtc {
*/
spinlock_t commit_lock;
-#ifdef CONFIG_DEBUG_FS
/**
* @debugfs_entry:
*
* Debugfs directory for this CRTC.
*/
struct dentry *debugfs_entry;
-#endif
/**
* @crc:
--
2.32.0
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
On Mon, Dec 13, 2021 at 09:09:40PM +0200, Jani Nikula wrote:
> On Mon, 13 Dec 2021, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Expose FBC debugfs files for each crtc. These may or may not point
> > to the same FBC instance depending on the platform.
> >
> > We leave the old global debugfs files in place until
> > igt catches up to the new per-crtc approach.
> >
> > v2: Take a trip via intel_crtc_debugfs_add() (Jani)
> >
> > Cc: Jani Nikula
> > Signed-off-by: Ville Syrjälä
>
> Reviewed-by: Jani Nikula
>
> > ---
> > .../drm/i915/display/intel_display_debugfs.c | 7 +++--
> > drivers/gpu/drm/i915/display/intel_fbc.c | 31 ---
> > drivers/gpu/drm/i915/display/intel_fbc.h | 1 +
> > 3 files changed, 25 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 572445299b04..f4de004d470f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -2402,6 +2402,9 @@ void intel_connector_debugfs_add(struct
> > intel_connector *intel_connector)
> > */
> > void intel_crtc_debugfs_add(struct drm_crtc *crtc)
> > {
> > - if (crtc->debugfs_entry)
> > - crtc_updates_add(crtc);
> > + if (!crtc->debugfs_entry)
> > + return;
>
> I think this is probably unnecessary, but that's for another patch.
I guess. Seems unlikely at best that we'd have failed to allocate
that.
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v3 4/4] drm/i915/fbc: Register per-crtc debugfs files
On Mon, 13 Dec 2021, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Expose FBC debugfs files for each crtc. These may or may not point
> to the same FBC instance depending on the platform.
>
> We leave the old global debugfs files in place until
> igt catches up to the new per-crtc approach.
>
> v2: Take a trip via intel_crtc_debugfs_add() (Jani)
>
> Cc: Jani Nikula
> Signed-off-by: Ville Syrjälä
Reviewed-by: Jani Nikula
> ---
> .../drm/i915/display/intel_display_debugfs.c | 7 +++--
> drivers/gpu/drm/i915/display/intel_fbc.c | 31 ---
> drivers/gpu/drm/i915/display/intel_fbc.h | 1 +
> 3 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 572445299b04..f4de004d470f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -2402,6 +2402,9 @@ void intel_connector_debugfs_add(struct intel_connector
> *intel_connector)
> */
> void intel_crtc_debugfs_add(struct drm_crtc *crtc)
> {
> - if (crtc->debugfs_entry)
> - crtc_updates_add(crtc);
> + if (!crtc->debugfs_entry)
> + return;
I think this is probably unnecessary, but that's for another patch.
> +
> + crtc_updates_add(crtc);
> + intel_fbc_crtc_debugfs_add(to_intel_crtc(crtc));
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 53c93387710c..987ea4c4b5d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1798,25 +1798,32 @@
> DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops,
> intel_fbc_debugfs_false_color_set,
> "%llu\n");
>
> -static void intel_fbc_debugfs_add(struct intel_fbc *fbc)
> +static void intel_fbc_debugfs_add(struct intel_fbc *fbc,
> + struct dentry *parent)
> {
> - struct drm_i915_private *i915 = fbc->i915;
> - struct drm_minor *minor = i915->drm.primary;
> -
> - debugfs_create_file("i915_fbc_status", 0444,
> - minor->debugfs_root, fbc,
> - &intel_fbc_debugfs_status_fops);
> + debugfs_create_file("i915_fbc_status", 0444, parent,
> + fbc, &intel_fbc_debugfs_status_fops);
>
> if (fbc->funcs->set_false_color)
> - debugfs_create_file("i915_fbc_false_color", 0644,
> - minor->debugfs_root, fbc,
> - &intel_fbc_debugfs_false_color_fops);
> + debugfs_create_file("i915_fbc_false_color", 0644, parent,
> + fbc, &intel_fbc_debugfs_false_color_fops);
> }
>
> +void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc)
> +{
> + struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> +
> + if (plane->fbc)
> + intel_fbc_debugfs_add(plane->fbc, crtc->base.debugfs_entry);
> +}
> +
> +/* FIXME: remove this once igt is on board with per-crtc stuff */
> void intel_fbc_debugfs_register(struct drm_i915_private *i915)
> {
> - struct intel_fbc *fbc = i915->fbc[INTEL_FBC_A];
> + struct drm_minor *minor = i915->drm.primary;
> + struct intel_fbc *fbc;
>
> + fbc = i915->fbc[INTEL_FBC_A];
> if (fbc)
> - intel_fbc_debugfs_add(fbc);
> + intel_fbc_debugfs_add(fbc, minor->debugfs_root);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h
> b/drivers/gpu/drm/i915/display/intel_fbc.h
> index 7b7631aec527..8c5a7339a27f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -42,6 +42,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane);
> void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *i915);
> void intel_fbc_reset_underrun(struct drm_i915_private *i915);
> +void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc);
> void intel_fbc_debugfs_register(struct drm_i915_private *i915);
>
> #endif /* __INTEL_FBC_H__ */
--
Jani Nikula, Intel Open Source Graphics Center
