All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/edid: index CEA/HDMI mode tables using the VIC
Date: Fri, 8 Jan 2016 15:31:00 +0100	[thread overview]
Message-ID: <20160108143100.GY8076@phenom.ffwll.local> (raw)
In-Reply-To: <20160108113029.GT4437@intel.com>

On Fri, Jan 08, 2016 at 01:30:29PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 08, 2016 at 01:21:51PM +0200, Jani Nikula wrote:
> > Add a dummy entry to CEA/HDMI mode tables so they can be indexed
> > directly using the VIC, avoiding a +1/-1 dance here and there. This adds
> > clarity to the error checking for various functions that return the VIC
> > on success and zero on failure; we can now explicitly check for 0
> > instead of just subtracting one from an unsigned type.
> > 
> > Also add drm_valid_cea_vic() and drm_valid_hdmi_vic() helpers for
> > checking valid VICs.
> > 
> > v2: add drm_valid_cea_vic and drm_valid_hdmi_vic helpers (Ville)
> >     use { } instead of { 0 } for initializing the dummy modes
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Applied to drm-misc, thanks.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/drm_edid.c | 94 ++++++++++++++++++++++++++--------------------
> >  1 file changed, 53 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index c214f1246cb4..04cb4877fabd 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -637,8 +637,12 @@ static const struct minimode extra_modes[] = {
> >  /*
> >   * Probably taken from CEA-861 spec.
> >   * This table is converted from xorg's hw/xfree86/modes/xf86EdidModes.c.
> > + *
> > + * Index using the VIC.
> >   */
> >  static const struct drm_display_mode edid_cea_modes[] = {
> > +	/* 0 - dummy, VICs start at 1 */
> > +	{ },
> >  	/* 1 - 640x480@60Hz */
> >  	{ DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175, 640, 656,
> >  		   752, 800, 0, 480, 490, 492, 525, 0,
> > @@ -987,9 +991,11 @@ static const struct drm_display_mode edid_cea_modes[] = {
> >  };
> >  
> >  /*
> > - * HDMI 1.4 4k modes.
> > + * HDMI 1.4 4k modes. Index using the VIC.
> >   */
> >  static const struct drm_display_mode edid_4k_modes[] = {
> > +	/* 0 - dummy, VICs start at 1 */
> > +	{ },
> >  	/* 1 - 3840x2160@30Hz */
> >  	{ DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> >  		   3840, 4016, 4104, 4400, 0,
> > @@ -2548,13 +2554,13 @@ cea_mode_alternate_clock(const struct drm_display_mode *cea_mode)
> >  static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match,
> >  					     unsigned int clock_tolerance)
> >  {
> > -	u8 mode;
> > +	u8 vic;
> >  
> >  	if (!to_match->clock)
> >  		return 0;
> >  
> > -	for (mode = 0; mode < ARRAY_SIZE(edid_cea_modes); mode++) {
> > -		const struct drm_display_mode *cea_mode = &edid_cea_modes[mode];
> > +	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> > +		const struct drm_display_mode *cea_mode = &edid_cea_modes[vic];
> >  		unsigned int clock1, clock2;
> >  
> >  		/* Check both 60Hz and 59.94Hz */
> > @@ -2566,7 +2572,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
> >  			continue;
> >  
> >  		if (drm_mode_equal_no_clocks(to_match, cea_mode))
> > -			return mode + 1;
> > +			return vic;
> >  	}
> >  
> >  	return 0;
> > @@ -2581,13 +2587,13 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m
> >   */
> >  u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> >  {
> > -	u8 mode;
> > +	u8 vic;
> >  
> >  	if (!to_match->clock)
> >  		return 0;
> >  
> > -	for (mode = 0; mode < ARRAY_SIZE(edid_cea_modes); mode++) {
> > -		const struct drm_display_mode *cea_mode = &edid_cea_modes[mode];
> > +	for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> > +		const struct drm_display_mode *cea_mode = &edid_cea_modes[vic];
> >  		unsigned int clock1, clock2;
> >  
> >  		/* Check both 60Hz and 59.94Hz */
> > @@ -2597,12 +2603,17 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> >  		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
> >  		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
> >  		    drm_mode_equal_no_clocks_no_stereo(to_match, cea_mode))
> > -			return mode + 1;
> > +			return vic;
> >  	}
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_match_cea_mode);
> >  
> > +static bool drm_valid_cea_vic(u8 vic)
> > +{
> > +	return vic > 0 && vic < ARRAY_SIZE(edid_cea_modes);
> > +}
> > +
> >  /**
> >   * drm_get_cea_aspect_ratio - get the picture aspect ratio corresponding to
> >   * the input VIC from the CEA mode list
> > @@ -2612,10 +2623,7 @@ EXPORT_SYMBOL(drm_match_cea_mode);
> >   */
> >  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code)
> >  {
> > -	/* return picture aspect ratio for video_code - 1 to access the
> > -	 * right array element
> > -	*/
> > -	return edid_cea_modes[video_code-1].picture_aspect_ratio;
> > +	return edid_cea_modes[video_code].picture_aspect_ratio;
> >  }
> >  EXPORT_SYMBOL(drm_get_cea_aspect_ratio);
> >  
> > @@ -2639,13 +2647,13 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode)
> >  static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_match,
> >  					      unsigned int clock_tolerance)
> >  {
> > -	u8 mode;
> > +	u8 vic;
> >  
> >  	if (!to_match->clock)
> >  		return 0;
> >  
> > -	for (mode = 0; mode < ARRAY_SIZE(edid_4k_modes); mode++) {
> > -		const struct drm_display_mode *hdmi_mode = &edid_4k_modes[mode];
> > +	for (vic = 1; vic < ARRAY_SIZE(edid_4k_modes); vic++) {
> > +		const struct drm_display_mode *hdmi_mode = &edid_4k_modes[vic];
> >  		unsigned int clock1, clock2;
> >  
> >  		/* Make sure to also match alternate clocks */
> > @@ -2657,7 +2665,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
> >  			continue;
> >  
> >  		if (drm_mode_equal_no_clocks(to_match, hdmi_mode))
> > -			return mode + 1;
> > +			return vic;
> >  	}
> >  
> >  	return 0;
> > @@ -2673,13 +2681,13 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
> >   */
> >  static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
> >  {
> > -	u8 mode;
> > +	u8 vic;
> >  
> >  	if (!to_match->clock)
> >  		return 0;
> >  
> > -	for (mode = 0; mode < ARRAY_SIZE(edid_4k_modes); mode++) {
> > -		const struct drm_display_mode *hdmi_mode = &edid_4k_modes[mode];
> > +	for (vic = 1; vic < ARRAY_SIZE(edid_4k_modes); vic++) {
> > +		const struct drm_display_mode *hdmi_mode = &edid_4k_modes[vic];
> >  		unsigned int clock1, clock2;
> >  
> >  		/* Make sure to also match alternate clocks */
> > @@ -2689,11 +2697,16 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
> >  		if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||
> >  		     KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) &&
> >  		    drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
> > -			return mode + 1;
> > +			return vic;
> >  	}
> >  	return 0;
> >  }
> >  
> > +static bool drm_valid_hdmi_vic(u8 vic)
> > +{
> > +	return vic > 0 && vic < ARRAY_SIZE(edid_4k_modes);
> > +}
> > +
> >  static int
> >  add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
> >  {
> > @@ -2713,16 +2726,16 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
> >  	list_for_each_entry(mode, &connector->probed_modes, head) {
> >  		const struct drm_display_mode *cea_mode = NULL;
> >  		struct drm_display_mode *newmode;
> > -		u8 mode_idx = drm_match_cea_mode(mode) - 1;
> > +		u8 vic = drm_match_cea_mode(mode);
> >  		unsigned int clock1, clock2;
> >  
> > -		if (mode_idx < ARRAY_SIZE(edid_cea_modes)) {
> > -			cea_mode = &edid_cea_modes[mode_idx];
> > +		if (drm_valid_cea_vic(vic)) {
> > +			cea_mode = &edid_cea_modes[vic];
> >  			clock2 = cea_mode_alternate_clock(cea_mode);
> >  		} else {
> > -			mode_idx = drm_match_hdmi_mode(mode) - 1;
> > -			if (mode_idx < ARRAY_SIZE(edid_4k_modes)) {
> > -				cea_mode = &edid_4k_modes[mode_idx];
> > +			vic = drm_match_hdmi_mode(mode);
> > +			if (drm_valid_hdmi_vic(vic)) {
> > +				cea_mode = &edid_4k_modes[vic];
> >  				clock2 = hdmi_mode_alternate_clock(cea_mode);
> >  			}
> >  		}
> > @@ -2773,17 +2786,17 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
> >  {
> >  	struct drm_device *dev = connector->dev;
> >  	struct drm_display_mode *newmode;
> > -	u8 cea_mode;
> > +	u8 vic;
> >  
> >  	if (video_db == NULL || video_index >= video_len)
> >  		return NULL;
> >  
> >  	/* CEA modes are numbered 1..127 */
> > -	cea_mode = (video_db[video_index] & 127) - 1;
> > -	if (cea_mode >= ARRAY_SIZE(edid_cea_modes))
> > +	vic = (video_db[video_index] & 127);
> > +	if (!drm_valid_cea_vic(vic))
> >  		return NULL;
> >  
> > -	newmode = drm_mode_duplicate(dev, &edid_cea_modes[cea_mode]);
> > +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> >  	if (!newmode)
> >  		return NULL;
> >  
> > @@ -2878,8 +2891,7 @@ static int add_hdmi_mode(struct drm_connector *connector, u8 vic)
> >  	struct drm_device *dev = connector->dev;
> >  	struct drm_display_mode *newmode;
> >  
> > -	vic--; /* VICs start at 1 */
> > -	if (vic >= ARRAY_SIZE(edid_4k_modes)) {
> > +	if (!drm_valid_hdmi_vic(vic)) {
> >  		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
> >  		return 0;
> >  	}
> > @@ -3170,24 +3182,24 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
> >  {
> >  	const struct drm_display_mode *cea_mode;
> >  	int clock1, clock2, clock;
> > -	u8 mode_idx;
> > +	u8 vic;
> >  	const char *type;
> >  
> >  	/*
> >  	 * allow 5kHz clock difference either way to account for
> >  	 * the 10kHz clock resolution limit of detailed timings.
> >  	 */
> > -	mode_idx = drm_match_cea_mode_clock_tolerance(mode, 5) - 1;
> > -	if (mode_idx < ARRAY_SIZE(edid_cea_modes)) {
> > +	vic = drm_match_cea_mode_clock_tolerance(mode, 5);
> > +	if (drm_valid_cea_vic(vic)) {
> >  		type = "CEA";
> > -		cea_mode = &edid_cea_modes[mode_idx];
> > +		cea_mode = &edid_cea_modes[vic];
> >  		clock1 = cea_mode->clock;
> >  		clock2 = cea_mode_alternate_clock(cea_mode);
> >  	} else {
> > -		mode_idx = drm_match_hdmi_mode_clock_tolerance(mode, 5) - 1;
> > -		if (mode_idx < ARRAY_SIZE(edid_4k_modes)) {
> > +		vic = drm_match_hdmi_mode_clock_tolerance(mode, 5);
> > +		if (drm_valid_hdmi_vic(vic)) {
> >  			type = "HDMI";
> > -			cea_mode = &edid_4k_modes[mode_idx];
> > +			cea_mode = &edid_4k_modes[vic];
> >  			clock1 = cea_mode->clock;
> >  			clock2 = hdmi_mode_alternate_clock(cea_mode);
> >  		} else {
> > @@ -3205,7 +3217,7 @@ static void fixup_detailed_cea_mode_clock(struct drm_display_mode *mode)
> >  		return;
> >  
> >  	DRM_DEBUG("detailed mode matches %s VIC %d, adjusting clock %d -> %d\n",
> > -		  type, mode_idx + 1, mode->clock, clock);
> > +		  type, vic, mode->clock, clock);
> >  	mode->clock = clock;
> >  }
> >  
> > -- 
> > 2.1.4
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

      reply	other threads:[~2016-01-08 14:31 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-01 15:16 [RFC PATCH] drm/edid: index CEA/HDMI mode tables using the VIC Jani Nikula
2015-12-01 15:20 ` Alex Deucher
2015-12-01 15:24 ` Ville Syrjälä
2015-12-02 11:46   ` Jani Nikula
2015-12-02 12:24     ` Ville Syrjälä
2015-12-02 12:57       ` Jani Nikula
2015-12-03  9:53         ` Ville Syrjälä
2016-01-08 11:21 ` [PATCH v2] " Jani Nikula
2016-01-08 11:30   ` Ville Syrjälä
2016-01-08 14:31     ` Daniel Vetter [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160108143100.GY8076@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jani.nikula@intel.com \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.