On Mon, 04 Apr 2022, "Modem, Bhanuprakash" <bhanuprakash.modem@xxxxxxxxx> wrote:
> On Fri-01-04-2022 06:10 pm, Jani Nikula wrote:
>> On Tue, 29 Mar 2022, Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>> Cc: Uma Shankar <uma.shankar@xxxxxxxxx>
>>> Cc: Swati Sharma <swati2.sharma@xxxxxxxxx>
>>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx>
>>> ---
>>> .../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);
>>> }
>>>
>>> /**
>>
>
--
Jani Nikula, Intel Open Source Graphics Center
