| Message ID | 20250224080847.326350-6-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | LOBF enablement fix | expand |
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Monday, February 24, 2025 4:26 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Cc: Hogander, Jouni <jouni.hogander@intel.com>; B, Jeevan > <jeevan.b@intel.com>; Manna, Animesh <animesh.manna@intel.com> > Subject: Re: [PATCH v4 5/8] drm/i915/lobf: Add debug interface for lobf > > On Mon, 24 Feb 2025, Animesh Manna <animesh.manna@intel.com> wrote: > > Add an interface in debugfs which will help in debugging LOBF feature. > > > > v1: Initial version. > > v2: > > - Remove FORCE_EN flag. [Jouni] > > - Change prefix from I915 to INTEL. [Jani] > > - Use u8 instead of bool for lobf-debug flag. [Jani] > > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_alpm.c | 51 +++++++++++++++++++ > > .../drm/i915/display/intel_display_types.h | 5 ++ > > 2 files changed, 56 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > > b/drivers/gpu/drm/i915/display/intel_alpm.c > > index 83719ee1721c..5c70677ac3c0 100644 > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > > @@ -276,6 +276,9 @@ void intel_alpm_lobf_compute_config(struct > intel_dp *intel_dp, > > int waketime_in_lines, first_sdp_position; > > int context_latency, guardband; > > > > + if (intel_dp->alpm_parameters.lobf_debug & > INTEL_LOBF_DEBUG_DISABLE) > > + return; > > + > > if (!intel_dp_is_edp(intel_dp)) > > return; > > > > @@ -449,6 +452,51 @@ static int i915_edp_lobf_info_show(struct > > seq_file *m, void *data) > > > > DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info); > > > > +static int > > +i915_edp_lobf_debug_get(void *data, u64 *val) { > > + struct intel_display *display = data; > > Where do you pass display as data? It's connector? I should pass display, thanks for catching. > > > + struct intel_encoder *encoder; > > + int ret = -ENODEV; > > + > > + for_each_intel_dp(display->drm, encoder) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + if (!intel_dp_is_edp(intel_dp)) > > + return ret; > > What is this supposed to do? Other than EDP connector lobg_debug is not available, so will remove the above condition check. > > > + > > + // TODO: split to each edp transcoder. > > What does this mean? Currently lobf debug option only supported with one EDP, TODO added to support second EDP. > > > + *val = READ_ONCE(intel_dp->alpm_parameters.lobf_debug); > > You read this from all intel_dp and combine into one? What? Will return from here, will take care in next version. > > + ret = 0; > > + } > > + > > + return ret; > > +} > > + > > +static int > > +i915_edp_lobf_debug_set(void *data, u64 val) { > > + struct intel_display *display = data; > > + struct intel_encoder *encoder; > > + int ret = -ENODEV; > > + > > + for_each_intel_dp(display->drm, encoder) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + if (!intel_dp_is_edp(intel_dp)) > > + return ret; > > + > > + // TODO: split to each edp transcoder. > > + intel_dp->alpm_parameters.lobf_debug = val; > > + } > > + > > + return ret; > > So this always returns failure? Missed to set the ret variable, will take care in next version. Thanks for review. Regards, Animesh > > > +} > > + > > +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_lobf_debug_fops, > > + i915_edp_lobf_debug_get, > i915_edp_lobf_debug_set, > > + "%llu\n"); > > + > > void intel_alpm_lobf_debugfs_add(struct intel_connector *connector) > > { > > struct intel_display *display = to_intel_display(connector); @@ > > -458,6 +506,9 @@ void intel_alpm_lobf_debugfs_add(struct > intel_connector *connector) > > connector->base.connector_type != > DRM_MODE_CONNECTOR_eDP) > > return; > > > > + debugfs_create_file("i915_edp_lobf_debug", 0644, root, > > + connector, &i915_edp_lobf_debug_fops); > > + > > debugfs_create_file("i915_edp_lobf_info", 0444, root, > > connector, &i915_edp_lobf_info_fops); } diff --git > > a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index b78721c451b8..b6ec9a8fadd9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1808,6 +1808,11 @@ struct intel_dp { > > u8 aux_less_wake_lines; > > u8 silence_period_sym_clocks; > > u8 lfps_half_cycle_num_of_syms; > > + > > +#define INTEL_LOBF_DEBUG_MODE_MASK 0x0f > > +#define INTEL_LOBF_DEBUG_DEFAULT 0x00 > > +#define INTEL_LOBF_DEBUG_DISABLE 0x01 > > + u8 lobf_debug; > > Just overly complex still. > > > } alpm_parameters; > > > > u8 alpm_dpcd; > > -- > Jani Nikula, Intel
On Tue, 25 Feb 2025, "Manna, Animesh" <animesh.manna@intel.com> wrote: >> -----Original Message----- >> From: Nikula, Jani <jani.nikula@intel.com> >> Sent: Monday, February 24, 2025 4:26 PM >> To: Manna, Animesh <animesh.manna@intel.com>; intel- >> gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org >> Cc: Hogander, Jouni <jouni.hogander@intel.com>; B, Jeevan >> <jeevan.b@intel.com>; Manna, Animesh <animesh.manna@intel.com> >> Subject: Re: [PATCH v4 5/8] drm/i915/lobf: Add debug interface for lobf >> >> On Mon, 24 Feb 2025, Animesh Manna <animesh.manna@intel.com> wrote: >> > Add an interface in debugfs which will help in debugging LOBF feature. >> > >> > v1: Initial version. >> > v2: >> > - Remove FORCE_EN flag. [Jouni] >> > - Change prefix from I915 to INTEL. [Jani] >> > - Use u8 instead of bool for lobf-debug flag. [Jani] >> > >> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_alpm.c | 51 +++++++++++++++++++ >> > .../drm/i915/display/intel_display_types.h | 5 ++ >> > 2 files changed, 56 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c >> > b/drivers/gpu/drm/i915/display/intel_alpm.c >> > index 83719ee1721c..5c70677ac3c0 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_alpm.c >> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c >> > @@ -276,6 +276,9 @@ void intel_alpm_lobf_compute_config(struct >> intel_dp *intel_dp, >> > int waketime_in_lines, first_sdp_position; >> > int context_latency, guardband; >> > >> > + if (intel_dp->alpm_parameters.lobf_debug & >> INTEL_LOBF_DEBUG_DISABLE) >> > + return; >> > + >> > if (!intel_dp_is_edp(intel_dp)) >> > return; >> > >> > @@ -449,6 +452,51 @@ static int i915_edp_lobf_info_show(struct >> > seq_file *m, void *data) >> > >> > DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info); >> > >> > +static int >> > +i915_edp_lobf_debug_get(void *data, u64 *val) { >> > + struct intel_display *display = data; >> >> Where do you pass display as data? It's connector? > > I should pass display, thanks for catching. It's a connector based debugfs file, so you should probably keep passing the connector, and just assign to connector here. > >> >> > + struct intel_encoder *encoder; >> > + int ret = -ENODEV; >> > + >> > + for_each_intel_dp(display->drm, encoder) { >> > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> > + >> > + if (!intel_dp_is_edp(intel_dp)) >> > + return ret; >> >> What is this supposed to do? > > Other than EDP connector lobg_debug is not available, so will remove the above condition check. This debugfs file is connector based. Why are we looping all dp anyway? I don't understand. >> >> > + >> > + // TODO: split to each edp transcoder. >> >> What does this mean? > > Currently lobf debug option only supported with one EDP, TODO added to support second EDP. Again, this debugfs file is connector based. I don't understand the TODO at all. > >> >> > + *val = READ_ONCE(intel_dp->alpm_parameters.lobf_debug); >> >> You read this from all intel_dp and combine into one? What? > > Will return from here, will take care in next version. > >> > + ret = 0; >> > + } >> > + >> > + return ret; >> > +} >> > + >> > +static int >> > +i915_edp_lobf_debug_set(void *data, u64 val) { >> > + struct intel_display *display = data; >> > + struct intel_encoder *encoder; >> > + int ret = -ENODEV; >> > + >> > + for_each_intel_dp(display->drm, encoder) { >> > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> > + >> > + if (!intel_dp_is_edp(intel_dp)) >> > + return ret; >> > + >> > + // TODO: split to each edp transcoder. >> > + intel_dp->alpm_parameters.lobf_debug = val; >> > + } >> > + >> > + return ret; >> >> So this always returns failure? > > Missed to set the ret variable, will take care in next version. > Thanks for review. > > Regards, > Animesh >> >> > +} >> > + >> > +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_lobf_debug_fops, >> > + i915_edp_lobf_debug_get, >> i915_edp_lobf_debug_set, >> > + "%llu\n"); >> > + >> > void intel_alpm_lobf_debugfs_add(struct intel_connector *connector) >> > { >> > struct intel_display *display = to_intel_display(connector); @@ >> > -458,6 +506,9 @@ void intel_alpm_lobf_debugfs_add(struct >> intel_connector *connector) >> > connector->base.connector_type != >> DRM_MODE_CONNECTOR_eDP) >> > return; You'll only have these debugfs files for eDP connectors anyway. There's no need for eDP checks later. BR, Jani. >> > >> > + debugfs_create_file("i915_edp_lobf_debug", 0644, root, >> > + connector, &i915_edp_lobf_debug_fops); >> > + >> > debugfs_create_file("i915_edp_lobf_info", 0444, root, >> > connector, &i915_edp_lobf_info_fops); } diff --git >> > a/drivers/gpu/drm/i915/display/intel_display_types.h >> > b/drivers/gpu/drm/i915/display/intel_display_types.h >> > index b78721c451b8..b6ec9a8fadd9 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> > @@ -1808,6 +1808,11 @@ struct intel_dp { >> > u8 aux_less_wake_lines; >> > u8 silence_period_sym_clocks; >> > u8 lfps_half_cycle_num_of_syms; >> > + >> > +#define INTEL_LOBF_DEBUG_MODE_MASK 0x0f >> > +#define INTEL_LOBF_DEBUG_DEFAULT 0x00 >> > +#define INTEL_LOBF_DEBUG_DISABLE 0x01 >> > + u8 lobf_debug; >> >> Just overly complex still. >> >> > } alpm_parameters; >> > >> > u8 alpm_dpcd; >> >> -- >> Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c index 83719ee1721c..5c70677ac3c0 100644 --- a/drivers/gpu/drm/i915/display/intel_alpm.c +++ b/drivers/gpu/drm/i915/display/intel_alpm.c @@ -276,6 +276,9 @@ void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, int waketime_in_lines, first_sdp_position; int context_latency, guardband; + if (intel_dp->alpm_parameters.lobf_debug & INTEL_LOBF_DEBUG_DISABLE) + return; + if (!intel_dp_is_edp(intel_dp)) return; @@ -449,6 +452,51 @@ static int i915_edp_lobf_info_show(struct seq_file *m, void *data) DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info); +static int +i915_edp_lobf_debug_get(void *data, u64 *val) +{ + struct intel_display *display = data; + struct intel_encoder *encoder; + int ret = -ENODEV; + + for_each_intel_dp(display->drm, encoder) { + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + + if (!intel_dp_is_edp(intel_dp)) + return ret; + + // TODO: split to each edp transcoder. + *val = READ_ONCE(intel_dp->alpm_parameters.lobf_debug); + ret = 0; + } + + return ret; +} + +static int +i915_edp_lobf_debug_set(void *data, u64 val) +{ + struct intel_display *display = data; + struct intel_encoder *encoder; + int ret = -ENODEV; + + for_each_intel_dp(display->drm, encoder) { + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + + if (!intel_dp_is_edp(intel_dp)) + return ret; + + // TODO: split to each edp transcoder. + intel_dp->alpm_parameters.lobf_debug = val; + } + + return ret; +} + +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_lobf_debug_fops, + i915_edp_lobf_debug_get, i915_edp_lobf_debug_set, + "%llu\n"); + void intel_alpm_lobf_debugfs_add(struct intel_connector *connector) { struct intel_display *display = to_intel_display(connector); @@ -458,6 +506,9 @@ void intel_alpm_lobf_debugfs_add(struct intel_connector *connector) connector->base.connector_type != DRM_MODE_CONNECTOR_eDP) return; + debugfs_create_file("i915_edp_lobf_debug", 0644, root, + connector, &i915_edp_lobf_debug_fops); + debugfs_create_file("i915_edp_lobf_info", 0444, root, connector, &i915_edp_lobf_info_fops); } diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index b78721c451b8..b6ec9a8fadd9 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1808,6 +1808,11 @@ struct intel_dp { u8 aux_less_wake_lines; u8 silence_period_sym_clocks; u8 lfps_half_cycle_num_of_syms; + +#define INTEL_LOBF_DEBUG_MODE_MASK 0x0f +#define INTEL_LOBF_DEBUG_DEFAULT 0x00 +#define INTEL_LOBF_DEBUG_DISABLE 0x01 + u8 lobf_debug; } alpm_parameters; u8 alpm_dpcd;
Add an interface in debugfs which will help in debugging LOBF feature. v1: Initial version. v2: - Remove FORCE_EN flag. [Jouni] - Change prefix from I915 to INTEL. [Jani] - Use u8 instead of bool for lobf-debug flag. [Jani] Signed-off-by: Animesh Manna <animesh.manna@intel.com> --- drivers/gpu/drm/i915/display/intel_alpm.c | 51 +++++++++++++++++++ .../drm/i915/display/intel_display_types.h | 5 ++ 2 files changed, 56 insertions(+)