[PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

[Jose Abreu]

Hi,



On 15-11-2016 10:52, Daniel Vetter wrote:
> On Tue, Nov 15, 2016 at 03:36:02PM +0530, Sharma, Shashank wrote:
>> On 11/15/2016 3:30 PM, Daniel Vetter wrote:
>>> On Tue, Nov 15, 2016 at 02:30:47PM +0530, Sharma, Shashank wrote:
>>>> On 11/15/2016 2:21 PM, Daniel Vetter wrote:
>>>>> On Mon, Nov 14, 2016 at 10:26:08PM +0530, Sharma, Shashank wrote:
>>>>>> In any case, I guess addition of a cap for aspect ratio should fix the
>>>>>> current objections for this implementation.
>>>>>>
>>>>>> And I will keep it 0 by default, so that no aspect ratio information is
>>>>>> added until userspace sets the cap to 1 on its own.
>>>>> Note that cap = needs new userspace.
>>>>> -Daniel
>>>> I guess you mean a new libdrm, so yes, I will add this new cap in libdrm.
>>>> Is that what you mean ?
>>> Full stack solution, including enabling in an Xorg driver (or somewhere
>>> else, we also have drm_hwcomposer as an option).
>>>
>>> And because that's probably going to take forever I'm leaning towards
>>> revert again. Ville?
>> Not really, with a kernel cap implementation, the code will be as it was
>> before this patch series ( as good as revert )
>> And then when we want to enable it, we can add the corresponding Xserver
>> patch.
>>
>> I guess the current problem is if is breaks something in some userspace, but
>> if I am loading the flags only when the cap is set, we should be good.
> This is not how new uabi gets merged, see:
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
>
> Userspace must be ready, or it will not land. The entire point of my
> suggestion to just extend the display modes was to avoid the need for
> userspace, but since existing userspace tries to be too clever already,
> that didn't work out.
> Thanks, Daniel

@Ville

I gave my tested by to these patches because I was facing the
same scenario as Shashank: HDMI compliance. I believe we need to
find a better way to handle this instead of just reverting. The
HDMI spec is evolving so we also need to evolve. Of course that
if this is breaking user space then we can revert and wait until
we figure the best way to implement this.

Anyway, this is a wild guess but I think the reason you are
loosing modes may be because of
drm_mode_equal_no_clocks_no_stereo() which is always expecting a
aspect ratio flag to be defined. If there isn't one defined then
it will unconditionally return false, even if the modes are
equal. Can you please test this patch and see if it fixes on your
side? WARNING: Not compile tested

Best regards,
Jose Miguel Abreu

diff --git a/drivers/gpu/drm/drm_modes.c
b/drivers/gpu/drm/drm_modes.c
index ce6eeda..a7973a9 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -971,6 +971,27 @@ bool drm_mode_equal_no_clocks(const struct
drm_display_mode *mode1, const struct
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks);
 
+static bool drm_mode_par_valid(const struct drm_display_mode *mode)
+{
+       switch (mode->picture_aspect_ratio) {
+       case HDMI_PICTURE_ASPECT_4_3:
+       case HDMI_PICTURE_ASPECT_16_9:
+       case HDMI_PICTURE_ASPECT_64_27:
+       case HDMI_PICTURE_ASPECT_256_135:
+               return true;
+       default:
+               return false;
+       }
+}
+
+static bool drm_mode_equal_par(const struct drm_display_mode *mode1,
+                              const struct drm_display_mode *mode2)
+{
+       if (!drm_mode_par_valid(mode1) || !drm_mode_par_valid(mode2))
+               return true;
+       return mode1->picture_aspect_ratio ==
mode2->picture_aspect_ratio;
+}
+
 /**
  * drm_mode_equal_no_clocks_no_stereo - test modes for equality
  * @mode1: first mode
@@ -995,7 +1016,7 @@ bool
drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode
*mode1,
            mode1->vsync_end == mode2->vsync_end &&
            mode1->vtotal == mode2->vtotal &&
            mode1->vscan == mode2->vscan &&
-           mode1->picture_aspect_ratio ==
mode2->picture_aspect_ratio &&
+           drm_mode_equal_par(mode1, mode2) &&
            (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
             (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
                return true;