Den 26.09.2022 12.01, skrev Maxime Ripard: > On Sat, Sep 24, 2022 at 05:52:29PM +0200, Noralf Trønnes wrote: >> Den 22.09.2022 16.25, skrev Maxime Ripard: >>> The TV mode property has been around for a while now to select and get the >>> current TV mode output on an analog TV connector. >>> >>> Despite that property name being generic, its content isn't and has been >>> driver-specific which makes it hard to build any generic behaviour on top >>> of it, both in kernel and user-space. >>> >>> Let's create a new enum tv norm property, that can contain any of the >>> analog TV standards currently supported by kernel drivers. Each driver can >>> then pass in a bitmask of the modes it supports, and the property >>> creation function will filter out the modes not supported. >>> >>> We'll then be able to phase out the older tv mode property. >>> >>> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx> >> >> Please can you add per patch changelogs, it's hard to review when I have >> to recall what might have happened with each patch. If you do it drm >> style and put in the commit message it should be easy enough to do. > > I certainly don't want to start that discussion, but I'm really not a > fan of that format either. I'll do it for that series if you prefer. > The format isn't important, but especially a big series like this and being weeks between each iteration it's difficult to follow and see which review comments that you have chosen to implement and how. It's almost a full review each time. Even if I see that I have acked/rewieved a patch, if I don't remember, I have to go back to the previous version and see if I had any comments and if you followed up on that. >>> +/** >>> + * enum drm_connector_tv_mode - Analog TV output mode >>> + * >>> + * This enum is used to indicate the TV output mode used on an analog TV >>> + * connector. >>> + * >>> + * WARNING: The values of this enum is uABI since they're exposed in the >>> + * "TV mode" connector property. >>> + */ >>> +enum drm_connector_tv_mode { >>> + /** >>> + * @DRM_MODE_TV_MODE_NONE: Placeholder to not default on one >>> + * variant or the other when nothing is set. >>> + */ >>> + DRM_MODE_TV_MODE_NONE = 0, >> >> How is this supposed to be used? > > It's not supposed to be used. It was a suggestion from Mateusz to avoid > to default to any standard when we don't initialize something. I don't > have any strong feeling about it, so I can drop it if you prefer. > The confusing thing to me is that "None" is part of the property enum list, so the idea is that it can end up in userspace if there's a driver error? Hmm, that won't work since TV_MODE_NONE won't be part of the bitmask that the driver sets. So userspace reading the property ends up with a value for which there's no enum name to match. So usespace should be trained to know that zero for this property is a driver error? No, not a good idea. I think to catch a bug like this drm_atomic_connector_get_property() should check the tv.mode value and see if it's a legal enum value and if not it has to just pick a legal one and print an error. But I'm not sure it's worth it to catch a bug like this. And I don't see any other enum properties being checked for validity either before being returned to userspace. Based on this reasoning I think you should drop the NONE value. Noralf.