Re: [Intel-gfx] [PATCH 2/8] drm/i915/sdvo: Setup DDC fully before output init

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 26 Oct 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Call intel_sdvo_select_ddc_bus() before initializing any
> of the outputs. And before that is functional (assuming no VBT)
> we have to set up the controlled_outputs thing. Otherwise DDC
> won't be functional during the output init but LVDS really
> needs it for the fixed mode setup.
>
> Note that the whole multi output support still looks very
> bogus, and more work will be needed to make it correct.
> But for now this should at least fix the LVDS EDID fixed mode
> setup.
>
> Cc: stable@vger.kernel.org
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7301
> Fixes: aa2b88074a56 ("drm/i915/sdvo: Fix multi function encoder stuff")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_sdvo.c | 31 +++++++++--------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index c6200a91a777..ccf81d616cb4 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2746,13 +2746,10 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  	if (!intel_sdvo_connector)
>  		return false;
>  
> -	if (device == 0) {
> -		intel_sdvo->controlled_output |= SDVO_OUTPUT_TMDS0;
> +	if (device == 0)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_TMDS0;
> -	} else if (device == 1) {
> -		intel_sdvo->controlled_output |= SDVO_OUTPUT_TMDS1;
> +	else if (device == 1)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_TMDS1;
> -	}
>  
>  	intel_connector = &intel_sdvo_connector->base;
>  	connector = &intel_connector->base;
> @@ -2807,7 +2804,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
>  	encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
>  	connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
>  
> -	intel_sdvo->controlled_output |= type;
>  	intel_sdvo_connector->output_flag = type;
>  
>  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> @@ -2848,13 +2844,10 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
>  	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
>  	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
>  
> -	if (device == 0) {
> -		intel_sdvo->controlled_output |= SDVO_OUTPUT_RGB0;
> +	if (device == 0)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB0;
> -	} else if (device == 1) {
> -		intel_sdvo->controlled_output |= SDVO_OUTPUT_RGB1;
> +	else if (device == 1)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
> -	}
>  
>  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
>  		kfree(intel_sdvo_connector);
> @@ -2884,13 +2877,10 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
>  	encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
>  	connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
>  
> -	if (device == 0) {
> -		intel_sdvo->controlled_output |= SDVO_OUTPUT_LVDS0;
> +	if (device == 0)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS0;
> -	} else if (device == 1) {
> -		intel_sdvo->controlled_output |= SDVO_OUTPUT_LVDS1;
> +	else if (device == 1)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
> -	}
>  
>  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
>  		kfree(intel_sdvo_connector);
> @@ -2945,8 +2935,14 @@ static u16 intel_sdvo_filter_output_flags(u16 flags)
>  static bool
>  intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, u16 flags)
>  {
> +	struct drm_i915_private *i915 = to_i915(intel_sdvo->base.base.dev);
> +
>  	flags = intel_sdvo_filter_output_flags(flags);
>  
> +	intel_sdvo->controlled_output = flags;
> +
> +	intel_sdvo_select_ddc_bus(i915, intel_sdvo);

AFAICT the ->controlled_outputs member could now be removed and just
passed by value here.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +
>  	if (flags & SDVO_OUTPUT_TMDS0)
>  		if (!intel_sdvo_dvi_init(intel_sdvo, 0))
>  			return false;
> @@ -2987,7 +2983,6 @@ intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, u16 flags)
>  	if (flags == 0) {
>  		unsigned char bytes[2];
>  
> -		intel_sdvo->controlled_output = 0;
>  		memcpy(bytes, &intel_sdvo->caps.output_flags, 2);
>  		DRM_DEBUG_KMS("%s: Unknown SDVO output type (0x%02x%02x)\n",
>  			      SDVO_NAME(intel_sdvo),
> @@ -3399,8 +3394,6 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>  	 */
>  	intel_sdvo->base.cloneable = 0;
>  
> -	intel_sdvo_select_ddc_bus(dev_priv, intel_sdvo);
> -
>  	/* Set the input timing to the screen. Assume always input 0. */
>  	if (!intel_sdvo_set_target_input(intel_sdvo))
>  		goto err_output;

-- 
Jani Nikula, Intel Open Source Graphics Center