diff mbox series

[4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)

Message ID 20191004141914.20600-4-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/4] drm/edid: Make drm_get_cea_aspect_ratio() static | expand

Commit Message

Ville Syrjälä Oct. 4, 2019, 2:19 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I think this should provide most of necessary logic for
adding aspecr ratios to the HDMI 4k modes.

Cc: Wayne Lin <waynelin@amd.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

Comments

Lin, Wayne Oct. 14, 2019, 9:27 a.m. UTC | #1
> -----Original Message-----
> From: Ville Syrjala <ville.syrjala@linux.intel.com>
> Sent: Friday, October 4, 2019 10:19 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Lin, Wayne <Wayne.Lin@amd.com>
> Subject: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I think this should provide most of necessary logic for adding aspecr ratios to
> the HDMI 4k modes.
> 
> Cc: Wayne Lin <waynelin@amd.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> c7f9f7ca75a2..c76814edc784 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3210,6 +3210,11 @@ static enum hdmi_picture_aspect
> drm_get_cea_aspect_ratio(const u8 video_code)
>  	return edid_cea_modes[video_code].picture_aspect_ratio;
>  }
> 
> +static enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8
> +video_code) {
> +	return edid_4k_modes[video_code].picture_aspect_ratio;
> +}
> +

There are no picture_aspect_ratio attributes defined for modes in 
edid_4k_modes[] now. Should add on those definitions.

>  /*
>   * Calculate the alternate clock for HDMI modes (those from the HDMI
> vendor
>   * specific block).
> @@ -3236,6 +3241,9 @@ static u8
> drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
>  	if (!to_match->clock)
>  		return 0;
> 
> +	if (to_match->picture_aspect_ratio)
> +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> +
>  	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;
> @@ -3271,6 +3279,9 @@ static u8 drm_match_hdmi_mode(const struct
> drm_display_mode *to_match)
>  	if (!to_match->clock)
>  		return 0;
> 
> +	if (to_match->picture_aspect_ratio)
> +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> +
>  	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;

Current code in drm_match_hdmi_mdoe() & drm_match_hdmi_mode_clock_tolerance()
use hdmi_mode_alternate_clock() to find alternate clocks.
In hdmi_mode_alternate_clock(), it adds an exception for VIC 4 mode (4096x2160@24)
due to there is no alternate clock defined for that mode in HDMI1.4b. But HDMI2.0 adds
23.98Hz for that mode. Maybe we should also revise that part.

> @@ -5218,6 +5229,7 @@
> drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> *frame,
>  					 const struct drm_display_mode *mode)  {
>  	enum hdmi_picture_aspect picture_aspect;
> +	u8 vic, hdmi_vic;
>  	int err;
> 
>  	if (!frame || !mode)
> @@ -5230,7 +5242,8 @@
> drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> *frame,
>  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		frame->pixel_repeat = 1;
> 
> -	frame->video_code = drm_mode_cea_vic(connector, mode);
> +	vic = drm_mode_cea_vic(connector, mode);
> +	hdmi_vic = drm_mode_hdmi_vic(connector, mode);
> 
>  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> 
> @@ -5244,11 +5257,15 @@
> drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> *frame,
> 
>  	/*
>  	 * Populate picture aspect ratio from either
> -	 * user input (if specified) or from the CEA mode list.
> +	 * user input (if specified) or from the CEA/HDMI mode lists.
>  	 */
>  	picture_aspect = mode->picture_aspect_ratio;
> -	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
> -		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
> +	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) {
> +		if (vic)
> +			picture_aspect = drm_get_cea_aspect_ratio(vic);
> +		else if (hdmi_vic)
> +			picture_aspect = drm_get_hdmi_aspect_ratio(hdmi_vic);
> +	}
> 
>  	/*
>  	 * The infoframe can't convey anything but none, 4:3 @@ -5256,12
> +5273,20 @@ drm_hdmi_avi_infoframe_from_display_mode(struct
> hdmi_avi_infoframe *frame,
>  	 * we can only satisfy it by specifying the right VIC.
>  	 */
>  	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
> -		if (picture_aspect !=
> -		    drm_get_cea_aspect_ratio(frame->video_code))
> +		if (vic) {
> +			if (picture_aspect != drm_get_cea_aspect_ratio(vic))
> +				return -EINVAL;
> +		} else if (hdmi_vic) {
> +			if (picture_aspect != drm_get_hdmi_aspect_ratio(hdmi_vic))
> +				return -EINVAL;
> +		} else {
>  			return -EINVAL;
> +		}
> +
>  		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
>  	}
> 
> +	frame->video_code = vic;
>  	frame->picture_aspect = picture_aspect;
>  	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
>  	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
> --
> 2.21.0

--
Wayne Lin
Ville Syrjälä Oct. 14, 2019, 2:41 p.m. UTC | #2
On Mon, Oct 14, 2019 at 09:27:07AM +0000, Lin, Wayne wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Sent: Friday, October 4, 2019 10:19 PM
> > To: dri-devel@lists.freedesktop.org
> > Cc: intel-gfx@lists.freedesktop.org; Lin, Wayne <Wayne.Lin@amd.com>
> > Subject: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I think this should provide most of necessary logic for adding aspecr ratios to
> > the HDMI 4k modes.
> > 
> > Cc: Wayne Lin <waynelin@amd.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 37 +++++++++++++++++++++++++++++++------
> >  1 file changed, 31 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> > c7f9f7ca75a2..c76814edc784 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3210,6 +3210,11 @@ static enum hdmi_picture_aspect
> > drm_get_cea_aspect_ratio(const u8 video_code)
> >  	return edid_cea_modes[video_code].picture_aspect_ratio;
> >  }
> > 
> > +static enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8
> > +video_code) {
> > +	return edid_4k_modes[video_code].picture_aspect_ratio;
> > +}
> > +
> 
> There are no picture_aspect_ratio attributes defined for modes in 
> edid_4k_modes[] now. Should add on those definitions.
> 
> >  /*
> >   * Calculate the alternate clock for HDMI modes (those from the HDMI
> > vendor
> >   * specific block).
> > @@ -3236,6 +3241,9 @@ static u8
> > drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
> >  	if (!to_match->clock)
> >  		return 0;
> > 
> > +	if (to_match->picture_aspect_ratio)
> > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > +
> >  	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;
> > @@ -3271,6 +3279,9 @@ static u8 drm_match_hdmi_mode(const struct
> > drm_display_mode *to_match)
> >  	if (!to_match->clock)
> >  		return 0;
> > 
> > +	if (to_match->picture_aspect_ratio)
> > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > +
> >  	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;
> 
> Current code in drm_match_hdmi_mdoe() & drm_match_hdmi_mode_clock_tolerance()
> use hdmi_mode_alternate_clock() to find alternate clocks.
> In hdmi_mode_alternate_clock(), it adds an exception for VIC 4 mode (4096x2160@24)
> due to there is no alternate clock defined for that mode in HDMI1.4b. But HDMI2.0 adds
> 23.98Hz for that mode. Maybe we should also revise that part.

I'm tempted to just remove that exception. I have a hard time imagining
it causing serious problems.

> 
> > @@ -5218,6 +5229,7 @@
> > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > *frame,
> >  					 const struct drm_display_mode *mode)  {
> >  	enum hdmi_picture_aspect picture_aspect;
> > +	u8 vic, hdmi_vic;
> >  	int err;
> > 
> >  	if (!frame || !mode)
> > @@ -5230,7 +5242,8 @@
> > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > *frame,
> >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> >  		frame->pixel_repeat = 1;
> > 
> > -	frame->video_code = drm_mode_cea_vic(connector, mode);
> > +	vic = drm_mode_cea_vic(connector, mode);
> > +	hdmi_vic = drm_mode_hdmi_vic(connector, mode);
> > 
> >  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > 
> > @@ -5244,11 +5257,15 @@
> > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > *frame,
> > 
> >  	/*
> >  	 * Populate picture aspect ratio from either
> > -	 * user input (if specified) or from the CEA mode list.
> > +	 * user input (if specified) or from the CEA/HDMI mode lists.
> >  	 */
> >  	picture_aspect = mode->picture_aspect_ratio;
> > -	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
> > -		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
> > +	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) {
> > +		if (vic)
> > +			picture_aspect = drm_get_cea_aspect_ratio(vic);
> > +		else if (hdmi_vic)
> > +			picture_aspect = drm_get_hdmi_aspect_ratio(hdmi_vic);
> > +	}
> > 
> >  	/*
> >  	 * The infoframe can't convey anything but none, 4:3 @@ -5256,12
> > +5273,20 @@ drm_hdmi_avi_infoframe_from_display_mode(struct
> > hdmi_avi_infoframe *frame,
> >  	 * we can only satisfy it by specifying the right VIC.
> >  	 */
> >  	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
> > -		if (picture_aspect !=
> > -		    drm_get_cea_aspect_ratio(frame->video_code))
> > +		if (vic) {
> > +			if (picture_aspect != drm_get_cea_aspect_ratio(vic))
> > +				return -EINVAL;
> > +		} else if (hdmi_vic) {
> > +			if (picture_aspect != drm_get_hdmi_aspect_ratio(hdmi_vic))
> > +				return -EINVAL;
> > +		} else {
> >  			return -EINVAL;
> > +		}
> > +
> >  		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> >  	}
> > 
> > +	frame->video_code = vic;
> >  	frame->picture_aspect = picture_aspect;
> >  	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> >  	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
> > --
> > 2.21.0
> 
> --
> Wayne Lin
Lin, Wayne Oct. 21, 2019, 6:28 a.m. UTC | #3
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Monday, October 14, 2019 10:42 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> 
> On Mon, Oct 14, 2019 at 09:27:07AM +0000, Lin, Wayne wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Sent: Friday, October 4, 2019 10:19 PM
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: intel-gfx@lists.freedesktop.org; Lin, Wayne <Wayne.Lin@amd.com>
> > > Subject: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > I think this should provide most of necessary logic for adding
> > > aspecr ratios to the HDMI 4k modes.
> > >
> > > Cc: Wayne Lin <waynelin@amd.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c | 37
> > > +++++++++++++++++++++++++++++++------
> > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index
> > > c7f9f7ca75a2..c76814edc784 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3210,6 +3210,11 @@ static enum hdmi_picture_aspect
> > > drm_get_cea_aspect_ratio(const u8 video_code)
> > >  	return edid_cea_modes[video_code].picture_aspect_ratio;
> > >  }
> > >
> > > +static enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8
> > > +video_code) {
> > > +	return edid_4k_modes[video_code].picture_aspect_ratio;
> > > +}
> > > +
> >
> > There are no picture_aspect_ratio attributes defined for modes in
> > edid_4k_modes[] now. Should add on those definitions.
> >
> > >  /*
> > >   * Calculate the alternate clock for HDMI modes (those from the
> > > HDMI vendor
> > >   * specific block).
> > > @@ -3236,6 +3241,9 @@ static u8
> > > drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode
> *to_
> > >  	if (!to_match->clock)
> > >  		return 0;
> > >
> > > +	if (to_match->picture_aspect_ratio)
> > > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > > +
> > >  	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;
> > > @@ -3271,6 +3279,9 @@ static u8 drm_match_hdmi_mode(const struct
> > > drm_display_mode *to_match)
> > >  	if (!to_match->clock)
> > >  		return 0;
> > >
> > > +	if (to_match->picture_aspect_ratio)
> > > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > > +
> > >  	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;
> >
> > Current code in drm_match_hdmi_mdoe() &
> > drm_match_hdmi_mode_clock_tolerance()
> > use hdmi_mode_alternate_clock() to find alternate clocks.
> > In hdmi_mode_alternate_clock(), it adds an exception for VIC 4 mode
> > (4096x2160@24) due to there is no alternate clock defined for that
> > mode in HDMI1.4b. But HDMI2.0 adds 23.98Hz for that mode. Maybe we
> should also revise that part.
> 
> I'm tempted to just remove that exception. I have a hard time imagining it
> causing serious problems.

Thanks for your time.
I've run smoke test and CTS to verify these patches (with adding the aspect ratio 
attribute to edid_4k_modes[] and removing the exception for23.98Hz). So far it
looks good on my environment. Is there any further modification should be done
on these patches?

> 
> >
> > > @@ -5218,6 +5229,7 @@
> > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > *frame,
> > >  					 const struct drm_display_mode *mode)  {
> > >  	enum hdmi_picture_aspect picture_aspect;
> > > +	u8 vic, hdmi_vic;
> > >  	int err;
> > >
> > >  	if (!frame || !mode)
> > > @@ -5230,7 +5242,8 @@
> > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > *frame,
> > >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > >  		frame->pixel_repeat = 1;
> > >
> > > -	frame->video_code = drm_mode_cea_vic(connector, mode);
> > > +	vic = drm_mode_cea_vic(connector, mode);
> > > +	hdmi_vic = drm_mode_hdmi_vic(connector, mode);
> > >
> > >  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > >
> > > @@ -5244,11 +5257,15 @@
> > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > *frame,
> > >
> > >  	/*
> > >  	 * Populate picture aspect ratio from either
> > > -	 * user input (if specified) or from the CEA mode list.
> > > +	 * user input (if specified) or from the CEA/HDMI mode lists.
> > >  	 */
> > >  	picture_aspect = mode->picture_aspect_ratio;
> > > -	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
> > > -		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
> > > +	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) {
> > > +		if (vic)
> > > +			picture_aspect = drm_get_cea_aspect_ratio(vic);
> > > +		else if (hdmi_vic)
> > > +			picture_aspect = drm_get_hdmi_aspect_ratio(hdmi_vic);
> > > +	}
> > >
> > >  	/*
> > >  	 * The infoframe can't convey anything but none, 4:3 @@ -5256,12
> > > +5273,20 @@ drm_hdmi_avi_infoframe_from_display_mode(struct
> > > hdmi_avi_infoframe *frame,
> > >  	 * we can only satisfy it by specifying the right VIC.
> > >  	 */
> > >  	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
> > > -		if (picture_aspect !=
> > > -		    drm_get_cea_aspect_ratio(frame->video_code))
> > > +		if (vic) {
> > > +			if (picture_aspect != drm_get_cea_aspect_ratio(vic))
> > > +				return -EINVAL;
> > > +		} else if (hdmi_vic) {
> > > +			if (picture_aspect !=
> drm_get_hdmi_aspect_ratio(hdmi_vic))
> > > +				return -EINVAL;
> > > +		} else {
> > >  			return -EINVAL;
> > > +		}
> > > +
> > >  		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > >  	}
> > >
> > > +	frame->video_code = vic;
> > >  	frame->picture_aspect = picture_aspect;
> > >  	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> > >  	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
> > > --
> > > 2.21.0
> >
> > --
> > Wayne Lin
> 
> --
> Ville Syrjälä
> Intel

--
Wayne Lin
Ville Syrjälä Oct. 25, 2019, 7:19 p.m. UTC | #4
On Mon, Oct 21, 2019 at 06:28:18AM +0000, Lin, Wayne wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Monday, October 14, 2019 10:42 PM
> > To: Lin, Wayne <Wayne.Lin@amd.com>
> > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> > 
> > On Mon, Oct 14, 2019 at 09:27:07AM +0000, Lin, Wayne wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Sent: Friday, October 4, 2019 10:19 PM
> > > > To: dri-devel@lists.freedesktop.org
> > > > Cc: intel-gfx@lists.freedesktop.org; Lin, Wayne <Wayne.Lin@amd.com>
> > > > Subject: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > I think this should provide most of necessary logic for adding
> > > > aspecr ratios to the HDMI 4k modes.
> > > >
> > > > Cc: Wayne Lin <waynelin@amd.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_edid.c | 37
> > > > +++++++++++++++++++++++++++++++------
> > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index
> > > > c7f9f7ca75a2..c76814edc784 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -3210,6 +3210,11 @@ static enum hdmi_picture_aspect
> > > > drm_get_cea_aspect_ratio(const u8 video_code)
> > > >  	return edid_cea_modes[video_code].picture_aspect_ratio;
> > > >  }
> > > >
> > > > +static enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8
> > > > +video_code) {
> > > > +	return edid_4k_modes[video_code].picture_aspect_ratio;
> > > > +}
> > > > +
> > >
> > > There are no picture_aspect_ratio attributes defined for modes in
> > > edid_4k_modes[] now. Should add on those definitions.
> > >
> > > >  /*
> > > >   * Calculate the alternate clock for HDMI modes (those from the
> > > > HDMI vendor
> > > >   * specific block).
> > > > @@ -3236,6 +3241,9 @@ static u8
> > > > drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode
> > *to_
> > > >  	if (!to_match->clock)
> > > >  		return 0;
> > > >
> > > > +	if (to_match->picture_aspect_ratio)
> > > > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > > > +
> > > >  	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;
> > > > @@ -3271,6 +3279,9 @@ static u8 drm_match_hdmi_mode(const struct
> > > > drm_display_mode *to_match)
> > > >  	if (!to_match->clock)
> > > >  		return 0;
> > > >
> > > > +	if (to_match->picture_aspect_ratio)
> > > > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > > > +
> > > >  	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;
> > >
> > > Current code in drm_match_hdmi_mdoe() &
> > > drm_match_hdmi_mode_clock_tolerance()
> > > use hdmi_mode_alternate_clock() to find alternate clocks.
> > > In hdmi_mode_alternate_clock(), it adds an exception for VIC 4 mode
> > > (4096x2160@24) due to there is no alternate clock defined for that
> > > mode in HDMI1.4b. But HDMI2.0 adds 23.98Hz for that mode. Maybe we
> > should also revise that part.
> > 
> > I'm tempted to just remove that exception. I have a hard time imagining it
> > causing serious problems.
> 
> Thanks for your time.
> I've run smoke test and CTS to verify these patches (with adding the aspect ratio 
> attribute to edid_4k_modes[] and removing the exception for23.98Hz). So far it
> looks good on my environment. Is there any further modification should be done
> on these patches?

Can't think of anything right now. I pushed the other three patches
already, so I guess now you could pull this last patch into your
patch set and repost the lot?

> 
> > 
> > >
> > > > @@ -5218,6 +5229,7 @@
> > > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > > *frame,
> > > >  					 const struct drm_display_mode *mode)  {
> > > >  	enum hdmi_picture_aspect picture_aspect;
> > > > +	u8 vic, hdmi_vic;
> > > >  	int err;
> > > >
> > > >  	if (!frame || !mode)
> > > > @@ -5230,7 +5242,8 @@
> > > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > > *frame,
> > > >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > >  		frame->pixel_repeat = 1;
> > > >
> > > > -	frame->video_code = drm_mode_cea_vic(connector, mode);
> > > > +	vic = drm_mode_cea_vic(connector, mode);
> > > > +	hdmi_vic = drm_mode_hdmi_vic(connector, mode);
> > > >
> > > >  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > > >
> > > > @@ -5244,11 +5257,15 @@
> > > > drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe
> > > > *frame,
> > > >
> > > >  	/*
> > > >  	 * Populate picture aspect ratio from either
> > > > -	 * user input (if specified) or from the CEA mode list.
> > > > +	 * user input (if specified) or from the CEA/HDMI mode lists.
> > > >  	 */
> > > >  	picture_aspect = mode->picture_aspect_ratio;
> > > > -	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
> > > > -		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
> > > > +	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) {
> > > > +		if (vic)
> > > > +			picture_aspect = drm_get_cea_aspect_ratio(vic);
> > > > +		else if (hdmi_vic)
> > > > +			picture_aspect = drm_get_hdmi_aspect_ratio(hdmi_vic);
> > > > +	}
> > > >
> > > >  	/*
> > > >  	 * The infoframe can't convey anything but none, 4:3 @@ -5256,12
> > > > +5273,20 @@ drm_hdmi_avi_infoframe_from_display_mode(struct
> > > > hdmi_avi_infoframe *frame,
> > > >  	 * we can only satisfy it by specifying the right VIC.
> > > >  	 */
> > > >  	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
> > > > -		if (picture_aspect !=
> > > > -		    drm_get_cea_aspect_ratio(frame->video_code))
> > > > +		if (vic) {
> > > > +			if (picture_aspect != drm_get_cea_aspect_ratio(vic))
> > > > +				return -EINVAL;
> > > > +		} else if (hdmi_vic) {
> > > > +			if (picture_aspect !=
> > drm_get_hdmi_aspect_ratio(hdmi_vic))
> > > > +				return -EINVAL;
> > > > +		} else {
> > > >  			return -EINVAL;
> > > > +		}
> > > > +
> > > >  		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > > >  	}
> > > >
> > > > +	frame->video_code = vic;
> > > >  	frame->picture_aspect = picture_aspect;
> > > >  	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> > > >  	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
> > > > --
> > > > 2.21.0
> > >
> > > --
> > > Wayne Lin
> > 
> > --
> > Ville Syrjälä
> > Intel
> 
> --
> Wayne Lin
Lin, Wayne Oct. 29, 2019, 10:21 a.m. UTC | #5
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Saturday, October 26, 2019 3:20 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio (WIP)
> 
> On Mon, Oct 21, 2019 at 06:28:18AM +0000, Lin, Wayne wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Sent: Monday, October 14, 2019 10:42 PM
> > > To: Lin, Wayne <Wayne.Lin@amd.com>
> > > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio
> > > (WIP)
> > >
> > > On Mon, Oct 14, 2019 at 09:27:07AM +0000, Lin, Wayne wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Sent: Friday, October 4, 2019 10:19 PM
> > > > > To: dri-devel@lists.freedesktop.org
> > > > > Cc: intel-gfx@lists.freedesktop.org; Lin, Wayne
> > > > > <Wayne.Lin@amd.com>
> > > > > Subject: [PATCH 4/4] drm/edid: Prep for HDMI VIC aspect ratio
> > > > > (WIP)
> > > > >
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > I think this should provide most of necessary logic for adding
> > > > > aspecr ratios to the HDMI 4k modes.
> > > > >
> > > > > Cc: Wayne Lin <waynelin@amd.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_edid.c | 37
> > > > > +++++++++++++++++++++++++++++++------
> > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c
> > > > > b/drivers/gpu/drm/drm_edid.c index
> > > > > c7f9f7ca75a2..c76814edc784 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -3210,6 +3210,11 @@ static enum hdmi_picture_aspect
> > > > > drm_get_cea_aspect_ratio(const u8 video_code)
> > > > >  	return edid_cea_modes[video_code].picture_aspect_ratio;
> > > > >  }
> > > > >
> > > > > +static enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const
> > > > > +u8
> > > > > +video_code) {
> > > > > +	return edid_4k_modes[video_code].picture_aspect_ratio;
> > > > > +}
> > > > > +
> > > >
> > > > There are no picture_aspect_ratio attributes defined for modes in
> > > > edid_4k_modes[] now. Should add on those definitions.
> > > >
> > > > >  /*
> > > > >   * Calculate the alternate clock for HDMI modes (those from the
> > > > > HDMI vendor
> > > > >   * specific block).
> > > > > @@ -3236,6 +3241,9 @@ static u8
> > > > > drm_match_hdmi_mode_clock_tolerance(const struct
> > > > > drm_display_mode
> > > *to_
> > > > >  	if (!to_match->clock)
> > > > >  		return 0;
> > > > >
> > > > > +	if (to_match->picture_aspect_ratio)
> > > > > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > > > > +
> > > > >  	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;
> > > > > @@ -3271,6 +3279,9 @@ static u8 drm_match_hdmi_mode(const
> struct
> > > > > drm_display_mode *to_match)
> > > > >  	if (!to_match->clock)
> > > > >  		return 0;
> > > > >
> > > > > +	if (to_match->picture_aspect_ratio)
> > > > > +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > > > > +
> > > > >  	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;
> > > >
> > > > Current code in drm_match_hdmi_mdoe() &
> > > > drm_match_hdmi_mode_clock_tolerance()
> > > > use hdmi_mode_alternate_clock() to find alternate clocks.
> > > > In hdmi_mode_alternate_clock(), it adds an exception for VIC 4
> > > > mode
> > > > (4096x2160@24) due to there is no alternate clock defined for that
> > > > mode in HDMI1.4b. But HDMI2.0 adds 23.98Hz for that mode. Maybe we
> > > should also revise that part.
> > >
> > > I'm tempted to just remove that exception. I have a hard time
> > > imagining it causing serious problems.
> >
> > Thanks for your time.
> > I've run smoke test and CTS to verify these patches (with adding the
> > aspect ratio attribute to edid_4k_modes[] and removing the exception
> > for23.98Hz). So far it looks good on my environment. Is there any
> > further modification should be done on these patches?
> 
> Can't think of anything right now. I pushed the other three patches already, so I
> guess now you could pull this last patch into your patch set and repost the lot?
> 
Sorry for late reply and thanks for your time.
Sure, I will post another new patch set then (including this last patch and my 
modification)
Thanks!

> >
> > >
> > > >
> > > > > @@ -5218,6 +5229,7 @@
> > > > > drm_hdmi_avi_infoframe_from_display_mode(struct
> > > > > hdmi_avi_infoframe *frame,
> > > > >  					 const struct drm_display_mode *mode)  {
> > > > >  	enum hdmi_picture_aspect picture_aspect;
> > > > > +	u8 vic, hdmi_vic;
> > > > >  	int err;
> > > > >
> > > > >  	if (!frame || !mode)
> > > > > @@ -5230,7 +5242,8 @@
> > > > > drm_hdmi_avi_infoframe_from_display_mode(struct
> > > > > hdmi_avi_infoframe *frame,
> > > > >  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > >  		frame->pixel_repeat = 1;
> > > > >
> > > > > -	frame->video_code = drm_mode_cea_vic(connector, mode);
> > > > > +	vic = drm_mode_cea_vic(connector, mode);
> > > > > +	hdmi_vic = drm_mode_hdmi_vic(connector, mode);
> > > > >
> > > > >  	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > > > >
> > > > > @@ -5244,11 +5257,15 @@
> > > > > drm_hdmi_avi_infoframe_from_display_mode(struct
> > > > > hdmi_avi_infoframe *frame,
> > > > >
> > > > >  	/*
> > > > >  	 * Populate picture aspect ratio from either
> > > > > -	 * user input (if specified) or from the CEA mode list.
> > > > > +	 * user input (if specified) or from the CEA/HDMI mode lists.
> > > > >  	 */
> > > > >  	picture_aspect = mode->picture_aspect_ratio;
> > > > > -	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
> > > > > -		picture_aspect =
> drm_get_cea_aspect_ratio(frame->video_code);
> > > > > +	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) {
> > > > > +		if (vic)
> > > > > +			picture_aspect = drm_get_cea_aspect_ratio(vic);
> > > > > +		else if (hdmi_vic)
> > > > > +			picture_aspect = drm_get_hdmi_aspect_ratio(hdmi_vic);
> > > > > +	}
> > > > >
> > > > >  	/*
> > > > >  	 * The infoframe can't convey anything but none, 4:3 @@
> > > > > -5256,12
> > > > > +5273,20 @@ drm_hdmi_avi_infoframe_from_display_mode(struct
> > > > > hdmi_avi_infoframe *frame,
> > > > >  	 * we can only satisfy it by specifying the right VIC.
> > > > >  	 */
> > > > >  	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
> > > > > -		if (picture_aspect !=
> > > > > -		    drm_get_cea_aspect_ratio(frame->video_code))
> > > > > +		if (vic) {
> > > > > +			if (picture_aspect != drm_get_cea_aspect_ratio(vic))
> > > > > +				return -EINVAL;
> > > > > +		} else if (hdmi_vic) {
> > > > > +			if (picture_aspect !=
> > > drm_get_hdmi_aspect_ratio(hdmi_vic))
> > > > > +				return -EINVAL;
> > > > > +		} else {
> > > > >  			return -EINVAL;
> > > > > +		}
> > > > > +
> > > > >  		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
> > > > >  	}
> > > > >
> > > > > +	frame->video_code = vic;
> > > > >  	frame->picture_aspect = picture_aspect;
> > > > >  	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> > > > >  	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
> > > > > --
> > > > > 2.21.0
> > > >
> > > > --
> > > > Wayne Lin
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> >
> > --
> > Wayne Lin
> 
> --
> Ville Syrjälä
> Intel

--
Wayne Lin
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c7f9f7ca75a2..c76814edc784 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3210,6 +3210,11 @@  static enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code)
 	return edid_cea_modes[video_code].picture_aspect_ratio;
 }
 
+static enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8 video_code)
+{
+	return edid_4k_modes[video_code].picture_aspect_ratio;
+}
+
 /*
  * Calculate the alternate clock for HDMI modes (those from the HDMI vendor
  * specific block).
@@ -3236,6 +3241,9 @@  static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_
 	if (!to_match->clock)
 		return 0;
 
+	if (to_match->picture_aspect_ratio)
+		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
+
 	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;
@@ -3271,6 +3279,9 @@  static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 	if (!to_match->clock)
 		return 0;
 
+	if (to_match->picture_aspect_ratio)
+		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
+
 	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;
@@ -5218,6 +5229,7 @@  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 					 const struct drm_display_mode *mode)
 {
 	enum hdmi_picture_aspect picture_aspect;
+	u8 vic, hdmi_vic;
 	int err;
 
 	if (!frame || !mode)
@@ -5230,7 +5242,8 @@  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
 		frame->pixel_repeat = 1;
 
-	frame->video_code = drm_mode_cea_vic(connector, mode);
+	vic = drm_mode_cea_vic(connector, mode);
+	hdmi_vic = drm_mode_hdmi_vic(connector, mode);
 
 	frame->picture_aspect = HDMI_PICTURE_ASPECT_NONE;
 
@@ -5244,11 +5257,15 @@  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 
 	/*
 	 * Populate picture aspect ratio from either
-	 * user input (if specified) or from the CEA mode list.
+	 * user input (if specified) or from the CEA/HDMI mode lists.
 	 */
 	picture_aspect = mode->picture_aspect_ratio;
-	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
-		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
+	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) {
+		if (vic)
+			picture_aspect = drm_get_cea_aspect_ratio(vic);
+		else if (hdmi_vic)
+			picture_aspect = drm_get_hdmi_aspect_ratio(hdmi_vic);
+	}
 
 	/*
 	 * The infoframe can't convey anything but none, 4:3
@@ -5256,12 +5273,20 @@  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 	 * we can only satisfy it by specifying the right VIC.
 	 */
 	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
-		if (picture_aspect !=
-		    drm_get_cea_aspect_ratio(frame->video_code))
+		if (vic) {
+			if (picture_aspect != drm_get_cea_aspect_ratio(vic))
+				return -EINVAL;
+		} else if (hdmi_vic) {
+			if (picture_aspect != drm_get_hdmi_aspect_ratio(hdmi_vic))
+				return -EINVAL;
+		} else {
 			return -EINVAL;
+		}
+
 		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
 	}
 
+	frame->video_code = vic;
 	frame->picture_aspect = picture_aspect;
 	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
 	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;