Re: [PATCH v2] drm/edid: Fill PAR in AVI infoframe based on CEA mode list

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Apr 01, 2014 at 06:43:42PM +0530, Vandana Kannan wrote:
> On Apr-01-2014 5:04 PM, Ville Syrjälä wrote:
> > On Tue, Apr 01, 2014 at 04:26:59PM +0530, Vandana Kannan wrote:
> >> Populate PAR in infoframe structure. If there is a user setting for PAR, then
> >> that value is set. Else, value is taken from CEA mode list if VIC is found.
> >> Else, PAR is calculated from resolution. If none of these conditions are
> >> satisfied, PAR is NONE as per initialization.
> >>
> >> v2: Removed the part which sets PAR according to user input, based on
> >> Daniel's review comments.
> >>
> >> A separate patch will be submitted to create a property that would enable a
> >> user space app to set aspect ratio for AVI infoframe.
> >>
> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> >> Cc: Jesse Barnes <jesse.barnes@intel.com>
> >> Cc: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: intel-gfx@lists.freedesktop.org
> >> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> ---
> >>  drivers/gpu/drm/drm_edid.c | 29 +++++++++++++++++++++++++++++
> >>  include/drm/drm_crtc.h     |  1 +
> >>  2 files changed, 30 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index d4e3f9d..fee24d3 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -2452,6 +2452,21 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> >>  }
> >>  EXPORT_SYMBOL(drm_match_cea_mode);
> >>  
> >> +/**
> >> + * drm_get_cea_aspect_ratio - get the picture aspect ratio corresponding to
> >> + * the input VIC from the CEA mode list
> >> + *
> >> + * Returns picture aspect ratio
> >> + */
> >> +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;
> >> +}
> >> +EXPORT_SYMBOL(drm_get_cea_aspect_ratio);
> >> +
> >>  /*
> >>   * Calculate the alternate clock for HDMI modes (those from the HDMI vendor
> >>   * specific block).
> >> @@ -3613,6 +3628,20 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> >>  	frame->video_code = drm_match_cea_mode(mode);
> >>  
> >>  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> >> +
> >> +	/* Populate picture aspect ratio from CEA mode list */
> >> +	if (frame->video_code > 0)
> >> +		frame->picture_aspect = drm_get_cea_aspect_ratio(
> >> +						frame->video_code);
> >> +	else {
> >> +		if (!(mode->vdisplay % 3) &&
> >> +			(((mode->vdisplay * 4) / 3) == mode->hdisplay))
> >> +			frame->picture_aspect = HDMI_PICTURE_ASPECT_4_3;
> >> +		else if (!(mode->vdisplay % 9) &&
> >> +			(((mode->vdisplay * 16) / 9) == mode->hdisplay))
> >> +			frame->picture_aspect = HDMI_PICTURE_ASPECT_16_9;
> >> +	}
> > 
> > I'm not sure if providing the PAR for non-CEA modes like this makes
> > any real difference. But I guess it can't hurt since you only provide
> > it for exact matches.
> > 
> > But the matches are maybe even a bit too exact. For instance 1366x768
> > will not match the 16:9 case. So maybe it should be calculated in a bit
> > more relaxed way.
> > 
> > Or just dropped totally. I'm not sure.
> > 
> Maybe we can drop it for now (for this patch) and come up with another
> patch containing calculations which are more relaxed than the above.
> What do you think?

That's fine by me.

> >> +
> >>  	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> >>  	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
> >>  
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index 27f828c..50dc55a 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -983,6 +983,7 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >>  extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
> >>  				    void *data, struct drm_file *file_priv);
> >>  extern u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
> >> +extern enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
> >>  extern bool drm_detect_hdmi_monitor(struct edid *edid);
> >>  extern bool drm_detect_monitor_audio(struct edid *edid);
> >>  extern bool drm_rgb_quant_range_selectable(struct edid *edid);
> >> -- 
> >> 1.9.1
> > 

-- 
Ville Syrjälä
Intel OTC