| Message ID | 20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + 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; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski
Hi Mateusz, On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote: > First of all, nice idea with the helper function that can be reused by > different drivers. This is neat! Yeah, it looked to me that given how complex it is, we don't want to duplicate it in each and every driver. > But looking at this function, it feels a bit overcomplicated. You're > creating the two modes, If reported as supported by the connector, yes. > then checking which one is the default, then set the preferred one and > possibly reorder them. Maybe it can be simplified somehow? Possibly, but I couldn't find something simpler. We should only expose the modes that the driver reports as supported, so we can have 0-2 modes. Then the preferred flag needs to be set on the default one like you suggested. But also, EDIDs define the preferred mode as either the mode with the flag set or the first mode listed. So a lot of program just use the heuristic to just pick the first mode listed. So it might be that I'm too careful, but it still seems useful to me. > Although when I tried to refactor it myself, I ended up with something that's > not better at all. Maybe it needs to be complicated, after all :( Yeah, that was my conclusion too :/ > Anyway, the current version seems to have a couple of bugs: > > > + 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; > > + > > + tv_modes[count++] = mode; > > + } > > If the 480i mode has been created properly, but there's an error creating the > 576i one (we enter the if (!mode) clause), the 480i mode will leak. > > > + if (count == 1) { > > You're handling the count == 1 case specially, but if count == 0, the rest of > the code will assume that two modes exist and probably segfault in the process. > > > + ret = drm_object_property_get_default_value(&connector->base, > > + dev->mode_config.tv_mode_property, > > + &default_mode); > > + if (ret) > > + return 0; > > + > > + if (cmdline->tv_mode_specified) > > + default_mode = cmdline->tv_mode; > > In case of an error (ret != 0), the modes created so far in the tv_modes array > will leak. Thanks for the review, I'll fix these bugs > Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go > first? If we're going to use the default from cmdline, there's no point in even > querying the property default value. Maybe, I don't know. I find the flow of the code more readable that way, but if you disagree I'll change it. Maxime
Den 26.10.2022 17.33, skrev maxime@cerno.tech: > 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 <maxime@cerno.tech> > > --- > 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.
Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski: > Hi Maxime, > > First of all, nice idea with the helper function that can be reused by different > drivers. This is neat! > > But looking at this function, it feels a bit overcomplicated. You're creating > the two modes, then checking which one is the default, then set the preferred > one and possibly reorder them. Maybe it can be simplified somehow? > > Although when I tried to refactor it myself, I ended up with something that's > not better at all. Maybe it needs to be complicated, after all :( > I also thought that the function was complicated/difficult to read, in particular the index stuff at the end, but I also failed in finding a "better" solution, just a different one ;) Noralf. My version: int drm_connector_helper_tv_get_modes(struct drm_connector *connector) { struct drm_device *dev = connector->dev; struct drm_property *tv_mode_property = dev->mode_config.tv_mode_property; struct drm_cmdline_mode *cmdline = &connector->cmdline_mode; 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 tv_modes[2] = { UINT_MAX, UINT_MAX }; unsigned int i, supported_tv_modes = 0; if (!tv_mode_property) return 0; for (i = 0; i < tv_mode_property->num_values; i++) supported_tv_modes |= BIT(tv_mode_property->values[i]); if ((supported_tv_modes & ntsc_modes) && (supported_tv_modes & pal_modes)) { uint64_t default_mode; if (drm_object_property_get_default_value(&connector->base, tv_mode_property, &default_mode)) return 0; if (cmdline->tv_mode_specified) default_mode = cmdline->tv_mode; if (BIT(default_mode) & ntsc_modes) { tv_modes[0] = DRM_MODE_TV_MODE_NTSC; tv_modes[1] = DRM_MODE_TV_MODE_PAL; } else { tv_modes[0] = DRM_MODE_TV_MODE_PAL; tv_modes[1] = DRM_MODE_TV_MODE_NTSC; } } else if (supported_tv_modes & ntsc_modes) { tv_modes[0] = DRM_MODE_TV_MODE_NTSC; } else if (supported_tv_modes & pal_modes) { tv_modes[0] = DRM_MODE_TV_MODE_PAL; } else { return 0; } for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { struct drm_display_mode *mode; if (tv_modes[i] == DRM_MODE_TV_MODE_NTSC) mode = drm_mode_analog_ntsc_480i(dev); else if (tv_modes[i] == DRM_MODE_TV_MODE_PAL) mode = drm_mode_analog_pal_576i(dev); else break; if (!mode) return i; if (!i) mode->type |= DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); } return i; }
Hi Noralf, On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Trønnes wrote: > > > Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski: > > Hi Maxime, > > > > First of all, nice idea with the helper function that can be reused by different > > drivers. This is neat! > > > > But looking at this function, it feels a bit overcomplicated. You're creating > > the two modes, then checking which one is the default, then set the preferred > > one and possibly reorder them. Maybe it can be simplified somehow? > > > > Although when I tried to refactor it myself, I ended up with something that's > > not better at all. Maybe it needs to be complicated, after all :( > > > > I also thought that the function was complicated/difficult to read, in > particular the index stuff at the end, but I also failed in finding a > "better" solution, just a different one ;) I think I like yours better still :) Can I bring it into my series, with your authorship and SoB? Maxime
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 maxime@cerno.tech: > > + > > + 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.07, skrev Maxime Ripard: > Hi Noralf, > > On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Trønnes wrote: >> >> >> Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski: >>> Hi Maxime, >>> >>> First of all, nice idea with the helper function that can be reused by different >>> drivers. This is neat! >>> >>> But looking at this function, it feels a bit overcomplicated. You're creating >>> the two modes, then checking which one is the default, then set the preferred >>> one and possibly reorder them. Maybe it can be simplified somehow? >>> >>> Although when I tried to refactor it myself, I ended up with something that's >>> not better at all. Maybe it needs to be complicated, after all :( >>> >> >> I also thought that the function was complicated/difficult to read, in >> particular the index stuff at the end, but I also failed in finding a >> "better" solution, just a different one ;) > > I think I like yours better still :) > > Can I bring it into my series, with your authorship and SoB? > Sure, no problem. Noralf.
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 maxime@cerno.tech: >>> + >>> + 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 maxime@cerno.tech: > >>> + > >>> + 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
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; + + 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 + * @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); + 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; + + tv_modes[count++] = mode; + } + + 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; + + if (cmdline->tv_mode_specified) + default_mode = cmdline->tv_mode; + + 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); diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h index 5880daa14624..4977e0ab72db 100644 --- a/include/drm/drm_probe_helper.h +++ b/include/drm/drm_probe_helper.h @@ -35,5 +35,6 @@ int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector); int drm_connector_helper_get_modes_fixed(struct drm_connector *connector, const struct drm_display_mode *fixed_mode); int drm_connector_helper_get_modes(struct drm_connector *connector); +int drm_connector_helper_tv_get_modes(struct drm_connector *connector); #endif
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 <maxime@cerno.tech> --- 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(+)