Re: [PATCH 4/5] drm: Add definitions for DP compliance Video pattern tests

Jani Nikula
2016-11-23T13:27:45Z

On Tue, 22 Nov 2016, Manasi Navare <manasi.d.navare@intel.com> wrote:
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> include/drm/drm_dp_helper.h | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 55bbeb0..f2c04ec 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -415,7 +415,21 @@
>
> #define DP_TEST_LANE_COUNT 0x220
>
> -#define DP_TEST_PATTERN 0x221
> +#define DP_TEST_PATTERN 0x221

Unnecessary indentation change. Please observe the code around you. It
may not have the best indentation style, but stick with what's there
for all the other DPCD address definitions.

> +#define DP_COLOR_RAMP (1 << 0)

See how all the other address *content* definitions have a space between
"#" and "define". I'm not saying I like it, but it's uniform in the
file.

While at it, why not add all of the defines for TEST_PATTERN. And
observe how they are not bit patterns, so (1 << 0) should be just 1.

DP_NO_TEST_PATTERN
DP_COLOR_RAMPS
DP_BLACK_AND_WHITE_VERTICAL_LINES
DP_COLOR_SQUARE

> +#define DP_TEST_H_WIDTH 0x22E

Note that across the file, almost all addrses defines have a blank line
between them, to separate content definitions from other addresses.

> +#define DP_TEST_V_HEIGHT 0x230

I guess I'd do

#define DP_TEST_V_HEIGHT_HI 0x230
#define DP_TEST_V_HEIGHT_LO 0x231

You don't actually have to *use* both definitions if you can write both
in one go, but this saves the trouble of checking the DP spec when it's
documented as #defines here.

> +#define DP_TEST_MISC 0x232
> +#define DP_VIDEO_PATTERN_RGB_FORMAT 0

The convention is to shift the 0 too so it's obvious where it
fits. _MASK goes before the values. Please add all the values.

> +#define DP_TEST_COLOR_FORMAT_MASK 0x6
> +#define DP_TEST_DYNAMIC_RANGE_MASK (1 << 3)

And the values?

> +#define DP_VIDEO_PATTERN_VESA 0
> +#define DP_TEST_BIT_DEPTH_MASK 0x00E0
> +#define DP_TEST_BIT_DEPTH_6 0

> +#define DP_TEST_BIT_DEPTH_8 1

Just add all of the values at once.

> +#define DP_TEST_MISC_BIT_1 1
> +#define DP_TEST_MISC_BIT_3 3
> +#define DP_TEST_MISC_BIT_5 5
>
> #define DP_TEST_CRC_R_CR 0x240
> #define DP_TEST_CRC_G_Y 0x242

--
Jani Nikula, Intel Open Source Technology Center