On Fri, 17 May 2019, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Fri, 17 May 2019, Aditya Swarup <aditya.swarup@xxxxxxxxx> wrote:
>> On Wed, May 15, 2019 at 05:40:10PM +0300, Jani Nikula wrote:
>>> On Tue, 02 Apr 2019, Aditya Swarup <aditya.swarup@xxxxxxxxx> wrote:
>>> > Adding N & CTS values for 10/12 bit deep color from Appendix C
>>> > table in HDMI 2.0 spec. The correct values for N is not chosen
>>> > automatically by hardware for deep color modes.
>>> >
>>> > v2: Remove redundant code and make it generic.(Jani)
>>> >
>>> > Signed-off-by: Aditya Swarup <aditya.swarup@xxxxxxxxx>
>>> > Cc: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
>>> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>>> > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
>>> > ---
>>> > drivers/gpu/drm/i915/intel_audio.c | 82 +++++++++++++++++++++++++-----
>>> > 1 file changed, 69 insertions(+), 13 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>>> > index 502b57ce72ab..ad53b04fa5a2 100644
>>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>>> > @@ -70,6 +70,13 @@ struct dp_aud_n_m {
>>> > u16 n;
>>> > };
>>> >
>>> > +struct hdmi_aud_ncts_table {
>>>
>>> The struct itself is not a table. Just make it struct hdmi_ncts or
>>> something.
>> Will do it in the next spin.
Aditya, please provide the updated version.
BR,
Jani.
>>>
>>> > + int sample_rate;
>>> > + int clock;
>>> > + int n;
>>> > + int cts;
>>> > +};
>>> > +
>>> > /* Values according to DP 1.4 Table 2-104 */
>>> > static const struct dp_aud_n_m dp_aud_n_m[] = {
>>> > { 32000, LC_162M, 1024, 10125 },
>>> > @@ -146,12 +153,7 @@ static const struct {
>>> > #define TMDS_594M 594000
>>> > #define TMDS_593M 593407
>>> >
>>> > -static const struct {
>>> > - int sample_rate;
>>> > - int clock;
>>> > - int n;
>>> > - int cts;
>>> > -} hdmi_aud_ncts[] = {
>>> > +static const struct hdmi_aud_ncts_table hdmi_aud_ncts_24bpp[] = {
>>> > { 32000, TMDS_296M, 5824, 421875 },
>>> > { 32000, TMDS_297M, 3072, 222750 },
>>> > { 32000, TMDS_593M, 5824, 843750 },
>>> > @@ -182,6 +184,49 @@ static const struct {
>>> > { 192000, TMDS_594M, 24576, 594000 },
>>> > };
>>> >
>>> > +/* Appendix C - N & CTS values for deep color from HDMI 2.0 spec*/
>>> > +/* HDMI N/CTS table for 10 bit deep color(30 bpp)*/
>>> > +#define TMDS_371M 371250
>>> > +#define TMDS_370M 370878
>>> > +
>>> > +static const struct hdmi_aud_ncts_table hdmi_aud_ncts_30bpp[] = {
>>> > + { 32000, TMDS_370M, 5824, 527344 },
>>>
>>> N should be 11648?
>> The value in the struct is correct. It is 5824 - Appendix C Table C-1 Pg
>> 234, for 371.25/1.001 and f = 32kHz => N = 5824
>
> Huh, looks like I have an outdated version of the spec then. For me the
> table is on page 232.
>
> Need to find the up-to-date spec, but looking at draft 2.1 the values
> you have match.
>
>>>
>>> > + { 32000, TMDS_371M, 6144, 556875 },
>>> > + { 44100, TMDS_370M, 8918, 585938 },
>>>
>>> N 17836
>> Value in the table is correct.
>>>
>>> > + { 44100, TMDS_371M, 4704, 309375 },
>>> > + { 88200, TMDS_370M, 17836, 585938 },
>>>
>>> N 35672
>> Value in the table is correct.
>>>
>>> > + { 88200, TMDS_371M, 9408, 309375 },
>>> > + { 176400, TMDS_370M, 35672, 585938 },
>>>
>>> N 71344
>> Value in the table is correct.
>>>
>>> > + { 176400, TMDS_371M, 18816, 309375 },
>>> > + { 48000, TMDS_370M, 11648, 703125 },
>>> > + { 48000, TMDS_371M, 5120, 309375 },
>>> > + { 96000, TMDS_370M, 23296, 703125 },
>>> > + { 96000, TMDS_371M, 10240, 309375 },
>>> > + { 192000, TMDS_370M, 46592, 703125 },
>>> > + { 192000, TMDS_371M, 20480, 309375 },
>>> > +};
>>> > +
>>> > +/* HDMI N/CTS table for 12 bit deep color(36 bpp)*/
>>> > +#define TMDS_445_5M 445500
>>> > +#define TMDS_445M 445054
>>> > +
>>> > +static const struct hdmi_aud_ncts_table hdmi_aud_ncts_36bpp[] = {
>>> > + { 32000, TMDS_445M, 5824, 632813 },
>>> > + { 32000, TMDS_445_5M, 4096, 445500 },
>>> > + { 44100, TMDS_445M, 8918, 703125 },
>>> > + { 44100, TMDS_445_5M, 4704, 371250 },
>>> > + { 88200, TMDS_445M, 17836, 703125 },
>>> > + { 88200, TMDS_445_5M, 9408, 371250 },
>>> > + { 176400, TMDS_445M, 35672, 703125 },
>>> > + { 176400, TMDS_445_5M, 18816, 371250 },
>>> > + { 48000, TMDS_445M, 5824, 421875 },
>>> > + { 48000, TMDS_445_5M, 5120, 371250 },
>>> > + { 96000, TMDS_445M, 11648, 421875 },
>>> > + { 96000, TMDS_445_5M, 10240, 371250 },
>>> > + { 192000, TMDS_445M, 23296, 421875 },
>>> > + { 192000, TMDS_445_5M, 20480, 371250 },
>>> > +};
>>> > +
>>> > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
>>> > static u32 audio_config_hdmi_pixel_clock(const struct intel_crtc_state *crtc_state)
>>> > {
>>> > @@ -210,16 +255,27 @@ static u32 audio_config_hdmi_pixel_clock(const struct intel_crtc_state *crtc_sta
>>> > static int audio_config_hdmi_get_n(const struct intel_crtc_state *crtc_state,
>>> > int rate)
>>> > {
>>> > - const struct drm_display_mode *adjusted_mode =
>>> > - &crtc_state->base.adjusted_mode;
>>> > - int i;
>>> > + const struct hdmi_aud_ncts_table *hdmi_ncts_table;
>>> > + int i, size = 0;
>>>
>>> Unnecessary initialization.
>>>
>>> > +
>>> > + if (crtc_state->pipe_bpp == 36) {
>>> > + hdmi_ncts_table = hdmi_aud_ncts_36bpp;
>>> > + size = ARRAY_SIZE(hdmi_aud_ncts_36bpp);
>>> > + } else if (crtc_state->pipe_bpp == 30) {
>>> > + hdmi_ncts_table = hdmi_aud_ncts_30bpp;
>>> > + size = ARRAY_SIZE(hdmi_aud_ncts_30bpp);
>>> > + } else {
>>> > + hdmi_ncts_table = hdmi_aud_ncts_24bpp;
>>> > + size = ARRAY_SIZE(hdmi_aud_ncts_24bpp);
>>> > + }
>>> >
>>> > - for (i = 0; i < ARRAY_SIZE(hdmi_aud_ncts); i++) {
>>> > - if (rate == hdmi_aud_ncts[i].sample_rate &&
>>> > - adjusted_mode->crtc_clock == hdmi_aud_ncts[i].clock) {
>>> > - return hdmi_aud_ncts[i].n;
>>> > + for (i = 0; i < size; i++) {
>>> ^
>>> Superfluous space.
>> Will correct it in next spin.
>>>
>>>
>>> > + if (rate == hdmi_ncts_table[i].sample_rate &&
>>> > + crtc_state->port_clock == hdmi_ncts_table[i].clock) {
>>>
>>> Changing from adjusted_mode->crtc_clock to crtc_state->port_clock is a
>>> separate change that needs to be a separate patch.
>> I don't think it should be a separate change. Working with deep color
>> mode requires crtc_state->port_clock as it is scaled according to the
>> deep color mode set. Using adjusted_mode->crtc_clock is incorrect as it
>> doesn't account for deep color mode; we will have to multiply with
>> scaling factor for the set deep color mode which is superfluous.
>
> Make the crtc_clock -> port_clock change *before* the deep color
> changes, that's the whole point. Ensure that this change does not impact
> non-deep color, and if it does, the bisect will point at the minimal
> change. Then add the deep color N & CTS when the code already uses
> port_clock.
>
> BR,
> Jani.
>
>>
>> I can add a comment in the commit message and comment before the if
>> block to explain the change. Let me know if it works?
>>>
>>>
>>> BR,
>>> Jani.
>>>
>>> > + return hdmi_ncts_table[i].n;
>>> > }
>>> > }
>>> > +
>>> > return 0;
>>> > }
>>>
>>> --
>>> Jani Nikula, Intel Open Source Graphics Center
>>
>> Regards,
>> Aditya Swarup
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
