| Message ID | 1491583168-20042-5-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: warning: No description found for parameter 'dp_output'
drivers/gpu/drm/i915/intel_lpe_audio.c:344: warning: No description found for parameter 'link_rate'
Documentation/core-api/assoc_array.rst:13: WARNING: Enumerated list ends without a blank line; unexpected unindent.
Documentation/doc-guide/sphinx.rst:126: ERROR: Unknown target name: "sphinx c domain".
kernel/sched/fair.c:7701: WARNING: Inline emphasis start-string without end-string.
kernel/time/timer.c:1200: ERROR: Unexpected indentation.
kernel/time/timer.c:1202: ERROR: Unexpected indentation.
kernel/time/timer.c:1203: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/wait.h:122: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/wait.h:125: ERROR: Unexpected indentation.
include/linux/wait.h:127: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/time/hrtimer.c:990: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/signal.c:322: WARNING: Inline literal start-string without end-string.
include/linux/iio/iio.h:219: ERROR: Unexpected indentation.
include/linux/iio/iio.h:220: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/iio/iio.h:226: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/iio/industrialio-core.c:638: ERROR: Unknown target name: "iio_val".
drivers/iio/industrialio-core.c:645: ERROR: Unknown target name: "iio_val".
drivers/message/fusion/mptbase.c:5051: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/tty/serial/serial_core.c:1898: WARNING: Definition list ends without a blank line; unexpected unindent.
include/linux/regulator/driver.h:271: ERROR: Unknown target name: "regulator_regmap_x_voltage".
include/linux/spi/spi.h:370: ERROR: Unexpected indentation.
drivers/usb/core/message.c:478: ERROR: Unexpected indentation.
drivers/usb/core/message.c:479: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/driver-api/usb.rst:623: ERROR: Unknown target name: "usb_type".
Documentation/driver-api/usb.rst:623: ERROR: Unknown target name: "usb_dir".
Documentation/driver-api/usb.rst:623: ERROR: Unknown target name: "usb_recip".
Documentation/driver-api/usb.rst:689: ERROR: Unknown target name: "usbdevfs_urb_type".
drivers/gpu/drm/drm_scdc_helper.c:203: ERROR: Unexpected indentation.
drivers/gpu/drm/drm_scdc_helper.c:204: WARNING: Block quote ends without a blank line; unexpected unindent.
drivers/gpu/drm/drm_ioctl.c:690: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/gpu/todo.rst:111: ERROR: Unknown target name: "drm_fb".
sound/soc/soc-core.c:2665: ERROR: Unknown target name: "snd_soc_daifmt".
sound/core/jack.c:312: ERROR: Unknown target name: "snd_jack_btn".
Documentation/usb/typec.rst:116: ERROR: Error in "kernel-doc" directive:
invalid option block.
vim +/ycbcr420_vcb_map +141 include/drm/drm_connector.h
62c58af32 Shashank Sharma 2017-03-13 125 */
62c58af32 Shashank Sharma 2017-03-13 126 struct drm_scrambling scrambling;
afa1c7636 Shashank Sharma 2017-03-13 127 };
afa1c7636 Shashank Sharma 2017-03-13 128
62c58af32 Shashank Sharma 2017-03-13 129
afa1c7636 Shashank Sharma 2017-03-13 130 /**
afa1c7636 Shashank Sharma 2017-03-13 131 * struct drm_hdmi_info - runtime information about the connected HDMI sink
afa1c7636 Shashank Sharma 2017-03-13 132 *
afa1c7636 Shashank Sharma 2017-03-13 133 * Describes if a given display supports advanced HDMI 2.0 features.
afa1c7636 Shashank Sharma 2017-03-13 134 * This information is available in CEA-861-F extension blocks (like HF-VSDB).
afa1c7636 Shashank Sharma 2017-03-13 135 */
afa1c7636 Shashank Sharma 2017-03-13 136 struct drm_hdmi_info {
86cc921cb Shashank Sharma 2017-03-28 137 /** @scdc: sink's scdc support and capabilities */
afa1c7636 Shashank Sharma 2017-03-13 138 struct drm_scdc scdc;
862bf9f3d Shashank Sharma 2017-04-07 139 u64 ycbcr420_vcb_map;
522171951 Daniel Vetter 2016-08-12 140 };
522171951 Daniel Vetter 2016-08-12 @141
b3c6c8bfe Daniel Vetter 2016-08-12 142 /**
40ee6fbef Manasi Navare 2016-12-16 143 * enum drm_link_status - connector's link_status property value
40ee6fbef Manasi Navare 2016-12-16 144 *
40ee6fbef Manasi Navare 2016-12-16 145 * This enum is used as the connector's link status property value.
40ee6fbef Manasi Navare 2016-12-16 146 * It is set to the values defined in uapi.
41232c1b7 Manasi Navare 2017-03-01 147 *
41232c1b7 Manasi Navare 2017-03-01 148 * @DRM_LINK_STATUS_GOOD: DP Link is Good as a result of successful
41232c1b7 Manasi Navare 2017-03-01 149 * link training
:::::: The code at line 141 was first introduced by commit
:::::: 522171951761153172c75b94ae1f4bc9ab631745 drm: Extract drm_connector.[hc]
:::::: TO: Daniel Vetter <daniel.vetter@ffwll.ch>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Shashank, On 7 April 2017 at 17:39, Shashank Sharma <shashank.sharma@intel.com> 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
Regards Shashank On 4/8/2017 11:13 PM, Emil Velikov wrote: > Hi Shashank, > > On 7 April 2017 at 17:39, Shashank Sharma <shashank.sharma@intel.com> 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
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 <ville.syrjala@linux.intel.com> > Cc: Jose Abreu <joabreu@synopsys.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > 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_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. > + * Clamping max bitmap block size at 8 bytes. Every byte can > + * address 8 CEA modes, in this way this map can address > + * 8*8 = first 64 SVDs. > + */ > + if (map_len > 8) > + map_len = 8; > + > + for (count = 0; count < map_len; count++) > + map = (db[2 + count] << 8 * count) | map; > + > + if (map) { > + DRM_DEBUG_KMS("Sink supports ycbcr 420\n"); > + info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; > + } > + > + hdmi->ycbcr420_vcb_map = map; > +} > + > static int > add_cea_modes(struct drm_connector *connector, struct edid *edid) > { > @@ -3561,6 +3627,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) > /* Add 4:2:0(only) modes present in EDID */ > modes += do_420_vdb_modes(connector, vdb420, > vdb420_len); > + } else if (cea_db_is_hdmi_vcb420(db)) { > + drm_parse_vcb_420_bitmap(connector, db); > } > } > } > @@ -4241,6 +4309,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, > drm_parse_hdmi_vsdb_video(connector, db); > if (cea_db_is_hdmi_forum_vsdb(db)) > drm_parse_hdmi_forum_vsdb(connector, db); > + if (cea_db_is_hdmi_vcb420(db)) > + drm_parse_vcb_420_bitmap(connector, db); > } > } > > @@ -4492,6 +4562,14 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) > num_modes += add_cvt_modes(connector, edid); > num_modes += add_standard_modes(connector, edid); > num_modes += add_established_modes(connector, edid); > + > + /* > + * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks. > + * To avoid multiple parsing of same block, lets get the sink info > + * before parsing CEA modes. > + */ > + drm_add_display_info(connector, edid); Is there any reason why this can't be moved to be called before any modes are added? Having it in the middle like this feels wrong. > + > num_modes += add_cea_modes(connector, edid); > num_modes += add_alternate_cea_modes(connector, edid); > num_modes += add_displayid_detailed_modes(connector, edid); > @@ -4501,8 +4579,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) > if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75)) > edid_fixup_preferred(connector, quirks); > > - drm_add_display_info(connector, edid); > - > if (quirks & EDID_QUIRK_FORCE_6BPC) > connector->display_info.bpc = 6; > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index cef76b2..dbfa6a1 100644 > --- 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; > }; > > /** > -- > 2.7.4
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 <ville.syrjala@linux.intel.com> >> Cc: Jose Abreu <joabreu@synopsys.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> 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_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. 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; >> + } >> + >> + /* >> + * 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. >> + * Clamping max bitmap block size at 8 bytes. Every byte can >> + * address 8 CEA modes, in this way this map can address >> + * 8*8 = first 64 SVDs. >> + */ >> + if (map_len > 8) >> + map_len = 8; >> + >> + for (count = 0; count < map_len; count++) >> + map = (db[2 + count] << 8 * count) | map; >> + >> + if (map) { >> + DRM_DEBUG_KMS("Sink supports ycbcr 420\n"); >> + info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; >> + } >> + >> + hdmi->ycbcr420_vcb_map = map; >> +} >> + >> static int >> add_cea_modes(struct drm_connector *connector, struct edid *edid) >> { >> @@ -3561,6 +3627,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) >> /* Add 4:2:0(only) modes present in EDID */ >> modes += do_420_vdb_modes(connector, vdb420, >> vdb420_len); >> + } else if (cea_db_is_hdmi_vcb420(db)) { >> + drm_parse_vcb_420_bitmap(connector, db); >> } >> } >> } >> @@ -4241,6 +4309,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, >> drm_parse_hdmi_vsdb_video(connector, db); >> if (cea_db_is_hdmi_forum_vsdb(db)) >> drm_parse_hdmi_forum_vsdb(connector, db); >> + if (cea_db_is_hdmi_vcb420(db)) >> + drm_parse_vcb_420_bitmap(connector, db); >> } >> } >> >> @@ -4492,6 +4562,14 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) >> num_modes += add_cvt_modes(connector, edid); >> num_modes += add_standard_modes(connector, edid); >> num_modes += add_established_modes(connector, edid); >> + >> + /* >> + * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks. >> + * To avoid multiple parsing of same block, lets get the sink info >> + * before parsing CEA modes. >> + */ >> + drm_add_display_info(connector, edid); > Is there any reason why this can't be moved to be called before any > modes are added? Having it in the middle like this feels wrong. In fact I was also thinking about moving it to the start, due to the look and feel of the code, then though it would be better to have minimum changes in the routine. But sure, I would be happy to move it. - Shashank > >> + >> num_modes += add_cea_modes(connector, edid); >> num_modes += add_alternate_cea_modes(connector, edid); >> num_modes += add_displayid_detailed_modes(connector, edid); >> @@ -4501,8 +4579,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) >> if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75)) >> edid_fixup_preferred(connector, quirks); >> >> - drm_add_display_info(connector, edid); >> - >> if (quirks & EDID_QUIRK_FORCE_6BPC) >> connector->display_info.bpc = 6; >> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index cef76b2..dbfa6a1 100644 >> --- 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; >> }; >> >> /** >> -- >> 2.7.4
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_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)) + 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) +{ + 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) +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; + + 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. + * Clamping max bitmap block size at 8 bytes. Every byte can + * address 8 CEA modes, in this way this map can address + * 8*8 = first 64 SVDs. + */ + if (map_len > 8) + map_len = 8; + + for (count = 0; count < map_len; count++) + map = (db[2 + count] << 8 * count) | map; + + if (map) { + DRM_DEBUG_KMS("Sink supports ycbcr 420\n"); + info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; + } + + hdmi->ycbcr420_vcb_map = map; +} + static int add_cea_modes(struct drm_connector *connector, struct edid *edid) { @@ -3561,6 +3627,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid) /* Add 4:2:0(only) modes present in EDID */ modes += do_420_vdb_modes(connector, vdb420, vdb420_len); + } else if (cea_db_is_hdmi_vcb420(db)) { + drm_parse_vcb_420_bitmap(connector, db); } } } @@ -4241,6 +4309,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector, drm_parse_hdmi_vsdb_video(connector, db); if (cea_db_is_hdmi_forum_vsdb(db)) drm_parse_hdmi_forum_vsdb(connector, db); + if (cea_db_is_hdmi_vcb420(db)) + drm_parse_vcb_420_bitmap(connector, db); } } @@ -4492,6 +4562,14 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) num_modes += add_cvt_modes(connector, edid); num_modes += add_standard_modes(connector, edid); num_modes += add_established_modes(connector, edid); + + /* + * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks. + * To avoid multiple parsing of same block, lets get the sink info + * before parsing CEA modes. + */ + drm_add_display_info(connector, edid); + num_modes += add_cea_modes(connector, edid); num_modes += add_alternate_cea_modes(connector, edid); num_modes += add_displayid_detailed_modes(connector, edid); @@ -4501,8 +4579,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75)) edid_fixup_preferred(connector, quirks); - drm_add_display_info(connector, edid); - if (quirks & EDID_QUIRK_FORCE_6BPC) connector->display_info.bpc = 6; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index cef76b2..dbfa6a1 100644 --- 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; }; /**
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 <ville.syrjala@linux.intel.com> Cc: Jose Abreu <joabreu@synopsys.com> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/drm_edid.c | 80 +++++++++++++++++++++++++++++++++++++++++++-- include/drm/drm_connector.h | 1 + 2 files changed, 79 insertions(+), 2 deletions(-)