Message-ID: <b59b4f07-c0fb-db9f-edee-ae95695ceeab@intel.com>
Date:   Mon, 9 Jan 2017 18:58:04 +0530
From:   "Sharma, Shashank" <shashank.sharma@...el.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        daniel.vetter@...el.com, linux-kernel@...r.kernel.org,
        ville.syrjala@...ux.intel.com, shashidhar.hiremath@...el.com,
        indranil.mukherjee@...el.com, benjamin.widawsky@...el.com
Subject: Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

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@...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.
>> 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]
>>>

Powered by blists - more mailing lists