Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF
On Fri, 2024-05-03 at 08:30 +, Manna, Animesh wrote: > > > > -Original Message- > > From: Hogander, Jouni > > Sent: Friday, May 3, 2024 1:02 PM > > To: Manna, Animesh ; intel- > > g...@lists.freedesktop.org > > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R > > ; Nikula, Jani > > Subject: Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF > > > > On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote: > > > For validation purpose add debugfs for LOBF. > > > > > > Signed-off-by: Animesh Manna > > > --- > > > drivers/gpu/drm/i915/display/intel_alpm.c | 48 > > > +++ > > > drivers/gpu/drm/i915/display/intel_alpm.h | 2 + > > > .../drm/i915/display/intel_display_debugfs.c | 2 + > > > 3 files changed, 52 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > > > b/drivers/gpu/drm/i915/display/intel_alpm.c > > > index b08799586b58..2d3027c2fb0a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > > > @@ -343,3 +343,51 @@ void intel_alpm_configure(struct intel_dp > > > *intel_dp, > > > { > > > lnl_alpm_configure(intel_dp, crtc_state); > > > } > > > + > > > +static int i915_edp_lobf_info_show(struct seq_file *m, void > > > *data) > > > +{ > > > + struct intel_connector *connector = m->private; > > > + struct drm_i915_private *dev_priv = to_i915(connector- > > > > base.dev); > > > + struct drm_crtc *crtc; > > > + struct intel_crtc_state *crtc_state; > > > + enum transcoder cpu_transcoder; > > > + bool lobf_enabled; > > > + int ret; > > > + > > > + ret = drm_modeset_lock_single_interruptible(_priv- > > > > drm.mode_config.connection_mutex); > > > + if (ret) > > > + return ret; > > > + > > > + crtc = connector->base.state->crtc; > > > + if (connector->base.status != connector_status_connected > > > || > > > !crtc) { > > > + ret = -ENODEV; > > > + goto out; > > > + } > > > + > > > + crtc_state = to_intel_crtc_state(crtc->state); > > > + seq_printf(m, "LOBF Criteria met: %s\n", > > > str_yes_no(crtc_state->has_lobf)); > > > > I'm still not convinced on this. has_lobf ~= ALPM_CTL_LOBF_ENABLE > > in > > ALPM_CTL. To my opinion it is enough to dump seq_printf(m, "LOBF > > status: %s\n", str_enabled_disabled(lobf_enabled)) below. Maybe > > AUX_WAKE and AUX_LESS_WAKE could be dumped instead? > > Can add aux-wake or aux-less info as well. > But as LOBF feature is dependent on adaptive sync fixed refresh rate > mode (which can be dynamic as per user input) and ALPM. I want to > expose both the conditions are satisfying or not along with status. If all those conditions are satisfied (i.e. has_lobf is true) then ALPM_CTL & ALPM_CTL_LOBF_ENABLE is true? So I'm wondering what is the benefit from dumping has_lobf? BR, Jouni Högander > > Regards, > Animesh > > > > > BR, > > > > Jouni Högander > > > > > + > > > + cpu_transcoder = crtc_state->cpu_transcoder; > > > + lobf_enabled = intel_de_read(dev_priv, > > > ALPM_CTL(cpu_transcoder)) & ALPM_CTL_LOBF_ENABLE; > > > + seq_printf(m, "LOBF status: %s\n", > > > str_enabled_disabled(lobf_enabled)); > > > + > > > +out: > > > + drm_modeset_unlock(_priv- > > > > drm.mode_config.connection_mutex); > > > + > > > + return ret; > > > +} > > > + > > > +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info); > > > + > > > +void intel_alpm_lobf_debugfs_add(struct intel_connector > > > *connector) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(connector- > > > >base.dev); > > > + struct dentry *root = connector->base.debugfs_entry; > > > + > > > + if (DISPLAY_VER(i915) < 20 || > > > + connector->base.connector_type != > > DRM_MODE_CONNECTOR_eDP) > > > + return; > > > + > > > + debugfs_create_file("i915_edp_lobf_info", 0444, root, > > > + connector, _edp
RE: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF
> -Original Message- > From: Hogander, Jouni > Sent: Friday, May 3, 2024 1:02 PM > To: Manna, Animesh ; intel- > g...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org; Murthy, Arun R > ; Nikula, Jani > Subject: Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF > > On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote: > > For validation purpose add debugfs for LOBF. > > > > Signed-off-by: Animesh Manna > > --- > > drivers/gpu/drm/i915/display/intel_alpm.c | 48 > > +++ > > drivers/gpu/drm/i915/display/intel_alpm.h | 2 + > > .../drm/i915/display/intel_display_debugfs.c | 2 + > > 3 files changed, 52 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > > b/drivers/gpu/drm/i915/display/intel_alpm.c > > index b08799586b58..2d3027c2fb0a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > > @@ -343,3 +343,51 @@ void intel_alpm_configure(struct intel_dp > > *intel_dp, > > { > > lnl_alpm_configure(intel_dp, crtc_state); > > } > > + > > +static int i915_edp_lobf_info_show(struct seq_file *m, void *data) > > +{ > > + struct intel_connector *connector = m->private; > > + struct drm_i915_private *dev_priv = to_i915(connector- > > >base.dev); > > + struct drm_crtc *crtc; > > + struct intel_crtc_state *crtc_state; > > + enum transcoder cpu_transcoder; > > + bool lobf_enabled; > > + int ret; > > + > > + ret = drm_modeset_lock_single_interruptible(_priv- > > >drm.mode_config.connection_mutex); > > + if (ret) > > + return ret; > > + > > + crtc = connector->base.state->crtc; > > + if (connector->base.status != connector_status_connected || > > !crtc) { > > + ret = -ENODEV; > > + goto out; > > + } > > + > > + crtc_state = to_intel_crtc_state(crtc->state); > > + seq_printf(m, "LOBF Criteria met: %s\n", > > str_yes_no(crtc_state->has_lobf)); > > I'm still not convinced on this. has_lobf ~= ALPM_CTL_LOBF_ENABLE in > ALPM_CTL. To my opinion it is enough to dump seq_printf(m, "LOBF > status: %s\n", str_enabled_disabled(lobf_enabled)) below. Maybe > AUX_WAKE and AUX_LESS_WAKE could be dumped instead? Can add aux-wake or aux-less info as well. But as LOBF feature is dependent on adaptive sync fixed refresh rate mode (which can be dynamic as per user input) and ALPM. I want to expose both the conditions are satisfying or not along with status. Regards, Animesh > > BR, > > Jouni Högander > > > + > > + cpu_transcoder = crtc_state->cpu_transcoder; > > + lobf_enabled = intel_de_read(dev_priv, > > ALPM_CTL(cpu_transcoder)) & ALPM_CTL_LOBF_ENABLE; > > + seq_printf(m, "LOBF status: %s\n", > > str_enabled_disabled(lobf_enabled)); > > + > > +out: > > + drm_modeset_unlock(_priv- > > >drm.mode_config.connection_mutex); > > + > > + return ret; > > +} > > + > > +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info); > > + > > +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector) > > +{ > > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > > + struct dentry *root = connector->base.debugfs_entry; > > + > > + if (DISPLAY_VER(i915) < 20 || > > + connector->base.connector_type != > DRM_MODE_CONNECTOR_eDP) > > + return; > > + > > + debugfs_create_file("i915_edp_lobf_info", 0444, root, > > + connector, _edp_lobf_info_fops); > > +} > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h > > b/drivers/gpu/drm/i915/display/intel_alpm.h > > index a9ca190da3e4..01fd08eb96f5 100644 > > --- a/drivers/gpu/drm/i915/display/intel_alpm.h > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h > > @@ -11,6 +11,7 @@ > > struct intel_dp; > > struct intel_crtc_state; > > struct drm_connector_state; > > +struct intel_connector; > > > > bool intel_alpm_get_aux_less_status(struct intel_dp *intel_dp); > > bool intel_alpm_compute_params(struct intel_dp *intel_dp, > > @@ -20,4 +21,5 @@ void intel_alpm_compute_lobf_config(struct intel_dp > > *intel_dp, > > struct drm_connector_state > > *conn_state); > > void intel
Re: [PATCH v3 6/6] drm/i915/alpm: Add debugfs for LOBF
On Thu, 2024-04-25 at 00:08 +0530, Animesh Manna wrote: > For validation purpose add debugfs for LOBF. > > Signed-off-by: Animesh Manna > --- > drivers/gpu/drm/i915/display/intel_alpm.c | 48 > +++ > drivers/gpu/drm/i915/display/intel_alpm.h | 2 + > .../drm/i915/display/intel_display_debugfs.c | 2 + > 3 files changed, 52 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c > b/drivers/gpu/drm/i915/display/intel_alpm.c > index b08799586b58..2d3027c2fb0a 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.c > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c > @@ -343,3 +343,51 @@ void intel_alpm_configure(struct intel_dp > *intel_dp, > { > lnl_alpm_configure(intel_dp, crtc_state); > } > + > +static int i915_edp_lobf_info_show(struct seq_file *m, void *data) > +{ > + struct intel_connector *connector = m->private; > + struct drm_i915_private *dev_priv = to_i915(connector- > >base.dev); > + struct drm_crtc *crtc; > + struct intel_crtc_state *crtc_state; > + enum transcoder cpu_transcoder; > + bool lobf_enabled; > + int ret; > + > + ret = drm_modeset_lock_single_interruptible(_priv- > >drm.mode_config.connection_mutex); > + if (ret) > + return ret; > + > + crtc = connector->base.state->crtc; > + if (connector->base.status != connector_status_connected || > !crtc) { > + ret = -ENODEV; > + goto out; > + } > + > + crtc_state = to_intel_crtc_state(crtc->state); > + seq_printf(m, "LOBF Criteria met: %s\n", > str_yes_no(crtc_state->has_lobf)); I'm still not convinced on this. has_lobf ~= ALPM_CTL_LOBF_ENABLE in ALPM_CTL. To my opinion it is enough to dump seq_printf(m, "LOBF status: %s\n", str_enabled_disabled(lobf_enabled)) below. Maybe AUX_WAKE and AUX_LESS_WAKE could be dumped instead? BR, Jouni Högander > + > + cpu_transcoder = crtc_state->cpu_transcoder; > + lobf_enabled = intel_de_read(dev_priv, > ALPM_CTL(cpu_transcoder)) & ALPM_CTL_LOBF_ENABLE; > + seq_printf(m, "LOBF status: %s\n", > str_enabled_disabled(lobf_enabled)); > + > +out: > + drm_modeset_unlock(_priv- > >drm.mode_config.connection_mutex); > + > + return ret; > +} > + > +DEFINE_SHOW_ATTRIBUTE(i915_edp_lobf_info); > + > +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector) > +{ > + struct drm_i915_private *i915 = to_i915(connector->base.dev); > + struct dentry *root = connector->base.debugfs_entry; > + > + if (DISPLAY_VER(i915) < 20 || > + connector->base.connector_type != DRM_MODE_CONNECTOR_eDP) > + return; > + > + debugfs_create_file("i915_edp_lobf_info", 0444, root, > + connector, _edp_lobf_info_fops); > +} > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h > b/drivers/gpu/drm/i915/display/intel_alpm.h > index a9ca190da3e4..01fd08eb96f5 100644 > --- a/drivers/gpu/drm/i915/display/intel_alpm.h > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h > @@ -11,6 +11,7 @@ > struct intel_dp; > struct intel_crtc_state; > struct drm_connector_state; > +struct intel_connector; > > bool intel_alpm_get_aux_less_status(struct intel_dp *intel_dp); > bool intel_alpm_compute_params(struct intel_dp *intel_dp, > @@ -20,4 +21,5 @@ void intel_alpm_compute_lobf_config(struct intel_dp > *intel_dp, > struct drm_connector_state > *conn_state); > void intel_alpm_configure(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > +void intel_alpm_lobf_debugfs_add(struct intel_connector *connector); > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > index 35f9f86ef70f..86d9900c40af 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > @@ -13,6 +13,7 @@ > #include "i915_debugfs.h" > #include "i915_irq.h" > #include "i915_reg.h" > +#include "intel_alpm.h" > #include "intel_crtc.h" > #include "intel_de.h" > #include "intel_crtc_state_dump.h" > @@ -1515,6 +1516,7 @@ void intel_connector_debugfs_add(struct > intel_connector *connector) > intel_drrs_connector_debugfs_add(connector); > intel_pps_connector_debugfs_add(connector); > intel_psr_connector_debugfs_add(connector); > + intel_alpm_lobf_debugfs_add(connector); > > if (connector_type == DRM_MODE_CONNECTOR_DisplayPort || > connector_type == DRM_MODE_CONNECTOR_HDMIA ||