Nouveau - Nov 2022 - [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20221107/6034718c/attachment.sig>