Den 26.10.2022 17.33, skrev [email protected]:
> Most of the TV connectors will need a similar get_modes implementation
> that will, depending on the drivers' capabilities, register the 480i and
> 576i modes.
>
> That implementation will also need to set the preferred flag and order
> the modes based on the driver and users preferrence.
>
> This is especially important to guarantee that a userspace stack such as
> Xorg can start and pick up the preferred mode while maintaining a
> working output.
>
> Signed-off-by: Maxime Ripard <[email protected]>
>
> ---
> Changes in v6:
> - New patch
> ---
> drivers/gpu/drm/drm_probe_helper.c | 97 ++++++++++++++++++++++++++++++++++++++
> include/drm/drm_probe_helper.h | 1 +
> 2 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 69b0b2b9cc1c..4a60575f5c66 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1147,3 +1147,100 @@ int drm_connector_helper_get_modes(struct drm_connector *connector)
> return count;
> }
> EXPORT_SYMBOL(drm_connector_helper_get_modes);
> +
> +static bool tv_mode_supported(struct drm_connector *connector,
> + enum drm_connector_tv_mode mode)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *property = dev->mode_config.tv_mode_property;
> +
Superfluous linebreak
> + unsigned int i;
> +
> + for (i = 0; i < property->num_values; i++)
> + if (property->values[i] == mode)
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * drm_connector_helper_tv_get_modes - Fills the modes availables to a TV connector
availables -> available
> + * @connector: The connector
> + *
> + * Fills the available modes for a TV connector based on the supported
> + * TV modes, and the default mode expressed by the kernel command line.
> + *
> + * This can be used as the default TV connector helper .get_modes() hook
> + * if the driver does not need any special processing.
> + *
> + * Returns:
> + * The number of modes added to the connector.
> + */
> +int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
> + struct drm_display_mode *tv_modes[2] = {};
> + struct drm_display_mode *mode;
> + unsigned int first_mode_idx;
> + unsigned int count = 0;
> + uint64_t default_mode;
> + int ret;
> +
> + if (!dev->mode_config.tv_mode_property)
> + return 0;
> +
> + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_443) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_J) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_M)) {
> + mode = drm_mode_analog_ntsc_480i(connector->dev);
Nit: You can use the dev variable here and below.
> + if (!mode)
> + return 0;
> +
> + tv_modes[count++] = mode;
> + }
> +
> + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) {
> + mode = drm_mode_analog_pal_576i(connector->dev);
> + if (!mode)
> + return 0;
You leak the ntsc mode when returning (possibly).
> +
> + tv_modes[count++] = mode;
> + }
> +
Maybe check for count being zero here?
> + if (count == 1) {
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> + drm_mode_probed_add(connector, mode);
> + return count;
> + }
> +
> + ret = drm_object_property_get_default_value(&connector->base,
> + dev->mode_config.tv_mode_property,
> + &default_mode);
> + if (ret)
> + return 0;
You leak both modes when returning here. Maybe move this up before
allocation to simplify error handling.
> +
> + if (cmdline->tv_mode_specified)
> + default_mode = cmdline->tv_mode;
I realised that we don't verify tv_mode coming from the command line,
not here and not in the reset helper. Should we do that? A driver should
be programmed defensively to handle an illegal/unsupported value, but it
doesn't feel right to allow an illegal enum value coming through the
core/helpers.
> +
> + if ((default_mode == DRM_MODE_TV_MODE_NTSC) ||
> + (default_mode == DRM_MODE_TV_MODE_NTSC_443) ||
> + (default_mode == DRM_MODE_TV_MODE_NTSC_J) ||
> + (default_mode == DRM_MODE_TV_MODE_PAL_M))
> + first_mode_idx = 0;
> + else
> + first_mode_idx = 1;
> +
> + mode = tv_modes[first_mode_idx];
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> + drm_mode_probed_add(connector, mode);
> +
> + mode = first_mode_idx ? tv_modes[0] : tv_modes[1];
> + drm_mode_probed_add(connector, mode);
> +
> + return count;
> +}
> +EXPORT_SYMBOL(drm_connector_helper_tv_get_modes);
I know this is not expensive, but you're looping over the property
values 7 times. An alternative solution is to rebuild the supported bitmask:
int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
{
...
unsigned int ntsc_modes = BIT(DRM_MODE_TV_MODE_NTSC) |
BIT(DRM_MODE_TV_MODE_NTSC_443) |
BIT(DRM_MODE_TV_MODE_NTSC_J) |
BIT(DRM_MODE_TV_MODE_PAL_M);
unsigned int pal_modes = BIT(DRM_MODE_TV_MODE_PAL) |
BIT(DRM_MODE_TV_MODE_PAL_N) |
BIT(DRM_MODE_TV_MODE_SECAM);
unsigned int supported_tv_modes = 0;
...
for (i = 0; i < property->num_values; i++)
supported_tv_modes |= BIT(property->values[i]);
if (supported_tv_modes & ntsc_modes)
...
if (supported_tv_modes & pal_modes)
...
if (BIT(default_mode) & ntsc_modes)
first_mode_idx = 0;
else
first_mode_idx = 1;
Up to you if you want to do this.
Noralf.
Hi Noralf,
I'll leave aside your comments on the code, since we'll use your implementation.
On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Tr?nnes wrote:
> Den 26.10.2022 17.33, skrev [email protected]:
> > +
> > + if (cmdline->tv_mode_specified)
> > + default_mode = cmdline->tv_mode;
>
> I realised that we don't verify tv_mode coming from the command line,
> not here and not in the reset helper. Should we do that? A driver should
> be programmed defensively to handle an illegal/unsupported value, but it
> doesn't feel right to allow an illegal enum value coming through the
> core/helpers.
I don't think we can end up with an invalid value here if it's been
specified.
We parse the command line through drm_mode_parse_tv_mode() (introduced
in patch 13 "drm/modes: Introduce the tv_mode property as a command-line
option") that will pick the tv mode part of the command line, and call
drm_get_tv_mode_from_name() using it.
drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we
expect, and mode->tv_mode is only set on success. And AFAIK, there's no
other path that will set tv_mode.
Maxime
Den 07.11.2022 11.21, skrev Maxime Ripard:
> Hi Noralf,
>
> I'll leave aside your comments on the code, since we'll use your implementation.
>
> On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Trønnes wrote:
>> Den 26.10.2022 17.33, skrev [email protected]:
>>> +
>>> + if (cmdline->tv_mode_specified)
>>> + default_mode = cmdline->tv_mode;
>>
>> I realised that we don't verify tv_mode coming from the command line,
>> not here and not in the reset helper. Should we do that? A driver should
>> be programmed defensively to handle an illegal/unsupported value, but it
>> doesn't feel right to allow an illegal enum value coming through the
>> core/helpers.
>
> I don't think we can end up with an invalid value here if it's been
> specified.
>
> We parse the command line through drm_mode_parse_tv_mode() (introduced
> in patch 13 "drm/modes: Introduce the tv_mode property as a command-line
> option") that will pick the tv mode part of the command line, and call
> drm_get_tv_mode_from_name() using it.
>
> drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we
> expect, and mode->tv_mode is only set on success. And AFAIK, there's no
> other path that will set tv_mode.
>
I see now that illegal was the wrong word, but if the driver only
supports ntsc, the user can still set tv_mode=PAL right? And that's an
unsupported value that the driver can't fulfill, so it errors out. But
then again maybe that's just how it is, we can also set a display mode
that the driver can't handle, so this is no different in that respect.
Yeah, my argument lost some of its strength here :)
Noralf.
On Mon, Nov 07, 2022 at 12:29:28PM +0100, Noralf Tr?nnes wrote:
>
>
> Den 07.11.2022 11.21, skrev Maxime Ripard:
> > Hi Noralf,
> >
> > I'll leave aside your comments on the code, since we'll use your implementation.
> >
> > On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Tr?nnes wrote:
> >> Den 26.10.2022 17.33, skrev [email protected]:
> >>> +
> >>> + if (cmdline->tv_mode_specified)
> >>> + default_mode = cmdline->tv_mode;
> >>
> >> I realised that we don't verify tv_mode coming from the command line,
> >> not here and not in the reset helper. Should we do that? A driver should
> >> be programmed defensively to handle an illegal/unsupported value, but it
> >> doesn't feel right to allow an illegal enum value coming through the
> >> core/helpers.
> >
> > I don't think we can end up with an invalid value here if it's been
> > specified.
> >
> > We parse the command line through drm_mode_parse_tv_mode() (introduced
> > in patch 13 "drm/modes: Introduce the tv_mode property as a command-line
> > option") that will pick the tv mode part of the command line, and call
> > drm_get_tv_mode_from_name() using it.
> >
> > drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we
> > expect, and mode->tv_mode is only set on success. And AFAIK, there's no
> > other path that will set tv_mode.
> >
>
> I see now that illegal was the wrong word, but if the driver only
> supports ntsc, the user can still set tv_mode=PAL right? And that's an
> unsupported value that the driver can't fulfill, so it errors out. But
> then again maybe that's just how it is, we can also set a display mode
> that the driver can't handle, so this is no different in that respect.
> Yeah, my argument lost some of its strength here :)
I don't think we can handle this better, really. Falling back to NTSC in
that case would really be a stretch: it's a different mode, with a
different TV mode, etc.
It's an even bigger stretch than picking another mode I guess, and like
you said we're not doing that if the mode isn't supported
Maxime