| Message ID | 1382569691-3130-1-git-send-email-mengdong.lin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Daniel, > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Sunday, October 27, 2013 9:59 PM > To: Lin, Mengdong > Cc: intel-gfx@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@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. 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
On Fri, Nov 01, 2013 at 03:13:17PM +0000, Lin, Mengdong wrote: > > 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? The hw has a bunch of CRC (checksum) functions. We've just enabled it at the pipe level, and it's extremely useful to test whether e.g. the cursor is displaying properly or whether it's not shown at all. We've had a few bugs where the cursor was disabled but shouldnt have been. For an example of such a testcase see i-g-t/tests/kms_cursor_crc.c. This is all really new, so still in flux. Now the hardware also has checksum support for each port, and afaik that includes the audio stream. Iirc never platforms even have special CRCs for the individual audio streams. My idea for a testcase would be to expose these port CRCs. Then check that the CRC is stable for each display frame while no audio is playing, and that it changes (and that the right port starts to change) once we play an audio stream. We can't test sync issues with that, but routing issues (which seems to have been the issue here, and I've also seen a few patches for routing issues in the past) should be testable. And with an automated testcase we can ensure that no regression creps in. The other upside of an automated test like that is that it should be easy to test all port combinations. That's more important for desktop platforms where we have 3 HDMI/DP ports. Anyway I've just thought I'll bring this up as an idea to look into for the next hw enabling project. It was a bit an effort to enable pipe CRCs for display testing, but I think long-term it will definitely be worth it. So maybe poke the audio hw engineers/validation ppl a bit and inquire whether/how exactly they use this? We've had a display micro architect help us out a bit on the display side. Cheers, Daniel
Hi Daniel, Thanks for your clarification! Could you share more info ... > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Saturday, November 02, 2013 12:03 AM > To: Lin, Mengdong > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/vlv: enable HDMI audio for > Valleyview2 > > On Fri, Nov 01, 2013 at 03:13:17PM +0000, Lin, Mengdong wrote: > > > 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? > > The hw has a bunch of CRC (checksum) functions. We've just enabled it at the > pipe level, and it's extremely useful to test whether e.g. the cursor is displaying > properly or whether it's not shown at all. We've had a few bugs where the > cursor was disabled but shouldnt have been. > > For an example of such a testcase see i-g-t/tests/kms_cursor_crc.c. This is all > really new, so still in flux. > > Now the hardware also has checksum support for each port, and afaik that > includes the audio stream. Iirc never platforms even have special CRCs for the > individual audio streams. My idea for a testcase would be to expose these port > CRCs. Then check that the CRC is stable for each display frame while no audio is > playing, and that it changes (and that the right port starts to change) once we > play an audio stream. What's 'IIRC never platforms'? Where can I find more information about port or audio CRC? In Baytrail b-spec, I saw CRCCtrlRedA-Pipe A CRC Color Channel Control Register can select source: CRC Source Select: These bits select the source of the data to put into the CRC logic. 0000: Pipe A .... 0110: DisplayPort B 0111: DisplayPort C 1000: Audio DP (Audio for DisplayPort (pcdclk). Only select when Audio is on DisplayPort on Pipe A) 1001: Audio HDMI (Audio for HDMI (dotclock) Only select when Audio is on HDMI on Pipe A) But I still cannot understand how to get CRC for both video and audio. And does the CRC does not change for each display frame, even when a video is playing and pictures content change from frame to frame? I hope to know more about how HW generates the CRC, so your info or any documentation will be appreciated. > We can't test sync issues with that, but routing issues (which seems to have > been the issue here, and I've also seen a few patches for routing issues in the > past) should be testable. And with an automated testcase we can ensure that no > regression creps in. If the audio CRC help to check audio data arrives to the proper pipe and port, it will help to check routing issues. > > The other upside of an automated test like that is that it should be easy to test > all port combinations. That's more important for desktop platforms where we > have 3 HDMI/DP ports. > > Anyway I've just thought I'll bring this up as an idea to look into for the next hw > enabling project. It was a bit an effort to enable pipe CRCs for display testing, > but I think long-term it will definitely be worth it. > So maybe poke the audio hw engineers/validation ppl a bit and inquire > whether/how exactly they use this? We've had a display micro architect help us > out a bit on the display side. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Nov 05, 2013 at 05:34:18AM +0000, Lin, Mengdong wrote: > Hi Daniel, > > Thanks for your clarification! Could you share more info ... > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > > Sent: Saturday, November 02, 2013 12:03 AM > > To: Lin, Mengdong > > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/vlv: enable HDMI audio for > > Valleyview2 > > > > On Fri, Nov 01, 2013 at 03:13:17PM +0000, Lin, Mengdong wrote: > > > > 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? > > > > The hw has a bunch of CRC (checksum) functions. We've just enabled it at the > > pipe level, and it's extremely useful to test whether e.g. the cursor is displaying > > properly or whether it's not shown at all. We've had a few bugs where the > > cursor was disabled but shouldnt have been. > > > > For an example of such a testcase see i-g-t/tests/kms_cursor_crc.c. This is all > > really new, so still in flux. > > > > Now the hardware also has checksum support for each port, and afaik that > > includes the audio stream. Iirc never platforms even have special CRCs for the > > individual audio streams. My idea for a testcase would be to expose these port > > CRCs. Then check that the CRC is stable for each display frame while no audio is > > playing, and that it changes (and that the right port starts to change) once we > > play an audio stream. > > What's 'IIRC never platforms'? Oops, I've meant to write "newer platforms". > Where can I find more information about port or audio CRC? > > In Baytrail b-spec, I saw CRCCtrlRedA-Pipe A CRC Color Channel Control Register can select source: > CRC Source Select: These bits select the source of the data to put into the CRC logic. > 0000: Pipe A .... > 0110: DisplayPort B > 0111: DisplayPort C > 1000: Audio DP (Audio for DisplayPort (pcdclk). Only select when Audio is on DisplayPort on Pipe A) > 1001: Audio HDMI (Audio for HDMI (dotclock) Only select when Audio is on HDMI on Pipe A) > > But I still cannot understand how to get CRC for both video and audio. > And does the CRC does not change for each display frame, even when a video is playing and pictures content change from frame to frame? > I hope to know more about how HW generates the CRC, so your info or any documentation will be appreciated. For baytrail we have many of these CRC sources enabled already, see vlv_pipe_crc_ctl_reg in i915_debugfs.c. So for a quick test I'd add the new targets and just see what happens. Iirc on Byt display pipe = audio port. Big core platforms (broadwell would be the next) have special CRC registers for the ports/audio stuff. Currently we only have CRC support up to Haswell, and only for the display pipe (not yet for ports or audio channels). You should find information about them in the internal Bspec. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9538502..2a4c33f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4844,6 +4844,18 @@ CPT_AUD_CNTL_ST_B) #define CPT_AUD_CNTRL_ST2 0xE50C0 +#define VLV_HDMIW_HDMIEDID_A (VLV_DISPLAY_BASE + 0x62050) +#define VLV_HDMIW_HDMIEDID_B (VLV_DISPLAY_BASE + 0x62150) +#define VLV_HDMIW_HDMIEDID(pipe) _PIPE(pipe, \ + VLV_HDMIW_HDMIEDID_A, \ + VLV_HDMIW_HDMIEDID_B) +#define VLV_AUD_CNTL_ST_A (VLV_DISPLAY_BASE + 0x620B4) +#define VLV_AUD_CNTL_ST_B (VLV_DISPLAY_BASE + 0x621B4) +#define VLV_AUD_CNTL_ST(pipe) _PIPE(pipe, \ + VLV_AUD_CNTL_ST_A, \ + VLV_AUD_CNTL_ST_B) +#define VLV_AUD_CNTL_ST2 (VLV_DISPLAY_BASE + 0x620C0) + /* These are the 4 32-bit write offset registers for each stream * output buffer. It determines the offset from the * 3DSTATE_SO_BUFFERs that the next streamed vertex output goes to. @@ -4860,6 +4872,12 @@ #define CPT_AUD_CFG(pipe) _PIPE(pipe, \ CPT_AUD_CONFIG_A, \ CPT_AUD_CONFIG_B) +#define VLV_AUD_CONFIG_A (VLV_DISPLAY_BASE + 0x62000) +#define VLV_AUD_CONFIG_B (VLV_DISPLAY_BASE + 0x62100) +#define VLV_AUD_CFG(pipe) _PIPE(pipe, \ + VLV_AUD_CONFIG_A, \ + VLV_AUD_CONFIG_B) + #define AUD_CONFIG_N_VALUE_INDEX (1 << 29) #define AUD_CONFIG_N_PROG_ENABLE (1 << 28) #define AUD_CONFIG_UPPER_N_SHIFT 20 diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ebe5d08..25abba96 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6896,6 +6896,11 @@ static void ironlake_write_eld(struct drm_connector *connector, aud_config = IBX_AUD_CFG(pipe); aud_cntl_st = IBX_AUD_CNTL_ST(pipe); aud_cntrl_st2 = IBX_AUD_CNTL_ST2; + } else if (IS_VALLEYVIEW(connector->dev)) { + hdmiw_hdmiedid = VLV_HDMIW_HDMIEDID(pipe); + aud_config = VLV_AUD_CFG(pipe); + aud_cntl_st = VLV_AUD_CNTL_ST(pipe); + aud_cntrl_st2 = VLV_AUD_CNTL_ST2; } else { hdmiw_hdmiedid = CPT_HDMIW_HDMIEDID(pipe); aud_config = CPT_AUD_CFG(pipe); @@ -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); + 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;