Re: [PATCH 1/3] drm/edid: Add drm_edid_get_monitor_name()

[Jani Nikula <jani.nikula@linux.intel.com>]

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]);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx