Message-ID: <98723d3a-7f9e-44f5-b51e-077552c37845@intel.com>
Date:   Mon, 9 Jan 2017 18:15:30 +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 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.
>> 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]
>

Powered by blists - more mailing lists