Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1554039 > unrolled thread

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

Started by"Sharma, Shashank" <shashank.sharma@intel.com>
First post2017-01-09 06:30 +0100
Last post2017-01-09 14:30 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB "Sharma, Shashank" <shashank.sharma@intel.com> - 2017-01-09 06:30 +0100
    Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-09 12:20 +0100
      Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB "Sharma, Shashank" <shashank.sharma@intel.com> - 2017-01-09 13:50 +0100
        Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB "Sharma, Shashank" <shashank.sharma@intel.com> - 2017-01-09 14:30 +0100
        Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-09 14:30 +0100

#1554039 — Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

From"Sharma, Shashank" <shashank.sharma@intel.com>
Date2017-01-09 06:30 +0100
SubjectRe: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB
Message-ID<sXAGR-8n5-1@gated-at.bofh.it>
Regards

Shashank


On 12/30/2016 10:23 PM, Jose Abreu wrote:
> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> According to the spec the EDID may contain two blocks that
> signal this sampling mode:
> 	- YCbCr 4:2:0 Video Data Block
> 	- YCbCr 4:2:0 Video Capability Map Data Block
>
> The video data block contains the list of vic's were
> only YCbCr 4:2:0 sampling mode shall be used while the
> video capability map data block contains a mask were
> YCbCr 4:2:0 sampling mode may be used.
>
> This RFC patch adds support for parsing these two new blocks
> and introduces new flags to signal the drivers if the
> mode is 4:2:0'only or 4:2:0'able.
>
> The reason this is still a RFC is because there is no
> reference in kernel for this new sampling mode (specially in
> AVI infoframe part), so, I was hoping to hear some feedback
> first.
>
> Tested in a HDMI 2.0 compliance scenario.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/drm_modes.c |  10 +++-
>   include/uapi/drm/drm_mode.h |   6 ++
>   3 files changed, 151 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 67d6a73..6ce1a38 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>   #define VENDOR_BLOCK    0x03
>   #define SPEAKER_BLOCK	0x04
>   #define VIDEO_CAPABILITY_BLOCK	0x07
> +#define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_420	0x0F
>   #define EDID_BASIC_AUDIO	(1 << 6)
>   #define EDID_CEA_YCRCB444	(1 << 5)
>   #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>   	return modes;
>   }
>   
> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *newmode;
> +
> +	if (!drm_valid_cea_vic(vic))
> +		return 0;
> +
> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
Sorry to start the review late, I missed this mail chain. It would be 
great if you can please keep me in CC for this chain.

Practically, YUV420 modes are being used for 4k and UHD video modes. Now 
here, when we want to
add these modes from edid_cea_db, using the VIC index, we should have 
full list of cea_modes from 1 - 107
Particularly 93-107 ( which is for new 38x21 and 40x21 modes, added in 
CEA-861-F). right now, edid_cea_modes
cant index 4k modes, so practically this this patch series will do 
nothing (even though its doing everything right)

To handle this scenario, I had added a patch series 
https://patchwork.freedesktop.org/patch/119627/
(complete the cea modedb (VIC=65 onwards)) Now, this patch series had 
dependency on new aspect ratios
being added in CEA-861-F which I tried to add in series 
(https://patchwork.freedesktop.org/patch/116095/)
Which was added and later reverted by Ville 
(https://patchwork.freedesktop.org/patch/119808/).

In short, the method/sequence for effective development would be:
- Add aspect ratio support in DRM
- Add HDMI 2.0 (CEA-861-F) aspect ratios 
(https://patchwork.freedesktop.org/patch/116095/)
- Complete edid_cea_modes, adding new modes as per 4k VICs 
(https://patchwork.freedesktop.org/patch/119627/ )
- Parse these modes from 420_vdb and 420_vcb using edid_cea_modes db[]  
(This patch series)

And that we should re-prioritize the aspect ratio handling to target YUV 
420 handling from CEA blocks.
Shashank
> +	if (!newmode)
> +		return 0;
> +
> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> +	drm_mode_probed_add(connector, newmode);
> +
> +	return 1;
> +}
> +
> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len)
> +{
> +	int modes = 0, i;
> +
> +	for (i = 0; i < svds_len; i++)
> +		modes += add_420_mode(connector, svds[i]);
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len, const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i, j;
> +
> +	for (i = 0; i < svds_len; i++) {
> +		u8 mask = svds[i];
> +		for (j = 0; j < 8; j++) {
> +			if (mask & (1 << j)) {
> +				newmode = drm_display_mode_from_vic_index(
> +						connector, video_db, video_len,
> +						i * 8 + j);
> +				if (newmode) {
> +					newmode->flags |= DRM_MODE_FLAG_420;
> +					drm_mode_probed_add(connector, newmode);
> +					modes++;
> +				}
> +			}
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> +		const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i;
> +
> +	for (i = 0; i < video_len; i++) {
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +				video_len, i);
> +		if (newmode) {
> +			newmode->flags |= DRM_MODE_FLAG_420;
> +			drm_mode_probed_add(connector, newmode);
> +			modes++;
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> +		u8 video_len)
> +{
> +	int modes = 0;
> +
> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> +
> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> +				video_db, video_len);
> +	else if (vcb) /* All modes support 4:2:0 mode */
> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> +
> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> +	return modes;
> +}
> +
>   /*
>    * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>    * @connector: connector corresponding to the HDMI sink
> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>   }
>   
>   static int
> +cea_db_extended_tag(const u8 *db)
> +{
> +	return db[1];
> +}
> +
> +static int
>   cea_revision(const u8 *cea)
>   {
>   	return cea[1];
> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   	return hdmi_id == HDMI_IEEE_OUI;
>   }
>   
> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
>   #define for_each_cea_db(cea, i, start, end) \
>   	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>   
> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   add_cea_modes(struct drm_connector *connector, struct edid *edid)
>   {
>   	const u8 *cea = drm_find_cea_extension(edid);
> -	const u8 *db, *hdmi = NULL, *video = NULL;
> -	u8 dbl, hdmi_len, video_len = 0;
> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> +	      *vcb420 = NULL;
> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>   	int modes = 0;
>   
>   	if (cea && cea_revision(cea) >= 3) {
> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   				hdmi = db;
>   				hdmi_len = dbl;
>   			}
> +			else if (cea_db_is_hdmi_vdb420(db)) {
> +				vdb420 = db;
> +				vdb420_len = dbl;
> +			}
> +			else if (cea_db_is_hdmi_vcb420(db)) {
> +				vcb420 = db;
> +				vcb420_len = dbl;
> +			}
>   		}
>   	}
>   
> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>   					    video_len);
>   
> +	if (vdb420 || vcb420)
> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> +				vcb420, vcb420_len, video, video_len);
> +
>   	return modes;
>   }
>   
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac6a352..53c65f6 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>   	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>   		return false;
>   
> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> +		return false;
> +
>   	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>   }
>   EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>   bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>   					const struct drm_display_mode *mode2)
>   {
> +	unsigned int flags_mask =
> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> +
>   	if (mode1->hdisplay == mode2->hdisplay &&
>   	    mode1->hsync_start == mode2->hsync_start &&
>   	    mode1->hsync_end == mode2->hsync_end &&
> @@ -995,8 +1002,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>   	    mode1->vsync_end == mode2->vsync_end &&
>   	    mode1->vtotal == mode2->vtotal &&
>   	    mode1->vscan == mode2->vscan &&
> -	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>   		return true;
>   
>   	return false;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2..dc8e285 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -84,6 +84,12 @@
>   #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>   #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>   #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> +/*
> + * HDMI 2.0
> + */
> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> +#define  DRM_MODE_FLAG_420			(1<<19)
> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>   
>   /* Picture aspect ratio options */
>   #define DRM_MODE_PICTURE_ASPECT_NONE		0

[toc] | [next] | [standalone]

#1554223

FromJose Abreu <Jose.Abreu@synopsys.com>
Date2017-01-09 12:20 +0100
Message-ID<sXG9A-3uG-17@gated-at.bofh.it>
In reply to#1554039
Hi Shashank,


Thanks for the review.


On 09-01-2017 05:22, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>> According to the spec the EDID may contain two blocks that
>> signal this sampling mode:
>>     - YCbCr 4:2:0 Video Data Block
>>     - YCbCr 4:2:0 Video Capability Map Data Block
>>
>> The video data block contains the list of vic's were
>> only YCbCr 4:2:0 sampling mode shall be used while the
>> video capability map data block contains a mask were
>> YCbCr 4:2:0 sampling mode may be used.
>>
>> This RFC patch adds support for parsing these two new blocks
>> and introduces new flags to signal the drivers if the
>> mode is 4:2:0'only or 4:2:0'able.
>>
>> The reason this is still a RFC is because there is no
>> reference in kernel for this new sampling mode (specially in
>> AVI infoframe part), so, I was hoping to hear some feedback
>> first.
>>
>> Tested in a HDMI 2.0 compliance scenario.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 139
>> +++++++++++++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/drm_modes.c |  10 +++-
>>   include/uapi/drm/drm_mode.h |   6 ++
>>   3 files changed, 151 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c
>> b/drivers/gpu/drm/drm_edid.c
>> index 67d6a73..6ce1a38 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>> drm_connector *connector,
>>   #define VENDOR_BLOCK    0x03
>>   #define SPEAKER_BLOCK    0x04
>>   #define VIDEO_CAPABILITY_BLOCK    0x07
>> +#define VIDEO_DATA_BLOCK_420    0x0E
>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>   #define EDID_BASIC_AUDIO    (1 << 6)
>>   #define EDID_CEA_YCRCB444    (1 << 5)
>>   #define EDID_CEA_YCRCB422    (1 << 4)
>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>> drm_connector *connector, u16 structure,
>>       return modes;
>>   }
>>   +static int add_420_mode(struct drm_connector *connector, u8
>> vic)
>> +{
>> +    struct drm_device *dev = connector->dev;
>> +    struct drm_display_mode *newmode;
>> +
>> +    if (!drm_valid_cea_vic(vic))
>> +        return 0;
>> +
>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> Sorry to start the review late, I missed this mail chain. It
> would be great if you can please keep me in CC for this chain.

Sure. Will do that next time.

>
> Practically, YUV420 modes are being used for 4k and UHD video
> modes. Now here, when we want to
> add these modes from edid_cea_db, using the VIC index, we
> should have full list of cea_modes from 1 - 107
> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
> added in CEA-861-F). right now, edid_cea_modes
> cant index 4k modes, so practically this this patch series will
> do nothing (even though its doing everything right)

This is correct but not entirely true. I realize 4:2:0 is mostly
used in 4k modes but it can also be used in any other video mode,
as long as it is declared in the VCB.

>
> To handle this scenario, I had added a patch series
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
> (complete the cea modedb (VIC=65 onwards)) Now, this patch
> series had dependency on new aspect ratios
> being added in CEA-861-F which I tried to add in series
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
> )
> Which was added and later reverted by Ville
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
> ).

Yes, I remember that. If it was breaking userspace then there was
nothing left to do, revert was needed. I thing we should take
your patch and rework/extend it so that userspace does not break
as this is a most welcome feature. The new HDMI spec is almost
ready, and yet, 2.0 features are still missing from the kernel.
We should take advantage from our capability of accessing these
specs, test equipment, compliance equipment ... and submit
patches for these new features :)

>
> In short, the method/sequence for effective development would be:
> - Add aspect ratio support in DRM
> - Add HDMI 2.0 (CEA-861-F) aspect ratios
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
> )
> - Complete edid_cea_modes, adding new modes as per 4k VICs
> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e= 
> )
> - Parse these modes from 420_vdb and 420_vcb using
> edid_cea_modes db[]  (This patch series)
>

I agree but this rfc does not depend (in terms of code) of any
other patches. The vcb parsing part can be used right now, as for
the vdb part we will have to wait until vic's list is completed.
Thats one of the reasons i sent it in RFC: So that i could ear
some comments before submitting a "real" patch.

Best regards,
Jose Miguel Abreu

> And that we should re-prioritize the aspect ratio handling to
> target YUV 420 handling from CEA blocks.
> Shashank

[snip]

[toc] | [prev] | [next] | [standalone]

#1554278

From"Sharma, Shashank" <shashank.sharma@intel.com>
Date2017-01-09 13:50 +0100
Message-ID<sXHyF-4eL-13@gated-at.bofh.it>
In reply to#1554223
Regards

Shashank


On 1/9/2017 4:41 PM, Jose Abreu wrote:
> Hi Shashank,
>
>
> Thanks for the review.
>
>
> On 09-01-2017 05:22, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>>> According to the spec the EDID may contain two blocks that
>>> signal this sampling mode:
>>>      - YCbCr 4:2:0 Video Data Block
>>>      - YCbCr 4:2:0 Video Capability Map Data Block
>>>
>>> The video data block contains the list of vic's were
>>> only YCbCr 4:2:0 sampling mode shall be used while the
>>> video capability map data block contains a mask were
>>> YCbCr 4:2:0 sampling mode may be used.
>>>
>>> This RFC patch adds support for parsing these two new blocks
>>> and introduces new flags to signal the drivers if the
>>> mode is 4:2:0'only or 4:2:0'able.
>>>
>>> The reason this is still a RFC is because there is no
>>> reference in kernel for this new sampling mode (specially in
>>> AVI infoframe part), so, I was hoping to hear some feedback
>>> first.
>>>
>>> Tested in a HDMI 2.0 compliance scenario.
>>>
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>    drivers/gpu/drm/drm_edid.c  | 139
>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>    drivers/gpu/drm/drm_modes.c |  10 +++-
>>>    include/uapi/drm/drm_mode.h |   6 ++
>>>    3 files changed, 151 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c
>>> b/drivers/gpu/drm/drm_edid.c
>>> index 67d6a73..6ce1a38 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>>> drm_connector *connector,
>>>    #define VENDOR_BLOCK    0x03
>>>    #define SPEAKER_BLOCK    0x04
>>>    #define VIDEO_CAPABILITY_BLOCK    0x07
>>> +#define VIDEO_DATA_BLOCK_420    0x0E
>>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>>    #define EDID_BASIC_AUDIO    (1 << 6)
>>>    #define EDID_CEA_YCRCB444    (1 << 5)
>>>    #define EDID_CEA_YCRCB422    (1 << 4)
>>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>>> drm_connector *connector, u16 structure,
>>>        return modes;
>>>    }
>>>    +static int add_420_mode(struct drm_connector *connector, u8
>>> vic)
>>> +{
>>> +    struct drm_device *dev = connector->dev;
>>> +    struct drm_display_mode *newmode;
>>> +
>>> +    if (!drm_valid_cea_vic(vic))
>>> +        return 0;
>>> +
>>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>> Sorry to start the review late, I missed this mail chain. It
>> would be great if you can please keep me in CC for this chain.
> Sure. Will do that next time.
>
>> Practically, YUV420 modes are being used for 4k and UHD video
>> modes. Now here, when we want to
>> add these modes from edid_cea_db, using the VIC index, we
>> should have full list of cea_modes from 1 - 107
>> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
>> added in CEA-861-F). right now, edid_cea_modes
>> cant index 4k modes, so practically this this patch series will
>> do nothing (even though its doing everything right)
> This is correct but not entirely true. I realize 4:2:0 is mostly
> used in 4k modes but it can also be used in any other video mode,
> as long as it is declared in the VCB.
I agree (that's why I called it practically).
As such I doubt we will find anything less than a 4k here, coz HDMI 1.4b 
itself can driver 4k@30.
So the biggest benefit of YUV 420, which is half the clock, is mostly 
useful in 4k@60 and above.
I guess we will see more cases of deep-color pixels at non-4k modes soon.
>> To handle this scenario, I had added a patch series
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>> (complete the cea modedb (VIC=65 onwards)) Now, this patch
>> series had dependency on new aspect ratios
>> being added in CEA-861-F which I tried to add in series
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>> )
>> Which was added and later reverted by Ville
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
>> ).
> Yes, I remember that. If it was breaking userspace then there was
> nothing left to do, revert was needed.
As we discovered over the discussions, It dint break anything as such :)
But it made the behavior change for some SW's (which was expected), 
Anyways its gone now.
> I thing we should take
> your patch and rework/extend it so that userspace does not break
> as this is a most welcome feature. The new HDMI spec is almost
> ready, and yet, 2.0 features are still missing from the kernel.
> We should take advantage from our capability of accessing these
> specs, test equipment, compliance equipment ... and submit
> patches for these new features :)
I know. Unfortunately, last time when we spoke about it,  we were 
required to write a full stack code across kernel, drm, libdrm and X 
level, as keeping it
under a cap was not accepted. This seems to be a long term plan to me.
>> In short, the method/sequence for effective development would be:
>> - Add aspect ratio support in DRM
>> - Add HDMI 2.0 (CEA-861-F) aspect ratios
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>> )
>> - Complete edid_cea_modes, adding new modes as per 4k VICs
>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>> )
>> - Parse these modes from 420_vdb and 420_vcb using
>> edid_cea_modes db[]  (This patch series)
>>
> I agree but this rfc does not depend (in terms of code) of any
> other patches. The vcb parsing part can be used right now, as for
> the vdb part we will have to wait until vic's list is completed.
> Thats one of the reasons i sent it in RFC: So that i could ear
> some comments before submitting a "real" patch.
Right, its so real code, that I forget almost every-time that its a RFC :-)
But I thought its the right place to call, that, we wont be able to test 
4k YUV 420 yet, until we finish the modedb.

- Shashank
>
> Best regards,
> Jose Miguel Abreu
>
>> And that we should re-prioritize the aspect ratio handling to
>> target YUV 420 handling from CEA blocks.
>> Shashank
> [snip]
>

[toc] | [prev] | [next] | [standalone]

#1554306

From"Sharma, Shashank" <shashank.sharma@intel.com>
Date2017-01-09 14:30 +0100
Message-ID<sXIbo-4KA-15@gated-at.bofh.it>
In reply to#1554278
Regards

Shashank


On 1/9/2017 6:53 PM, Jose Abreu wrote:
> Hi Shashank,
>
>
> On 09-01-2017 12:45, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/9/2017 4:41 PM, Jose Abreu wrote:
>>> Hi Shashank,
>>>
>>>
>>> Thanks for the review.
>>>
>>>
>>> On 09-01-2017 05:22, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>>>>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>>>>> According to the spec the EDID may contain two blocks that
>>>>> signal this sampling mode:
>>>>>       - YCbCr 4:2:0 Video Data Block
>>>>>       - YCbCr 4:2:0 Video Capability Map Data Block
>>>>>
>>>>> The video data block contains the list of vic's were
>>>>> only YCbCr 4:2:0 sampling mode shall be used while the
>>>>> video capability map data block contains a mask were
>>>>> YCbCr 4:2:0 sampling mode may be used.
>>>>>
>>>>> This RFC patch adds support for parsing these two new blocks
>>>>> and introduces new flags to signal the drivers if the
>>>>> mode is 4:2:0'only or 4:2:0'able.
>>>>>
>>>>> The reason this is still a RFC is because there is no
>>>>> reference in kernel for this new sampling mode (specially in
>>>>> AVI infoframe part), so, I was hoping to hear some feedback
>>>>> first.
>>>>>
>>>>> Tested in a HDMI 2.0 compliance scenario.
>>>>>
>>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>>> Cc: David Airlie <airlied@linux.ie>
>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> ---
>>>>>     drivers/gpu/drm/drm_edid.c  | 139
>>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>>     drivers/gpu/drm/drm_modes.c |  10 +++-
>>>>>     include/uapi/drm/drm_mode.h |   6 ++
>>>>>     3 files changed, 151 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_edid.c
>>>>> b/drivers/gpu/drm/drm_edid.c
>>>>> index 67d6a73..6ce1a38 100644
>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>>>>> drm_connector *connector,
>>>>>     #define VENDOR_BLOCK    0x03
>>>>>     #define SPEAKER_BLOCK    0x04
>>>>>     #define VIDEO_CAPABILITY_BLOCK    0x07
>>>>> +#define VIDEO_DATA_BLOCK_420    0x0E
>>>>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>>>>     #define EDID_BASIC_AUDIO    (1 << 6)
>>>>>     #define EDID_CEA_YCRCB444    (1 << 5)
>>>>>     #define EDID_CEA_YCRCB422    (1 << 4)
>>>>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>>>>> drm_connector *connector, u16 structure,
>>>>>         return modes;
>>>>>     }
>>>>>     +static int add_420_mode(struct drm_connector *connector, u8
>>>>> vic)
>>>>> +{
>>>>> +    struct drm_device *dev = connector->dev;
>>>>> +    struct drm_display_mode *newmode;
>>>>> +
>>>>> +    if (!drm_valid_cea_vic(vic))
>>>>> +        return 0;
>>>>> +
>>>>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>>>> Sorry to start the review late, I missed this mail chain. It
>>>> would be great if you can please keep me in CC for this chain.
>>> Sure. Will do that next time.
>>>
>>>> Practically, YUV420 modes are being used for 4k and UHD video
>>>> modes. Now here, when we want to
>>>> add these modes from edid_cea_db, using the VIC index, we
>>>> should have full list of cea_modes from 1 - 107
>>>> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
>>>> added in CEA-861-F). right now, edid_cea_modes
>>>> cant index 4k modes, so practically this this patch series will
>>>> do nothing (even though its doing everything right)
>>> This is correct but not entirely true. I realize 4:2:0 is mostly
>>> used in 4k modes but it can also be used in any other video mode,
>>> as long as it is declared in the VCB.
>> I agree (that's why I called it practically).
>> As such I doubt we will find anything less than a 4k here, coz
>> HDMI 1.4b itself can driver 4k@30.
>> So the biggest benefit of YUV 420, which is half the clock, is
>> mostly useful in 4k@60 and above.
>> I guess we will see more cases of deep-color pixels at non-4k
>> modes soon.
>>>> To handle this scenario, I had added a patch series
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>>
>>>> (complete the cea modedb (VIC=65 onwards)) Now, this patch
>>>> series had dependency on new aspect ratios
>>>> being added in CEA-861-F which I tried to add in series
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>>
>>>> )
>>>> Which was added and later reverted by Ville
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
>>>>
>>>> ).
>>> Yes, I remember that. If it was breaking userspace then there was
>>> nothing left to do, revert was needed.
>> As we discovered over the discussions, It dint break anything
>> as such :)
>> But it made the behavior change for some SW's (which was
>> expected), Anyways its gone now.
>>> I thing we should take
>>> your patch and rework/extend it so that userspace does not break
>>> as this is a most welcome feature. The new HDMI spec is almost
>>> ready, and yet, 2.0 features are still missing from the kernel.
>>> We should take advantage from our capability of accessing these
>>> specs, test equipment, compliance equipment ... and submit
>>> patches for these new features :)
>> I know. Unfortunately, last time when we spoke about it,  we
>> were required to write a full stack code across kernel, drm,
>> libdrm and X level, as keeping it
>> under a cap was not accepted. This seems to be a long term plan
>> to me.
> I really think we should make the exposure of this new 2.0
> features optional. Change the drivers and drm core first and then
> move to userland. We can't expect user to deploy the changes at
> the same time we apply them to kernel.
@Daniel, do you think we should re-visit our decision about keeping 
aspect ratio support under a cap, and add the kernel
mode support, so that it could unblock other things like this vcb and 
420_vdb parsing ?

- Shashank
>>>> In short, the method/sequence for effective development would
>>>> be:
>>>> - Add aspect ratio support in DRM
>>>> - Add HDMI 2.0 (CEA-861-F) aspect ratios
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>>
>>>> )
>>>> - Complete edid_cea_modes, adding new modes as per 4k VICs
>>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>>
>>>> )
>>>> - Parse these modes from 420_vdb and 420_vcb using
>>>> edid_cea_modes db[]  (This patch series)
>>>>
>>> I agree but this rfc does not depend (in terms of code) of any
>>> other patches. The vcb parsing part can be used right now, as for
>>> the vdb part we will have to wait until vic's list is completed.
>>> Thats one of the reasons i sent it in RFC: So that i could ear
>>> some comments before submitting a "real" patch.
>> Right, its so real code, that I forget almost every-time that
>> its a RFC :-)
>> But I thought its the right place to call, that, we wont be
>> able to test 4k YUV 420 yet, until we finish the modedb.
> At the time I tested 420 in full-hd, I think. In order to test
> vcb parsing. But yes, normal tv's wont have this kind of strange
> EDIDs. And, modedb will increase even more in the next months: 8k
> is almost out :)
>
> Best regards,
> Jose Miguel Abreu
>
>> - Shashank
>>> Best regards,
>>> Jose Miguel Abreu
>>>
>>>> And that we should re-prioritize the aspect ratio handling to
>>>> target YUV 420 handling from CEA blocks.
>>>> Shashank
>>> [snip]
>>>

[toc] | [prev] | [next] | [standalone]

#1554307

FromJose Abreu <Jose.Abreu@synopsys.com>
Date2017-01-09 14:30 +0100
Message-ID<sXIbo-4KA-17@gated-at.bofh.it>
In reply to#1554278
Hi Shashank,


On 09-01-2017 12:45, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 1/9/2017 4:41 PM, Jose Abreu wrote:
>> Hi Shashank,
>>
>>
>> Thanks for the review.
>>
>>
>> On 09-01-2017 05:22, Sharma, Shashank wrote:
>>> Regards
>>>
>>> Shashank
>>>
>>>
>>> On 12/30/2016 10:23 PM, Jose Abreu wrote:
>>>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>>>> According to the spec the EDID may contain two blocks that
>>>> signal this sampling mode:
>>>>      - YCbCr 4:2:0 Video Data Block
>>>>      - YCbCr 4:2:0 Video Capability Map Data Block
>>>>
>>>> The video data block contains the list of vic's were
>>>> only YCbCr 4:2:0 sampling mode shall be used while the
>>>> video capability map data block contains a mask were
>>>> YCbCr 4:2:0 sampling mode may be used.
>>>>
>>>> This RFC patch adds support for parsing these two new blocks
>>>> and introduces new flags to signal the drivers if the
>>>> mode is 4:2:0'only or 4:2:0'able.
>>>>
>>>> The reason this is still a RFC is because there is no
>>>> reference in kernel for this new sampling mode (specially in
>>>> AVI infoframe part), so, I was hoping to hear some feedback
>>>> first.
>>>>
>>>> Tested in a HDMI 2.0 compliance scenario.
>>>>
>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>> Cc: David Airlie <airlied@linux.ie>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c  | 139
>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>    drivers/gpu/drm/drm_modes.c |  10 +++-
>>>>    include/uapi/drm/drm_mode.h |   6 ++
>>>>    3 files changed, 151 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c
>>>> b/drivers/gpu/drm/drm_edid.c
>>>> index 67d6a73..6ce1a38 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct
>>>> drm_connector *connector,
>>>>    #define VENDOR_BLOCK    0x03
>>>>    #define SPEAKER_BLOCK    0x04
>>>>    #define VIDEO_CAPABILITY_BLOCK    0x07
>>>> +#define VIDEO_DATA_BLOCK_420    0x0E
>>>> +#define VIDEO_CAP_BLOCK_420    0x0F
>>>>    #define EDID_BASIC_AUDIO    (1 << 6)
>>>>    #define EDID_CEA_YCRCB444    (1 << 5)
>>>>    #define EDID_CEA_YCRCB422    (1 << 4)
>>>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct
>>>> drm_connector *connector, u16 structure,
>>>>        return modes;
>>>>    }
>>>>    +static int add_420_mode(struct drm_connector *connector, u8
>>>> vic)
>>>> +{
>>>> +    struct drm_device *dev = connector->dev;
>>>> +    struct drm_display_mode *newmode;
>>>> +
>>>> +    if (!drm_valid_cea_vic(vic))
>>>> +        return 0;
>>>> +
>>>> +    newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>>> Sorry to start the review late, I missed this mail chain. It
>>> would be great if you can please keep me in CC for this chain.
>> Sure. Will do that next time.
>>
>>> Practically, YUV420 modes are being used for 4k and UHD video
>>> modes. Now here, when we want to
>>> add these modes from edid_cea_db, using the VIC index, we
>>> should have full list of cea_modes from 1 - 107
>>> Particularly 93-107 ( which is for new 38x21 and 40x21 modes,
>>> added in CEA-861-F). right now, edid_cea_modes
>>> cant index 4k modes, so practically this this patch series will
>>> do nothing (even though its doing everything right)
>> This is correct but not entirely true. I realize 4:2:0 is mostly
>> used in 4k modes but it can also be used in any other video mode,
>> as long as it is declared in the VCB.
> I agree (that's why I called it practically).
> As such I doubt we will find anything less than a 4k here, coz
> HDMI 1.4b itself can driver 4k@30.
> So the biggest benefit of YUV 420, which is half the clock, is
> mostly useful in 4k@60 and above.
> I guess we will see more cases of deep-color pixels at non-4k
> modes soon.
>>> To handle this scenario, I had added a patch series
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>
>>> (complete the cea modedb (VIC=65 onwards)) Now, this patch
>>> series had dependency on new aspect ratios
>>> being added in CEA-861-F which I tried to add in series
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>
>>> )
>>> Which was added and later reverted by Ville
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119808_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=FAa6aHQ_HjlaVRzDm282p9bSY_tBiN1PngZBhsTqYdI&e=
>>>
>>> ).
>> Yes, I remember that. If it was breaking userspace then there was
>> nothing left to do, revert was needed.
> As we discovered over the discussions, It dint break anything
> as such :)
> But it made the behavior change for some SW's (which was
> expected), Anyways its gone now.
>> I thing we should take
>> your patch and rework/extend it so that userspace does not break
>> as this is a most welcome feature. The new HDMI spec is almost
>> ready, and yet, 2.0 features are still missing from the kernel.
>> We should take advantage from our capability of accessing these
>> specs, test equipment, compliance equipment ... and submit
>> patches for these new features :)
> I know. Unfortunately, last time when we spoke about it,  we
> were required to write a full stack code across kernel, drm,
> libdrm and X level, as keeping it
> under a cap was not accepted. This seems to be a long term plan
> to me.

I really think we should make the exposure of this new 2.0
features optional. Change the drivers and drm core first and then
move to userland. We can't expect user to deploy the changes at
the same time we apply them to kernel.

>>> In short, the method/sequence for effective development would
>>> be:
>>> - Add aspect ratio support in DRM
>>> - Add HDMI 2.0 (CEA-861-F) aspect ratios
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_116095_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=PX2M1hM2cF_aWiDe5oZeLWjsOgL-hvUR54Ion9kYMxM&e=
>>>
>>> )
>>> - Complete edid_cea_modes, adding new modes as per 4k VICs
>>> (https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.freedesktop.org_patch_119627_&d=DgICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=6JwJ_PEZj3MSeRaLTHuLtPZCb0uPPjUqR5raxY_qILI&s=DPBQ2MpLgngWGJEOg2v9CQhg2CSf_4LOIAC30B6AAyg&e=
>>>
>>> )
>>> - Parse these modes from 420_vdb and 420_vcb using
>>> edid_cea_modes db[]  (This patch series)
>>>
>> I agree but this rfc does not depend (in terms of code) of any
>> other patches. The vcb parsing part can be used right now, as for
>> the vdb part we will have to wait until vic's list is completed.
>> Thats one of the reasons i sent it in RFC: So that i could ear
>> some comments before submitting a "real" patch.
> Right, its so real code, that I forget almost every-time that
> its a RFC :-)
> But I thought its the right place to call, that, we wont be
> able to test 4k YUV 420 yet, until we finish the modedb.

At the time I tested 420 in full-hd, I think. In order to test
vcb parsing. But yes, normal tv's wont have this kind of strange
EDIDs. And, modedb will increase even more in the next months: 8k
is almost out :)

Best regards,
Jose Miguel Abreu

>
> - Shashank
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> And that we should re-prioritize the aspect ratio handling to
>>> target YUV 420 handling from CEA blocks.
>>> Shashank
>> [snip]
>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web