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

[Jani Nikula]

On Thu, 27 Oct 2022, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> 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.

That's completely fine, it was just an observation.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center