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@xxxxxxxxxxxx] >>> Sent: Wednesday, August 3, 2016 6:38 PM >>> To: Daniel Vetter <daniel@xxxxxxxx>; Sharma, Shashank >>> <shashank.sharma@xxxxxxxxx> >>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >>> dri-devel@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel >>> <daniel.vetter@xxxxxxxxx>; Carlos Palminha >>> <CARLOS.PALMINHA@xxxxxxxxxxxx> >>> 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@60 4:3 >>>>> VIC 3 = 720x480@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@xxxxxxxxxxxxxxxxxxxxx >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> Best regards, >>> Jose Miguel Abreu > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
