Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
> > ---
> > 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
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
> ---
> 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
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
> >>> ---
> >>> 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
signature.asc
Description: PGP signature
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
>>> ---
>>> 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.
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
> > ---
> > 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
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
> ---
> 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 =
> _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 a501db7d..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);
>
>
[Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
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
---
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)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(drm_tv_mode_enum_list); i++) {
+ const struct drm_prop_enum_list *item =
_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 a501db7d..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);
--
b4 0.11.0-dev-7da52
