[v2] drm/i915/hdmi: add debugfs to contorl HDMI bpc

[Lee, Shawn C]

On Fri, Aug 15, 2025 at 09:35:00AM +0000, Imre Deak C wrote:
>On Mon, Aug 11, 2025 at 09:46:55AM +0300, Lee, Shawn C wrote:
>> On Fri, Aug 08, 2025 at 09:40:00AM +0000, Imre Deak C wrote:
>> >On Fri, Aug 08, 2025 at 09:16:02AM +0000, Lee Shawn C wrote:
>> >> While performing HDMI compliance testing, test equipment may 
>> >> request different bpc output for signal measurement. However, 
>> >> display driver typically determines the maximum available bpc based on HW bandwidth.
>> >> 
>> >> Introduce a new debugfs that allows user to configure dedicated bpc 
>> >> manually, and making HDMI compliance test much easier.
>> >> 
>> >> v2: Using exist variable max_requested_bpc.
>> >
>> > How come this doesn't get reset after a hotplug as you described for 
>> > the case when the property is used, even though both the property 
>> > and this debug entries use the same state variable? (Not saying that 
>> > the reset happing after a hotplug is a valid justification for a new 
>> > debugfs entry, the hotplug could be also handled by the user, but 
>> > you could argue the debugfs entry is more convenient.)
>> 
>> Hi Imre, thank you for the prompt response.
>> 
>> https://elixir.bootlin.com/linux/v6.16/source/drivers/gpu/drm/i915/dis
>> play/intel_hdmi.c#L2672
>>
>> The max_bpc_property and max_bpc values are initialized during 
>> connector initialization process.
>> 
>> https://elixir.bootlin.com/linux/v6.16/source/drivers/gpu/drm/drm_atom
>> ic.c#L468
>>
>> The max_bpc will be restored to info->bpc (from EDID) at
>> drm_atomic_connector_check() everytime.
>> 
>> When max_bpc_property is available, this funciton also compares 
>> max_bpc with max_requested_bpc and updates max_bpc to the smaller of 
>> the two values.
>> 
>> The i915 display driver then relies on this max_bpc value to determine 
>> whether to update the pipe bpp value in compute_sink_pipe_bpp().
>> Therefore, we can simply update max_requested_bpc to affect pipe bpp 
>> output. And no additional driver changes are required.
>
>The point above was to figure out how the "max bpc" connector property gets reset after a hotplug, based on your earlier explanation for the reason to add a debugfs entry instead. But based on the above and also based on my tests, that connector property actually keeps its value across hotplugs.
>
>So I think that the property could be used for your tests then, or if you wanted to have a debugfs entry for convenience, then the intel_force_link_bpp debugfs entry could be added for all HDMI connectors, as suggested earlier, instead of adding a new debugfs entry.
>The following would do that, could you give it a go?

Hi Imre,

Thanks! Your change works well. And we don't need to create new debugfs to force HDMI bpp. 

Best regards,
Shawn

>
>diff --git a/drivers/gpu/drm/i915/display/g4x_hdmi.c b/drivers/gpu/drm/i915/display/g4x_hdmi.c
>index 108ebd97f9e4..b31fb1e4bc1a 100644
>--- a/drivers/gpu/drm/i915/display/g4x_hdmi.c
>+++ b/drivers/gpu/drm/i915/display/g4x_hdmi.c
>@@ -136,11 +136,8 @@ static int g4x_hdmi_compute_config(struct intel_encoder *encoder,
> 	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>
>-	if (HAS_PCH_SPLIT(display)) {
>+	if (HAS_PCH_SPLIT(display))
> 		crtc_state->has_pch_encoder = true;
>-		if (!intel_fdi_compute_pipe_bpp(crtc_state))
>-			return -EINVAL;
>-	}
>
> 	if (display->platform.g4x)
> 		crtc_state->has_hdmi_sink = g4x_compute_has_hdmi_sink(state, crtc); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
>index cbee628eb26b..027e8ed0cea8 100644
>--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>@@ -55,6 +55,7 @@
> #include "intel_display_regs.h"
> #include "intel_display_types.h"
> #include "intel_dp.h"
>+#include "intel_fdi.h"
> #include "intel_gmbus.h"
> #include "intel_hdcp.h"
> #include "intel_hdcp_regs.h"
>@@ -2345,6 +2346,9 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
> 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> 		pipe_config->pixel_multiplier = 2;
>
>+	if (!intel_fdi_compute_pipe_bpp(pipe_config))
>+		return -EINVAL;
>+
> 	pipe_config->has_audio =
> 		intel_hdmi_has_audio(encoder, pipe_config, conn_state) &&
> 		intel_audio_compute_config(encoder, pipe_config, conn_state); diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.c b/drivers/gpu/drm/i915/display/intel_link_bw.c
>index 3caef7f9c7c4..9e8220d86a4a 100644
>--- a/drivers/gpu/drm/i915/display/intel_link_bw.c
>+++ b/drivers/gpu/drm/i915/display/intel_link_bw.c
>@@ -449,6 +449,7 @@ void intel_link_bw_connector_debugfs_add(struct intel_connector *connector)
> 	switch (connector->base.connector_type) {
> 	case DRM_MODE_CONNECTOR_DisplayPort:
> 	case DRM_MODE_CONNECTOR_eDP:
>+	case DRM_MODE_CONNECTOR_HDMIA:
> 		break;
> 	case DRM_MODE_CONNECTOR_VGA:
> 	case DRM_MODE_CONNECTOR_SVIDEO:
>@@ -456,13 +457,6 @@ void intel_link_bw_connector_debugfs_add(struct intel_connector *connector)
> 	case DRM_MODE_CONNECTOR_DVID:
> 		if (HAS_FDI(display))
> 			break;
>-
>-		return;
>-	case DRM_MODE_CONNECTOR_HDMIA:
>-		if (HAS_FDI(display) && !HAS_DDI(display))
>-			break;
>-
>-		return;
> 	default:
> 		return;
> 	}
>