Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <be9b6b71-fa2a-3290-2bce-901342e01981@tronnes.org>
Date:   Tue, 16 Aug 2022 21:35:24 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v1 05/35] drm/connector: Add TV standard property
To:     Maxime Ripard <maxime@cerno.tech>
Cc:     Jernej Skrabec <jernej.skrabec@gmail.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Samuel Holland <samuel@sholland.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Daniel Vetter <daniel@ffwll.ch>, Emma Anholt <emma@anholt.net>,
        David Airlie <airlied@linux.ie>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
        Phil Elwell <phil@raspberrypi.com>,
        Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>,
        linux-arm-kernel@lists.infradead.org,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        linux-amlogic@lists.infradead.org, dri-devel@lists.freedesktop.org,
        Dom Cobley <dom@raspberrypi.com>,
        =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= <noralf@tronnes.org>
References: <20220728-rpi-analog-tv-properties-v1-0-3d53ae722097@cerno.tech>
 <20220728-rpi-analog-tv-properties-v1-5-3d53ae722097@cerno.tech>
 <9fdecae2-80ad-6212-0522-7dccf9fb57be@tronnes.org>
 <20220816082612.grebxql5ynnfnvfd@houat>
 <ea6ebdda-f779-78af-1ffd-bd86d715077f@tronnes.org>
 <20220816094922.oqhrhefv327zo2ou@houat>
From:   =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= <noralf@tronnes.org>
In-Reply-To: <20220816094922.oqhrhefv327zo2ou@houat>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


Den 16.08.2022 11.49, skrev Maxime Ripard:
> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 16.08.2022 10.26, skrev Maxime Ripard:
>>> Hi,
>>>
>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote:
>>>> Den 29.07.2022 18.34, skrev Maxime Ripard:
>>>>> The TV mode property has been around for a while now to select and get the
>>>>> current TV mode output on an analog TV connector.
>>>>>
>>>>> Despite that property name being generic, its content isn't and has been
>>>>> driver-specific which makes it hard to build any generic behaviour on top
>>>>> of it, both in kernel and user-space.
>>>>>
>>>>> Let's create a new bitmask tv norm property, that can contain any of the
>>>>> analog TV standards currently supported by kernel drivers. Each driver can
>>>>> then pass in a bitmask of the modes it supports.
>>>>>
>>>>> We'll then be able to phase out the older tv mode property.
>>>>>
>>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>>>>
>>>>
>>>> Please also update Documentation/gpu/kms-properties.csv
>>>>
>>>> Requirements for adding a new property is found in
>>>> Documentation/gpu/drm-kms.rst
>>>
>>> I knew this was going to be raised at some point, so I'm glad it's that
>>> early :)
>>>
>>> I really don't know what to do there. If we stick by our usual rules,
>>> then we can't have any of that work merged.
>>>
>>> However, I think the status quo is not really satisfactory either.
>>> Indeed, we have a property, that doesn't follow those requirements
>>> either, with a driver-specific content, and that stands in the way of
>>> fixes and further improvements at both the core framework and driver
>>> levels.
>>>
>>> So having that new property still seems like a net improvement at the
>>> driver, framework and uAPI levels, even if it's not entirely following
>>> the requirements we have in place.
>>>
>>> Even more so since, realistically, those kind of interfaces will never
>>> get any new development on the user-space side of things, it's
>>> considered by everyone as legacy.
>>>
>>> This also is something we need to support at some point if we want to
>>> completely deprecate the fbdev drivers and provide decent alternatives
>>> in KMS.
>>>
>>> So yeah, strictly speaking, we would not qualify for our requirements
>>> there. I still think we have a strong case for an exception though.
>>
>> Which requirements would that be? The only one I can see is the
>> documentation and maybe an IGT test.
> 
> This is the one I had in mind
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> 

Oh right, I had forgotten about that one.

One benefit of having a userspace implementation is that it increases
the chance of widespread adoption having a working implementation to
look at. I don't think the reason tv.mode is not used anywhere (that I
know of) is because the driver picks the enum values resulting in no
standard names. It's a niche thing and way down on the todo list.
nouveau and ch7006 has a tv_norm module parameter which certainly
doesn't help in moving people/projects over to the DRM property
(downstream rpi also has it now).

mpv[1] is a commandline media player that after a quick look might be a
candidate for implementing the property without too much effort.

How do you test the property? I've used modetest but I can only change
to a tv.mode that matches the current display mode. I can't switch from
ntsc to pal for instance.

I have tried changing mode on rpi-5.15 (which I will switch to for the
gud gadget), but I always end up with flip timeouts when changing the value:

$ cat /proc/cpuinfo | grep Model
Model           : Raspberry Pi 4 Model B Rev 1.1
$ uname -a
Linux pi4t 5.15.56-v7l+ #1575 SMP Fri Jul 22 20:29:46 BST 2022 armv7l
GNU/Linux
$ sudo dmesg -C
$ modetest -M vc4 -s 45:720x480i -w 45:mode:1
setting mode 720x480i-29.97Hz on connectors 45, crtc 73
failed to set gamma: Function not implemented

$ dmesg
$ modetest -M vc4 -s 45:720x480i -w 45:mode:0
setting mode 720x480i-29.97Hz on connectors 45, crtc 73
failed to set gamma: Function not implemented

$ dmesg
[   95.193059] [drm:drm_atomic_helper_wait_for_flip_done
[drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] flip_done timed out
[  105.433112] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out
[  105.433519] [drm:drm_atomic_helper_wait_for_dependencies
[drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] commit wait timed out
[  115.673095] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out
[  115.673498] [drm:drm_atomic_helper_wait_for_dependencies
[drm_kms_helper]] *ERROR* [PLANE:63:plane-1] commit wait timed out
[  125.913106] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out
[  125.913510] vc4-drm gpu: [drm] *ERROR* Timed out waiting for commit
[  136.153411] [drm:drm_atomic_helper_wait_for_flip_done
[drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] flip_done timed out

I doesn't help to reload vc4, I have to reboot to get it working again.
I get this when reloading:
[  776.799784] vc4_vec fec13000.vec: Unbalanced pm_runtime_enable!

I know this was working in rpi-5.10 on the Pi Zero (Pi4 tvout using vc4
did not work at all when I tried).


Noralf.

[1] https://mpv.io/
    https://github.com/mpv-player/mpv/blob/master/video/out/drm_common.c
    https://github.com/mpv-player/mpv/blob/master/video/out/drm_atomic.c