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

[Ville Syrjälä]

On Thu, Oct 27, 2022 at 05:49:53PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 27, 2022 at 05:36:24PM +0300, Jani Nikula wrote:
> > On Wed, 26 Oct 2022, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> > > From: Ville Syrjälä <ville.syrjala at 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 at 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 at 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.
> 
> Hmm. True. Though the whole thing is utter garbage anyway.
> intel_sdvo_guess_ddc_bus() really expects controlled_outputs
> to contain only a single bit. I guess it kinda works by luck
> most or the time, at least for single output devices.
> I suppose I can still include the controlled_outputs nukage
> into this patch.

On second though I think I'll do it as a followup. Less chance
of backport conflicts that way.

-- 
Ville Syrjälä
Intel