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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Fri, Jan 20, 2017 at 05:33:51PM +0200, Jani Nikula wrote:
>> On Fri, 20 Jan 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
>> > v3:
>> > * Fix the conventions in bit definitions (Jani Nikula)
>> > v2:
>> > * Add all the other DP Complianec TEST register defs (Jani Nikula)
>> > 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 | 64 +++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 64 insertions(+)
>> >
>> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> > index 0468135..7ab4153 100644
>> > --- a/include/drm/drm_dp_helper.h
>> > +++ b/include/drm/drm_dp_helper.h
>> > @@ -417,6 +417,70 @@
>> >  #define DP_TEST_LANE_COUNT		    0x220
>> >  
>> >  #define DP_TEST_PATTERN			    0x221
>> > +# define DP_NO_TEST_PATTERN                 0x0
>> > +# define DP_COLOR_RAMP                      0x1
>> > +# define DP_BLACK_AND_WHITE_VERTICAL_LINES  0x2
>> > +# define DP_COLOR_SQUARE                    0x3
>> > +
>> > +#define DP_TEST_H_TOTAL_HI                  0x222
>> > +#define DP_TEST_H_TOTAl_LO                  0x223
>>                          ^
>> Sorry, didn't notice it before, but the l in TOTAL is lower case.
>>
>
> Oh yes even my eyes didnt catch this.
>
>  
>> > +
>> > +#define DP_TEST_V_TOTAL_HI                  0x224
>> > +#define DP_TEST_V_TOTAl_LO                  0x225
>>                          ^
>> Ditto.
>> 
>> > +
>> > +#define DP_TEST_H_START_HI                  0x226
>> > +#define DP_TEST_H_START_LO                  0x227
>> > +
>> > +#define DP_TEST_V_START_HI                  0x228
>> > +#define DP_TEST_V_START_LO                  0x229
>> > +
>> > +#define DP_TEST_HSYNC_HI                    0x22A
>> > +# define DP_TEST_HSYNC_POLARITY             (1 << 7)
>> > +# define DP_TEST_HSYNC_WIDTH_HI_MASK        (127 << 0)
>> > +#define DP_TEST_HSYNC_WIDTH_LO              0x22B
>> > +
>> > +#define DP_TEST_VSYNC_HI                    0x22C
>> > +# define DP_TEST_VSYNC_POLARITY             (1 << 7)
>> > +# define DP_TEST_VSYNC_WIDTH_HI_MASK        (127 << 0)
>> > +#define DP_TEST_VSYNC_WIDTH_LO              0x22D
>> > +
>> > +#define DP_TEST_H_WIDTH_HI                  0x22E
>> > +#define DP_TEST_H_WIDTH_LO                  0x22F
>> > +
>> > +#define DP_TEST_V_HEIGHT_HI                 0x230
>> > +#define DP_TEST_V_HEIGHT_LO                 0x231
>> > +
>> > +#define DP_TEST_MISC0                       0x232
>> > +# define DP_TEST_SYNC_CLOCK                 (1 << 0)
>> > +# define DP_CLOCK_ASYNC                     0x0
>> > +# define DP_CLOCK_SYNC                      0x1
>> 
>> The above two are not necessary.
>>
>
> Wont these above defs be required for checking if it is DP_CLOCK_ASYNC or SYNC?
> I can remove the DP_CLOCK_SINK.

If the bit is set, it's synchronous, and asynchronous otherwise? You
don't need three macros to check one bit.

>  
>> > +# define DP_TEST_COLOR_FORMAT_MASK          (3 << 1)
>> > +# define DP_TEST_COLOR_FORMAT_SHIFT         1
>> > +# define DP_COLOR_FORMAT_RGB                (0 << 1)
>> > +# define DP_COLOR_FORMAT_YCbCr422           (1 << 1)
>> > +# define DP_COLOR_FORMAT_YCbCr444           (2 << 1)
>> > +# define DP_TEST_DYNAMIC_RANGE              (1 << 3)
>> 
>> I'd just name this DP_TEST_DYNAMIC_RANGE_CEA...
>> 
>> > +# define DP_VESA_RANGE                      (0 << 3)
>> > +# define DP_CEA_RANGE                       (1 << 3)
>> 
>> ...and drop these two. But whatever.
>>
>
> But then how do I check for VESA?

...if it's not CEA, it's VESA?

>  
>> > +# define DP_TEST_YCBCR_COEFFICIENTS         (1 << 4)
>> > +# define DP_YCBCR_COEFFICIENTS_ITU601       (0 << 4)
>> > +# define DP_YCBCR_COEFFICIENTS_ITU709       (1 << 4)
>> > +# define DP_TEST_BIT_DEPTH_MASK             (7 << 5)
>> > +# define DP_TEST_BIT_DEPTH_SHIFT            5
>> > +# define DP_TEST_BIT_DEPTH_6                (0 << 5)
>> > +# define DP_TEST_BIT_DEPTH_8                (1 << 5)
>> > +# define DP_TEST_BIT_DEPTH_10               (2 << 5)
>> > +# define DP_TEST_BIT_DEPTH_12               (3 << 5)
>> > +# define DP_TEST_BIT_DEPTH_16               (4 << 5)
>> 
>> Blank line here.
>>
>
> This blank line is on purpose since I am starting a diff register

There was *no* blank line here, I meant you should add a blank line
here.

>
> Manasi
>
>  
>> > +#define DP_TEST_MISC1                       0x233
>> > +# define DP_TEST_REFRESH_DENOMINATOR        (1 << 0)
>> > +# define REFRESH_DENOMINATOR_1              0x0
>> > +# define REFRESH_DENOMINATOR_1_001          0x1
>> 
>> DP_ prefix.
>> 
>> > +# define DP_TEST_INTERLACED                 (1 << 1)
>> > +# define DP_NON_INTERLACED                  (0 << 1)
>> > +# define DP_INTERLACED                      (1 << 1)
>> 
>> No need for these two. DP_TEST_INTERLACED is sufficient.
>> 
>> > +
>> > +#define DP_TEST_REFRESH_RATE_NUMERATOR      0x234
>> >  
>> >  #define DP_TEST_CRC_R_CR		    0x240
>> >  #define DP_TEST_CRC_G_Y			    0x242
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel