Re: [Intel-gfx] [PATCH 3/9] drm/edid: use struct detailed_timing member access in is_rb()

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

On Fri, Mar 25, 2022 at 02:25:25PM +0200, Jani Nikula wrote:
> Use struct detailed_timing member access instead of direct offsets to
> avoid casting.
> 
> Use BUILD_BUG_ON() for sanity check.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> Note: Why can we use range.formula.cvt.flags directly in is_rb() while
> gtf2 functions check for range.flags == 0x02 first to ensure it's gtf2?

Looks to me like is_rb() is just borked.

Other weird stuff I just noticed is get_monitor_range() not doing
anything for flags!=0x1 cases. It also fails to handle the other
flags that were added to byte 4 in EDID 1.4.

> ---
>  drivers/gpu/drm/drm_edid.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 48707eef1dc2..5396fa78e864 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2405,15 +2405,17 @@ drm_for_each_detailed_block(u8 *raw_edid, detailed_cb *cb, void *closure)
>  }
>  
>  static void
> -is_rb(struct detailed_timing *t, void *data)
> +is_rb(struct detailed_timing *timing, void *data)
>  {
> -	u8 *r = (u8 *)t;
> +	bool *res = data;
>  
> -	if (!is_display_descriptor(t, EDID_DETAIL_MONITOR_RANGE))
> +	if (!is_display_descriptor(timing, EDID_DETAIL_MONITOR_RANGE))

Calling this 'timing' when it's not is a bit confusing. I'd probably
rename it to something else for all the display descriptor cases.

>  		return;
>  
> -	if (r[15] & 0x10)
> -		*(bool *)data = true;
> +	BUILD_BUG_ON(offsetof(typeof(*timing), data.other_data.data.range.formula.cvt.flags) != 15);
> +
> +	if (timing->data.other_data.data.range.formula.cvt.flags & 0x10)
> +		*res = true;
>  }
>  
>  /* EDID 1.4 defines this explicitly.  For EDID 1.3, we guess, badly. */
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel