[Intel-gfx] [PATCH v2] drm/i915/vlv: enable HDMI audio for Valleyview2

[Lin, Mengdong]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Sunday, October 27, 2013 9:59 PM
> To: Lin, Mengdong
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/vlv: enable HDMI audio for
> Valleyview2
> 
> On Wed, Oct 23, 2013 at 07:08:11PM -0400, mengdong.lin at intel.com wrote:
> > From: Mengdong Lin <mengdong.lin at intel.com>
> >
> > This patch defines audio configuration registers and adds audio
> > enabling code for Valleyview2.
> >
> > Signed-off-by: Mengdong Lin <mengdong.lin at intel.com>
> > Reviewed-by: Jesse Barnes <jbarnes at 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.

Fixed in v4 patch.

> 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.

Many thanks for your suggestions! I'll follow the rules.

> 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.

As you say, v1 patch wrote to a bad address and so HDMI audio happens to work just because the default value matches the value I want to set.
If I test DP audio with v1 which request changing the default value, sound cannot be heard at all. I'll be more careful.

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

Would you please clarify what's port CRC stuff?

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

Regards
Mengdong