Re: [Intel-gfx] [PATCH] drm/i915/display/debugfs: Add connector debugfs for "output_bpc"

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

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
>