Dear Paul,
On Sun, Sep 04, 2011 at 07:11:54PM +0800, Paul Menzel wrote:
> Dear Wu,
>
>
> I hope that is your first name.
My first name is Fengguang. "LAST NAME, FIRSTNAME" is the convention
in Intel emails. However I often forgot this and pick whatever comes
first...and happily accept both Fengguang and Wu :)
> Am Sonntag, den 04.09.2011, 05:15 +0800 schrieb Wu Fengguang:
> > Changes from v4: remove a debug call to dump_stack().
> > Thanks to Bossart for catching this!
>
> His first name is Pierre-Louis. I do not know how you address people at
> Intel though.
Thanks for the reminding!
> > ---
>
> I think your format will confuse `git am`. Please always put that under
> the »---« under the Signed-off-by lines.
OK, good point!
> > Add ELD support for Intel Eaglelake, IbexPeak/Ironlake,
> > SandyBridge/CougarPoint and IvyBridge/PantherPoint chips.
> >
> > ELD (EDID-Like Data) describes to the HDMI/DP audio driver the audio
> > capabilities of the plugged monitor. It's built and passed to audio
> > driver in 2 steps:
> >
> > (1) at get_modes time, parse EDID and save ELD to drm_connector.eld[]
> >
> > (2) at mode_set time, write drm_connector.eld[] to the Transcoder's hw
> > ELD buffer and set the ELD_valid bit to inform HDMI/DP audio driver
> >
> > ELD selection policy: it's possible for one encoder to be associated
> > with multiple connectors (ie. monitors), in which case the first found
> > ELD will be used. This policy may not be suitable for all users, but
> > let's start it simple first.
> >
> > The impact of ELD selection policy: assume there are two monitors, one
> > supports stereo playback and the other has 8-channel output; cloned
> > display mode is used, so that the two monitors are associated with the
> > same internal encoder. If only the stereo playback capability is reported,
> > the user won't be able to start 8-channel playback; if the 8-channel ELD
> > is reported, then user space applications may send 8-channel samples
> > down, however the user may actually be listening to the 2-channel
> > monitor and not connecting speakers to the 8-channel monitor. Overall,
> > it's more safe to report maximum profiles to the user space, so that
> > the user can at least be able to do 8-channel playback if he want to.
>
> s'he's/he'
Fixed.
> > This patch is tested OK on G45/HDMI, IbexPeak/HDMI and IvyBridge/HDMI+DP.
>
> What is the correct way to test this patch. Just plug in the HDMI
> monitor and it should work out of the box?
Just plug in the HDMI/DP monitor, and run
cat /proc/asound/card0/eld*
to check if the monitor name, HDMI/DP type, etc. show up correctly.
> > Minor imperfection: the GEN6_AUD_CNTL_ST/DIP_Port_Select field always
> > reads 0 (reserved). Without knowing the port number, I worked it around
> > by setting the ELD_valid bit for ALL the three ports. It's tested to not
> > be a problem, because the audio driver will find invalid ELD data and
> > hence rightfully abort, even when it sees the ELD_valid indicator.
> >
> > Thanks to Zhenyu and Bossart for a lot of valuable help and testing.
>
> Again the first name is Pierre-Louis or put Mr in front of it.
Got it, so the convention is either "Pierre-Louis", or "Mr. Bossart".
> > CC: Zhao Yakui <yakui.zhao@xxxxxxxxx>
> > CC: Wang Zhenyu <zhenyu.z.wang@xxxxxxxxx>
> > CC: Jeremy Bush <contractfrombelow@xxxxxxxxx>
> > CC: Christopher White <c.white@xxxxxxxxxxxxxx>
> > CC: "Bossart, Pierre-louis" <pierre-louis.bossart@xxxxxxxxx>
>
> Pierre-Louis Bossart
Corrected.
> > Signed-off-by: Ben Skeggs <bskeggs@xxxxxxxxxx>
> > Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_edid.c | 171 +++++++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_drv.h | 2
> > drivers/gpu/drm/i915/i915_reg.h | 25 +++
> > drivers/gpu/drm/i915/intel_display.c | 131 +++++++++++++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 6
> > drivers/gpu/drm/i915/intel_drv.h | 2
> > drivers/gpu/drm/i915/intel_hdmi.c | 3
> > drivers/gpu/drm/i915/intel_modes.c | 2
> > include/drm/drm_crtc.h | 9 +
> > include/drm/drm_edid.h | 9 +
> > 10 files changed, 358 insertions(+), 2 deletions(-)
>
> Some more style things follow.
>
> […]
>
> > +/**
> > + * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in milli-seconds
> > + * @connector: connector associated with the HDMI/DP sink
> > + * @mode: the display mode
> > + */
> > +int drm_av_sync_delay(struct drm_connector *connector,
> > + struct drm_display_mode *mode)
> > +{
> > + int i = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> > + int a, v;
> > +
> > + if (!connector->latency_present[0])
> > + return 0;
> > + if (!connector->latency_present[1])
> > + i = 0;
> > +
> > + a = connector->audio_latency[i];
> > + v = connector->video_latency[i];
> > +
> > + /*
> > + * HDMI/DP sink doesn't support audio or video?
> > + */
> > + if (a == 255 || v == 255)
> > + return 0;
> > +
> > + /*
> > + * Convert raw edid values to milli-seconds.
>
> s/edid/EDID/ (nitpick)
> s/milli-seconds/millisecond/
Fixed.
> http://www.merriam-webster.com/dictionary/millisecond
>
> > + * Treat unknown latency as 0ms.
> > + */
> > + if (a)
> > + a = min(2 * (a - 1), 500);
> > + if (v)
> > + v = min(2 * (v - 1), 500);
> > +
> > + return max(v - a, 0);
> > +}
> > +EXPORT_SYMBOL(drm_av_sync_delay);
>
> […]
>
> > --- linux-next.orig/drivers/gpu/drm/i915/i915_reg.h 2011-09-02 15:59:31.000000000 +0800
> > +++ linux-next/drivers/gpu/drm/i915/i915_reg.h 2011-09-02 15:59:40.000000000 +0800
> > @@ -3470,4 +3470,29 @@
> > #define GEN6_PCODE_DATA 0x138128
> > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
> >
> > +#define G4X_AUD_VID_DID 0x62020
> > +#define INTEL_AUDIO_DEVCL 0x808629FB
>
> Alignment two lines above. Separate clean up patch to fix alignment to
> send before?
>
> > +#define INTEL_AUDIO_DEVBLC 0x80862801
> > +#define INTEL_AUDIO_DEVCTG 0x80862802
> > +
> > +#define G4X_AUD_CNTL_ST 0x620B4
>
> Alignment?
>
> > +#define G4X_ELDV_DEVCL_DEVBLC (1 << 13)
> > +#define G4X_ELDV_DEVCTG (1 << 14)
>
> Dito?
>
> > +#define G4X_ELD_ADDR (0xf << 5)
> > +#define G4X_ELD_ACK (1 << 4)
> > +#define G4X_HDMIW_HDMIEDID 0x6210C
> > +
> > +#define GEN6_HDMIW_HDMIEDID_A 0xE2050
> > +#define GEN6_AUD_CNTL_ST_A 0xE20B4
> > +#define GEN6_ELD_BUFFER_SIZE (0x1f << 10)
> > +#define GEN6_ELD_ADDRESS (0x1f << 5)
> > +#define GEN6_ELD_ACK (1 << 4)
> > +#define GEN6_AUD_CNTL_ST2 0xE20C0
> > +#define GEN6_ELD_VALIDB (1 << 0)
>
> Dito?
The alignments are actually good in the source code. It's the leading
"+" in the patch file that makes some of the lines "appear" to be
unaligned.
Thanks for the careful review!
Regards,
Fengguang
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
