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@xxxxxxxxxxxx>
> >> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> >> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> >> Cc: David Airlie <airlied@xxxxxxxx>
> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
