| Message ID | 1459887821-8142-2-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 05 Apr 2016, Jim Bride <jim.bride@linux.intel.com> wrote: > In order to include monitor name information in debugfs > output we needed to add a function that would extract the > monitor name from the EDID, and that function needed to > reside in the file where the rest of the EDID helper > functions are implemented. > > cc: dri-devel@lists.freedesktop.org > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/drm_edid.c | 28 ++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 1 + > 2 files changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 558ef9f..fc69a46 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3569,6 +3569,34 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder) > EXPORT_SYMBOL(drm_select_eld); > > /** > + * drm_edid_get_monitor_name - fetch the monitor name from the edid > + * @edid: monitor EDID information > + * @name: pointer to a character array of at least 13 chars to hold the name Mixed feelings about "at least 13 chars". It might be simpler for this one thing, but I hate to see this assumption of "at least 13 chars" get spread around in patch 2 to where it's no longer obvious where this requirement comes from. Seeing that this is mostly copy-paste from drm_edid_to_eld(), I think this patch should be an abstraction of that code to a separate function. > + * > + * Return: True if the name could be extracted, false otherwise > + */ > +bool drm_edid_get_monitor_name(struct edid *edid, char *name) > +{ > + char *edid_name = NULL; > + int mnl; > + > + if (!edid || !name) > + return false; > + > + drm_for_each_detailed_block((u8 *)edid, monitor_name, &edid_name); > + for (mnl = 0; edid_name && mnl < 13; mnl++) { > + if (edid_name[mnl] == 0x0a) { > + name[mnl] = '\0'; Depending on edid_name you might not terminate the string. And, in fact, if you make this an abstraction for drm_edid_to_eld(), you might be better off *not* terminating, which OTOH is not nice for your other use in patch 2. So how about first adding an internal abstraction: static int get_monitor_name(struct edid *edid, char name[13]) which does *not* terminate the string, and returns length, 0 for edid_name == NULL. Works nicely for drm_edid_to_eld(). Then you could add the external interface on top of that: static void drm_edid_get_monitor_name(struct edid *edid, char *buf, int bufsize) which would always terminate the string (assuming bufsize > 0), even on errors so you can get rid of the return value. This simplifies your follow up code, and if we later on need more robust error handling, it's easy to add the return value. BR, Jani. > + break; > + } > + name[mnl] = edid_name[mnl]; > + } > + > + return mnl ? true : false; > +} > +EXPORT_SYMBOL(drm_edid_get_monitor_name); > + > +/** > * drm_detect_hdmi_monitor - detect whether monitor is HDMI > * @edid: monitor EDID information > * > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 8cb377c..2590168 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -2500,6 +2500,7 @@ extern int drm_edid_header_is_valid(const u8 *raw_edid); > extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, > bool *edid_corrupt); > extern bool drm_edid_is_valid(struct edid *edid); > +extern bool drm_edid_get_monitor_name(struct edid *edid, char *name); > > extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, > char topology[8]);
On Wed, Apr 06, 2016 at 11:35:44AM +0300, Jani Nikula wrote: > On Tue, 05 Apr 2016, Jim Bride <jim.bride@linux.intel.com> wrote: > > In order to include monitor name information in debugfs > > output we needed to add a function that would extract the > > monitor name from the EDID, and that function needed to > > reside in the file where the rest of the EDID helper > > functions are implemented. > > > > cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > --- > > drivers/gpu/drm/drm_edid.c | 28 ++++++++++++++++++++++++++++ > > include/drm/drm_crtc.h | 1 + > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 558ef9f..fc69a46 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -3569,6 +3569,34 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder) > > EXPORT_SYMBOL(drm_select_eld); > > > > /** > > + * drm_edid_get_monitor_name - fetch the monitor name from the edid > > + * @edid: monitor EDID information > > + * @name: pointer to a character array of at least 13 chars to hold the name > > Mixed feelings about "at least 13 chars". It might be simpler for this > one thing, but I hate to see this assumption of "at least 13 chars" get > spread around in patch 2 to where it's no longer obvious where this > requirement comes from. > > Seeing that this is mostly copy-paste from drm_edid_to_eld(), I think > this patch should be an abstraction of that code to a separate function. Yeah, I see what you mean. Writing something that works with both this use and drm_edid_to_eld() for the name parsing makes sense. > > + * > > + * Return: True if the name could be extracted, false otherwise > > + */ > > +bool drm_edid_get_monitor_name(struct edid *edid, char *name) > > +{ > > + char *edid_name = NULL; > > + int mnl; > > + > > + if (!edid || !name) > > + return false; > > + > > + drm_for_each_detailed_block((u8 *)edid, monitor_name, &edid_name); > > + for (mnl = 0; edid_name && mnl < 13; mnl++) { > > + if (edid_name[mnl] == 0x0a) { > > + name[mnl] = '\0'; > > Depending on edid_name you might not terminate the string. And, in fact, > if you make this an abstraction for drm_edid_to_eld(), you might be > better off *not* terminating, which OTOH is not nice for your other use > in patch 2. > > So how about first adding an internal abstraction: > > static int get_monitor_name(struct edid *edid, char name[13]) > > which does *not* terminate the string, and returns length, 0 for > edid_name == NULL. Works nicely for drm_edid_to_eld(). > > Then you could add the external interface on top of that: > > static void drm_edid_get_monitor_name(struct edid *edid, char *buf, int bufsize) > > which would always terminate the string (assuming bufsize > 0), even on > errors so you can get rid of the return value. This simplifies your > follow up code, and if we later on need more robust error handling, it's > easy to add the return value. Seems reasonable; I'll update the patch. Jim > BR, > Jani. > > > > + break; > > + } > > + name[mnl] = edid_name[mnl]; > > + } > > + > > + return mnl ? true : false; > > +} > > +EXPORT_SYMBOL(drm_edid_get_monitor_name); > > + > > +/** > > * drm_detect_hdmi_monitor - detect whether monitor is HDMI > > * @edid: monitor EDID information > > * > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 8cb377c..2590168 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -2500,6 +2500,7 @@ extern int drm_edid_header_is_valid(const u8 *raw_edid); > > extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, > > bool *edid_corrupt); > > extern bool drm_edid_is_valid(struct edid *edid); > > +extern bool drm_edid_get_monitor_name(struct edid *edid, char *name); > > > > extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, > > char topology[8]); > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 558ef9f..fc69a46 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3569,6 +3569,34 @@ struct drm_connector *drm_select_eld(struct drm_encoder *encoder) EXPORT_SYMBOL(drm_select_eld); /** + * drm_edid_get_monitor_name - fetch the monitor name from the edid + * @edid: monitor EDID information + * @name: pointer to a character array of at least 13 chars to hold the name + * + * Return: True if the name could be extracted, false otherwise + */ +bool drm_edid_get_monitor_name(struct edid *edid, char *name) +{ + char *edid_name = NULL; + int mnl; + + if (!edid || !name) + return false; + + drm_for_each_detailed_block((u8 *)edid, monitor_name, &edid_name); + for (mnl = 0; edid_name && mnl < 13; mnl++) { + if (edid_name[mnl] == 0x0a) { + name[mnl] = '\0'; + break; + } + name[mnl] = edid_name[mnl]; + } + + return mnl ? true : false; +} +EXPORT_SYMBOL(drm_edid_get_monitor_name); + +/** * drm_detect_hdmi_monitor - detect whether monitor is HDMI * @edid: monitor EDID information * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8cb377c..2590168 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -2500,6 +2500,7 @@ extern int drm_edid_header_is_valid(const u8 *raw_edid); extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *edid_corrupt); extern bool drm_edid_is_valid(struct edid *edid); +extern bool drm_edid_get_monitor_name(struct edid *edid, char *name); extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev, char topology[8]);
In order to include monitor name information in debugfs output we needed to add a function that would extract the monitor name from the EDID, and that function needed to reside in the file where the rest of the EDID helper functions are implemented. cc: dri-devel@lists.freedesktop.org Signed-off-by: Jim Bride <jim.bride@linux.intel.com> --- drivers/gpu/drm/drm_edid.c | 28 ++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 1 + 2 files changed, 29 insertions(+)