diff mbox

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

Message ID 1397496369-2746-3-git-send-email-eich@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Egbert Eich April 14, 2014, 5:26 p.m. UTC 
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>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Chris Wilson April 15, 2014, 9:39 a.m. UTC | #1 
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>

Please kill the redundant encoder->encoder_type = DRM_MODE_ENCODER_NONE
as I think that will help clarify that during init we set the
intel_sdvo->type and then during the detection of the actual connected
output we assign the encoder_type.

Otherwise, lgtm.
-Chris
Daniel Vetter April 15, 2014, 7:12 p.m. UTC | #2 
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 ...

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.

Much worse if there's a tv-out detect issue ;-)

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
Egbert Eich April 16, 2014, 5:58 a.m. UTC | #3 
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
Daniel Vetter April 16, 2014, 7:55 a.m. UTC | #4 
On Wed, Apr 16, 2014 at 07:58:48AM +0200, Egbert Eich wrote:
> 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.

I'm vary of changing the encoder type at runtime tbh. We use the same hack
in intel_ddi.c to switch between hdmi and dp mode, and the results aren't
pretty imo. For debug output imo adding a new "multi" encoder type is
better.

And if we need the type somehow to set up the right connector for sdvo
encoders with more than one detected output, then we need to track this
piece of state somewhere in our modeset state (either with the
connector->encoder links or with a bit of state in the pipe config
structure). Which also means we need state read-out and cross-check
support to make sure it really does what we want.

Storing such state in global structure tends to lead to subtile bugs and
prevent proper pre-compute/commit semantics for modeset changes.
-Daniel
diff mbox

Patch

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++) {