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