Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550769 > unrolled thread

Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-04 14:30 +0100
    Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-04 17:20 +0100
      Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-04 17:50 +0100
        Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-05 11:10 +0100
          Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-05 13:00 +0100
            Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-05 15:50 +0100
              Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-10 12:20 +0100
                Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-10 13:30 +0100
                  Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-10 16:40 +0100
                    Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-10 17:00 +0100
                      Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-10 18:10 +0100
                        Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-10 18:30 +0100
                          Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-11 11:30 +0100
                            Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-11 12:40 +0100
                              Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Jose Abreu <Jose.Abreu@synopsys.com> - 2017-01-16 14:30 +0100
                                Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-01-16 15:00 +0100

#1550769 — Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> According to the spec the EDID may contain two blocks that
> signal this sampling mode:
> 	- YCbCr 4:2:0 Video Data Block
> 	- YCbCr 4:2:0 Video Capability Map Data Block
> 
> The video data block contains the list of vic's were
> only YCbCr 4:2:0 sampling mode shall be used while the
> video capability map data block contains a mask were
> YCbCr 4:2:0 sampling mode may be used.
> 
> This RFC patch adds support for parsing these two new blocks
> and introduces new flags to signal the drivers if the
> mode is 4:2:0'only or 4:2:0'able.
> 
> The reason this is still a RFC is because there is no
> reference in kernel for this new sampling mode (specially in
> AVI infoframe part), so, I was hoping to hear some feedback
> first.
> 
> Tested in a HDMI 2.0 compliance scenario.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/drm_modes.c |  10 +++-
>  include/uapi/drm/drm_mode.h |   6 ++
>  3 files changed, 151 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 67d6a73..6ce1a38 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>  #define VENDOR_BLOCK    0x03
>  #define SPEAKER_BLOCK	0x04
>  #define VIDEO_CAPABILITY_BLOCK	0x07
> +#define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_420	0x0F
>  #define EDID_BASIC_AUDIO	(1 << 6)
>  #define EDID_CEA_YCRCB444	(1 << 5)
>  #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  	return modes;
>  }
>  
> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_mode *newmode;
> +
> +	if (!drm_valid_cea_vic(vic))
> +		return 0;
> +
> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> +	if (!newmode)
> +		return 0;
> +
> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;

Why does userspace need to know this? My thinking has been that the
driver would do the right thing automagically.

We do probably want some kind of output colorspace property to allow the
user to select between RGB vs. YCbCr etc. But I think even with that we
should still allow the driver to automagically select YCbCr 4:2:0 output
since that's the only way the mode will work.

> +	drm_mode_probed_add(connector, newmode);
> +
> +	return 1;
> +}
> +
> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len)
> +{
> +	int modes = 0, i;
> +
> +	for (i = 0; i < svds_len; i++)
> +		modes += add_420_mode(connector, svds[i]);
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> +		u8 svds_len, const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i, j;
> +
> +	for (i = 0; i < svds_len; i++) {
> +		u8 mask = svds[i];
> +		for (j = 0; j < 8; j++) {
> +			if (mask & (1 << j)) {
> +				newmode = drm_display_mode_from_vic_index(
> +						connector, video_db, video_len,
> +						i * 8 + j);
> +				if (newmode) {
> +					newmode->flags |= DRM_MODE_FLAG_420;
> +					drm_mode_probed_add(connector, newmode);
> +					modes++;
> +				}
> +			}
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> +		const u8 *video_db, u8 video_len)
> +{
> +	struct drm_display_mode *newmode = NULL;
> +	int modes = 0, i;
> +
> +	for (i = 0; i < video_len; i++) {
> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> +				video_len, i);
> +		if (newmode) {
> +			newmode->flags |= DRM_MODE_FLAG_420;
> +			drm_mode_probed_add(connector, newmode);
> +			modes++;
> +		}
> +	}
> +
> +	return modes;
> +}
> +
> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> +		u8 video_len)
> +{
> +	int modes = 0;
> +
> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> +
> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> +				video_db, video_len);
> +	else if (vcb) /* All modes support 4:2:0 mode */
> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> +
> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> +	return modes;
> +}
> +
>  /*
>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>   * @connector: connector corresponding to the HDMI sink
> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>  }
>  
>  static int
> +cea_db_extended_tag(const u8 *db)
> +{
> +	return db[1];
> +}
> +
> +static int
>  cea_revision(const u8 *cea)
>  {
>  	return cea[1];
> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	return hdmi_id == HDMI_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> +		return false;
> +
> +	return true;
> +}
> +
>  #define for_each_cea_db(cea, i, start, end) \
>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>  
> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
>  	const u8 *cea = drm_find_cea_extension(edid);
> -	const u8 *db, *hdmi = NULL, *video = NULL;
> -	u8 dbl, hdmi_len, video_len = 0;
> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> +	      *vcb420 = NULL;
> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>  	int modes = 0;
>  
>  	if (cea && cea_revision(cea) >= 3) {
> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  				hdmi = db;
>  				hdmi_len = dbl;
>  			}
> +			else if (cea_db_is_hdmi_vdb420(db)) {
> +				vdb420 = db;
> +				vdb420_len = dbl;
> +			}
> +			else if (cea_db_is_hdmi_vcb420(db)) {
> +				vcb420 = db;
> +				vcb420_len = dbl;
> +			}
>  		}
>  	}
>  
> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>  					    video_len);
>  
> +	if (vdb420 || vcb420)
> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> +				vcb420, vcb420_len, video, video_len);
> +
>  	return modes;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac6a352..53c65f6 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>  		return false;
>  
> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> +		return false;
> +
>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  					const struct drm_display_mode *mode2)
>  {
> +	unsigned int flags_mask =
> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> +
>  	if (mode1->hdisplay == mode2->hdisplay &&
>  	    mode1->hsync_start == mode2->hsync_start &&
>  	    mode1->hsync_end == mode2->hsync_end &&
> @@ -995,8 +1002,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  	    mode1->vsync_end == mode2->vsync_end &&
>  	    mode1->vtotal == mode2->vtotal &&
>  	    mode1->vscan == mode2->vscan &&
> -	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>  		return true;
>  
>  	return false;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2..dc8e285 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -84,6 +84,12 @@
>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> +/*
> + * HDMI 2.0
> + */
> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> +#define  DRM_MODE_FLAG_420			(1<<19)
> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>  
>  /* Picture aspect ratio options */
>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [next] | [standalone]

#1550966

Hi Ville,


On 04-01-2017 13:22, Ville Syrjälä wrote:
> On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
>> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
>> According to the spec the EDID may contain two blocks that
>> signal this sampling mode:
>> 	- YCbCr 4:2:0 Video Data Block
>> 	- YCbCr 4:2:0 Video Capability Map Data Block
>>
>> The video data block contains the list of vic's were
>> only YCbCr 4:2:0 sampling mode shall be used while the
>> video capability map data block contains a mask were
>> YCbCr 4:2:0 sampling mode may be used.
>>
>> This RFC patch adds support for parsing these two new blocks
>> and introduces new flags to signal the drivers if the
>> mode is 4:2:0'only or 4:2:0'able.
>>
>> The reason this is still a RFC is because there is no
>> reference in kernel for this new sampling mode (specially in
>> AVI infoframe part), so, I was hoping to hear some feedback
>> first.
>>
>> Tested in a HDMI 2.0 compliance scenario.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Carlos Palminha <palminha@synopsys.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/drm_modes.c |  10 +++-
>>  include/uapi/drm/drm_mode.h |   6 ++
>>  3 files changed, 151 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 67d6a73..6ce1a38 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
>>  #define VENDOR_BLOCK    0x03
>>  #define SPEAKER_BLOCK	0x04
>>  #define VIDEO_CAPABILITY_BLOCK	0x07
>> +#define VIDEO_DATA_BLOCK_420	0x0E
>> +#define VIDEO_CAP_BLOCK_420	0x0F
>>  #define EDID_BASIC_AUDIO	(1 << 6)
>>  #define EDID_CEA_YCRCB444	(1 << 5)
>>  #define EDID_CEA_YCRCB422	(1 << 4)
>> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>  	return modes;
>>  }
>>  
>> +static int add_420_mode(struct drm_connector *connector, u8 vic)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_display_mode *newmode;
>> +
>> +	if (!drm_valid_cea_vic(vic))
>> +		return 0;
>> +
>> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>> +	if (!newmode)
>> +		return 0;
>> +
>> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> Why does userspace need to know this? My thinking has been that the
> driver would do the right thing automagically.
>
> We do probably want some kind of output colorspace property to allow the
> user to select between RGB vs. YCbCr etc. But I think even with that we
> should still allow the driver to automagically select YCbCr 4:2:0 output
> since that's the only way the mode will work.

I agree. When only 4:2:0 is supported there is no need to expose
the flag to userspace. How shall then I signal drivers for this
4:2:0'only sampling mode?

So, for the remaining modes, you propose a new field in the mode
structure called 'colorspace' which contains the list of
supported sampling modes for the given mode? I think it would be
a nice addition. This way if a mode supports only RGB we only
passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
flag, ... And then user could select. We also have to inform user
which one is being actually used.

Best regards,
Jose Miguel Abreu

>
>> +	drm_mode_probed_add(connector, newmode);
>> +
>> +	return 1;
>> +}
>> +
>> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
>> +		u8 svds_len)
>> +{
>> +	int modes = 0, i;
>> +
>> +	for (i = 0; i < svds_len; i++)
>> +		modes += add_420_mode(connector, svds[i]);
>> +
>> +	return modes;
>> +}
>> +
>> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
>> +		u8 svds_len, const u8 *video_db, u8 video_len)
>> +{
>> +	struct drm_display_mode *newmode = NULL;
>> +	int modes = 0, i, j;
>> +
>> +	for (i = 0; i < svds_len; i++) {
>> +		u8 mask = svds[i];
>> +		for (j = 0; j < 8; j++) {
>> +			if (mask & (1 << j)) {
>> +				newmode = drm_display_mode_from_vic_index(
>> +						connector, video_db, video_len,
>> +						i * 8 + j);
>> +				if (newmode) {
>> +					newmode->flags |= DRM_MODE_FLAG_420;
>> +					drm_mode_probed_add(connector, newmode);
>> +					modes++;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return modes;
>> +}
>> +
>> +static int add_420_vcb_modes_all(struct drm_connector *connector,
>> +		const u8 *video_db, u8 video_len)
>> +{
>> +	struct drm_display_mode *newmode = NULL;
>> +	int modes = 0, i;
>> +
>> +	for (i = 0; i < video_len; i++) {
>> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
>> +				video_len, i);
>> +		if (newmode) {
>> +			newmode->flags |= DRM_MODE_FLAG_420;
>> +			drm_mode_probed_add(connector, newmode);
>> +			modes++;
>> +		}
>> +	}
>> +
>> +	return modes;
>> +}
>> +
>> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
>> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
>> +		u8 video_len)
>> +{
>> +	int modes = 0;
>> +
>> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
>> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
>> +
>> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
>> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
>> +				video_db, video_len);
>> +	else if (vcb) /* All modes support 4:2:0 mode */
>> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
>> +
>> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
>> +	return modes;
>> +}
>> +
>>  /*
>>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
>>   * @connector: connector corresponding to the HDMI sink
>> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>  }
>>  
>>  static int
>> +cea_db_extended_tag(const u8 *db)
>> +{
>> +	return db[1];
>> +}
>> +
>> +static int
>>  cea_revision(const u8 *cea)
>>  {
>>  	return cea[1];
>> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  	return hdmi_id == HDMI_IEEE_OUI;
>>  }
>>  
>> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  #define for_each_cea_db(cea, i, start, end) \
>>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
>>  
>> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>>  {
>>  	const u8 *cea = drm_find_cea_extension(edid);
>> -	const u8 *db, *hdmi = NULL, *video = NULL;
>> -	u8 dbl, hdmi_len, video_len = 0;
>> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
>> +	      *vcb420 = NULL;
>> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
>>  	int modes = 0;
>>  
>>  	if (cea && cea_revision(cea) >= 3) {
>> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  				hdmi = db;
>>  				hdmi_len = dbl;
>>  			}
>> +			else if (cea_db_is_hdmi_vdb420(db)) {
>> +				vdb420 = db;
>> +				vdb420_len = dbl;
>> +			}
>> +			else if (cea_db_is_hdmi_vcb420(db)) {
>> +				vcb420 = db;
>> +				vcb420_len = dbl;
>> +			}
>>  		}
>>  	}
>>  
>> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>>  					    video_len);
>>  
>> +	if (vdb420 || vcb420)
>> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
>> +				vcb420, vcb420_len, video, video_len);
>> +
>>  	return modes;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ac6a352..53c65f6 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
>>  		return false;
>>  
>> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
>> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
>> +		return false;
>> +
>>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
>>  }
>>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
>> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
>>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>  					const struct drm_display_mode *mode2)
>>  {
>> +	unsigned int flags_mask =
>> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
>> +
>>  	if (mode1->hdisplay == mode2->hdisplay &&
>>  	    mode1->hsync_start == mode2->hsync_start &&
>>  	    mode1->hsync_end == mode2->hsync_end &&
>> @@ -995,8 +1002,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>>  	    mode1->vsync_end == mode2->vsync_end &&
>>  	    mode1->vtotal == mode2->vtotal &&
>>  	    mode1->vscan == mode2->vscan &&
>> -	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
>>  		return true;
>>  
>>  	return false;
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index ce7efe2..dc8e285 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -84,6 +84,12 @@
>>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
>> +/*
>> + * HDMI 2.0
>> + */
>> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
>> +#define  DRM_MODE_FLAG_420			(1<<19)
>> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
>>  
>>  /* Picture aspect ratio options */
>>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
>> -- 
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DgIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=wrEITML_IgrIDSea7Q5Fi_ybz9_TVdQZ4aIpgjsRuvo&s=zbMeMXxRIWLaSbyyljhlMOS74zM106xHxld21xu4kxU&e= 

[toc] | [prev] | [next] | [standalone]

#1550987

On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 04-01-2017 13:22, Ville Syrjälä wrote:
> > On Fri, Dec 30, 2016 at 04:53:16PM +0000, Jose Abreu wrote:
> >> HDMI 2.0 introduces a new sampling mode called YCbCr 4:2:0.
> >> According to the spec the EDID may contain two blocks that
> >> signal this sampling mode:
> >> 	- YCbCr 4:2:0 Video Data Block
> >> 	- YCbCr 4:2:0 Video Capability Map Data Block
> >>
> >> The video data block contains the list of vic's were
> >> only YCbCr 4:2:0 sampling mode shall be used while the
> >> video capability map data block contains a mask were
> >> YCbCr 4:2:0 sampling mode may be used.
> >>
> >> This RFC patch adds support for parsing these two new blocks
> >> and introduces new flags to signal the drivers if the
> >> mode is 4:2:0'only or 4:2:0'able.
> >>
> >> The reason this is still a RFC is because there is no
> >> reference in kernel for this new sampling mode (specially in
> >> AVI infoframe part), so, I was hoping to hear some feedback
> >> first.
> >>
> >> Tested in a HDMI 2.0 compliance scenario.
> >>
> >> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> >> Cc: Carlos Palminha <palminha@synopsys.com>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Sean Paul <seanpaul@chromium.org>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>  drivers/gpu/drm/drm_edid.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
> >>  drivers/gpu/drm/drm_modes.c |  10 +++-
> >>  include/uapi/drm/drm_mode.h |   6 ++
> >>  3 files changed, 151 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 67d6a73..6ce1a38 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -2549,6 +2549,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
> >>  #define VENDOR_BLOCK    0x03
> >>  #define SPEAKER_BLOCK	0x04
> >>  #define VIDEO_CAPABILITY_BLOCK	0x07
> >> +#define VIDEO_DATA_BLOCK_420	0x0E
> >> +#define VIDEO_CAP_BLOCK_420	0x0F
> >>  #define EDID_BASIC_AUDIO	(1 << 6)
> >>  #define EDID_CEA_YCRCB444	(1 << 5)
> >>  #define EDID_CEA_YCRCB422	(1 << 4)
> >> @@ -3050,6 +3052,98 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
> >>  	return modes;
> >>  }
> >>  
> >> +static int add_420_mode(struct drm_connector *connector, u8 vic)
> >> +{
> >> +	struct drm_device *dev = connector->dev;
> >> +	struct drm_display_mode *newmode;
> >> +
> >> +	if (!drm_valid_cea_vic(vic))
> >> +		return 0;
> >> +
> >> +	newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> >> +	if (!newmode)
> >> +		return 0;
> >> +
> >> +	newmode->flags |= DRM_MODE_FLAG_420_ONLY;
> > Why does userspace need to know this? My thinking has been that the
> > driver would do the right thing automagically.
> >
> > We do probably want some kind of output colorspace property to allow the
> > user to select between RGB vs. YCbCr etc. But I think even with that we
> > should still allow the driver to automagically select YCbCr 4:2:0 output
> > since that's the only way the mode will work.
> 
> I agree. When only 4:2:0 is supported there is no need to expose
> the flag to userspace. How shall then I signal drivers for this
> 4:2:0'only sampling mode?
> 
> So, for the remaining modes, you propose a new field in the mode
> structure called 'colorspace' which contains the list of
> supported sampling modes for the given mode? I think it would be
> a nice addition. This way if a mode supports only RGB we only
> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
> flag, ... And then user could select. We also have to inform user
> which one is being actually used.

IIRC there aren't any "RGB only" modes or anything like that. So
YCbCr 4:2:0 is the special case here. We could just add something to the
mode struct for it, or do we already have some other flags thing that's
not exposed to userspace? And I guess drivers should be able to opt into
supporting these 4:2:0 modes in similar way they opt into
interlaced/stereo/whatever.

> 
> Best regards,
> Jose Miguel Abreu
> 
> >
> >> +	drm_mode_probed_add(connector, newmode);
> >> +
> >> +	return 1;
> >> +}
> >> +
> >> +static int add_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
> >> +		u8 svds_len)
> >> +{
> >> +	int modes = 0, i;
> >> +
> >> +	for (i = 0; i < svds_len; i++)
> >> +		modes += add_420_mode(connector, svds[i]);
> >> +
> >> +	return modes;
> >> +}
> >> +
> >> +static int add_420_vcb_modes(struct drm_connector *connector, const u8 *svds,
> >> +		u8 svds_len, const u8 *video_db, u8 video_len)
> >> +{
> >> +	struct drm_display_mode *newmode = NULL;
> >> +	int modes = 0, i, j;
> >> +
> >> +	for (i = 0; i < svds_len; i++) {
> >> +		u8 mask = svds[i];
> >> +		for (j = 0; j < 8; j++) {
> >> +			if (mask & (1 << j)) {
> >> +				newmode = drm_display_mode_from_vic_index(
> >> +						connector, video_db, video_len,
> >> +						i * 8 + j);
> >> +				if (newmode) {
> >> +					newmode->flags |= DRM_MODE_FLAG_420;
> >> +					drm_mode_probed_add(connector, newmode);
> >> +					modes++;
> >> +				}
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return modes;
> >> +}
> >> +
> >> +static int add_420_vcb_modes_all(struct drm_connector *connector,
> >> +		const u8 *video_db, u8 video_len)
> >> +{
> >> +	struct drm_display_mode *newmode = NULL;
> >> +	int modes = 0, i;
> >> +
> >> +	for (i = 0; i < video_len; i++) {
> >> +		newmode = drm_display_mode_from_vic_index(connector, video_db,
> >> +				video_len, i);
> >> +		if (newmode) {
> >> +			newmode->flags |= DRM_MODE_FLAG_420;
> >> +			drm_mode_probed_add(connector, newmode);
> >> +			modes++;
> >> +		}
> >> +	}
> >> +
> >> +	return modes;
> >> +}
> >> +
> >> +static int do_hdmi_420_modes(struct drm_connector *connector, const u8 *vdb,
> >> +		u8 vdb_len, const u8 *vcb, u8 vcb_len, const u8 *video_db,
> >> +		u8 video_len)
> >> +{
> >> +	int modes = 0;
> >> +
> >> +	if (vdb && (vdb_len > 1)) /* Add 4:2:0 modes present in EDID */
> >> +		modes += add_420_vdb_modes(connector, &vdb[2], vdb_len - 1);
> >> +
> >> +	if (vcb && (vcb_len > 1)) /* Parse bit mask of supported modes */
> >> +		modes += add_420_vcb_modes(connector, &vcb[2], vcb_len - 1,
> >> +				video_db, video_len);
> >> +	else if (vcb) /* All modes support 4:2:0 mode */
> >> +		modes += add_420_vcb_modes_all(connector, video_db, video_len);
> >> +
> >> +	DRM_DEBUG("added %d 4:2:0 modes\n", modes);
> >> +	return modes;
> >> +}
> >> +
> >>  /*
> >>   * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
> >>   * @connector: connector corresponding to the HDMI sink
> >> @@ -3206,6 +3300,12 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
> >>  }
> >>  
> >>  static int
> >> +cea_db_extended_tag(const u8 *db)
> >> +{
> >> +	return db[1];
> >> +}
> >> +
> >> +static int
> >>  cea_revision(const u8 *cea)
> >>  {
> >>  	return cea[1];
> >> @@ -3239,6 +3339,28 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  	return hdmi_id == HDMI_IEEE_OUI;
> >>  }
> >>  
> >> +static bool cea_db_is_hdmi_vdb420(const u8 *db)
> >> +{
> >> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >> +		return false;
> >> +
> >> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
> >> +{
> >> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >> +		return false;
> >> +
> >> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_420)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +
> >>  #define for_each_cea_db(cea, i, start, end) \
> >>  	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
> >>  
> >> @@ -3246,8 +3368,9 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
> >>  {
> >>  	const u8 *cea = drm_find_cea_extension(edid);
> >> -	const u8 *db, *hdmi = NULL, *video = NULL;
> >> -	u8 dbl, hdmi_len, video_len = 0;
> >> +	const u8 *db, *hdmi = NULL, *video = NULL, *vdb420 = NULL,
> >> +	      *vcb420 = NULL;
> >> +	u8 dbl, hdmi_len, video_len = 0, vdb420_len = 0, vcb420_len = 0;
> >>  	int modes = 0;
> >>  
> >>  	if (cea && cea_revision(cea) >= 3) {
> >> @@ -3269,6 +3392,14 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  				hdmi = db;
> >>  				hdmi_len = dbl;
> >>  			}
> >> +			else if (cea_db_is_hdmi_vdb420(db)) {
> >> +				vdb420 = db;
> >> +				vdb420_len = dbl;
> >> +			}
> >> +			else if (cea_db_is_hdmi_vcb420(db)) {
> >> +				vcb420 = db;
> >> +				vcb420_len = dbl;
> >> +			}
> >>  		}
> >>  	}
> >>  
> >> @@ -3280,6 +3411,10 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
> >>  					    video_len);
> >>  
> >> +	if (vdb420 || vcb420)
> >> +		modes += do_hdmi_420_modes(connector, vdb420, vdb420_len,
> >> +				vcb420, vcb420_len, video, video_len);
> >> +
> >>  	return modes;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >> index ac6a352..53c65f6 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -967,6 +967,10 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
> >>  	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
> >>  		return false;
> >>  
> >> +	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
> >> +	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
> >> +		return false;
> >> +
> >>  	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
> >>  }
> >>  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
> >> @@ -985,6 +989,9 @@ bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
> >>  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>  					const struct drm_display_mode *mode2)
> >>  {
> >> +	unsigned int flags_mask =
> >> +		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
> >> +
> >>  	if (mode1->hdisplay == mode2->hdisplay &&
> >>  	    mode1->hsync_start == mode2->hsync_start &&
> >>  	    mode1->hsync_end == mode2->hsync_end &&
> >> @@ -995,8 +1002,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>  	    mode1->vsync_end == mode2->vsync_end &&
> >>  	    mode1->vtotal == mode2->vtotal &&
> >>  	    mode1->vscan == mode2->vscan &&
> >> -	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> >> -	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> >> +	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
> >>  		return true;
> >>  
> >>  	return false;
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index ce7efe2..dc8e285 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -84,6 +84,12 @@
> >>  #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
> >>  #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
> >>  #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> >> +/*
> >> + * HDMI 2.0
> >> + */
> >> +#define DRM_MODE_FLAG_420_MASK			(0x03<<19)
> >> +#define  DRM_MODE_FLAG_420			(1<<19)
> >> +#define  DRM_MODE_FLAG_420_ONLY			(1<<20)
> >>  
> >>  /* Picture aspect ratio options */
> >>  #define DRM_MODE_PICTURE_ASPECT_NONE		0
> >> -- 
> >> 1.9.1
> >>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DgIDAw&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=wrEITML_IgrIDSea7Q5Fi_ybz9_TVdQZ4aIpgjsRuvo&s=zbMeMXxRIWLaSbyyljhlMOS74zM106xHxld21xu4kxU&e= 

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1551843

Hi Ville,


On 04-01-2017 16:36, Ville Syrjälä wrote:
> On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
>>
 
[snip]

>>> Why does userspace need to know this? My thinking has been that the
>>> driver would do the right thing automagically.
>>>
>>> We do probably want some kind of output colorspace property to allow the
>>> user to select between RGB vs. YCbCr etc. But I think even with that we
>>> should still allow the driver to automagically select YCbCr 4:2:0 output
>>> since that's the only way the mode will work.
>> I agree. When only 4:2:0 is supported there is no need to expose
>> the flag to userspace. How shall then I signal drivers for this
>> 4:2:0'only sampling mode?
>>
>> So, for the remaining modes, you propose a new field in the mode
>> structure called 'colorspace' which contains the list of
>> supported sampling modes for the given mode? I think it would be
>> a nice addition. This way if a mode supports only RGB we only
>> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
>> flag, ... And then user could select. We also have to inform user
>> which one is being actually used.
> IIRC there aren't any "RGB only" modes or anything like that. So
> YCbCr 4:2:0 is the special case here. We could just add something to the
> mode struct for it, or do we already have some other flags thing that's
> not exposed to userspace? And I guess drivers should be able to opt into
> supporting these 4:2:0 modes in similar way they opt into
> interlaced/stereo/whatever.

I mean, if a source EDID does not declare support for YCbCr modes
(4:2:2 and 4:4:4 [i think they have to be both supported if sink
supports != RGB]) then only RGB can be used. Or is any YCbCr that
is pre-required? Still, I see your point. When EDID declares
support for YCbCr then all modes can use it, and not only some of
them.

I think for stereo modes the flags can be opt in/out in userspace
exposing. There is a function called
drm_mode_expose_to_userspace() which only exposes stereo modes if
user asks to. We could do something similar for 4:2:0 modes (or
even for all pixel encoding). i.e. expose which encoding can be
used in current video mode. What do you think?

About drivers opting in for 4:2:0 modes, then you propose a new
field in drm_connector (called for example ycbcr_420_allowed)
which only does the parsing of the 4:2:0 modes and adds them to
the list when set to true?

Best regards,
Jose Miguel Abreu

>
>> Best regards,
>> Jose Miguel Abreu
>>
>>>

[snip]

[toc] | [prev] | [next] | [standalone]

#1551929

On Thu, Jan 05, 2017 at 10:07:45AM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 04-01-2017 16:36, Ville Syrjälä wrote:
> > On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
> >>
>  
> [snip]
> 
> >>> Why does userspace need to know this? My thinking has been that the
> >>> driver would do the right thing automagically.
> >>>
> >>> We do probably want some kind of output colorspace property to allow the
> >>> user to select between RGB vs. YCbCr etc. But I think even with that we
> >>> should still allow the driver to automagically select YCbCr 4:2:0 output
> >>> since that's the only way the mode will work.
> >> I agree. When only 4:2:0 is supported there is no need to expose
> >> the flag to userspace. How shall then I signal drivers for this
> >> 4:2:0'only sampling mode?
> >>
> >> So, for the remaining modes, you propose a new field in the mode
> >> structure called 'colorspace' which contains the list of
> >> supported sampling modes for the given mode? I think it would be
> >> a nice addition. This way if a mode supports only RGB we only
> >> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
> >> flag, ... And then user could select. We also have to inform user
> >> which one is being actually used.
> > IIRC there aren't any "RGB only" modes or anything like that. So
> > YCbCr 4:2:0 is the special case here. We could just add something to the
> > mode struct for it, or do we already have some other flags thing that's
> > not exposed to userspace? And I guess drivers should be able to opt into
> > supporting these 4:2:0 modes in similar way they opt into
> > interlaced/stereo/whatever.
> 
> I mean, if a source EDID does not declare support for YCbCr modes
> (4:2:2 and 4:4:4 [i think they have to be both supported if sink
> supports != RGB]) then only RGB can be used. Or is any YCbCr that
> is pre-required? Still, I see your point. When EDID declares
> support for YCbCr then all modes can use it, and not only some of
> them.
> 
> I think for stereo modes the flags can be opt in/out in userspace
> exposing. There is a function called
> drm_mode_expose_to_userspace() which only exposes stereo modes if
> user asks to. We could do something similar for 4:2:0 modes (or
> even for all pixel encoding). i.e. expose which encoding can be
> used in current video mode. What do you think?
> 
> About drivers opting in for 4:2:0 modes, then you propose a new
> field in drm_connector (called for example ycbcr_420_allowed)
> which only does the parsing of the 4:2:0 modes and adds them to
> the list when set to true?

Thinking a bit more about this, we do have a slight problem with not
exposing this information to userspace. Namely we can't actually tell
whether any user provided mode would need to output as 4:2:0 or not.
With the new flag userspace could at least inherit that from the kernel
and pass it back in. But still, expanding the uapi for something like
this feels quite wrong to me. Can we simply look at a particular user
supplied mode and tell whether it needs to be output as 4:2:0 (based
on eg. dotclock)?

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1552028

Hi Ville,


On 05-01-2017 11:46, Ville Syrjälä wrote:
> On Thu, Jan 05, 2017 at 10:07:45AM +0000, Jose Abreu wrote:
>> Hi Ville,
>>
>>
>> On 04-01-2017 16:36, Ville Syrjälä wrote:
>>> On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
>>  
>> [snip]
>>
>>>>> Why does userspace need to know this? My thinking has been that the
>>>>> driver would do the right thing automagically.
>>>>>
>>>>> We do probably want some kind of output colorspace property to allow the
>>>>> user to select between RGB vs. YCbCr etc. But I think even with that we
>>>>> should still allow the driver to automagically select YCbCr 4:2:0 output
>>>>> since that's the only way the mode will work.
>>>> I agree. When only 4:2:0 is supported there is no need to expose
>>>> the flag to userspace. How shall then I signal drivers for this
>>>> 4:2:0'only sampling mode?
>>>>
>>>> So, for the remaining modes, you propose a new field in the mode
>>>> structure called 'colorspace' which contains the list of
>>>> supported sampling modes for the given mode? I think it would be
>>>> a nice addition. This way if a mode supports only RGB we only
>>>> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
>>>> flag, ... And then user could select. We also have to inform user
>>>> which one is being actually used.
>>> IIRC there aren't any "RGB only" modes or anything like that. So
>>> YCbCr 4:2:0 is the special case here. We could just add something to the
>>> mode struct for it, or do we already have some other flags thing that's
>>> not exposed to userspace? And I guess drivers should be able to opt into
>>> supporting these 4:2:0 modes in similar way they opt into
>>> interlaced/stereo/whatever.
>> I mean, if a source EDID does not declare support for YCbCr modes
>> (4:2:2 and 4:4:4 [i think they have to be both supported if sink
>> supports != RGB]) then only RGB can be used. Or is any YCbCr that
>> is pre-required? Still, I see your point. When EDID declares
>> support for YCbCr then all modes can use it, and not only some of
>> them.
>>
>> I think for stereo modes the flags can be opt in/out in userspace
>> exposing. There is a function called
>> drm_mode_expose_to_userspace() which only exposes stereo modes if
>> user asks to. We could do something similar for 4:2:0 modes (or
>> even for all pixel encoding). i.e. expose which encoding can be
>> used in current video mode. What do you think?
>>
>> About drivers opting in for 4:2:0 modes, then you propose a new
>> field in drm_connector (called for example ycbcr_420_allowed)
>> which only does the parsing of the 4:2:0 modes and adds them to
>> the list when set to true?
> Thinking a bit more about this, we do have a slight problem with not
> exposing this information to userspace. Namely we can't actually tell
> whether any user provided mode would need to output as 4:2:0 or not.
> With the new flag userspace could at least inherit that from the kernel
> and pass it back in. But still, expanding the uapi for something like
> this feels quite wrong to me. Can we simply look at a particular user
> supplied mode and tell whether it needs to be output as 4:2:0 (based
> on eg. dotclock)?
>

The pixel clock rate is half of the TMDS character rate in 4:2:0
(in 24 bit), but for example in deep color 48 bit it will be the
same rate. There is also a reduction to half of htotal, hactive,
hblank, hfront, hsync and hback but I don't think it's a good
solution to guide us from there. Why does it feel wrong to you
expanding the uapi?

I think its important to say that the chosen colorspace can
improve performance in systems: for example, as I said, 4:2:0
24-bit uses half the rate that RGB 24-bit uses so we have less
trafic in the bus. I am recently working with a FPGA connected
trough pcie and I can definitely say that this is true. But, as
expected, less traffic means less quality in final image so its
not a matter of letting kernel decide, I think its a matter of
user choosing between performance vs. quality.

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1555163

On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 05-01-2017 11:46, Ville Syrjälä wrote:
> > On Thu, Jan 05, 2017 at 10:07:45AM +0000, Jose Abreu wrote:
> >> Hi Ville,
> >>
> >>
> >> On 04-01-2017 16:36, Ville Syrjälä wrote:
> >>> On Wed, Jan 04, 2017 at 04:15:01PM +0000, Jose Abreu wrote:
> >>  
> >> [snip]
> >>
> >>>>> Why does userspace need to know this? My thinking has been that the
> >>>>> driver would do the right thing automagically.
> >>>>>
> >>>>> We do probably want some kind of output colorspace property to allow the
> >>>>> user to select between RGB vs. YCbCr etc. But I think even with that we
> >>>>> should still allow the driver to automagically select YCbCr 4:2:0 output
> >>>>> since that's the only way the mode will work.
> >>>> I agree. When only 4:2:0 is supported there is no need to expose
> >>>> the flag to userspace. How shall then I signal drivers for this
> >>>> 4:2:0'only sampling mode?
> >>>>
> >>>> So, for the remaining modes, you propose a new field in the mode
> >>>> structure called 'colorspace' which contains the list of
> >>>> supported sampling modes for the given mode? I think it would be
> >>>> a nice addition. This way if a mode supports only RGB we only
> >>>> passed RGB flag; if 4:2:0 was also supported we passed the 4:2:0
> >>>> flag, ... And then user could select. We also have to inform user
> >>>> which one is being actually used.
> >>> IIRC there aren't any "RGB only" modes or anything like that. So
> >>> YCbCr 4:2:0 is the special case here. We could just add something to the
> >>> mode struct for it, or do we already have some other flags thing that's
> >>> not exposed to userspace? And I guess drivers should be able to opt into
> >>> supporting these 4:2:0 modes in similar way they opt into
> >>> interlaced/stereo/whatever.
> >> I mean, if a source EDID does not declare support for YCbCr modes
> >> (4:2:2 and 4:4:4 [i think they have to be both supported if sink
> >> supports != RGB]) then only RGB can be used. Or is any YCbCr that
> >> is pre-required? Still, I see your point. When EDID declares
> >> support for YCbCr then all modes can use it, and not only some of
> >> them.
> >>
> >> I think for stereo modes the flags can be opt in/out in userspace
> >> exposing. There is a function called
> >> drm_mode_expose_to_userspace() which only exposes stereo modes if
> >> user asks to. We could do something similar for 4:2:0 modes (or
> >> even for all pixel encoding). i.e. expose which encoding can be
> >> used in current video mode. What do you think?
> >>
> >> About drivers opting in for 4:2:0 modes, then you propose a new
> >> field in drm_connector (called for example ycbcr_420_allowed)
> >> which only does the parsing of the 4:2:0 modes and adds them to
> >> the list when set to true?
> > Thinking a bit more about this, we do have a slight problem with not
> > exposing this information to userspace. Namely we can't actually tell
> > whether any user provided mode would need to output as 4:2:0 or not.
> > With the new flag userspace could at least inherit that from the kernel
> > and pass it back in. But still, expanding the uapi for something like
> > this feels quite wrong to me. Can we simply look at a particular user
> > supplied mode and tell whether it needs to be output as 4:2:0 (based
> > on eg. dotclock)?
> >
> 
> The pixel clock rate is half of the TMDS character rate in 4:2:0
> (in 24 bit), but for example in deep color 48 bit it will be the
> same rate. There is also a reduction to half of htotal, hactive,
> hblank, hfront, hsync and hback but I don't think it's a good
> solution to guide us from there.

I was asking if we can look at a specific modeline and whether we
can tell from that if we would need to output it as 4:2:0.

> Why does it feel wrong to you
> expanding the uapi?

Because it requires changing every single userspace kms client. And
it's not something userspace should have to worry about.

> 
> I think its important to say that the chosen colorspace can
> improve performance in systems: for example, as I said, 4:2:0
> 24-bit uses half the rate that RGB 24-bit uses so we have less
> trafic in the bus. I am recently working with a FPGA connected
> trough pcie and I can definitely say that this is true. But, as
> expected, less traffic means less quality in final image so its
> not a matter of letting kernel decide, I think its a matter of
> user choosing between performance vs. quality.

Image quality control for userspace is a much bigger topic. And
something we have no real precedent for at the moment (apart from 
user choosing a different fb pixel format).

The performance arument is very hardware dependent, and not really
all that relevant IMO. If the user wants the big mode they either
get it or not depending on whether the system can deliver.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1555222

Hi Ville,


On 10-01-2017 11:16, Ville Syrjälä wrote:
> On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
>>

[snip]

>> The pixel clock rate is half of the TMDS character rate in 4:2:0
>> (in 24 bit), but for example in deep color 48 bit it will be the
>> same rate. There is also a reduction to half of htotal, hactive,
>> hblank, hfront, hsync and hback but I don't think it's a good
>> solution to guide us from there.
> I was asking if we can look at a specific modeline and whether we
> can tell from that if we would need to output it as 4:2:0.

Hmm, according to HDMI 2.0 spec there are no 4:2:0 only modes and
the only way to figure out if the mode is 4:2:0 only (or able) is
to parse the VCB and VBD blocks from EDID. The clock is half rate
but this is the source that has to figure it out. The mode is
still passed in a regular way (By VIC, by timing, ...).

>
>> Why does it feel wrong to you
>> expanding the uapi?
> Because it requires changing every single userspace kms client. And
> it's not something userspace should have to worry about.

I agree but, as Daniel said [1], we could make these new HDMI 2.0
features optional and only pass them to userspace if client asked
for them. What do you think?

[1]
https://lists.freedesktop.org/archives/dri-devel/2017-January/128683.html

>
>> I think its important to say that the chosen colorspace can
>> improve performance in systems: for example, as I said, 4:2:0
>> 24-bit uses half the rate that RGB 24-bit uses so we have less
>> trafic in the bus. I am recently working with a FPGA connected
>> trough pcie and I can definitely say that this is true. But, as
>> expected, less traffic means less quality in final image so its
>> not a matter of letting kernel decide, I think its a matter of
>> user choosing between performance vs. quality.
> Image quality control for userspace is a much bigger topic. And
> something we have no real precedent for at the moment (apart from 
> user choosing a different fb pixel format).
>
> The performance arument is very hardware dependent, and not really
> all that relevant IMO. If the user wants the big mode they either
> get it or not depending on whether the system can deliver.
>

Ok. But note that there is no nice way to figure this out. For
example, for a graphics card it all depends (apart from the
graphics HW) on the PCIe bus. If the bus is not free for enough
data rate then user can reach bottlenecks and not output at best
performance. If we gave user the ability to switch from, for
example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
Unless of course we always prefer YCbCr 4:2:0, when possible. I
did this internally for bridge driver dw-hdmi. We always prefer
YCbCr over RGB when they are available. It is user transparent as
the controller does the necessary color space conversion, though,
not ideal in my opinion.

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1555624

On Tue, Jan 10, 2017 at 12:26:53PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 10-01-2017 11:16, Ville Syrjälä wrote:
> > On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
> >>
> 
> [snip]
> 
> >> The pixel clock rate is half of the TMDS character rate in 4:2:0
> >> (in 24 bit), but for example in deep color 48 bit it will be the
> >> same rate. There is also a reduction to half of htotal, hactive,
> >> hblank, hfront, hsync and hback but I don't think it's a good
> >> solution to guide us from there.
> > I was asking if we can look at a specific modeline and whether we
> > can tell from that if we would need to output it as 4:2:0.
> 
> Hmm, according to HDMI 2.0 spec there are no 4:2:0 only modes and
> the only way to figure out if the mode is 4:2:0 only (or able) is
> to parse the VCB and VBD blocks from EDID. The clock is half rate
> but this is the source that has to figure it out. The mode is
> still passed in a regular way (By VIC, by timing, ...).
> 
> >
> >> Why does it feel wrong to you
> >> expanding the uapi?
> > Because it requires changing every single userspace kms client. And
> > it's not something userspace should have to worry about.
> 
> I agree but, as Daniel said [1], we could make these new HDMI 2.0
> features optional and only pass them to userspace if client asked
> for them. What do you think?

Are you going to update all the userspace clients? Exposing HDMI 2.0
modes only for your favorite client doesn't sound like a good plan to
me.

If we simply compute from a specific modeline whether it needs to be
transmitted as 4:2:0, I suppose we could simply look for a matching
mode in the 4:2:0 mode. But that would mean that only the exact modes
listed by the EDID will work, and others might not.

> 
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2017-January/128683.html
> 
> >
> >> I think its important to say that the chosen colorspace can
> >> improve performance in systems: for example, as I said, 4:2:0
> >> 24-bit uses half the rate that RGB 24-bit uses so we have less
> >> trafic in the bus. I am recently working with a FPGA connected
> >> trough pcie and I can definitely say that this is true. But, as
> >> expected, less traffic means less quality in final image so its
> >> not a matter of letting kernel decide, I think its a matter of
> >> user choosing between performance vs. quality.
> > Image quality control for userspace is a much bigger topic. And
> > something we have no real precedent for at the moment (apart from 
> > user choosing a different fb pixel format).
> >
> > The performance arument is very hardware dependent, and not really
> > all that relevant IMO. If the user wants the big mode they either
> > get it or not depending on whether the system can deliver.
> >
> 
> Ok. But note that there is no nice way to figure this out. For
> example, for a graphics card it all depends (apart from the
> graphics HW) on the PCIe bus. If the bus is not free for enough
> data rate then user can reach bottlenecks and not output at best
> performance. If we gave user the ability to switch from, for
> example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.

Userspace won't know anything about such bottlenecks. The kernel
can know it and hence should automagically drop into 4:2:0 mode
if necessary.

> Unless of course we always prefer YCbCr 4:2:0, when possible. I
> did this internally for bridge driver dw-hdmi. We always prefer
> YCbCr over RGB when they are available. It is user transparent as
> the controller does the necessary color space conversion, though,
> not ideal in my opinion.

My idea was that we'd have a property for the output colorspace and
would perhaps default to YCbCr for the CEA modes (as per CEA-861).
Though I'm sure some people would cry about that behaviour as well.
But for the cases where there is no choice but to use a specific
output colorspace, the kernel should just do it automagically IMO. No
point in manking life diffcult for userspace.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1555644

On Tue, Jan 10, 2017 at 05:33:15PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 10, 2017 at 12:26:53PM +0000, Jose Abreu wrote:
> > Hi Ville,
> > 
> > 
> > On 10-01-2017 11:16, Ville Syrjälä wrote:
> > > On Thu, Jan 05, 2017 at 02:46:06PM +0000, Jose Abreu wrote:
> > >>
> > 
> > [snip]
> > 
> > >> The pixel clock rate is half of the TMDS character rate in 4:2:0
> > >> (in 24 bit), but for example in deep color 48 bit it will be the
> > >> same rate. There is also a reduction to half of htotal, hactive,
> > >> hblank, hfront, hsync and hback but I don't think it's a good
> > >> solution to guide us from there.
> > > I was asking if we can look at a specific modeline and whether we
> > > can tell from that if we would need to output it as 4:2:0.
> > 
> > Hmm, according to HDMI 2.0 spec there are no 4:2:0 only modes and
> > the only way to figure out if the mode is 4:2:0 only (or able) is
> > to parse the VCB and VBD blocks from EDID. The clock is half rate
> > but this is the source that has to figure it out. The mode is
> > still passed in a regular way (By VIC, by timing, ...).
> > 
> > >
> > >> Why does it feel wrong to you
> > >> expanding the uapi?
> > > Because it requires changing every single userspace kms client. And
> > > it's not something userspace should have to worry about.
> > 
> > I agree but, as Daniel said [1], we could make these new HDMI 2.0
> > features optional and only pass them to userspace if client asked
> > for them. What do you think?
> 
> Are you going to update all the userspace clients? Exposing HDMI 2.0
> modes only for your favorite client doesn't sound like a good plan to
> me.
> 
> If we simply compute from a specific modeline whether it needs to be
> transmitted as 4:2:0, I suppose we could simply look for a matching
> mode in the 4:2:0 mode. But that would mean that only the exact modes
> listed by the EDID will work, and others might not.

OK, so the 4:2:0 support is apparently listed only for specific VICs.
Hence we will need to just go through those lists to see if we can
(or in fact must) use 4:2:0 for a specific user specified mode.

I would say the only slight question mark at this point is whether we
should favor 4:4:4 at lower bpc or 4:2:0 at higher bpc if we can choose
between the two. My first instinct is to favor the 4:4:4 modes for now.
We could add some knobs later to let the user make that choice.

> 
> > 
> > [1]
> > https://lists.freedesktop.org/archives/dri-devel/2017-January/128683.html
> > 
> > >
> > >> I think its important to say that the chosen colorspace can
> > >> improve performance in systems: for example, as I said, 4:2:0
> > >> 24-bit uses half the rate that RGB 24-bit uses so we have less
> > >> trafic in the bus. I am recently working with a FPGA connected
> > >> trough pcie and I can definitely say that this is true. But, as
> > >> expected, less traffic means less quality in final image so its
> > >> not a matter of letting kernel decide, I think its a matter of
> > >> user choosing between performance vs. quality.
> > > Image quality control for userspace is a much bigger topic. And
> > > something we have no real precedent for at the moment (apart from 
> > > user choosing a different fb pixel format).
> > >
> > > The performance arument is very hardware dependent, and not really
> > > all that relevant IMO. If the user wants the big mode they either
> > > get it or not depending on whether the system can deliver.
> > >
> > 
> > Ok. But note that there is no nice way to figure this out. For
> > example, for a graphics card it all depends (apart from the
> > graphics HW) on the PCIe bus. If the bus is not free for enough
> > data rate then user can reach bottlenecks and not output at best
> > performance. If we gave user the ability to switch from, for
> > example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
> 
> Userspace won't know anything about such bottlenecks. The kernel
> can know it and hence should automagically drop into 4:2:0 mode
> if necessary.
> 
> > Unless of course we always prefer YCbCr 4:2:0, when possible. I
> > did this internally for bridge driver dw-hdmi. We always prefer
> > YCbCr over RGB when they are available. It is user transparent as
> > the controller does the necessary color space conversion, though,
> > not ideal in my opinion.
> 
> My idea was that we'd have a property for the output colorspace and
> would perhaps default to YCbCr for the CEA modes (as per CEA-861).
> Though I'm sure some people would cry about that behaviour as well.
> But for the cases where there is no choice but to use a specific
> output colorspace, the kernel should just do it automagically IMO. No
> point in manking life diffcult for userspace.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1555715

Hi Ville,


[snip]

>> Are you going to update all the userspace clients? Exposing HDMI 2.0
>> modes only for your favorite client doesn't sound like a good plan to
>> me.
>>
>> If we simply compute from a specific modeline whether it needs to be
>> transmitted as 4:2:0, I suppose we could simply look for a matching
>> mode in the 4:2:0 mode. But that would mean that only the exact modes
>> listed by the EDID will work, and others might not.
> OK, so the 4:2:0 support is apparently listed only for specific VICs.

Hmm, the spec is not very clear. It lists video timings which may
be used with YCbCr 4:2:0 but does not explicitly say that only
these timings can be used. Anyway, I checked with a colleague who
has direct access to HDMI Forum and indeed only VIC 96, 97, 101,
102, 106 and 107 can be used with 4:2:0, so you are correct. He
said that the initial intention of this pixel encoding was to
give 60Hz refresh rate in 4k to users who had limited
performance, so it was only intended for higher resolutions.

> Hence we will need to just go through those lists to see if we can
> (or in fact must) use 4:2:0 for a specific user specified mode.

We don't have yet support for these VICs, so this will have to
wait :(

>
> I would say the only slight question mark at this point is whether we
> should favor 4:4:4 at lower bpc or 4:2:0 at higher bpc if we can choose
> between the two. My first instinct is to favor the 4:4:4 modes for now.
> We could add some knobs later to let the user make that choice.

I agree that 4:4:4 should be preferred.

[snip]

>>> Ok. But note that there is no nice way to figure this out. For
>>> example, for a graphics card it all depends (apart from the
>>> graphics HW) on the PCIe bus. If the bus is not free for enough
>>> data rate then user can reach bottlenecks and not output at best
>>> performance. If we gave user the ability to switch from, for
>>> example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
>> Userspace won't know anything about such bottlenecks. The kernel
>> can know it and hence should automagically drop into 4:2:0 mode
>> if necessary.
>>
>>> Unless of course we always prefer YCbCr 4:2:0, when possible. I
>>> did this internally for bridge driver dw-hdmi. We always prefer
>>> YCbCr over RGB when they are available. It is user transparent as
>>> the controller does the necessary color space conversion, though,
>>> not ideal in my opinion.
>> My idea was that we'd have a property for the output colorspace and
>> would perhaps default to YCbCr for the CEA modes (as per CEA-861).
>> Though I'm sure some people would cry about that behaviour as well.
>> But for the cases where there is no choice but to use a specific
>> output colorspace, the kernel should just do it automagically IMO. No
>> point in manking life diffcult for userspace.

But we already have color_formats field in drm_display_info
struct, right? Shouldn't we instead create for example a helper
which returns the best output colorspace? According to what you
said it would be something like:

if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
    return YCBCR444;
else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
    return YCBCR422;
else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
&& vic_is_420)
    return YCBCR420;
else
    return RGB444; /* Mandatory by spec */

>>
>> -- 
>> Ville Syrjälä
>> Intel OTC

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1555735

On Tue, Jan 10, 2017 at 05:01:08PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> [snip]
> 
> >> Are you going to update all the userspace clients? Exposing HDMI 2.0
> >> modes only for your favorite client doesn't sound like a good plan to
> >> me.
> >>
> >> If we simply compute from a specific modeline whether it needs to be
> >> transmitted as 4:2:0, I suppose we could simply look for a matching
> >> mode in the 4:2:0 mode. But that would mean that only the exact modes
> >> listed by the EDID will work, and others might not.
> > OK, so the 4:2:0 support is apparently listed only for specific VICs.
> 
> Hmm, the spec is not very clear. It lists video timings which may
> be used with YCbCr 4:2:0 but does not explicitly say that only
> these timings can be used. Anyway, I checked with a colleague who
> has direct access to HDMI Forum and indeed only VIC 96, 97, 101,
> 102, 106 and 107 can be used with 4:2:0, so you are correct. He
> said that the initial intention of this pixel encoding was to
> give 60Hz refresh rate in 4k to users who had limited
> performance, so it was only intended for higher resolutions.
> 
> > Hence we will need to just go through those lists to see if we can
> > (or in fact must) use 4:2:0 for a specific user specified mode.
> 
> We don't have yet support for these VICs, so this will have to
> wait :(
> 
> >
> > I would say the only slight question mark at this point is whether we
> > should favor 4:4:4 at lower bpc or 4:2:0 at higher bpc if we can choose
> > between the two. My first instinct is to favor the 4:4:4 modes for now.
> > We could add some knobs later to let the user make that choice.
> 
> I agree that 4:4:4 should be preferred.
> 
> [snip]
> 
> >>> Ok. But note that there is no nice way to figure this out. For
> >>> example, for a graphics card it all depends (apart from the
> >>> graphics HW) on the PCIe bus. If the bus is not free for enough
> >>> data rate then user can reach bottlenecks and not output at best
> >>> performance. If we gave user the ability to switch from, for
> >>> example, RGB to YCbCr 4:2:0 this bottleneck could be eliminated.
> >> Userspace won't know anything about such bottlenecks. The kernel
> >> can know it and hence should automagically drop into 4:2:0 mode
> >> if necessary.
> >>
> >>> Unless of course we always prefer YCbCr 4:2:0, when possible. I
> >>> did this internally for bridge driver dw-hdmi. We always prefer
> >>> YCbCr over RGB when they are available. It is user transparent as
> >>> the controller does the necessary color space conversion, though,
> >>> not ideal in my opinion.
> >> My idea was that we'd have a property for the output colorspace and
> >> would perhaps default to YCbCr for the CEA modes (as per CEA-861).
> >> Though I'm sure some people would cry about that behaviour as well.
> >> But for the cases where there is no choice but to use a specific
> >> output colorspace, the kernel should just do it automagically IMO. No
> >> point in manking life diffcult for userspace.
> 
> But we already have color_formats field in drm_display_info
> struct, right? Shouldn't we instead create for example a helper
> which returns the best output colorspace? According to what you
> said it would be something like:
> 
> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>     return YCBCR444;
> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>     return YCBCR422;
> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> && vic_is_420)
>     return YCBCR420;
> else
>     return RGB444; /* Mandatory by spec */

Perhaps. But it would have to be more involved than that since there
might limitations on eg. the max TMDS clock imposed by the source or
cable/dongle which presumably might require that we pick 4:2:0 over
4:4:4.

It would also need to account which formats are actually supported by
the source.

I guess for bpc it would be enough to just consider 8bpc in such a
function, and then the driver can bump it up afterwards if possible.

As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
4:4:4 and thus doesn't provide any benefit as such. We could later add
a property to let the user choose between RGB vs. YCbCr more explicitly.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1556380

Hi Ville,


On 10-01-2017 17:21, Ville Syrjälä wrote:

[snip]

>> But we already have color_formats field in drm_display_info
>> struct, right? Shouldn't we instead create for example a helper
>> which returns the best output colorspace? According to what you
>> said it would be something like:
>>
>> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>     return YCBCR444;
>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>>     return YCBCR422;
>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
>> && vic_is_420)
>>     return YCBCR420;
>> else
>>     return RGB444; /* Mandatory by spec */
> Perhaps. But it would have to be more involved than that since there
> might limitations on eg. the max TMDS clock imposed by the source or
> cable/dongle which presumably might require that we pick 4:2:0 over
> 4:4:4.
>
> It would also need to account which formats are actually supported by
> the source.
>
> I guess for bpc it would be enough to just consider 8bpc in such a
> function, and then the driver can bump it up afterwards if possible.

But the max tmds clock will probably be involved in deep color
modes, as 24bpp has always a 1x factor in every YCbCr, except
4:2:0. So, the sink has a max tmds but this gets into account
when the vic list present in the EDID is built, but not
considered in deep color modes, unless the EDID is broken.

>
> As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> 4:4:4 and thus doesn't provide any benefit as such. We could later add
> a property to let the user choose between RGB vs. YCbCr more explicitly.
>

Hmm, I am trying to implement this but I am facing a difficulty:
how will I fallback to YCbCr? RGB is always supported and the max
tmds only enters in deep color modes. For reference here is a
simple struct i created with the different tmds character rate
factors for the different encodings (I think they are correct,
but cross check please):

#define DRM_CS_DESC(cs, f, b) \
    .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)

static const struct drm_mode_colorspace_desc {
    u32 colorspace;
    u32 factor_to_khz;
    u32 bpc;
} drm_mode_colorspace_factors = { /* Ordered by descending
preference */
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
    { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },

Notice how YCbCr 4:4:4 will never get picked: it has the same
factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
4:4:4 and may support YCbCr. What I didn't check was that if it
is possible to have support for deep color YCbCr without having
support for deep color RGB 4:4:4. Do you know if this can happen?

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1556411

On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 10-01-2017 17:21, Ville Syrjälä wrote:
> 
> [snip]
> 
> >> But we already have color_formats field in drm_display_info
> >> struct, right? Shouldn't we instead create for example a helper
> >> which returns the best output colorspace? According to what you
> >> said it would be something like:
> >>
> >> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> >>     return YCBCR444;
> >> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> >>     return YCBCR422;
> >> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> >> && vic_is_420)
> >>     return YCBCR420;
> >> else
> >>     return RGB444; /* Mandatory by spec */
> > Perhaps. But it would have to be more involved than that since there
> > might limitations on eg. the max TMDS clock imposed by the source or
> > cable/dongle which presumably might require that we pick 4:2:0 over
> > 4:4:4.
> >
> > It would also need to account which formats are actually supported by
> > the source.
> >
> > I guess for bpc it would be enough to just consider 8bpc in such a
> > function, and then the driver can bump it up afterwards if possible.
> 
> But the max tmds clock will probably be involved in deep color
> modes, as 24bpp has always a 1x factor in every YCbCr, except
> 4:2:0. So, the sink has a max tmds but this gets into account
> when the vic list present in the EDID is built, but not
> considered in deep color modes, unless the EDID is broken.
> 
> >
> > As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> > for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> > leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> > 4:4:4 and thus doesn't provide any benefit as such. We could later add
> > a property to let the user choose between RGB vs. YCbCr more explicitly.
> >
> 
> Hmm, I am trying to implement this but I am facing a difficulty:
> how will I fallback to YCbCr? RGB is always supported and the max
> tmds only enters in deep color modes. For reference here is a
> simple struct i created with the different tmds character rate
> factors for the different encodings (I think they are correct,
> but cross check please):
> 
> #define DRM_CS_DESC(cs, f, b) \
>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
> 
> static const struct drm_mode_colorspace_desc {
>     u32 colorspace;
>     u32 factor_to_khz;
>     u32 bpc;
> } drm_mode_colorspace_factors = { /* Ordered by descending
> preference */
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
> 
> Notice how YCbCr 4:4:4 will never get picked: it has the same
> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
> 4:4:4 and may support YCbCr. What I didn't check was that if it
> is possible to have support for deep color YCbCr without having
> support for deep color RGB 4:4:4. Do you know if this can happen?

I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
probably something we have to leave up to the user. Although I have
a vague recollection that CEA-861 says that you should prefer YCbCr
for CE modes and RGB for IT modes. If we want to follow that I think we
want a property similar to the "Broadcast RGB" thing that allows you to
select between "Automatic", "RGB", and "YCbCr". Not sure if we should
also allow the user to explicitly select the subsampling mode for YCbCr.
I also think we should probably still fall back to YCbCr 4:2:0
automagically even if the user explicitly asked for RGB if we can't
light up the mode with RGB 4:4:4.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1559717

Hi Ville,


Sorry for the late reply.


On 11-01-2017 11:36, Ville Syrjälä wrote:
> On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
>> Hi Ville,
>>
>>
>> On 10-01-2017 17:21, Ville Syrjälä wrote:
>>
>> [snip]
>>
>>>> But we already have color_formats field in drm_display_info
>>>> struct, right? Shouldn't we instead create for example a helper
>>>> which returns the best output colorspace? According to what you
>>>> said it would be something like:
>>>>
>>>> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>>>     return YCBCR444;
>>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>>>>     return YCBCR422;
>>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
>>>> && vic_is_420)
>>>>     return YCBCR420;
>>>> else
>>>>     return RGB444; /* Mandatory by spec */
>>> Perhaps. But it would have to be more involved than that since there
>>> might limitations on eg. the max TMDS clock imposed by the source or
>>> cable/dongle which presumably might require that we pick 4:2:0 over
>>> 4:4:4.
>>>
>>> It would also need to account which formats are actually supported by
>>> the source.
>>>
>>> I guess for bpc it would be enough to just consider 8bpc in such a
>>> function, and then the driver can bump it up afterwards if possible.
>> But the max tmds clock will probably be involved in deep color
>> modes, as 24bpp has always a 1x factor in every YCbCr, except
>> 4:2:0. So, the sink has a max tmds but this gets into account
>> when the vic list present in the EDID is built, but not
>> considered in deep color modes, unless the EDID is broken.
>>
>>> As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
>>> for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
>>> leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
>>> 4:4:4 and thus doesn't provide any benefit as such. We could later add
>>> a property to let the user choose between RGB vs. YCbCr more explicitly.
>>>
>> Hmm, I am trying to implement this but I am facing a difficulty:
>> how will I fallback to YCbCr? RGB is always supported and the max
>> tmds only enters in deep color modes. For reference here is a
>> simple struct i created with the different tmds character rate
>> factors for the different encodings (I think they are correct,
>> but cross check please):
>>
>> #define DRM_CS_DESC(cs, f, b) \
>>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
>>
>> static const struct drm_mode_colorspace_desc {
>>     u32 colorspace;
>>     u32 factor_to_khz;
>>     u32 bpc;
>> } drm_mode_colorspace_factors = { /* Ordered by descending
>> preference */
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
>>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
>>
>> Notice how YCbCr 4:4:4 will never get picked: it has the same
>> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
>> 4:4:4 and may support YCbCr. What I didn't check was that if it
>> is possible to have support for deep color YCbCr without having
>> support for deep color RGB 4:4:4. Do you know if this can happen?
> I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
> probably something we have to leave up to the user. Although I have
> a vague recollection that CEA-861 says that you should prefer YCbCr
> for CE modes and RGB for IT modes. 

RGB Full Range is the default for IT modes. As for CE modes it
says it depends on vactive, which I am not quite understanding
why (pg. 34, CEA-861-F).

> If we want to follow that I think we
> want a property similar to the "Broadcast RGB" thing that allows you to
> select between "Automatic", "RGB", and "YCbCr". Not sure if we should
> also allow the user to explicitly select the subsampling mode for YCbCr.

I think we can start by only RGB vs. YCbCr vs. automatic.

> I also think we should probably still fall back to YCbCr 4:2:0
> automagically even if the user explicitly asked for RGB if we can't
> light up the mode with RGB 4:4:4.
>

Agreed. I will start by that but in order for this to work in a
real scenario (I got it working by changing EDID manually) the
list of VIC's must be expanded to the new HDMI 2.0 VIC's. A patch
was sent here a while ago (not by me) and I think you commented
on that. I don't know whats the status of the patch now but it
wasn't merged.

Anyway, regarding this I think we could:
    - Reuse this existing RFC where it concerns EDID parsing
    - Add new flags (which will not be exported to userspace) to
signal YCbCr 4:2:0'only and 'able modes
    - Add a helper function to compute best colorspace which will
always return RGB (for now) unless the mode is 4:2:0 only or
unless the mode can't use RGB because of max clock limitations.

What do you think?

Best regards,
Jose Miguel Abreu

[toc] | [prev] | [next] | [standalone]

#1559754

On Mon, Jan 16, 2017 at 01:24:01PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> Sorry for the late reply.
> 
> 
> On 11-01-2017 11:36, Ville Syrjälä wrote:
> > On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
> >> Hi Ville,
> >>
> >>
> >> On 10-01-2017 17:21, Ville Syrjälä wrote:
> >>
> >> [snip]
> >>
> >>>> But we already have color_formats field in drm_display_info
> >>>> struct, right? Shouldn't we instead create for example a helper
> >>>> which returns the best output colorspace? According to what you
> >>>> said it would be something like:
> >>>>
> >>>> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> >>>>     return YCBCR444;
> >>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> >>>>     return YCBCR422;
> >>>> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> >>>> && vic_is_420)
> >>>>     return YCBCR420;
> >>>> else
> >>>>     return RGB444; /* Mandatory by spec */
> >>> Perhaps. But it would have to be more involved than that since there
> >>> might limitations on eg. the max TMDS clock imposed by the source or
> >>> cable/dongle which presumably might require that we pick 4:2:0 over
> >>> 4:4:4.
> >>>
> >>> It would also need to account which formats are actually supported by
> >>> the source.
> >>>
> >>> I guess for bpc it would be enough to just consider 8bpc in such a
> >>> function, and then the driver can bump it up afterwards if possible.
> >> But the max tmds clock will probably be involved in deep color
> >> modes, as 24bpp has always a 1x factor in every YCbCr, except
> >> 4:2:0. So, the sink has a max tmds but this gets into account
> >> when the vic list present in the EDID is built, but not
> >> considered in deep color modes, unless the EDID is broken.
> >>
> >>> As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> >>> for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> >>> leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> >>> 4:4:4 and thus doesn't provide any benefit as such. We could later add
> >>> a property to let the user choose between RGB vs. YCbCr more explicitly.
> >>>
> >> Hmm, I am trying to implement this but I am facing a difficulty:
> >> how will I fallback to YCbCr? RGB is always supported and the max
> >> tmds only enters in deep color modes. For reference here is a
> >> simple struct i created with the different tmds character rate
> >> factors for the different encodings (I think they are correct,
> >> but cross check please):
> >>
> >> #define DRM_CS_DESC(cs, f, b) \
> >>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
> >>
> >> static const struct drm_mode_colorspace_desc {
> >>     u32 colorspace;
> >>     u32 factor_to_khz;
> >>     u32 bpc;
> >> } drm_mode_colorspace_factors = { /* Ordered by descending
> >> preference */
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
> >>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
> >>
> >> Notice how YCbCr 4:4:4 will never get picked: it has the same
> >> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
> >> 4:4:4 and may support YCbCr. What I didn't check was that if it
> >> is possible to have support for deep color YCbCr without having
> >> support for deep color RGB 4:4:4. Do you know if this can happen?
> > I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
> > probably something we have to leave up to the user. Although I have
> > a vague recollection that CEA-861 says that you should prefer YCbCr
> > for CE modes and RGB for IT modes. 
> 
> RGB Full Range is the default for IT modes. As for CE modes it
> says it depends on vactive, which I am not quite understanding
> why (pg. 34, CEA-861-F).

I think that vactive note is just referring to the SD->BT.601 and
HD->BT.709 rule.

> 
> > If we want to follow that I think we
> > want a property similar to the "Broadcast RGB" thing that allows you to
> > select between "Automatic", "RGB", and "YCbCr". Not sure if we should
> > also allow the user to explicitly select the subsampling mode for YCbCr.
> 
> I think we can start by only RGB vs. YCbCr vs. automatic.
> 
> > I also think we should probably still fall back to YCbCr 4:2:0
> > automagically even if the user explicitly asked for RGB if we can't
> > light up the mode with RGB 4:4:4.
> >
> 
> Agreed. I will start by that but in order for this to work in a
> real scenario (I got it working by changing EDID manually) the
> list of VIC's must be expanded to the new HDMI 2.0 VIC's. A patch
> was sent here a while ago (not by me) and I think you commented
> on that. I don't know whats the status of the patch now but it
> wasn't merged.

The new VICs were reverted due to the "exposing aspect ratio as mode
flags broke userspace" thing. What we need to do is add the new VICs
without changing the userspace API. And I don't see any reason why
we can't do just that. But no such patch has materialized AFAIK.

> 
> Anyway, regarding this I think we could:
>     - Reuse this existing RFC where it concerns EDID parsing
>     - Add new flags (which will not be exported to userspace) to
> signal YCbCr 4:2:0'only and 'able modes
>     - Add a helper function to compute best colorspace which will
> always return RGB (for now) unless the mode is 4:2:0 only or
> unless the mode can't use RGB because of max clock limitations.
> 
> What do you think?

Sounds reasonable to me.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web