Re: [Intel-gfx] [PATCH 04/11] drm: parse ycbcr420 vcb block
Regards
Shashank
On 5/8/2017 10:28 PM, Ville Syrjälä wrote:
On Fri, Apr 07, 2017 at 07:39:21PM +0300, Shashank Sharma wrote:
HDMI 2.0 spec adds support for ycbcr420 subsampled output.
CEA-861-F adds two new blocks in EDID, to provide information about
sink's support for ycbcr420 output.
One of these new blocks is: ycbcr420 vcb
- ycbcr420 video capability data (vcb) block: video modes which can be
support in ycbcr420 output mode also (along with RGB, YCBCR 444/422 etc)
This patch adds parsing and handling of ycbcr420-vcb in the DRM layer.
This block contains a bitmap about which mode, from among the list of
normal svd videomodes, can support ycbcr420 output too.
So if bit 0 from first vcb byte is set, means first video mode in the
svd list, can be supported in ycbcr420 output too. Bit 2 means second
video mode from svd list, and so on.
Cc: Ville Syrjala
Cc: Jose Abreu
Signed-off-by: Shashank Sharma
---
drivers/gpu/drm/drm_edid.c | 80 +++--
include/drm/drm_connector.h | 1 +
2 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 64d8e2e..d01b7df 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2778,6 +2778,7 @@ add_detailed_modes(struct drm_connector *connector,
struct edid *edid,
#define SPEAKER_BLOCK 0x04
#define VIDEO_CAPABILITY_BLOCK0x07
#define VIDEO_DATA_BLOCK_420 0x0E
+#define VIDEO_CAP_BLOCK_4200x0F
#define EDID_BASIC_AUDIO (1 << 6)
#define EDID_CEA_YCRCB444 (1 << 5)
#define EDID_CEA_YCRCB422 (1 << 4)
@@ -3143,11 +3144,21 @@ static int
do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
{
int i, modes = 0;
+ u64 hdmi_420_cap_map = connector->display_info.hdmi.ycbcr420_vcb_map;
for (i = 0; i < len; i++) {
struct drm_display_mode *mode;
mode = drm_display_mode_from_vic_index(connector, db, len, i);
if (mode) {
+ /*
+* ycbcr420 capability block contains a bitmap which
+* gives the index of such CEA modes in VDB, which can
+* support ycbcr420 sampling output also.
+* For example, if the bit 0 in bitmap is set,
+* first mode in VDB can support ycbcr420 output too.
+*/
+ if (hdmi_420_cap_map & (1 << i))
We'd need to make sure 'len' is <=64. Not sure if there's any point in
making it possible to have more than 64 VDBs. If there is, then the standard
bitops.h stuff could be used to make the map longer quite trivially.
I know, among the monitors I have tested with (ACER and SAMSUNG) I have
found only 1 or 2 modes in the capability map.
But as there is no limit of a 420_VBD block length from the spec, so
thought it would be good to assume some safe limit.
Whatever limit we choose I think we should print some kind of warning
to indicate that we've exceeded whatever arbitrary limit we chose. And
I definitely think it should be a warning level at least to make sure
we get a bug report about it.
Sure, I can add a warning, for the cases where we cross 64 modes.
+ mode->flags |= DRM_MODE_FLAG_420;
drm_mode_probed_add(connector, mode);
modes++;
}
@@ -3526,9 +3537,64 @@ static bool cea_db_is_hdmi_vdb420(const u8 *db)
return true;
}
+static bool cea_db_is_hdmi_vcb420(const u8 *db)
To keep with the spec terminology this should probably be called
cea_db_is_y420cmdb(). Same for the define and other functions dealing
witrh this block.
Got it.
+{
+ if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+ return false;
+
We need to make sure the payload is long enough to actuall contain the
extended tag.
Dint I check that in the caller ? anyways, I will make sure there is a
check.
+ 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)
+static void drm_parse_vcb_420_bitmap(struct drm_connector *connector,
+const u8 *db)
+{
+ struct drm_display_info *info = &connector->display_info;
+ struct drm_hdmi_info *hdmi = &info->hdmi;
+ u8 map_len = cea_db_payload_len(db) - 1;
+ u8 count;
+ u64 map = 0;
+
+ if (!db)
+ return;
Is that possible somehow?
Bad, corrupt EDID extension from a bad monitor ;-) ?
+
+ if (map_len == 0) {
+ /* All CEA modes support ycbcr420 sampling also.*/
+ hdmi->ycbcr420_vcb_map = U64_MAX;
+ return;
+ }
+
+ /*
+*
Re: [Intel-gfx] [PATCH 04/11] drm: parse ycbcr420 vcb block
On Fri, Apr 07, 2017 at 07:39:21PM +0300, Shashank Sharma wrote:
> HDMI 2.0 spec adds support for ycbcr420 subsampled output.
> CEA-861-F adds two new blocks in EDID, to provide information about
> sink's support for ycbcr420 output.
>
> One of these new blocks is: ycbcr420 vcb
> - ycbcr420 video capability data (vcb) block: video modes which can be
> support in ycbcr420 output mode also (along with RGB, YCBCR 444/422 etc)
>
> This patch adds parsing and handling of ycbcr420-vcb in the DRM layer.
> This block contains a bitmap about which mode, from among the list of
> normal svd videomodes, can support ycbcr420 output too.
>
> So if bit 0 from first vcb byte is set, means first video mode in the
> svd list, can be supported in ycbcr420 output too. Bit 2 means second
> video mode from svd list, and so on.
>
> Cc: Ville Syrjala
> Cc: Jose Abreu
> Signed-off-by: Shashank Sharma
> ---
> drivers/gpu/drm/drm_edid.c | 80
> +++--
> include/drm/drm_connector.h | 1 +
> 2 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 64d8e2e..d01b7df 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2778,6 +2778,7 @@ add_detailed_modes(struct drm_connector *connector,
> struct edid *edid,
> #define SPEAKER_BLOCK0x04
> #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)
> @@ -3143,11 +3144,21 @@ static int
> do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
> {
> int i, modes = 0;
> + u64 hdmi_420_cap_map = connector->display_info.hdmi.ycbcr420_vcb_map;
>
> for (i = 0; i < len; i++) {
> struct drm_display_mode *mode;
> mode = drm_display_mode_from_vic_index(connector, db, len, i);
> if (mode) {
> + /*
> + * ycbcr420 capability block contains a bitmap which
> + * gives the index of such CEA modes in VDB, which can
> + * support ycbcr420 sampling output also.
> + * For example, if the bit 0 in bitmap is set,
> + * first mode in VDB can support ycbcr420 output too.
> + */
> + if (hdmi_420_cap_map & (1 << i))
We'd need to make sure 'len' is <=64. Not sure if there's any point in
making it possible to have more than 64 VDBs. If there is, then the standard
bitops.h stuff could be used to make the map longer quite trivially.
Whatever limit we choose I think we should print some kind of warning
to indicate that we've exceeded whatever arbitrary limit we chose. And
I definitely think it should be a warning level at least to make sure
we get a bug report about it.
> + mode->flags |= DRM_MODE_FLAG_420;
> drm_mode_probed_add(connector, mode);
> modes++;
> }
> @@ -3526,9 +3537,64 @@ static bool cea_db_is_hdmi_vdb420(const u8 *db)
> return true;
> }
>
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)
To keep with the spec terminology this should probably be called
cea_db_is_y420cmdb(). Same for the define and other functions dealing
witrh this block.
> +{
> + if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> + return false;
> +
We need to make sure the payload is long enough to actuall contain the
extended tag.
> + 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)
>
> +static void drm_parse_vcb_420_bitmap(struct drm_connector *connector,
> + const u8 *db)
> +{
> + struct drm_display_info *info = &connector->display_info;
> + struct drm_hdmi_info *hdmi = &info->hdmi;
> + u8 map_len = cea_db_payload_len(db) - 1;
> + u8 count;
> + u64 map = 0;
> +
> + if (!db)
> + return;
Is that possible somehow?
> +
> + if (map_len == 0) {
> + /* All CEA modes support ycbcr420 sampling also.*/
> + hdmi->ycbcr420_vcb_map = U64_MAX;
> + return;
> + }
> +
> + /*
> + * This map indicates which of the existing CEA block modes
> + * from VDB can support YCBCR420 output too. So if bit=0 is
> + * set, first mode from VDB can support YCBCR420 output too.
> + * We will parse and keep this map, before parsing VDB itself
> + * to avoid going through the same block again and again.
> + *
> + * Spec is not clear about max possible size of this block.
>
Re: [Intel-gfx] [PATCH 04/11] drm: parse ycbcr420 vcb block
Regards
Shashank
On 4/8/2017 11:13 PM, Emil Velikov wrote:
Hi Shashank,
On 7 April 2017 at 17:39, Shashank Sharma wrote:
+ u64 hdmi_420_cap_map = connector->display_info.hdmi.ycbcr420_vcb_map;
for (i = 0; i < len; i++) {
struct drm_display_mode *mode;
mode = drm_display_mode_from_vic_index(connector, db, len, i);
if (mode) {
+ /*
+* ycbcr420 capability block contains a bitmap which
+* gives the index of such CEA modes in VDB, which can
+* support ycbcr420 sampling output also.
+* For example, if the bit 0 in bitmap is set,
+* first mode in VDB can support ycbcr420 output too.
+*/
+ if (hdmi_420_cap_map & (1 << i))
Since map is u64 you really want to use something like (1ull << i)
here. Otherwise you'll get a 32 bit value, regardless of i.
Thanks Emil, this was a good catch.
+ for (count = 0; count < map_len; count++)
+ map = (db[2 + count] << 8 * count) | map;
+
The above issue is applicable here as well. With a small nitpick the
whole thing will read
map |= (u64)db[2 + count] << (8 * count);
Agree, thanks.
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -136,6 +136,7 @@ struct drm_scdc {
struct drm_hdmi_info {
/** @scdc: sink's scdc support and capabilities */
struct drm_scdc scdc;
+ u64 ycbcr420_vcb_map;
As pointed by the kbuild robot - you really want to document the field.
Agree, will handle this in next patch set.
Regards,
Emil
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/11] drm: parse ycbcr420 vcb block
Hi Shashank,
On 7 April 2017 at 17:39, Shashank Sharma wrote:
> + u64 hdmi_420_cap_map = connector->display_info.hdmi.ycbcr420_vcb_map;
>
> for (i = 0; i < len; i++) {
> struct drm_display_mode *mode;
> mode = drm_display_mode_from_vic_index(connector, db, len, i);
> if (mode) {
> + /*
> +* ycbcr420 capability block contains a bitmap which
> +* gives the index of such CEA modes in VDB, which can
> +* support ycbcr420 sampling output also.
> +* For example, if the bit 0 in bitmap is set,
> +* first mode in VDB can support ycbcr420 output too.
> +*/
> + if (hdmi_420_cap_map & (1 << i))
Since map is u64 you really want to use something like (1ull << i)
here. Otherwise you'll get a 32 bit value, regardless of i.
> + for (count = 0; count < map_len; count++)
> + map = (db[2 + count] << 8 * count) | map;
> +
The above issue is applicable here as well. With a small nitpick the
whole thing will read
map |= (u64)db[2 + count] << (8 * count);
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -136,6 +136,7 @@ struct drm_scdc {
> struct drm_hdmi_info {
> /** @scdc: sink's scdc support and capabilities */
> struct drm_scdc scdc;
> + u64 ycbcr420_vcb_map;
As pointed by the kbuild robot - you really want to document the field.
Regards,
Emil
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/11] drm: parse ycbcr420 vcb block
Hi Shashank, [auto build test WARNING on next-20170407] [cannot apply to drm/drm-next drm-intel/for-linux-next tegra/for-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Shashank-Sharma/HDMI-YCBCR-output-handling-in-DRM-layer/20170408-190651 reproduce: make htmldocs All warnings (new ones prefixed by >>): WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) arch/x86/include/asm/uaccess_32.h:1: warning: no structured comments found include/linux/init.h:1: warning: no structured comments found kernel/sched/core.c:2088: warning: No description found for parameter 'rf' kernel/sched/core.c:2088: warning: Excess function parameter 'cookie' description in 'try_to_wake_up_local' include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create' kernel/sys.c:1: warning: no structured comments found include/linux/device.h:969: warning: No description found for parameter 'dma_ops' drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found include/linux/iio/iio.h:597: warning: No description found for parameter 'trig_readonly' include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev' include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig' include/linux/device.h:970: warning: No description found for parameter 'dma_ops' include/drm/drm_drv.h:524: warning: No description found for parameter 'set_busid' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_handler' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_preinstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_postinstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_uninstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'debugfs_init' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_open_object' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_close_object' include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_handle_to_fd' include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_fd_to_handle' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_export' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_pin' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_unpin' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_res_obj' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_get_sg_table' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import_sg_table' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vunmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_mmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_vm_ops' include/drm/drm_drv.h:524: warning: No description found for parameter 'major' include/drm/drm_drv.h:524: warning: No description found for parameter 'minor' include/drm/drm_drv.h:524: warning: No description found for parameter 'patchlevel' include/drm/drm_drv.h:524: warning: No description found for parameter 'name' include/drm/drm_drv.h:524: warning: No description found for parameter 'desc' include/drm/drm_drv.h:524: warning: No description found for parameter 'date' include/drm/drm_drv.h:524: warning: No description found for parameter 'driver_features' include/drm/drm_drv.h:524: warning: No description found for parameter 'ioctls' include/drm/drm_drv.h:524: warning: No description found for parameter 'num_ioctls' include/drm/drm_drv.h:524: warning: No description found for parameter 'fops' >> include/drm/drm_connector.h:141: warning: No description found for parameter >> 'ycbcr420_vcb_map' include/drm/drm_color_mgmt.h:1: warning: no structured comments found drivers/gpu/drm/drm_plane_helper.c:403: warning: No description found for parameter 'ctx' drivers/gpu/drm/drm_plane_helper.c:404: warning: No description found for parameter 'ctx' drivers/gpu/drm/i915/intel_lpe_audio.c:343: warning: No description found for parameter 'dp_output' drivers/gpu/drm/i915/intel_lpe_audio.c:343: warning: No description found for parameter 'link_rate' drivers/gpu/drm/i915/intel_lpe_audio.c:344
