Regards

Shashank


On 6/27/2017 5:22 PM, Ville Syrjälä wrote:
On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:
HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
CEA-861-F adds two new blocks in EDID's CEA extension blocks,
to provide information about sink's YCBCR420 output capabilities.

These blocks are:

- YCBCR420vdb(YCBCR 420 video data block):
This block contains VICs of video modes, which can be sopported only
in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
SVD block, valid for YCBCR420 modes only.

- YCBCR420cmdb(YCBCR 420 capability map data block):
This block gives information about video modes which can support
YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
block contains a bitmap index of normal svd videomodes, which can
support YCBCR420 output too.
So if bit 0 from first vcb byte is set, first video mode in the svd
list can support YCBCR420 output too. Bit 1 means second video mode
from svd list can support YCBCR420 output too, and so on.

This patch adds two bitmaps in display's hdmi_info structure, one each
for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
adds:
- VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
   an entry in the vdb_bitmap per vic.
- VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.

Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
Cc: Jose Abreu <joab...@synopsys.com>
Cc: Emil Velikov <emil.l.veli...@gmail.com>

V2: Addressed
     Review comments from Emil:
     - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
     - Use the suggested method for updating dbmap.
     - Add documentation for YCBCR420_vcb_map to fix kbuild warning.

     Review comments from Ville:
     - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
     - Save a map of YCBCR420 modes for future reference.
     - Check db length before trying to parse extended tag.
     - Add a warning if there are > 64 modes in capability map block.
     - Use y420cmdb in function names and macros while dealing with vcb
       to be aligned with spec.
     - Move the display information parsing block ahead of mode parsing
       blocks.

V3: Addressed design/review comments from Ville
     - Do not add flags in video modes, else we have to expose them to user
     - There should not be a UABI change, and kernel should detect the
       choice of the output based on type of mode, and the bitmaps.
     - Use standard bitops from kernel bitmap header, instead of calculating
       bit positions manually.

V4: Addressed review comments from Ville:
     - s/ycbcr_420_vdb/y420vdb
     - s/ycbcr_420_vcb/y420cmdb
     - Be less verbose on description of do_y420vdb_modes
     - Move newmode variable in the loop scope.
     - Use svd_to_vic() to get a VIC, instead of 0x7f
     - Remove bitmap description for CMDB modes & VDB modes
     - Dont add connector->ycbcr_420_allowed check for cmdb modes
     - Remove 'len' variable, in is_y420cmdb function, which is used
       only once
     - Add length check in is_y420vdb function
     - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap
     - Do not add print about YCBCR 420 modes
     - Fix indentation in few places
     - Move ycbcr420_dc_modes in next patch, where its used
     - Add a separate patch for movement of drm_add_display_info()

Signed-off-by: Shashank Sharma <shashank.sha...@intel.com>
---
  drivers/gpu/drm/drm_edid.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
  include/drm/drm_connector.h |  20 ++++++
  2 files changed, 174 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e2d1b30..8c7e73b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, 
struct edid *edid,
  #define VIDEO_BLOCK     0x02
  #define VENDOR_BLOCK    0x03
  #define SPEAKER_BLOCK 0x04
-#define VIDEO_CAPABILITY_BLOCK 0x07
+#define VIDEO_CAPABILITY_BLOCK 0x07
+#define VIDEO_DATA_BLOCK_420   0x0E
+#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
  #define EDID_BASIC_AUDIO      (1 << 6)
  #define EDID_CEA_YCRCB444     (1 << 5)
  #define EDID_CEA_YCRCB422     (1 << 4)
@@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector 
*connector,
        return newmode;
  }
+/*
+ * do_y420vdb_modes - Parse YCBCR 420 only modes
+ * @connector: connector corresponding to the HDMI sink
+ * @svds: start of the data block of CEA YCBCR 420 VDB
+ * @len: length of the CEA YCBCR 420 VDB
+ *
+ * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
+ * which contains modes which can be supported in YCBCR 420
+ * output format only.
+ */
+static int do_y420vdb_modes(struct drm_connector *connector,
+                                  const u8 *svds, u8 svds_len)
+{
+       int modes = 0, i;
+       struct drm_device *dev = connector->dev;
+       struct drm_display_info *info = &connector->display_info;
+       struct drm_hdmi_info *hdmi = &info->hdmi;
+
+       for (i = 0; i < svds_len; i++) {
+               u8 vic = svd_to_vic(svds[i]);
+               struct drm_display_mode *newmode;
+
+               newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
+               if (!newmode)
+                       break;
+               bitmap_set(hdmi->y420_vdb_modes, vic, 1);
+               drm_mode_probed_add(connector, newmode);
+               modes++;
+       }
+
+       if (modes > 0)
+               info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+       return modes;
+}
+
+/*
+ * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
+ * @connector: connector corresponding to the HDMI sink
+ * @vic: CEA vic for the video mode to be added in the map
+ *
+ * Makes an entry for a videomode in the YCBCR 420 bitmap
+ */
+static void
+drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
+{
+       u8 vic = svd_to_vic(svd);
+       struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
+
+       bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
+}
+
  static int
  do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
  {
        int i, modes = 0;
+       struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
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 CEA modes from CEA VDB, which
+                        * can support YCBCR 420 sampling output also (apart
+                        * from RGB/YCBCR444 etc).
+                        * For example, if the bit 0 in bitmap is set,
+                        * first mode in VDB can support YCBCR420 output too.
+                        * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
+                        */
+                       if (test_bit(i, &hdmi->y420_cmdb_map))
+                               drm_add_cmdb_modes(connector, db[i]);
+
                        drm_mode_probed_add(connector, mode);
                        modes++;
                }
@@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, 
const u8 *db, u8 len,
  }
static int
+cea_db_extended_tag(const u8 *db)
+{
+       return db[1];
+}
+
+static int
  cea_db_payload_len(const u8 *db)
  {
        return db[0] & 0x1f;
@@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
        return oui == HDMI_FORUM_IEEE_OUI;
  }
+static bool cea_db_is_y420cmdb(const u8 *db)
+{
+
+       if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
Still would like to see that patch to clean up the current extended tag
usage...
Yep, its on the way, running a bit loaded.
+               return false;
+
+       if (!cea_db_payload_len(db))
+               return false;
+
+       if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
+               return false;
+
+       return true;
+}
+
+static bool cea_db_is_y420vdb(const u8 *db)
+{
+       if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+               return false;
+
+       if (!cea_db_payload_len(db))
+               return false;
+
+       if (cea_db_extended_tag(db) != VIDEO_DATA_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_y420cmdb_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 (map_len == 0) {
+               /* All CEA modes support ycbcr420 sampling also.*/
+               hdmi->y420_cmdb_map = U64_MAX;
+               info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+               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 (WARN_ON_ONCE(map_len > 8))
+               map_len = 8;
+
+       for (count = 0; count < map_len; count++)
+               map |= (u64)db[2 + count] << (8 * count);
+
+       if (map)
+               info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+
+       hdmi->y420_cmdb_map = map;
+}
+
  static int
  add_cea_modes(struct drm_connector *connector, struct edid *edid)
  {
@@ -3522,10 +3663,17 @@ add_cea_modes(struct drm_connector *connector, struct 
edid *edid)
                                video = db + 1;
                                video_len = dbl;
                                modes += do_cea_modes(connector, video, dbl);
-                       }
-                       else if (cea_db_is_hdmi_vsdb(db)) {
+                       } else if (cea_db_is_hdmi_vsdb(db)) {
                                hdmi = db;
                                hdmi_len = dbl;
+                       } else if (cea_db_is_y420vdb(db) &&
+                                  connector->ycbcr_420_allowed) {
+                               const u8 *vdb420 = &db[2];
+
+                               /* Add 4:2:0(only) modes present in EDID */
+                               modes += do_y420vdb_modes(connector,
+                                                         vdb420,
+                                                         dbl - 1);
                        }
                }
        }
@@ -4206,6 +4354,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_y420cmdb(db))
+                       drm_parse_y420cmdb_bitmap(connector, db);
        }
  }
@@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
        num_modes += add_cea_modes(connector, edid);
        num_modes += add_alternate_cea_modes(connector, edid);
        num_modes += add_displayid_detailed_modes(connector, edid);
+
        if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
                num_modes += add_inferred_modes(connector, edid);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7493fd3..ce2212d 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -137,6 +137,25 @@ struct drm_scdc {
  struct drm_hdmi_info {
        /** @scdc: sink's scdc support and capabilities */
        struct drm_scdc scdc;
+
+       /**
+        * @y420_vdb_modes: bitmap of modes which can support ycbcr420
+        * output only (not normal RGB/YCBCR444/422 outputs). There are total
+        * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
+        * upto 128 VICs;
+        */
+       unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
+
+       /**
+        * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
+        * output also, along with normal HDMI outputs. There are total 107
+        * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
+        * 128 VICs;
+        */
+       unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
+
+       /** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
+       unsigned long y420_cmdb_map;
u64, otherwise it's busted on 32bit machines.
Got it.

Mostl this looks pretty good. I think the main problem is the lack of
mode validation that would reject the 4:2:0 only modes if not supported
by the driver/device. I guess that's in a later patch?
Yep, its the next patch in the series.
  If so we need to
reorder the series a bit to avoid regeressions.
That patch uses parameters added in this patch, to detect the 4:2:0 only modes. So cant make it before this patch.
Let me see what best can I do on the reordering.

- Shashank
  };
/**
@@ -200,6 +219,7 @@ struct drm_display_info {
  #define DRM_COLOR_FORMAT_RGB444               (1<<0)
  #define DRM_COLOR_FORMAT_YCRCB444     (1<<1)
  #define DRM_COLOR_FORMAT_YCRCB422     (1<<2)
+#define DRM_COLOR_FORMAT_YCRCB420      (1<<3)
/**
         * @color_formats: HDMI Color formats, selects between RGB and YCrCb
--
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to