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 at synopsys.com>
>>> Cc: Carlos Palminha <palminha at synopsys.com>
>>> Cc: Daniel Vetter <daniel.vetter at intel.com>
>>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>>> Cc: Sean Paul <seanpaul at chromium.org>
>>> Cc: David Airlie <airlied at linux.ie>
>>> Cc: dri-devel at lists.freedesktop.org
>>> Cc: linux-kernel at 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 at 30.
So the biggest benefit of YUV 420, which is half the clock, is mostly
useful in 4k at 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]
>
