Re: [PATCH v2] drm/i915/vlv: enable HDMI audio for Valleyview2

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 23, 2013 at 07:08:11PM -0400, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> This patch defines audio configuration registers and adds audio enabling code
> for Valleyview2.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

[snip]

> @@ -6905,8 +6910,19 @@ static void ironlake_write_eld(struct drm_connector *connector,
>  
>  	DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
>  
> -	i = I915_READ(aud_cntl_st);
> -	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 = PortB */
> +	if (IS_VALLEYVIEW(connector->dev))  {
> +		struct intel_encoder *intel_encoder;
> +		int port = 0;
> +		intel_encoder = intel_attached_encoder(connector);
> +		if (intel_encoder)
> +			port = intel_ddi_get_encoder_port(intel_encoder);

This is a haswell specific function. Please use
enc_to_dig_port(intel_encoder)->port instead, which does the right thing
on all platforms for hdmi/dp ports.

Also, when poking for review feedback (like you've done in private to
Jesse and me) please consider next time around that:
- Never drop the public mailings lists. That way other people can also
  notice that a patch needs attention. Also Jesse's r-b tag is now lost
  since he replied to your private mail.
- Leave more than 8 hours of time for review to happen. In that time frame
  most of our team was off-duty. A few days is a good waiting time.

For the patch itself please add a patch changelog so that everyone knows
what changed from v1 to v2. This is doubly important since the review
happened off-list.

Finally please submit updated patches in reply to the original submission
or the patch review. Tightly grouping discussions like that helps with
managing the mail flood on a busy place like intel-gfx.

Furthermore v1 was rather decently broken with the wrong register offset
due to lack of the VLV_DISPLAY_BASE offset. So I'm wondering how solid
your testing is and whether we can automate this somehow to ensure we
cover all ugly combinations of ports and pipes. As-is I suspect you often
won't notice that things work simply due to settings left behind by the
bios or register default values matching what we want.

Maybe we could use the port CRC stuff to make sure that audio is actually
working ...

I won't block byt enabling on this, but expect me to be a royal pita for
the next platform enabling ;-)

Cheers, Daniel


> +		i = port;
> +	} else {
> +		i = I915_READ(aud_cntl_st);
> +		i = (i >> 29) & DIP_PORT_SEL_MASK;
> +		/* DIP_Port_Select, 0x1 = PortB */
> +	}
> +
>  	if (!i) {
>  		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
>  		/* operate blindly on all ports */
> @@ -10195,7 +10211,8 @@ static void intel_init_display(struct drm_device *dev)
>  		}
>  	} else if (IS_G4X(dev)) {
>  		dev_priv->display.write_eld = g4x_write_eld;
> -	}
> +	} else if (IS_VALLEYVIEW(dev))
> +		dev_priv->display.write_eld = ironlake_write_eld;
>  
>  	/* Default just returns -ENODEV to indicate unsupported */
>  	dev_priv->display.queue_flip = intel_default_queue_flip;
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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