From: Dave Stevenson <dave.stevenson@raspberrypi.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: "David Airlie" <airlied@linux.ie>,
nouveau@lists.freedesktop.org,
"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
dri-devel@lists.freedesktop.org,
"Phil Elwell" <phil@raspberrypi.com>,
"Samuel Holland" <samuel@sholland.org>,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
"Chen-Yu Tsai" <wens@csie.org>,
"Geert Uytterhoeven" <geert@linux-m68k.org>,
linux-sunxi@lists.linux.dev, "Daniel Vetter" <daniel@ffwll.ch>,
intel-gfx@lists.freedesktop.org,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Jani Nikula" <jani.nikula@linux.intel.com>,
"Hans de Goede" <hdegoede@redhat.com>,
"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
linux-arm-kernel@lists.infradead.org,
"Tvrtko Ursulin" <tvrtko.ursulin@linux.intel.com>,
"Dom Cobley" <dom@raspberrypi.com>,
linux-kernel@vger.kernel.org,
"Mateusz Kwiatkowski" <kfyatek+publicgit@gmail.com>,
"Noralf Trønnes" <noralf@tronnes.org>
Subject: Re: [PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function
Date: Wed, 3 Jan 2024 17:14:46 +0000 [thread overview]
Message-ID: <CAPY8ntCdsvLvFm1fezQamKibbcsQoOCNc2nBsvj6vhLH3s0udw@mail.gmail.com> (raw)
In-Reply-To: <CAPY8ntB0V2yRWVnr6HYby0g2seDL5P03iO+7E_TLa3niPGSfPw@mail.gmail.com>
On Wed, 3 Jan 2024 at 14:02, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Maxime
>
> On Wed, 3 Jan 2024 at 13:33, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Dave,
> >
> > Happy new year :)
>
> And to you.
>
> > On Tue, Jan 02, 2024 at 03:12:26PM +0000, Dave Stevenson wrote:
> > > Hi Maxime
> > >
> > > On Mon, 14 Nov 2022 at 13:00, Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > The current construction of the named mode parsing doesn't allow to extend
> > > > it easily. Let's move it to a separate function so we can add more
> > > > parameters and modes.
> > > >
> > > > In order for the tests to still pass, some extra checks are needed, so
> > > > it's not a 1:1 move.
> > > >
> > > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> > > > Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > >
> > > > ---
> > > > Changes in v7:
> > > > - Add Noralf Reviewed-by
> > > >
> > > > Changes in v6:
> > > > - Simplify the test for connection status extras
> > > > - Simplify the code path to call drm_mode_parse_cmdline_named_mode
> > > >
> > > > Changes in v4:
> > > > - Fold down all the named mode patches that were split into a single
> > > > patch again to maintain bisectability
> > > > ---
> > > > drivers/gpu/drm/drm_modes.c | 70 +++++++++++++++++++++++++++++++++++++--------
> > > > 1 file changed, 58 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > > index 71c050c3ee6b..37542612912b 100644
> > > > --- a/drivers/gpu/drm/drm_modes.c
> > > > +++ b/drivers/gpu/drm/drm_modes.c
> > > > @@ -2229,6 +2229,51 @@ static const char * const drm_named_modes_whitelist[] = {
> > > > "PAL",
> > > > };
> > > >
> > > > +static int drm_mode_parse_cmdline_named_mode(const char *name,
> > > > + unsigned int name_end,
> > > > + struct drm_cmdline_mode *cmdline_mode)
> > > > +{
> > > > + unsigned int i;
> > > > +
> > > > + if (!name_end)
> > > > + return 0;
> > > > +
> > > > + /* If the name starts with a digit, it's not a named mode */
> > > > + if (isdigit(name[0]))
> > > > + return 0;
> > > > +
> > > > + /*
> > > > + * If there's an equal sign in the name, the command-line
> > > > + * contains only an option and no mode.
> > > > + */
> > > > + if (strnchr(name, name_end, '='))
> > > > + return 0;
> > > > +
> > > > + /* The connection status extras can be set without a mode. */
> > > > + if (name_end == 1 &&
> > > > + (name[0] == 'd' || name[0] == 'D' || name[0] == 'e'))
> > > > + return 0;
> > > > +
> > > > + /*
> > > > + * We're sure we're a named mode at this point, iterate over the
> > > > + * list of modes we're aware of.
> > > > + */
> > > > + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > > > + int ret;
> > > > +
> > > > + ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > > > + if (ret != name_end)
> > > > + continue;
> > > > +
> > > > + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> > > > + cmdline_mode->specified = true;
> > > > +
> > > > + return 1;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > /**
> > > > * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
> > > > * @mode_option: optional per connector mode option
> > > > @@ -2265,7 +2310,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > > > const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> > > > const char *options_ptr = NULL;
> > > > char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> > > > - int i, len, ret;
> > > > + int len, ret;
> > > >
> > > > memset(mode, 0, sizeof(*mode));
> > > > mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > > > @@ -2306,18 +2351,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> > > > parse_extras = true;
> > > > }
> > > >
> > > > - /* First check for a named mode */
> > > > - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> > > > - ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> > > > - if (ret == mode_end) {
> > > > - if (refresh_ptr)
> > > > - return false; /* named + refresh is invalid */
> > > > + if (!mode_end)
> > > > + return false;
> > >
> > > I'm chasing down a change in behaviour between 6.1 and 6.6, and this
> > > patch seems to be at least part of the cause.
> > >
> > > Since [1] we've had the emulated framebuffer on Pi being 16bpp to save
> > > memory. All good.
> > >
> > > It used to be possible to use "video=HDMI-A-1:-32" on the kernel
> > > command line to set it back to 32bpp.
> > >
> > > After this patch that is no longer possible. "mode_end = bpp_off", and
> > > "bpp_off = bpp_ptr - name", so with bpp_ptr = name we get mode_end
> > > being 0. That fails this conditional.
> > > drm_mode_parse_cmdline_named_mode already aborts early but with no
> > > error if name_end / mode_end is 0, so this "if" clause seems
> > > redundant, and is a change in behaviour.
> > >
> > > We do then get a second parsing failure due to the check if (bpp_ptr
> > > || refresh_ptr) at [2].
> > > Prior to this patch my video= line would get mode->specified set via
> > > "if (ret == mode_end)" removed above, as ret = mode_end = 0. We
> > > therefore didn't evaluate the conditional that now fails.
> > >
> > > So I guess my question is whether my command line is valid or not, and
> > > therefore is this a regression?
> >
> > It's a mess :)
> >
> > Documentation/fb/modedb.rst defines the video parameter syntax as:
> >
> > <xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd]
> >
> > And thus mandates the x and y resolution. I guess that's what I use as a
> > base, and thus -bpp alone would be invalid.
> >
> > But then it contradicts itself some time later by documenting that
> > video=HDMI-1:D is ok.
> >
> > I guess what you experienced was just an oversight: it was not
> > documented anywhere that it was valid, so we didn't really tested it
> > either. We should add a unit test for it and fix it.
>
> Does dropping this "if (!mode_end)" check, and at least the check for
> bpp_ptr in the "No mode?" block below it, seem reasonable to you?
>
> I guess there is also the question of whether a refresh rate without a
> mode is valid. That one seems less useful, and all uses of
> refresh_specified appear to be after some form of checking xres and
> yres.
>
> I can put a couple of patches together to deal with this if you're
> happy with the principle.
In looking at this in more detail and writing the unit test, it does
become a bit of a mess.
drm_connector_get_cmdline_mode calls
drm_mode_parse_command_line_for_connector, passing in
connector->cmdline_mode as the mode to fill.
drm_mode_parse_command_line_for_connector fills in bits of the mode as
it goes along, so a failure late in the function leaves bits from the
parsing in connector->cmdline_mode.
My video=HDMI-A-1:-32 does fail later on due to
drm_mode_parse_cmdline_options, but as it had set bpp and
bpp_specified directly in connector->cmdline_mode I got the behaviour
I was desiring.
It feels like this should be an all or nothing thing. Allocate a
struct drm_cmdline_mode on the stack, and only copy it into
connector->cmdline_mode if parsing succeeds. The downside is that you
take out some command line entries that used to work (like mine).
Thoughts?
Dave
next prev parent reply other threads:[~2024-01-03 17:15 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-14 13:00 [Nouveau] [PATCH v9 00/25] drm: Analog TV Improvements Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 01/25] docs/fb: Document current named modes Maxime Ripard
2022-11-14 17:39 ` Noralf Trønnes
2022-11-15 9:12 ` [Nouveau] (subset) " Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 02/25] drm/tests: Add Kunit Helpers Maxime Ripard
2022-11-15 9:12 ` [Nouveau] (subset) " Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 03/25] drm/connector: Rename legacy TV property Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 04/25] drm/connector: Only register TV mode property if present Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 05/25] drm/connector: Rename drm_mode_create_tv_properties Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 06/25] drm/connector: Add TV standard property Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 07/25] drm/modes: Add a function to generate analog display modes Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 08/25] drm/client: Add some tests for drm_connector_pick_cmdline_mode() Maxime Ripard
2022-11-15 9:13 ` [Nouveau] (subset) " Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function Maxime Ripard
2022-11-15 9:13 ` [Nouveau] (subset) " Maxime Ripard
2024-01-02 15:12 ` Dave Stevenson
2024-01-03 13:32 ` Maxime Ripard
2024-01-03 14:02 ` Dave Stevenson
2024-01-03 17:14 ` Dave Stevenson [this message]
2022-11-14 13:00 ` [Nouveau] [PATCH v9 10/25] drm/modes: Switch to named mode descriptors Maxime Ripard
2022-11-15 9:13 ` [Nouveau] (subset) " Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 11/25] drm/modes: Fill drm_cmdline mode from named modes Maxime Ripard
2022-11-15 9:13 ` [Nouveau] (subset) " Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 12/25] drm/connector: Add pixel clock to cmdline mode Maxime Ripard
2022-11-15 9:13 ` [Nouveau] (subset) " Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 13/25] drm/connector: Add a function to lookup a TV mode by its name Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 14/25] drm/modes: Introduce the tv_mode property as a command-line option Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 15/25] drm/modes: Properly generate a drm_display_mode from a named mode Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 16/25] drm/client: Remove match on mode name Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 17/25] drm/modes: Introduce more named modes Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 18/25] drm/probe-helper: Provide a TV get_modes helper Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 19/25] drm/atomic-helper: Add a TV properties reset helper Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 20/25] drm/atomic-helper: Add an analog TV atomic_check implementation Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 21/25] drm/vc4: vec: Use TV Reset implementation Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 22/25] drm/vc4: vec: Check for VEC output constraints Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 23/25] drm/vc4: vec: Convert to the new TV mode property Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 24/25] drm/vc4: vec: Add support for more analog TV standards Maxime Ripard
2022-11-14 13:00 ` [Nouveau] [PATCH v9 25/25] drm/sun4i: tv: Convert to the new TV mode property Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPY8ntCdsvLvFm1fezQamKibbcsQoOCNc2nBsvj6vhLH3s0udw@mail.gmail.com \
--to=dave.stevenson@raspberrypi.com \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dom@raspberrypi.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=geert@linux-m68k.org \
--cc=hdegoede@redhat.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=jernej.skrabec@gmail.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=kfyatek+publicgit@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=maarten.lankhorst@linux.intel.com \
--cc=maxime@cerno.tech \
--cc=noralf@tronnes.org \
--cc=nouveau@lists.freedesktop.org \
--cc=phil@raspberrypi.com \
--cc=rodrigo.vivi@intel.com \
--cc=samuel@sholland.org \
--cc=tvrtko.ursulin@linux.intel.com \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).