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

[Jim Bride <jim.bride@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx