On Wed, Apr 06, 2016 at 11:35:44AM +0300, Jani Nikula wrote:
> On Tue, 05 Apr 2016, Jim Bride <jim.bride@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
