[Intel-gfx] [PATCH 0/4]: Picture aspect ratio support in DRM layer

Thu Aug 4 15:13:34 UTC 2016

On 04-08-2016 15:29, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 8/4/2016 7:46 PM, Jose Abreu wrote:
>> Hi Sharma,
>>
>>
>> On 03-08-2016 16:47, Sharma, Shashank wrote:
>>> Hello Joes,
>>>> I've also seen this before and I am using them in order to
>>>> pass HDMI compliance. Without
>>>> these patches the compliance fails. Still, I've made some
>>>> changes which I can submit. I've
>>>> some comments to you (Shashank):
>>> Thanks for addressing these patches. You are welcome to
>>> review the series.
>>> Will it be possible for you, to comment in-line with the
>>> patch code, it's easier that way, and kind of conventional too.
>>>
>>>> Second, you are expecting that the picture aspect field is
>>>> correctly set in the HDMI parsing but, at least in the test
>>>> equipment that I am using, this field is not set by the DRM
>>>> layer because the mode is coming in the detailed timings
>>>> section which does not
>>>> include a picture aspect field. In my implementation I add a
>>>> function which given the mode width and height (fields
>>>> ->width_mm and ->height_mm of mode) computes the aspect
>>>> ratio and populates the field.
>>> Please note that we can run the aspect ratio test cases (7-27
>>> and similar) for CEA modes only. For the modes coming from
>>> DTDs and VSDBs can be with or without aspect ratio.
>>> But the suggestion to initialize all the drm_modes with
>>> ASPECT_RATIO_NONE/DEFAULT is a good one, and it might help
>>> for these modes too.
>>> I think Daniel also had similar suggestion last time, in a
>>> different context.
>> In our compliance equipment the modes are coming from DTD and
>> from the video datablock but the kernel is preferring the DTD
>> modes so we found a way of determining the picture aspect ratio
>> from the DTD section. We do not populate the field with
>> ASPECT_RATIO_NONE/DEFAULT, we populate it given the ratio of
>> width_mm and height_mm that is sent in the DTD (of course if
>> these values are zero we set aspect ratio to none). I think it
>> could be a nice addition to the EDID parsing.
> I agree, I will come up with another patch, which does either
> of this:
> - initialize all the DRM_MODE with aspect ratio default while
> creating the new mode itself
> - initialize all the DRM_MODES  with aspect ratio default while
> parsing the modes.

Ok, please cc me when you send the new patches.

>>>> Third, I am facing some difficulties when using Xserver and
>>>> Xrandr. Using libdrm's modetest application everything works
>>>> ok but with xrandr the aspect ratio gets lost between the
>>>> link DRM
>>>> -> Xserver -> DRM. I set the aspect ratio in the flags field
>>>> when
>>>> passing the mode to user level (just like you do in patch 2)
>>>> but then when the mode is returned and delivered to the DRM
>>>> driver the picture aspect is not present. I think this is
>>>> due to how Xserver or xrandr sets the mode but I am not sure.
>>> I think while parsing the aspect ratio from libdrm to
>>> userspace (X), it's getting lost, and we have to fix your
>>> Xserver implementation.
>>> We had added similar support in our HWComposer, and I guess
>>> it would be required for X and Wayland too, coz finally these
>>> guys issue the modeset.
>>> So May be X server is not handling these flags, ignoring
>>> these flags, and sending the flagless modeset back to libdrm.
>> Do you mean Xserver, the X driver that I am using (which is
>> modesetting), the xrandr or all of them? I guess that if they
>> don't touch the flags field and return the mode exactly the same
>> as it was probed to DRM then it will work as expected.
> I agree, in fact, if the userspace is not even touching the
> flag field, we should get the aspect ratio information intact.
> But if we are populating the aspect ratio information while
> reading the modes from EDID, and passing the right flags to
> userspace, but still we see the modeset doesn't contain the
> right flags, means userspace is clearing the flags or modifying
> the flags. So we should check:
> - While creating a user_mode from kernel_mode, are we
> populating the aspect ratio flags (If you have my patches, this
> shoud work)

Yes, this is correct.

> - These modes are passed to userspace via a get_modes or
> get_connector IOCTLs, so we should check these IOCTLS

I think it is also happening correctly.

> - Once user space sends a modeset, does it set the flags
> properly ?

If it is a mode created by the user then I think not. If it is a
mode that was passed by the kernel then it will if the flags are
not touched. This correctly happens using libdrm's modetest but
it doesn't happen when using xrandr.

> - while creating a kernel_mode from user_mode, during modeset,
> are we preserving these flags ? (if you have my patches, this
> should work)

Again correct if the user does not touch the flags field.

Best regards,
Jose Miguel Abreu

>
> Regards
> Shashank
>> Best regards,
>> Jose Miguel Abreu
>>
>>> Regards
>>> Shashank
>>> -----Original Message-----
>>> From: Jose Abreu [mailto:Jose.Abreu at synopsys.com]
>>> Sent: Wednesday, August 3, 2016 6:38 PM
>>> To: Daniel Vetter <daniel at ffwll.ch>; Sharma, Shashank
>>> <shashank.sharma at intel.com>
>>> Cc: intel-gfx at lists.freedesktop.org;
>>> dri-devel at lists.freedesktop.org; Vetter, Daniel
>>> <daniel.vetter at intel.com>; Carlos Palminha
>>> <CARLOS.PALMINHA at synopsys.com>
>>> Subject: Re: [PATCH 0/4]: Picture aspect ratio support in DRM
>>> layer
>>>
>>> Hi,
>>>
>>>
>>> On 03-08-2016 12:48, Daniel Vetter wrote:
>>>> On Wed, Aug 03, 2016 at 04:26:24PM +0530, Shashank Sharma
>>>> wrote:
>>>>> This patch series adds 4 patches.
>>>>> - The first two patches add aspect ratio support in DRM layes
>>>>> - Next two patches add new aspect ratios defined in CEA-861-F
>>>>>    supported for HDMI 2.0 4k modes.
>>>>>
>>>>> Adding aspect ratio support in DRM layer:
>>>>> - The CEA videmodes contain aspect ratio information, which we
>>>>>    parse when we read the modes from EDID. But while
>>>>> transforming
>>>>>    user_mode to kernel_mode or viceversa, DRM layer lose this
>>>>>    information.
>>>>> - HDMI compliance testing for CEA modes, expects the AVI
>>>>> info frames
>>>>>    to contain exact VIC no for the 'video mode under test'.
>>>>> Now CEA
>>>>>    modes have different VIC for same modes but different
>>>>> aspect ratio
>>>>>    for example:
>>>>>     VIC 2 = 720x480 at 60 4:3
>>>>>     VIC 3 = 720x480 at 60 16:9
>>>>>    In this way, lack of aspect ratio information, can cause
>>>>> wrong VIC
>>>>>    no in AVI IF, causing HDMI complaince test to fail.
>>>>> - This patch set adds code, which embeds the aspect ratio
>>>>> information
>>>>>    also in DRM video mode flags, and uses it while
>>>>> comparing two modes.
>>>>>
>>>>> Adding new aspect ratios for HDMI 2.0
>>>>> - CEA-861-F defines two new aspect ratios, to be used for
>>>>> 4k HDMI 2.0
>>>>>    modes.
>>>>>     - 64:27
>>>>>     - 256:135
>>>>> Last two patches in the series, adds code to handle these
>>>>> new aspect
>>>>> ratios.
>>>>>
>>>>> Shashank Sharma (4):
>>>>>    drm: add picture aspect ratio flags
>>>>>    drm: Add aspect ratio parsing in DRM layer
>>>>>    video: Add new aspect ratios for HDMI 2.0
>>>>>    drm: Add and handle new aspect ratios in DRM layer
>>>> Patch series seems to have 0 changelogs anywhere, but I'm
>>>> pretty sure
>>>> I've seen this before. Please try again and state what
>>>> changed and why
>>>> you are resubmitting this.
>>>> -Daniel
>>> I've also seen this before and I am using them in order to
>>> pass HDMI compliance. Without these patches the compliance
>>> fails.
>>> Still, I've made some changes which I can submit. I've some
>>> comments to you (Shashank):
>>>
>>> First, you add an if condition in
>>> drm_mode_equal_no_clocks_no_stereo() (patch 2) which
>>> unconditionally compares the aspect ratio. But I think that
>>> you have to take into account that some modes handed by the
>>> user to the DRM layer do not initialize this field so I think
>>> the best solution would be to compare the aspect ratios only
>>> when the field is populated (i.e. picture_aspect_ratio !=
>>> HDMI_PICTURE_ASPECT_NONE).
>>>
>>> Second, you are expecting that the picture aspect field is
>>> correctly set in the HDMI parsing but, at least in the test
>>> equipment that I am using, this field is not set by the DRM
>>> layer because the mode is coming in the detailed timings
>>> section which does not include a picture aspect field. In my
>>> implementation I add a function which given the mode width
>>> and height (fields
>>> ->width_mm and ->height_mm of mode) computes the aspect ratio
>>> and
>>> populates the field.
>>>
>>> Third, I am facing some difficulties when using Xserver and
>>> Xrandr. Using libdrm's modetest application everything works
>>> ok but with xrandr the aspect ratio gets lost between the
>>> link DRM
>>> -> Xserver -> DRM. I set the aspect ratio in the flags field
>>> when
>>> passing the mode to user level (just like you do in patch 2)
>>> but then when the mode is returned and delivered to the DRM
>>> driver the picture aspect is not present. I think this is due
>>> to how Xserver or xrandr sets the mode but I am not sure.
>>>
>>> @Daniel, can you give some comments regarding this?
>>>
>>>>>   drivers/gpu/drm/drm_modes.c | 43
>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>   drivers/video/hdmi.c        |  4 ++++
>>>>>   include/linux/hdmi.h        |  2 ++
>>>>>   include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++-----
>>>>>   4 files changed, 68 insertions(+), 5 deletions(-)
>>>>>
>>>>> -- 
>>>>> 1.9.1
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> Best regards,
>>> Jose Miguel Abreu
>