| Message ID | 20220329060731.785476-1-bhanuprakash.modem@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display/debugfs: Add connector debugfs for "output_bpc" | expand |
On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: > This new debugfs will expose the connector's max supported bpc > and the bpc currently using. It is very useful for verifying > whether we enter the correct output color depth from IGT. > > Example: > cat /sys/kernel/debug/dri/0/DP-1/output_bpc > Current: 8 > Maximum: 10 > > V2: Add connector's max bpc to i915_display_info > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Swati Sharma <swati2.sharma@intel.com> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > --- > .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > index c1e74a13a0828..694d27f3b109c 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, > seq_puts(m, "\tHDCP version: "); > intel_hdcp_info(m, intel_connector); > > + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); > + > intel_panel_info(m, intel_connector); > > seq_printf(m, "\tmodes:\n"); > @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { > .write = i915_dsc_bpp_write > }; > > +/* > + * Returns the maximum output bpc for the connector. > + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc > + */ > +static int output_bpc_show(struct seq_file *m, void *data) > +{ > + struct drm_connector *connector = m->private; > + struct drm_device *dev = connector->dev; > + struct drm_crtc *crtc; > + struct intel_crtc_state *crtc_state; > + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); > + int res; > + > + if (!encoder) > + return -ENODEV; > + > + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); > + if (res) > + return res; > + > + crtc = connector->state->crtc; > + if (connector->status != connector_status_connected || !crtc) { > + res = -ENODEV; > + goto unlock; > + } > + > + crtc_state = to_intel_crtc_state(crtc->state); > + if (!crtc_state->hw.active) > + goto unlock; > + > + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); > + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); > + res = 0; > + > +unlock: > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > + return res; > +} > +DEFINE_SHOW_ATTRIBUTE(output_bpc); Looks like an excessive amount of code for a single value. BR, Jani. > + > /** > * intel_connector_debugfs_add - add i915 specific connector debugfs files > * @connector: pointer to a registered drm_connector > @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) > connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > debugfs_create_file("i915_lpsp_capability", 0444, root, > connector, &i915_lpsp_capability_fops); > + > + debugfs_create_file("output_bpc", 0444, root, > + connector, &output_bpc_fops); > } > > /**
On Fri-01-04-2022 06:10 pm, Jani Nikula wrote: > On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >> This new debugfs will expose the connector's max supported bpc >> and the bpc currently using. It is very useful for verifying >> whether we enter the correct output color depth from IGT. >> >> Example: >> cat /sys/kernel/debug/dri/0/DP-1/output_bpc >> Current: 8 >> Maximum: 10 >> >> V2: Add connector's max bpc to i915_display_info >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Uma Shankar <uma.shankar@intel.com> >> Cc: Swati Sharma <swati2.sharma@intel.com> >> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> >> --- >> .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> index c1e74a13a0828..694d27f3b109c 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, >> seq_puts(m, "\tHDCP version: "); >> intel_hdcp_info(m, intel_connector); >> >> + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); >> + >> intel_panel_info(m, intel_connector); >> >> seq_printf(m, "\tmodes:\n"); >> @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { >> .write = i915_dsc_bpp_write >> }; >> >> +/* >> + * Returns the maximum output bpc for the connector. >> + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc >> + */ >> +static int output_bpc_show(struct seq_file *m, void *data) >> +{ >> + struct drm_connector *connector = m->private; >> + struct drm_device *dev = connector->dev; >> + struct drm_crtc *crtc; >> + struct intel_crtc_state *crtc_state; >> + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); >> + int res; >> + >> + if (!encoder) >> + return -ENODEV; >> + >> + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); >> + if (res) >> + return res; >> + >> + crtc = connector->state->crtc; >> + if (connector->status != connector_status_connected || !crtc) { >> + res = -ENODEV; >> + goto unlock; >> + } >> + >> + crtc_state = to_intel_crtc_state(crtc->state); >> + if (!crtc_state->hw.active) >> + goto unlock; >> + >> + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); >> + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); >> + res = 0; >> + >> +unlock: >> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >> + >> + return res; >> +} >> +DEFINE_SHOW_ATTRIBUTE(output_bpc); > > Looks like an excessive amount of code for a single value. Yeah, but these values are very helpful in many IGT tests like kms_color, kms_hdr, kms_dither, kms_dsc etc.. Otherwise IGT needs to request the kernel to get the EDID, and parse that EDID to get the "Maximum" value which is redundant (Kernel is already doing the same) and not recommended in IGT. And there is no way to get the "Current" value except reading it from i915_display_info which is again not recommended in IGT (As i915_display_info is Intel specific). This debugfs is already introduced & using by AMD: https://patchwork.freedesktop.org/patch/308586 - Bhanu > > BR, > Jani. > >> + >> /** >> * intel_connector_debugfs_add - add i915 specific connector debugfs files >> * @connector: pointer to a registered drm_connector >> @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) >> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) >> debugfs_create_file("i915_lpsp_capability", 0444, root, >> connector, &i915_lpsp_capability_fops); >> + >> + debugfs_create_file("output_bpc", 0444, root, >> + connector, &output_bpc_fops); >> } >> >> /** >
On Mon, 04 Apr 2022, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com> wrote: > On Fri-01-04-2022 06:10 pm, Jani Nikula wrote: >> On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >>> This new debugfs will expose the connector's max supported bpc >>> and the bpc currently using. It is very useful for verifying >>> whether we enter the correct output color depth from IGT. >>> >>> Example: >>> cat /sys/kernel/debug/dri/0/DP-1/output_bpc >>> Current: 8 >>> Maximum: 10 >>> >>> V2: Add connector's max bpc to i915_display_info >>> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Uma Shankar <uma.shankar@intel.com> >>> Cc: Swati Sharma <swati2.sharma@intel.com> >>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> >>> --- >>> .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ >>> 1 file changed, 46 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>> index c1e74a13a0828..694d27f3b109c 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>> @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, >>> seq_puts(m, "\tHDCP version: "); >>> intel_hdcp_info(m, intel_connector); >>> >>> + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); >>> + >>> intel_panel_info(m, intel_connector); >>> >>> seq_printf(m, "\tmodes:\n"); >>> @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { >>> .write = i915_dsc_bpp_write >>> }; >>> >>> +/* >>> + * Returns the maximum output bpc for the connector. >>> + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc >>> + */ >>> +static int output_bpc_show(struct seq_file *m, void *data) >>> +{ >>> + struct drm_connector *connector = m->private; >>> + struct drm_device *dev = connector->dev; >>> + struct drm_crtc *crtc; >>> + struct intel_crtc_state *crtc_state; >>> + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); >>> + int res; >>> + >>> + if (!encoder) >>> + return -ENODEV; >>> + >>> + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); >>> + if (res) >>> + return res; >>> + >>> + crtc = connector->state->crtc; >>> + if (connector->status != connector_status_connected || !crtc) { >>> + res = -ENODEV; >>> + goto unlock; >>> + } >>> + >>> + crtc_state = to_intel_crtc_state(crtc->state); >>> + if (!crtc_state->hw.active) >>> + goto unlock; >>> + >>> + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); >>> + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); >>> + res = 0; >>> + >>> +unlock: >>> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >>> + >>> + return res; >>> +} >>> +DEFINE_SHOW_ATTRIBUTE(output_bpc); >> >> Looks like an excessive amount of code for a single value. > > Yeah, but these values are very helpful in many IGT tests like > kms_color, kms_hdr, kms_dither, kms_dsc etc.. > > Otherwise IGT needs to request the kernel to get the EDID, and parse > that EDID to get the "Maximum" value which is redundant (Kernel is > already doing the same) and not recommended in IGT. > > And there is no way to get the "Current" value except reading it from > i915_display_info which is again not recommended in IGT (As > i915_display_info is Intel specific). Note how we have intel_connector_debugfs_add() for connector debugfs and intel_crtc_debugfs_add() for crtc debugfs, and how this patch just mixes up the two by looking up crtc and state from the connector debugfs. > This debugfs is already introduced & using by AMD: > https://patchwork.freedesktop.org/patch/308586 Wait, what? Both amd and i915 adding the name "output_bpc" is *not* the way to roll. We only add i915_ prefixed debugfs files from i915.ko. If you need this to be a standard interface across drivers, IMO it should be added in common drm code, not sprinkled around in drivers. I see that amd is already using this in what is basically drm core debugfs namespace, and there's amd specific IGT built on top. Adding a bunch of Cc's to get some clarity on drm debugfs naming and usage. BR, Jani. > > - Bhanu > >> >> BR, >> Jani. >> >>> + >>> /** >>> * intel_connector_debugfs_add - add i915 specific connector debugfs files >>> * @connector: pointer to a registered drm_connector >>> @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) >>> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) >>> debugfs_create_file("i915_lpsp_capability", 0444, root, >>> connector, &i915_lpsp_capability_fops); >>> + >>> + debugfs_create_file("output_bpc", 0444, root, >>> + connector, &output_bpc_fops); >>> } >>> >>> /** >> >
On Mon-04-04-2022 04:16 pm, Jani Nikula wrote: > On Mon, 04 Apr 2022, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com> wrote: >> On Fri-01-04-2022 06:10 pm, Jani Nikula wrote: >>> On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >>>> This new debugfs will expose the connector's max supported bpc >>>> and the bpc currently using. It is very useful for verifying >>>> whether we enter the correct output color depth from IGT. >>>> >>>> Example: >>>> cat /sys/kernel/debug/dri/0/DP-1/output_bpc >>>> Current: 8 >>>> Maximum: 10 >>>> >>>> V2: Add connector's max bpc to i915_display_info >>>> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> Cc: Uma Shankar <uma.shankar@intel.com> >>>> Cc: Swati Sharma <swati2.sharma@intel.com> >>>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> >>>> --- >>>> .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ >>>> 1 file changed, 46 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>>> index c1e74a13a0828..694d27f3b109c 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>>> @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, >>>> seq_puts(m, "\tHDCP version: "); >>>> intel_hdcp_info(m, intel_connector); >>>> >>>> + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); >>>> + >>>> intel_panel_info(m, intel_connector); >>>> >>>> seq_printf(m, "\tmodes:\n"); >>>> @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { >>>> .write = i915_dsc_bpp_write >>>> }; >>>> >>>> +/* >>>> + * Returns the maximum output bpc for the connector. >>>> + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc >>>> + */ >>>> +static int output_bpc_show(struct seq_file *m, void *data) >>>> +{ >>>> + struct drm_connector *connector = m->private; >>>> + struct drm_device *dev = connector->dev; >>>> + struct drm_crtc *crtc; >>>> + struct intel_crtc_state *crtc_state; >>>> + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); >>>> + int res; >>>> + >>>> + if (!encoder) >>>> + return -ENODEV; >>>> + >>>> + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); >>>> + if (res) >>>> + return res; >>>> + >>>> + crtc = connector->state->crtc; >>>> + if (connector->status != connector_status_connected || !crtc) { >>>> + res = -ENODEV; >>>> + goto unlock; >>>> + } >>>> + >>>> + crtc_state = to_intel_crtc_state(crtc->state); >>>> + if (!crtc_state->hw.active) >>>> + goto unlock; >>>> + >>>> + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); >>>> + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); >>>> + res = 0; >>>> + >>>> +unlock: >>>> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >>>> + >>>> + return res; >>>> +} >>>> +DEFINE_SHOW_ATTRIBUTE(output_bpc); >>> >>> Looks like an excessive amount of code for a single value. >> >> Yeah, but these values are very helpful in many IGT tests like >> kms_color, kms_hdr, kms_dither, kms_dsc etc.. >> >> Otherwise IGT needs to request the kernel to get the EDID, and parse >> that EDID to get the "Maximum" value which is redundant (Kernel is >> already doing the same) and not recommended in IGT. >> >> And there is no way to get the "Current" value except reading it from >> i915_display_info which is again not recommended in IGT (As >> i915_display_info is Intel specific). > > Note how we have intel_connector_debugfs_add() for connector debugfs and > intel_crtc_debugfs_add() for crtc debugfs, and how this patch just mixes > up the two by looking up crtc and state from the connector debugfs. I just tried to adopt from existing AMD's implementation, and it may be reduce the number of debugfs nodes. "Maximum" is from connector's display_info (Needs connector debugfs) "Current" is from crtc state (Needs crtc debugfs) > >> This debugfs is already introduced & using by AMD: >> https://patchwork.freedesktop.org/patch/308586 > > Wait, what? > > Both amd and i915 adding the name "output_bpc" is *not* the way to > roll. We only add i915_ prefixed debugfs files from i915.ko. > > If you need this to be a standard interface across drivers, IMO it > should be added in common drm code, not sprinkled around in drivers. As display_info is the member of drm_connector, perhaps we can move "Maximum" value to drm layer and can use common name. Example: /sys/kernel/debug/dri/0/DP-1/max_bpc And each hardware specific driver could create a crtc debugfs node for "Current" value in their name space. Example: /sys/kernel/debug/dri/0/crtc-0/i915_current_bpc Please suggest. - Bhanu > > I see that amd is already using this in what is basically drm core > debugfs namespace, and there's amd specific IGT built on top. > > Adding a bunch of Cc's to get some clarity on drm debugfs naming and > usage. > > > BR, > Jani. > > >> >> - Bhanu >> >>> >>> BR, >>> Jani. >>> >>>> + >>>> /** >>>> * intel_connector_debugfs_add - add i915 specific connector debugfs files >>>> * @connector: pointer to a registered drm_connector >>>> @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) >>>> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) >>>> debugfs_create_file("i915_lpsp_capability", 0444, root, >>>> connector, &i915_lpsp_capability_fops); >>>> + >>>> + debugfs_create_file("output_bpc", 0444, root, >>>> + connector, &output_bpc_fops); >>>> } >>>> >>>> /** >>> >> >
On Mon, Apr 04, 2022 at 01:46:23PM +0300, Jani Nikula wrote: > On Mon, 04 Apr 2022, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com> wrote: > > On Fri-01-04-2022 06:10 pm, Jani Nikula wrote: > >> On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: > >>> This new debugfs will expose the connector's max supported bpc > >>> and the bpc currently using. It is very useful for verifying > >>> whether we enter the correct output color depth from IGT. > >>> > >>> Example: > >>> cat /sys/kernel/debug/dri/0/DP-1/output_bpc > >>> Current: 8 > >>> Maximum: 10 > >>> > >>> V2: Add connector's max bpc to i915_display_info > >>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>> Cc: Uma Shankar <uma.shankar@intel.com> > >>> Cc: Swati Sharma <swati2.sharma@intel.com> > >>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > >>> --- > >>> .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ > >>> 1 file changed, 46 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > >>> index c1e74a13a0828..694d27f3b109c 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > >>> @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, > >>> seq_puts(m, "\tHDCP version: "); > >>> intel_hdcp_info(m, intel_connector); > >>> > >>> + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); > >>> + > >>> intel_panel_info(m, intel_connector); > >>> > >>> seq_printf(m, "\tmodes:\n"); > >>> @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { > >>> .write = i915_dsc_bpp_write > >>> }; > >>> > >>> +/* > >>> + * Returns the maximum output bpc for the connector. > >>> + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc > >>> + */ > >>> +static int output_bpc_show(struct seq_file *m, void *data) > >>> +{ > >>> + struct drm_connector *connector = m->private; > >>> + struct drm_device *dev = connector->dev; > >>> + struct drm_crtc *crtc; > >>> + struct intel_crtc_state *crtc_state; > >>> + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); > >>> + int res; > >>> + > >>> + if (!encoder) > >>> + return -ENODEV; > >>> + > >>> + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); > >>> + if (res) > >>> + return res; > >>> + > >>> + crtc = connector->state->crtc; > >>> + if (connector->status != connector_status_connected || !crtc) { > >>> + res = -ENODEV; > >>> + goto unlock; > >>> + } > >>> + > >>> + crtc_state = to_intel_crtc_state(crtc->state); > >>> + if (!crtc_state->hw.active) > >>> + goto unlock; > >>> + > >>> + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); > >>> + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); > >>> + res = 0; > >>> + > >>> +unlock: > >>> + drm_modeset_unlock(&dev->mode_config.connection_mutex); > >>> + > >>> + return res; > >>> +} > >>> +DEFINE_SHOW_ATTRIBUTE(output_bpc); > >> > >> Looks like an excessive amount of code for a single value. > > > > Yeah, but these values are very helpful in many IGT tests like > > kms_color, kms_hdr, kms_dither, kms_dsc etc.. > > > > Otherwise IGT needs to request the kernel to get the EDID, and parse > > that EDID to get the "Maximum" value which is redundant (Kernel is > > already doing the same) and not recommended in IGT. > > > > And there is no way to get the "Current" value except reading it from > > i915_display_info which is again not recommended in IGT (As > > i915_display_info is Intel specific). > > Note how we have intel_connector_debugfs_add() for connector debugfs and > intel_crtc_debugfs_add() for crtc debugfs, and how this patch just mixes > up the two by looking up crtc and state from the connector debugfs. > > > This debugfs is already introduced & using by AMD: > > https://patchwork.freedesktop.org/patch/308586 > > Wait, what? > > Both amd and i915 adding the name "output_bpc" is *not* the way to > roll. We only add i915_ prefixed debugfs files from i915.ko. Yeah vendor prefix would be nice, but it's debugfs so we can always fix it. Also would be really good to pull that output_bpc into drm core if it's something standard that igts need in general, so ideally we don't just standardize the drm side, but also the testcases that need this and make them generic to run on any kms driver. -Daniel > > If you need this to be a standard interface across drivers, IMO it > should be added in common drm code, not sprinkled around in drivers. > > I see that amd is already using this in what is basically drm core > debugfs namespace, and there's amd specific IGT built on top. > > Adding a bunch of Cc's to get some clarity on drm debugfs naming and > usage. > > > BR, > Jani. > > > > > > - Bhanu > > > >> > >> BR, > >> Jani. > >> > >>> + > >>> /** > >>> * intel_connector_debugfs_add - add i915 specific connector debugfs files > >>> * @connector: pointer to a registered drm_connector > >>> @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) > >>> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > >>> debugfs_create_file("i915_lpsp_capability", 0444, root, > >>> connector, &i915_lpsp_capability_fops); > >>> + > >>> + debugfs_create_file("output_bpc", 0444, root, > >>> + connector, &output_bpc_fops); > >>> } > >>> > >>> /** > >> > > > > -- > Jani Nikula, Intel Open Source Graphics Center
On Mon-04-04-2022 09:11 pm, Daniel Vetter wrote: > On Mon, Apr 04, 2022 at 01:46:23PM +0300, Jani Nikula wrote: >> On Mon, 04 Apr 2022, "Modem, Bhanuprakash" <bhanuprakash.modem@intel.com> wrote: >>> On Fri-01-04-2022 06:10 pm, Jani Nikula wrote: >>>> On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@intel.com> wrote: >>>>> This new debugfs will expose the connector's max supported bpc >>>>> and the bpc currently using. It is very useful for verifying >>>>> whether we enter the correct output color depth from IGT. >>>>> >>>>> Example: >>>>> cat /sys/kernel/debug/dri/0/DP-1/output_bpc >>>>> Current: 8 >>>>> Maximum: 10 >>>>> >>>>> V2: Add connector's max bpc to i915_display_info >>>>> >>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>>> Cc: Uma Shankar <uma.shankar@intel.com> >>>>> Cc: Swati Sharma <swati2.sharma@intel.com> >>>>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> >>>>> --- >>>>> .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ >>>>> 1 file changed, 46 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>>>> index c1e74a13a0828..694d27f3b109c 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >>>>> @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, >>>>> seq_puts(m, "\tHDCP version: "); >>>>> intel_hdcp_info(m, intel_connector); >>>>> >>>>> + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); >>>>> + >>>>> intel_panel_info(m, intel_connector); >>>>> >>>>> seq_printf(m, "\tmodes:\n"); >>>>> @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { >>>>> .write = i915_dsc_bpp_write >>>>> }; >>>>> >>>>> +/* >>>>> + * Returns the maximum output bpc for the connector. >>>>> + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc >>>>> + */ >>>>> +static int output_bpc_show(struct seq_file *m, void *data) >>>>> +{ >>>>> + struct drm_connector *connector = m->private; >>>>> + struct drm_device *dev = connector->dev; >>>>> + struct drm_crtc *crtc; >>>>> + struct intel_crtc_state *crtc_state; >>>>> + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); >>>>> + int res; >>>>> + >>>>> + if (!encoder) >>>>> + return -ENODEV; >>>>> + >>>>> + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); >>>>> + if (res) >>>>> + return res; >>>>> + >>>>> + crtc = connector->state->crtc; >>>>> + if (connector->status != connector_status_connected || !crtc) { >>>>> + res = -ENODEV; >>>>> + goto unlock; >>>>> + } >>>>> + >>>>> + crtc_state = to_intel_crtc_state(crtc->state); >>>>> + if (!crtc_state->hw.active) >>>>> + goto unlock; >>>>> + >>>>> + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); >>>>> + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); >>>>> + res = 0; >>>>> + >>>>> +unlock: >>>>> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >>>>> + >>>>> + return res; >>>>> +} >>>>> +DEFINE_SHOW_ATTRIBUTE(output_bpc); >>>> >>>> Looks like an excessive amount of code for a single value. >>> >>> Yeah, but these values are very helpful in many IGT tests like >>> kms_color, kms_hdr, kms_dither, kms_dsc etc.. >>> >>> Otherwise IGT needs to request the kernel to get the EDID, and parse >>> that EDID to get the "Maximum" value which is redundant (Kernel is >>> already doing the same) and not recommended in IGT. >>> >>> And there is no way to get the "Current" value except reading it from >>> i915_display_info which is again not recommended in IGT (As >>> i915_display_info is Intel specific). >> >> Note how we have intel_connector_debugfs_add() for connector debugfs and >> intel_crtc_debugfs_add() for crtc debugfs, and how this patch just mixes >> up the two by looking up crtc and state from the connector debugfs. >> >>> This debugfs is already introduced & using by AMD: >>> https://patchwork.freedesktop.org/patch/308586 >> >> Wait, what? >> >> Both amd and i915 adding the name "output_bpc" is *not* the way to >> roll. We only add i915_ prefixed debugfs files from i915.ko. > > Yeah vendor prefix would be nice, but it's debugfs so we can always fix > it. > > Also would be really good to pull that output_bpc into drm core if it's > something standard that igts need in general, so ideally we don't just > standardize the drm side, but also the testcases that need this and make > them generic to run on any kms driver. I made the required changes in both IGT & Kernel and floated to ML, please help to review. IGT: https://patchwork.freedesktop.org/series/102387/ Kernel: https://patchwork.freedesktop.org/series/102390/ - Bhanu > -Daniel > >> >> If you need this to be a standard interface across drivers, IMO it >> should be added in common drm code, not sprinkled around in drivers. >> >> I see that amd is already using this in what is basically drm core >> debugfs namespace, and there's amd specific IGT built on top. >> >> Adding a bunch of Cc's to get some clarity on drm debugfs naming and >> usage. >> >> >> BR, >> Jani. >> >> >>> >>> - Bhanu >>> >>>> >>>> BR, >>>> Jani. >>>> >>>>> + >>>>> /** >>>>> * intel_connector_debugfs_add - add i915 specific connector debugfs files >>>>> * @connector: pointer to a registered drm_connector >>>>> @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) >>>>> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) >>>>> debugfs_create_file("i915_lpsp_capability", 0444, root, >>>>> connector, &i915_lpsp_capability_fops); >>>>> + >>>>> + debugfs_create_file("output_bpc", 0444, root, >>>>> + connector, &output_bpc_fops); >>>>> } >>>>> >>>>> /** >>>> >>> >> >> -- >> Jani Nikula, Intel Open Source Graphics Center >
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index c1e74a13a0828..694d27f3b109c 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c @@ -663,6 +663,8 @@ static void intel_connector_info(struct seq_file *m, seq_puts(m, "\tHDCP version: "); intel_hdcp_info(m, intel_connector); + seq_printf(m, "\tmax bpc: %u\n", connector->display_info.bpc); + intel_panel_info(m, intel_connector); seq_printf(m, "\tmodes:\n"); @@ -2275,6 +2277,47 @@ static const struct file_operations i915_dsc_bpp_fops = { .write = i915_dsc_bpp_write }; +/* + * Returns the maximum output bpc for the connector. + * Example usage: cat /sys/kernel/debug/dri/0/DP-1/output_bpc + */ +static int output_bpc_show(struct seq_file *m, void *data) +{ + struct drm_connector *connector = m->private; + struct drm_device *dev = connector->dev; + struct drm_crtc *crtc; + struct intel_crtc_state *crtc_state; + struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector)); + int res; + + if (!encoder) + return -ENODEV; + + res = drm_modeset_lock_single_interruptible(&dev->mode_config.connection_mutex); + if (res) + return res; + + crtc = connector->state->crtc; + if (connector->status != connector_status_connected || !crtc) { + res = -ENODEV; + goto unlock; + } + + crtc_state = to_intel_crtc_state(crtc->state); + if (!crtc_state->hw.active) + goto unlock; + + seq_printf(m, "Current: %u\n", crtc_state->pipe_bpp / 3); + seq_printf(m, "Maximum: %u\n", connector->display_info.bpc); + res = 0; + +unlock: + drm_modeset_unlock(&dev->mode_config.connection_mutex); + + return res; +} +DEFINE_SHOW_ATTRIBUTE(output_bpc); + /** * intel_connector_debugfs_add - add i915 specific connector debugfs files * @connector: pointer to a registered drm_connector @@ -2330,6 +2373,9 @@ void intel_connector_debugfs_add(struct intel_connector *intel_connector) connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) debugfs_create_file("i915_lpsp_capability", 0444, root, connector, &i915_lpsp_capability_fops); + + debugfs_create_file("output_bpc", 0444, root, + connector, &output_bpc_fops); } /**
This new debugfs will expose the connector's max supported bpc and the bpc currently using. It is very useful for verifying whether we enter the correct output color depth from IGT. Example: cat /sys/kernel/debug/dri/0/DP-1/output_bpc Current: 8 Maximum: 10 V2: Add connector's max bpc to i915_display_info Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Uma Shankar <uma.shankar@intel.com> Cc: Swati Sharma <swati2.sharma@intel.com> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com> --- .../drm/i915/display/intel_display_debugfs.c | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+)