| Message ID | 20220728-rpi-analog-tv-properties-v5-12-d841cc64fe4b@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
Den 13.10.2022 15.18, skrev Maxime Ripard: > As part of the command line parsing rework coming in the next patches, > we'll need to lookup drm_connector_tv_mode values by their name, already > defined in drm_tv_mode_enum_list. > > In order to avoid any code duplication, let's do a function that will > perform a lookup of a TV mode name and return its value. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ > include/drm/drm_connector.h | 2 ++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 820f4c730b38..30611c616435 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > }; > DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) > > +/** > + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value > + * @name: TV Mode name we want to convert > + * @len: Length of @name > + * > + * Translates @name into an enum drm_connector_tv_mode. > + * > + * Returns: the enum value on success, a negative errno otherwise. > + */ > +int drm_get_tv_mode_from_name(const char *name, size_t len) Do we really need to pass in length here? item->name has to always be NUL terminated otherwise things would break elsewhere, so it shouldn't be necessary AFAICS. Noralf. > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(drm_tv_mode_enum_list); i++) { > + const struct drm_prop_enum_list *item = &drm_tv_mode_enum_list[i]; > + > + if (strlen(item->name) == len && !strncmp(item->name, name, len)) > + return item->type; > + } > + > + return -EINVAL; > +} > +EXPORT_SYMBOL(drm_get_tv_mode_from_name); > + > static const struct drm_prop_enum_list drm_tv_select_enum_list[] = { > { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */ > { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */ > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index a501db7d2222..a33f24a76738 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -1864,6 +1864,8 @@ const char *drm_get_dp_subconnector_name(int val); > const char *drm_get_content_protection_name(int val); > const char *drm_get_hdcp_content_type_name(int val); > > +int drm_get_tv_mode_from_name(const char *name, size_t len); > + > int drm_mode_create_dvi_i_properties(struct drm_device *dev); > void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector); > >
On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote: > Den 13.10.2022 15.18, skrev Maxime Ripard: > > As part of the command line parsing rework coming in the next patches, > > we'll need to lookup drm_connector_tv_mode values by their name, already > > defined in drm_tv_mode_enum_list. > > > > In order to avoid any code duplication, let's do a function that will > > perform a lookup of a TV mode name and return its value. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ > > include/drm/drm_connector.h | 2 ++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 820f4c730b38..30611c616435 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > > }; > > DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) > > > > +/** > > + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value > > + * @name: TV Mode name we want to convert > > + * @len: Length of @name > > + * > > + * Translates @name into an enum drm_connector_tv_mode. > > + * > > + * Returns: the enum value on success, a negative errno otherwise. > > + */ > > +int drm_get_tv_mode_from_name(const char *name, size_t len) > > Do we really need to pass in length here? item->name has to always be > NUL terminated otherwise things would break elsewhere, so it shouldn't > be necessary AFAICS. The only user so far is the command-line parsing code, and we might very well have an option after the tv_mode, something like 720x480i,tv_mode=NTSC,rotate=180 In this case, we won't get a NULL-terminated name. Maxime
Den 18.10.2022 11.33, skrev Maxime Ripard: > On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote: >> Den 13.10.2022 15.18, skrev Maxime Ripard: >>> As part of the command line parsing rework coming in the next patches, >>> we'll need to lookup drm_connector_tv_mode values by their name, already >>> defined in drm_tv_mode_enum_list. >>> >>> In order to avoid any code duplication, let's do a function that will >>> perform a lookup of a TV mode name and return its value. >>> >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >>> --- >>> drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ >>> include/drm/drm_connector.h | 2 ++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>> index 820f4c730b38..30611c616435 100644 >>> --- a/drivers/gpu/drm/drm_connector.c >>> +++ b/drivers/gpu/drm/drm_connector.c >>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { >>> }; >>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) >>> >>> +/** >>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value >>> + * @name: TV Mode name we want to convert >>> + * @len: Length of @name >>> + * >>> + * Translates @name into an enum drm_connector_tv_mode. >>> + * >>> + * Returns: the enum value on success, a negative errno otherwise. >>> + */ >>> +int drm_get_tv_mode_from_name(const char *name, size_t len) >> >> Do we really need to pass in length here? item->name has to always be >> NUL terminated otherwise things would break elsewhere, so it shouldn't >> be necessary AFAICS. > > The only user so far is the command-line parsing code, and we might very > well have an option after the tv_mode, something like > 720x480i,tv_mode=NTSC,rotate=180 > > In this case, we won't get a NULL-terminated name. > My point is that item->name will always be NUL terminated so strcmp() will never look past that. Noralf.
On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote: > > > Den 18.10.2022 11.33, skrev Maxime Ripard: > > On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote: > >> Den 13.10.2022 15.18, skrev Maxime Ripard: > >>> As part of the command line parsing rework coming in the next patches, > >>> we'll need to lookup drm_connector_tv_mode values by their name, already > >>> defined in drm_tv_mode_enum_list. > >>> > >>> In order to avoid any code duplication, let's do a function that will > >>> perform a lookup of a TV mode name and return its value. > >>> > >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > >>> --- > >>> drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ > >>> include/drm/drm_connector.h | 2 ++ > >>> 2 files changed, 26 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > >>> index 820f4c730b38..30611c616435 100644 > >>> --- a/drivers/gpu/drm/drm_connector.c > >>> +++ b/drivers/gpu/drm/drm_connector.c > >>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > >>> }; > >>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) > >>> > >>> +/** > >>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value > >>> + * @name: TV Mode name we want to convert > >>> + * @len: Length of @name > >>> + * > >>> + * Translates @name into an enum drm_connector_tv_mode. > >>> + * > >>> + * Returns: the enum value on success, a negative errno otherwise. > >>> + */ > >>> +int drm_get_tv_mode_from_name(const char *name, size_t len) > >> > >> Do we really need to pass in length here? item->name has to always be > >> NUL terminated otherwise things would break elsewhere, so it shouldn't > >> be necessary AFAICS. > > > > The only user so far is the command-line parsing code, and we might very > > well have an option after the tv_mode, something like > > 720x480i,tv_mode=NTSC,rotate=180 > > > > In this case, we won't get a NULL-terminated name. > > My point is that item->name will always be NUL terminated so strcmp() > will never look past that. Right, but we don't have the guarantee that strlen(item->name) < strlen(name), and we could thus just access after the end of our name Maxime
Den 19.10.2022 10.48, skrev Maxime Ripard: > On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote: >> >> >> Den 18.10.2022 11.33, skrev Maxime Ripard: >>> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote: >>>> Den 13.10.2022 15.18, skrev Maxime Ripard: >>>>> As part of the command line parsing rework coming in the next patches, >>>>> we'll need to lookup drm_connector_tv_mode values by their name, already >>>>> defined in drm_tv_mode_enum_list. >>>>> >>>>> In order to avoid any code duplication, let's do a function that will >>>>> perform a lookup of a TV mode name and return its value. >>>>> >>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >>>>> --- >>>>> drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ >>>>> include/drm/drm_connector.h | 2 ++ >>>>> 2 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>>>> index 820f4c730b38..30611c616435 100644 >>>>> --- a/drivers/gpu/drm/drm_connector.c >>>>> +++ b/drivers/gpu/drm/drm_connector.c >>>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { >>>>> }; >>>>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) >>>>> >>>>> +/** >>>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value >>>>> + * @name: TV Mode name we want to convert >>>>> + * @len: Length of @name >>>>> + * >>>>> + * Translates @name into an enum drm_connector_tv_mode. >>>>> + * >>>>> + * Returns: the enum value on success, a negative errno otherwise. >>>>> + */ >>>>> +int drm_get_tv_mode_from_name(const char *name, size_t len) >>>> >>>> Do we really need to pass in length here? item->name has to always be >>>> NUL terminated otherwise things would break elsewhere, so it shouldn't >>>> be necessary AFAICS. >>> >>> The only user so far is the command-line parsing code, and we might very >>> well have an option after the tv_mode, something like >>> 720x480i,tv_mode=NTSC,rotate=180 >>> >>> In this case, we won't get a NULL-terminated name. >> >> My point is that item->name will always be NUL terminated so strcmp() >> will never look past that. > > Right, but we don't have the guarantee that strlen(item->name) < > strlen(name), and we could thus just access after the end of our name > Ok, using the length limiting str funtions is the safe thing to do, so len needs to stay. But I don't get the 'strlen(item->name) == len' check. strncmp() will stop when it reaches a NUL in either string so no need for the length check? Anyways: Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Hi Noralf, On Wed, Oct 19, 2022 at 12:43:19PM +0200, Noralf Trønnes wrote: > > > Den 19.10.2022 10.48, skrev Maxime Ripard: > > On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote: > >> > >> > >> Den 18.10.2022 11.33, skrev Maxime Ripard: > >>> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote: > >>>> Den 13.10.2022 15.18, skrev Maxime Ripard: > >>>>> As part of the command line parsing rework coming in the next patches, > >>>>> we'll need to lookup drm_connector_tv_mode values by their name, already > >>>>> defined in drm_tv_mode_enum_list. > >>>>> > >>>>> In order to avoid any code duplication, let's do a function that will > >>>>> perform a lookup of a TV mode name and return its value. > >>>>> > >>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > >>>>> --- > >>>>> drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ > >>>>> include/drm/drm_connector.h | 2 ++ > >>>>> 2 files changed, 26 insertions(+) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > >>>>> index 820f4c730b38..30611c616435 100644 > >>>>> --- a/drivers/gpu/drm/drm_connector.c > >>>>> +++ b/drivers/gpu/drm/drm_connector.c > >>>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > >>>>> }; > >>>>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) > >>>>> > >>>>> +/** > >>>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value > >>>>> + * @name: TV Mode name we want to convert > >>>>> + * @len: Length of @name > >>>>> + * > >>>>> + * Translates @name into an enum drm_connector_tv_mode. > >>>>> + * > >>>>> + * Returns: the enum value on success, a negative errno otherwise. > >>>>> + */ > >>>>> +int drm_get_tv_mode_from_name(const char *name, size_t len) > >>>> > >>>> Do we really need to pass in length here? item->name has to always be > >>>> NUL terminated otherwise things would break elsewhere, so it shouldn't > >>>> be necessary AFAICS. > >>> > >>> The only user so far is the command-line parsing code, and we might very > >>> well have an option after the tv_mode, something like > >>> 720x480i,tv_mode=NTSC,rotate=180 > >>> > >>> In this case, we won't get a NULL-terminated name. > >> > >> My point is that item->name will always be NUL terminated so strcmp() > >> will never look past that. > > > > Right, but we don't have the guarantee that strlen(item->name) < > > strlen(name), and we could thus just access after the end of our name > > > > Ok, using the length limiting str funtions is the safe thing to do, so > len needs to stay. But I don't get the 'strlen(item->name) == len' > check. strncmp() will stop when it reaches a NUL in either string so no > need for the length check? Yeah, but if the cmdline is truncated, we'll pass a shorter len than strlen(item->name), and it will consider the string as equal. For example strncmp("NTS", "NTSC", strlen("NTS"))) == 0, while it obviously isn't for us. > Anyways: > > Reviewed-by: Noralf Trønnes <noralf@tronnes.org> Thanks! Maxime
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 820f4c730b38..30611c616435 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { }; DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) +/** + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value + * @name: TV Mode name we want to convert + * @len: Length of @name + * + * Translates @name into an enum drm_connector_tv_mode. + * + * Returns: the enum value on success, a negative errno otherwise. + */ +int drm_get_tv_mode_from_name(const char *name, size_t len) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(drm_tv_mode_enum_list); i++) { + const struct drm_prop_enum_list *item = &drm_tv_mode_enum_list[i]; + + if (strlen(item->name) == len && !strncmp(item->name, name, len)) + return item->type; + } + + return -EINVAL; +} +EXPORT_SYMBOL(drm_get_tv_mode_from_name); + static const struct drm_prop_enum_list drm_tv_select_enum_list[] = { { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */ { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */ diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index a501db7d2222..a33f24a76738 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1864,6 +1864,8 @@ const char *drm_get_dp_subconnector_name(int val); const char *drm_get_content_protection_name(int val); const char *drm_get_hdcp_content_type_name(int val); +int drm_get_tv_mode_from_name(const char *name, size_t len); + int drm_mode_create_dvi_i_properties(struct drm_device *dev); void drm_connector_attach_dp_subconnector_property(struct drm_connector *connector);
As part of the command line parsing rework coming in the next patches, we'll need to lookup drm_connector_tv_mode values by their name, already defined in drm_tv_mode_enum_list. In order to avoid any code duplication, let's do a function that will perform a lookup of a TV mode name and return its value. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/drm_connector.c | 24 ++++++++++++++++++++++++ include/drm/drm_connector.h | 2 ++ 2 files changed, 26 insertions(+)