Re: [PATCH 2/2] drm/i915: Set up SDVO encoder type only at detect

[Egbert Eich <eich@freedesktop.org>]

Daniel Vetter writes:
 > On Mon, Apr 14, 2014 at 07:26:09PM +0200, Egbert Eich wrote:
 > > Depending on the SDVO output_flags SDVO may have multiple connectors.
 > > These are all initialized in intel_sdvo_output_setup(). The connector
 > > that is initialized later will override the encoder_type that has been
 > > set up by an earlier connector type initialization. Eventually the
 > > one that comes last wins.
 > > Eventually when intel_sdvo_detect() is called the active connector is
 > > determined.
 > > Delay encoder type initialization until the active connector is known
 > > and set it to the type that corresponds to this connector.
 > > 
 > > Signed-off-by: Egbert Eich <eich@suse.de>
 > 
 > Hm, has this any effect on the code itself? I think if we want to fix this
 > just for optics we should add a new DRM_MODE_ENCODER_MULTI. At least I
 > have an sdvo here which has a dac, hdmi and tv-out ...

With the present logic the last connector in the list matching a flag bit
will win the encoder type. The encoder type is presently just used for
(debug) messages, so it is cosmetic.

 > 
 > This also reminds that I should finally polish of the multi-sdvo fixes I
 > have around here - currently the last encoder detected wins on a
 > multi-encoder chip, which means if you have an lvds+hdmi combo and plug in
 > a screeen you'll never be able to use the lvds again until the hdmi is
 > unplugged.

You know, from looking at the code I was wondering already if this was 
possible at all. 
I stayed away tinkering with this - there doesn't seem to be documentation 
on the SDVO command set publically available. Moreover I'm moslty dealing 
with cash registers here - so I doubt they have all the SVIDEO connectors 
they advertise ;) But I can't tell as I don't even have physical access!

As I read the current code it seems like bad things could happen 
if more than one bit is set in intel_sdvo->attached_output: after all
tv-out should have quite different timing requirements than a TMDS.
 > 
 > Much worse if there's a tv-out detect issue ;-)

;p

Cheers,
	Egbert.
 > 
 > Cheers, Daniel
 > 
 > > ---
 > >  drivers/gpu/drm/i915/intel_sdvo.c | 23 ++++++++++++++++++-----
 > >  1 file changed, 18 insertions(+), 5 deletions(-)
 > > 
 > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
 > > index d27155a..5043f16 100644
 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
 > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
 > > @@ -154,6 +154,9 @@ struct intel_sdvo_connector {
 > >  	/* Mark the type of connector */
 > >  	uint16_t output_flag;
 > >  
 > > +	/* store encoder type for convenience */
 > > +	int encoder_type;
 > > +
 > >  	enum hdmi_force_audio force_audio;
 > >  
 > >  	/* This contains all current supported TV format */
 > > @@ -1746,6 +1749,7 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 > >  	if (response == 0)
 > >  		return connector_status_disconnected;
 > >  
 > > +	intel_sdvo->base.base.encoder_type = intel_sdvo_connector->encoder_type;
 > >  	intel_sdvo->attached_output = response;
 > >  
 > >  	intel_sdvo->has_hdmi_monitor = false;
 > > @@ -2489,7 +2493,9 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 > >  	} else {
 > >  		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 > >  	}
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_TMDS;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TMDS;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_DVID;
 > >  
 > >  	if (intel_sdvo_is_hdmi_connector(intel_sdvo, device)) {
 > > @@ -2524,7 +2530,9 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 > >  
 > >  	intel_connector = &intel_sdvo_connector->base;
 > >  	connector = &intel_connector->base;
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_TVDAC;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_TVDAC;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_SVIDEO;
 > >  
 > >  	intel_sdvo->controlled_output |= type;
 > > @@ -2568,7 +2576,9 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 > >  	intel_connector = &intel_sdvo_connector->base;
 > >  	connector = &intel_connector->base;
 > >  	intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_DAC;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_DAC;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_VGA;
 > >  
 > >  	if (device == 0) {
 > > @@ -2603,7 +2613,9 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 > >  
 > >  	intel_connector = &intel_sdvo_connector->base;
 > >  	connector = &intel_connector->base;
 > > -	encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
 > > +	/* delay encoder_type setting until detection */
 > > +	intel_sdvo_connector->encoder_type = DRM_MODE_ENCODER_LVDS;
 > > +	encoder->encoder_type = DRM_MODE_ENCODER_NONE;
 > >  	connector->connector_type = DRM_MODE_CONNECTOR_LVDS;
 > >  
 > >  	if (device == 0) {
 > > @@ -2984,7 +2996,8 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 > >  	/* encoder type will be decided later */
 > >  	intel_encoder = &intel_sdvo->base;
 > >  	intel_encoder->type = INTEL_OUTPUT_SDVO;
 > > -	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs, 0);
 > > +	drm_encoder_init(dev, &intel_encoder->base, &intel_sdvo_enc_funcs,
 > > +			 DRM_MODE_ENCODER_NONE);
 > >  
 > >  	/* Read the regs to test if we can talk to the device */
 > >  	for (i = 0; i < 0x40; i++) {
 > > -- 
 > > 1.8.4.5
 > > 
 > > _______________________________________________
 > > Intel-gfx mailing list
 > > Intel-gfx@lists.freedesktop.org
 > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
 > 
 > -- 
 > Daniel Vetter
 > Software Engineer, Intel Corporation
 > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Egbert Eich (Res. & Dev.)                SUSE LINUX Products GmbH
SUSE Labs KMS / DRM / X Window System Development
Tel: +49 911-740 53 0                       http://www.suse.de 
-----------------------------------------------------------------
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany