[RFC PATCH] drm/edid: index CEA/HDMI mode tables using the VIC

[Jani Nikula]

On Wed, 02 Dec 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Wed, Dec 02, 2015 at 01:46:40PM +0200, Jani Nikula wrote:
>> On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> > On Tue, Dec 01, 2015 at 05:16:54PM +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 now explicitly check for 0 instead of
>> >> just subtracting one from an unsigned type.
>> >> 
>> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> >> 
>> >> ---
>> >> 
>> >> This is on top of
>> >> 
>> >> commit 4c6bcf44549907cb50b67f98eb13717a4adc6b33
>> >> Author: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >> Date:   Mon Nov 16 21:05:12 2015 +0200
>> >> 
>> >>     drm/edid: Make the detailed timing CEA/HDMI mode fixup accept up to 5kHz clock difference
>> >> 
>> >> in drm-intel/topic/drm-misc
>> >> ---
>> >>  drivers/gpu/drm/drm_edid.c | 84 ++++++++++++++++++++++++----------------------
>> >>  1 file changed, 43 insertions(+), 41 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> >> index c214f1246cb4..aea09eae407e 100644
>> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> +++ b/drivers/gpu/drm/drm_edid.c
>> > <snip>
>> >> @@ -2713,16 +2716,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 (vic > 0 && vic < ARRAY_SIZE(edid_cea_modes)) {
>> >
>> > Maybe add some sort of vic_is_valid() helpers to avoid duplicating
>> > these in so many places?
>> 
>> Any thoughts on how that should handle CEA vs. HDMI VICs, comparing
>> against ARRAY_SIZE(edid_cea_modes) and ARRAY_SIZE(edid_4k_modes)?
>
> I was thinking one function for each type, and yeah just
>
> return vic != 0 && vic < ARRAY_SIZE(whatever_modes);
>
> or something along those lines.
>
>> 
>> Add versions of drm_match_*_mode*() that return struct drm_display_mode
>> * or NULL on errors?
>
> Some time ago I was readin the DP spec, and ended up sketching out
> some functions that might help deal with EDIDless DP branch devices.
> For that I cooked up a function to return a pointer to the appropriate
> CEA mode based on the vic (or NULL for bad vics). So I guess if you did
> something like that + take the already existing match function, you
> could wrap them in a neat package that returns the matching cea mode
> for whatever mode is passed in.

Mmmh, so how about just taking this patch as-is for now...?

BR,
Jani.


>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> > Otherwise I like it.
>> >
>> >> +			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 (vic > 0 && vic < ARRAY_SIZE(edid_4k_modes)) {
>> >> +				cea_mode = &edid_4k_modes[vic];
>> >>  				clock2 = hdmi_mode_alternate_clock(cea_mode);
>> >>  			}
>> >>  		}
>> >> @@ -2773,17 +2776,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 (vic == 0 || vic >= ARRAY_SIZE(edid_cea_modes))
>> >>  		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 +2881,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 (vic == 0 || vic >= ARRAY_SIZE(edid_4k_modes)) {
>> >>  		DRM_ERROR("Unknown HDMI VIC: %d\n", vic);
>> >>  		return 0;
>> >>  	}
>> >> @@ -3170,24 +3172,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 (vic > 0 && vic < ARRAY_SIZE(edid_cea_modes)) {
>> >>  		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 (vic > 0 && vic < ARRAY_SIZE(edid_4k_modes)) {
>> >>  			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 +3207,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
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center