Re: [PATCH v9 09/25] drm/modes: Move named modes parsing to a separate function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maxime

On Wed, 3 Jan 2024 at 13:33, Maxime Ripard <maxime@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > > Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit@xxxxxxxxx>
> > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > >
> > > ---
> > > 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.

Thanks
  Dave




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux