Message-ID: <d4f5e101-3dd5-2f3a-6c14-6b32ee37c223@redhat.com>
Date:   Fri, 18 Feb 2022 12:54:36 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Simon Ser <contact@...rsion.fr>
Cc:     Emil Velikov <emil.l.velikov@...il.com>,
        Maxime Ripard <mripard@...nel.org>,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        devicetree <devicetree@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        amd-gfx mailing list <amd-gfx@...ts.freedesktop.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-mediatek@...ts.infradead.org,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Alex Deucher <alexander.deucher@....com>,
        Harry Wentland <harry.wentland@....com>,
        LAKML <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [Intel-gfx] [PATCH v8 1/3] gpu: drm: separate panel orientation
 property creating and value setting

Hi,

On 2/18/22 12:39, Simon Ser wrote:
> On Friday, February 18th, 2022 at 11:38, Hans de Goede <hdegoede@...hat.com> wrote:
> 
>> What I'm reading in the above is that it is being considered to allow
>> changing the panel-orientation value after the connector has been made
>> available to userspace; and let userspace know about this through a uevent.
>>
>> I believe that this is a bad idea, it is important to keep in mind here
>> what userspace (e.g. plymouth) uses this prorty for. This property is
>> used to rotate the image being rendered / shown on the framebuffer to
>> adjust for the panel orientation.
>>
>> So now lets assume we apply the correct upside-down orientation later
>> on a device with an upside-down mounted LCD panel. Then on boot the
>> following could happen:
>>
>> 1. amdgpu exports a connector for the LCD panel to userspace without
>> setting panel-orient=upside-down
>> 2. plymouth sees this and renders its splash normally, but since the
>> panel is upside-down it will now actually show upside-down
> 
> At this point amdgpu hasn't probed the connector yet. So the connector
> will be marked as disconnected, and plymouth shouldn't render anything.

If before the initial probe of the connector there is a /dev/dri/card0 
which plymouth can access, then plymouth may at this point decide
to disable any seemingly unused crtcs, which will make the screen go black...

I'm not sure if plymouth will actually do this, but AFAICT this would
not be invalid behavior for a userspace kms consumer to do and I
believe it is likely that mutter will disable unused crtcs.

IMHO it is just a bad idea to register /dev/dri/card0 with userspace
before the initial connector probe is done. Nothing good can come
of that.

If all the exposed connectors initially are going to show up as
disconnected anyways what is the value in registering /dev/dri/card0
with userspace early ?

>> 3. amdgpu adjusts the panel-orient prop to upside-down, sends out
>> uevents
> 
> That's when amdgpu marks the connector as connected. So everything
> should be fine I believe, no bad frame.

See above.

>> 4. Lets assume plymouth handles this well (i) and now adjust its
>> rendering and renders the next frame of the bootsplash 180° rotated
>> to compensate for the panel being upside down. Then from now on
>> the user will see the splash normally
>>
>> So this means that the user will briefly see the bootsplash rendered
>> upside down which IMHO is not acceptable behavior. Also see my footnote
>> about how I seriously doubt plymouth will see the panel-orient change
>> at all.
>>
>> I'm also a bit unsure about:
>>
>> a) How you can register the panel connector with userspace before
>> reading the edid, don't you need the edid to give the physical size +
>> modeline to userspace, which you cannot just leave out ?
> 
> Yup. The KMS EDID property is created before the EDID is read, and is set
> to zero (NULL blob). The width/height in mm and other info are also zero.
> You can try inspecting the state printed by drm_info on any disconnected
> connector to see for yourself.

Right, I missed the detail hat the connector is initially marked as
disconnected. That solves the issue of invalid panel-orient / mode /
dpi info, bit it opens up other problems.

>> I guess the initial modeline is inherited from the video-bios, but
>> what about the physical size? Note that you cannot just change the
>> physical size later either, that gets used to determine the hidpi
>> scaling factor in the bootsplash, and changing that after the initial
>> bootsplash dislay will also look ugly
>>
>> b) Why you need the edid for the panel-orientation property at all,
>> typically the edid prom is part of the panel and the panel does not
>> know that it is mounted e.g. upside down at all, that is a property
>> of the system as a whole not of the panel as a standalone unit so
>> in my experience getting panel-orient info is something which comes
>> from the firmware /video-bios not from edid ?
> 
> This is an internal DRM thing. The orientation quirks logic uses the
> mode size advertised by the EDID.

The DMI based quirking does, yes. But e.g. the quirk code directly
reading this from the Intel VBT does not rely on the mode.

But if you are planning on using a DMI based quirk for the steamdeck
then yes that needs the mode.

Thee mode check is there for 2 reasons:

1. To avoid also applying the quirk to external displays, but
I think that that is also solved in most drivers by only checking for
a quirk at all on the eDP connector

2. Some laptop models ship with different panels in different badges
some of these are portrait (so need a panel-orient) setting and others
are landscape.

> I agree that at least in the Steam
> Deck case it may not make a lot of sense to use any info from the
> EDID, but that's needed for the current status quo.

We could extend the DMI quirk mechanism to allow quirks which don't
do the mode check, for use on devices where we can guarantee neither
1 nor 2 happens, then amdgpu could call the quirk code early simply
passing 0x0 as resolution.

> Also note, DisplayID has a bit to indicate the panel orientation IIRC.
> Would be nice to support parsing this at some point.

Ack.

Regards,

Hans

Powered by blists - more mailing lists