Message-ID: <e8529747-0ac3-a739-f5b1-5984ff71eb1c@synopsys.com>
Date:   Mon, 9 Jan 2017 11:11:01 +0000
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     "Sharma, Shashank" <shashank.sharma@...el.com>
CC:     dri-devel <dri-devel@...ts.freedesktop.org>,
        <Jose.Abreu@...opsys.com>, <daniel.vetter@...el.com>,
        <linux-kernel@...r.kernel.org>, <ville.syrjala@...ux.intel.com>,
        <shashidhar.hiremath@...el.com>, <indranil.mukherjee@...el.com>
Subject: Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

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@...opsys.com>
>> Cc: Carlos Palminha <palminha@...opsys.com>
>> Cc: Daniel Vetter <daniel.vetter@...el.com>
>> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
>> Cc: Sean Paul <seanpaul@...omium.org>
>> Cc: David Airlie <airlied@...ux.ie>
>> Cc: dri-devel@...ts.freedesktop.org
>> Cc: linux-kernel@...r.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]

Powered by blists - more mailing lists