Regards
Shashank
On 6/22/2017 12:35 PM, Daniel Vetter wrote:
On Wed, Jun 21, 2017 at 04:04:06PM +0530, Shashank Sharma wrote:
This patch adds set of helper functions for YCBCR HDMI output
handling. These functions provide functionality like:
- check if a given video mode is YCBCR 420 only mode.
- check if a given video mode is YCBCR 420 mode.
- get the highest subsamled ycbcr output.
- get the lowest subsamled ycbcr output.
- check if a given source and sink combination can support any
YCBCR HDMI output.
- check if a given source and sink combination can support a particular
YCBCR HDMI output.
Currently all these helpers are kept static, and only one wrapper function
is exposed to callers, which is "drm_find_hdmi_output_type". This function
takes inputs as current mode, user preference and src + sink capabilities,
and provides the most suitable HDMI output format to match all of these
inputs.
This wrapper function will be used in next few patches in this
series from I915 driver.
V2: Added YCBCR functions as helpers in DRM layer, instead of
keeping it in I915 layer.
V3: Added handling for YCBCR-420 only modes too.
V4: EXPORT_SYMBOL(drm_find_hdmi_output_type)
Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
Cc: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx>
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
Not sure why you mark the series as RESEND-CI when not everything is
reviewed yet. Anyway, stumbled over this, some comments below.
The previous patch set caused some failures on BAT, spoke to Jani S and
Tomi, and we decided to send another patch set with fix, but added this
tag, so that reviewers should not get confuse.
But doesn't look like it worked smoothly :-)
---
drivers/gpu/drm/drm_edid.c | 2 +-
drivers/gpu/drm/drm_modes.c | 307 ++++++++++++++++++++++++++++++++++++++++++++
include/drm/drm_edid.h | 1 +
include/drm/drm_modes.h | 5 +
4 files changed, 314 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index edba190..2f8810a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2949,7 +2949,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
}
EXPORT_SYMBOL(drm_match_cea_mode);
-static bool drm_valid_cea_vic(u8 vic)
+bool drm_valid_cea_vic(u8 vic)
{
return vic > 0 && vic < ARRAY_SIZE(edid_cea_modes);
}
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f2493b9..09e6b10 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1576,3 +1576,310 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
out:
return ret;
}
+
+/**
+ * drm_mode_is_420_only - if a given videomode can be only supported in YCBCR420
+ * output format
+ *
+ * @connector: drm connector under action.
+ * @mode: video mode to be tested.
+ *
+ * Returns:
+ * true if the mode can support YCBCR420 output
+ * false if not.
+ */
For static functions we generally don't ever document them using
kernel-doc. And within the drm core generally only the functions exported
to driver (i.e. relevant for driver authors) are fully documented.
Especially for such small helpers, if you need docs the function name
isn't descriptive enough. I'd remove them all (except the one kerneldoc
for the exported function).
Two reasons:
- These are absolutely new functions and reviewers might want to
understand design and usages well.
- I am expecting these functions get used directly from few drivers
soon, and be non-static soon.
In any case, is there anything called 'harm with more doc' .. ? I have
always heard other way around :-)
On function naming itself, the usual pattern is
object_prefix_verb_stuff
In your case here all the helpers check against limits in
drm_display_info, so I'd go with
drm_display_info_is_420_only|also_mode
drm_display_supports_ycbcr_mode
drm_display_supports_ycbcr
(can_support is a bit tedious, supports is shorter)
Those are all bikesheds, but for the main exported function I think it
matters. I'll discuss that below.
Why not, makes sense ...
+static bool drm_mode_is_420_only(struct drm_display_info *display,
+ struct drm_display_mode *mode)
+{
+ u8 vic = drm_match_cea_mode(mode);
+
+ /*
+ * Requirements of a 420_only mode:
+ * must be a valid cea mode
+ * entry in 420_only bitmap
+ */
+ if (!drm_valid_cea_vic(vic))
+ return false;
+
+ return test_bit(vic, display->hdmi.y420_vdb_modes);
+}
+
+/**
+ * drm_mode_is_420_also - if a given videomode can be supported in YCBCR420
+ * output format also (along with RGB/YCBCR444/422)
+ *
+ * @display: display under action.
+ * @mode: video mode to be tested.
+ *
+ * Returns:
+ * true if the mode can support YCBCR420 output
+ * false if not.
+ */
+static bool drm_mode_is_420_also(struct drm_display_info *display,
+ struct drm_display_mode *mode)
+{
+ u8 vic = drm_match_cea_mode(mode);
+
+ /*
+ * Requirements of a 420_also mode:
+ * must be a valid cea mode
+ * entry in 420_also bitmap
+ */
+ if (!drm_valid_cea_vic(vic))
+ return false;
+
+ return test_bit(vic, display->hdmi.y420_cmdb_modes);
+}
+
+
+static bool drm_mode_is_420(struct drm_display_info *display,
+ struct drm_display_mode *mode)
+{
+ return drm_mode_is_420_only(display, mode) ||
+ drm_mode_is_420_also(display, mode);
+}
+
+/**
+ * drm_can_support_this_ycbcr_output - can a given source and sink combination
+ * support a particular YCBCR output type.
+ *
+ * @display: sink information.
+ * @mode: video mode from modeset
+ * @type: enum indicating YCBCR output type
+ * @source_outputs: bitmap of source supported HDMI output formats.
+ *
+ * Returns:
+ * true on success.
+ * false on failure.
+ */
+static bool drm_can_support_this_ycbcr_output(struct drm_display_info *display,
+ struct drm_display_mode *mode,
+ enum drm_hdmi_output_type type,
+ u32 source_outputs)
+{
+ /* YCBCR420 output support can be per mode basis */
+ if (type == DRM_HDMI_OUTPUT_YCBCR420 &&
+ !drm_mode_is_420(display, mode))
+ return false;
+
+ return display->color_formats & source_outputs & type;
+}
+
+/**
+ * drm_can_support_ycbcr_output - can a given source and sink combination
+ * support any YCBCR outputs ?
+ *
+ * @display: sink information.
+ * @source_outputs: bitmap of source supported HDMI output formats.
+ *
+ * Returns:
+ * true on success.
+ * false on failure.
+ */
+static bool drm_can_support_ycbcr_output(struct drm_display_info *display,
+ u32 source_outputs)
+{
+ u32 supported_formats;
+
+ if (!source_outputs || !display->color_formats) {
+ DRM_DEBUG_KMS("Source/Sink doesn't support any output ?\n");
+ return DRM_HDMI_OUTPUT_INVALID;
+ }
+
+ /* Get the common supported fromats between source and sink */
+ supported_formats = display->color_formats & source_outputs;
+ if (!supported_formats || (supported_formats ==
+ DRM_COLOR_FORMAT_RGB444)) {
+ DRM_ERROR("No common YCBCR formats between source and sink\n");
+ return false;
+ }
+
+ DRM_DEBUG_KMS("Src and Sink combination can support YCBCR output\n");
+ return true;
+}
+
+/**
+ * drm_get_highest_quality_ycbcr_supported - get the ycbcr output mode
+ * with highest subsampling rate
+ * @display: struct drm_display_info from current connector
+ * @mode: video mode from modeset
+ * @source_output_map: bitmap of supported HDMI output modes from source
+ *
+ * Finds the best possible ycbcr output mode (based on subsampling), for the
+ * given source and sink combination.
+ *
+ * Returns:
+ * enum corresponding to best output mode on success.
+ * DRM_HDMI_OUTPUT_INVALID on failure.
+ */
+static enum drm_hdmi_output_type
+drm_get_highest_quality_ycbcr_supported(struct drm_display_info *display,
+ struct drm_display_mode *mode,
+ u32 source_output_map)
+{
+ enum drm_hdmi_output_type output = DRM_HDMI_OUTPUT_INVALID;
+ u32 supported_formats = source_output_map & display->color_formats;
+
+ /*
+ * Get the ycbcr output with the highest possible subsampling rate.
+ * Preference should go as:
+ * ycbcr 444
+ * ycbcr 422
+ * ycbcr 420
+ */
+ if (supported_formats & DRM_COLOR_FORMAT_YCRCB444)
+ output = DRM_COLOR_FORMAT_YCRCB444;
+ else if (supported_formats & DRM_COLOR_FORMAT_YCRCB422)
+ output = DRM_COLOR_FORMAT_YCRCB422;
+ else if (supported_formats & DRM_COLOR_FORMAT_YCRCB420 &&
+ drm_mode_is_420(display, mode))
+ output = DRM_COLOR_FORMAT_YCRCB420;
+
+ DRM_DEBUG_KMS("Highest subsampled YCBCR mode supported is %s\n",
+ drm_get_hdmi_output_name(supported_formats));
+ return output;
+}
+
+/**
+ * drm_get_lowest_quality_ycbcr_supported - get the ycbcr output mode
+ * with lowest subsampling rate
+ * @display: struct drm_display_info from current connector
+ * @mode: video mode from modeset
+ * @source_output_map: bitmap of supported HDMI output modes from source
+ *
+ * Finds the lowest possible ycbcr output mode (based on subsampling), for the
+ * given source and sink combination.
+ *
+ * Returns:
+ * enum corresponding to best output mode on success.
+ * DRM_HDMI_OUTPUT_INVALID on failure.
+ */
+static enum drm_hdmi_output_type
+drm_get_lowest_quality_ycbcr_supported(struct drm_display_info *display,
+ struct drm_display_mode *mode,
+ u32 source_output_map)
+{
+ enum drm_hdmi_output_type output = DRM_HDMI_OUTPUT_INVALID;
+ u32 supported_formats = source_output_map & display->color_formats;
+
+ /*
+ * Get the ycbcr output with the lowest possible subsampling rate.
+ * Preference should go as:
+ * ycbcr 420
+ * ycbcr 422
+ * ycbcr 444
+ */
+ if (supported_formats & DRM_COLOR_FORMAT_YCRCB420 &&
+ drm_mode_is_420(display, mode))
+ output = DRM_HDMI_OUTPUT_YCBCR420;
+ else if (display->color_formats & DRM_COLOR_FORMAT_YCRCB422)
+ output = DRM_HDMI_OUTPUT_YCBCR422;
+ else if (display->color_formats & DRM_COLOR_FORMAT_YCRCB444)
+ output = DRM_HDMI_OUTPUT_YCBCR420;
+
+ DRM_DEBUG_KMS("Lowest subsampled YCBCR mode supported is %s\n",
+ drm_get_hdmi_output_name(supported_formats));
+ return output;
+}
+
+/**
+ * drm_find_hdmi_output_type - get the most suitable output
+ * Find the best suitable HDMI output considering source capability,
+ * sink capability and user's choice (expressed in form of drm property)
+ *
+ * @connector: drm connector in action
+ * @mode: video mode under modeset
+ * @type: user's choice for preferred mode, set via drm property
+ * "hdmi_output_format"
+ * @src_output_cap: bitmap of source's supported outputs formats
+ * src_output_cap = (1 << DRM_COLOR_FORMAT_RGB444) means source
+ * supports RGB444
+ * src_output_cap = (1 << DRM_COLOR_FORMAT_YCRCB444) means source
+ * supports YUV444, and so on.
You should explain in 1-2 sentences what exactly this function does, and
when a driver should use it. Just documenting the input/output stuff
doesn't make the kerneldoc all that useful.
Did you miss the first 3 lines above ?
"get the most suitable output.
Find the best suitable HDMI output considering source capability, sink
capability and user's choice (expressed in form of drm property)"
Or you mean that's not enough ?
+ *
+ * Returns:
+ * enum corresponding to best suitable output type on success.
+ * DRM_HDMI_OUTPUT_INVALID on failure.
+ */
+enum drm_hdmi_output_type
+drm_find_hdmi_output_type(struct drm_connector *connector,
+ struct drm_display_mode *mode,
+ enum drm_hdmi_output_type type,
+ u32 src_output_cap)
+{
+ bool ret;
+ struct drm_display_info *info = &connector->display_info;
Imo you should pass the display_info, not connector, to this function. For
consistency.
Sure, can be done.
I'm also not sure we should place this in here, since
drm_display_info is in drm_connector.h (but maybe that's just misplaced
and perhaps we should move it to drm_modes.h). I wouldn't bother with
moving it, but we definitely need a consistent prefix for exported
functiosn. Since drm_display_info_set_bus_formats() already exists, I
think the interface here should be:
enum drm_hdmi_output_type
drm_display_info_hdmi_output_type(struct drm_display_info *display_info,
struct drm_display_mode *mode,
enum drm_hdmi_output_type type,
u32 src_output_cap)
You can also drop the _find_ (sometimes we also call it compute e.g. in i915
modeset code), since computing stuff is implied.
Sure, I like the compute idea too.
- Shashank
Cheers, Daniel
+ bool mode_is_420_only = drm_mode_is_420_only(info, mode);
+
+ /*
+ * If the preferred output is not set to YCBCR by user, and the mode
+ * doesn't force us to drive YCBCR420 output, respect the user
+ * preference for the output type. But if the mode is 420_only, we will
+ * be force to drive YCBCR420 output.
+ */
+ if (!mode_is_420_only) {
+ if (type == DRM_HDMI_OUTPUT_DEFAULT_RGB)
+ return DRM_HDMI_OUTPUT_DEFAULT_RGB;
+ } else {
+ type = DRM_HDMI_OUTPUT_YCBCR420;
+ DRM_DEBUG_KMS("Got a 420 only mode(%s)\n", mode->name);
+ }
+
+ /* If this src + sink combination can support any YCBCR output */
+ ret = drm_can_support_ycbcr_output(info, src_output_cap);
+ if (!ret) {
+ DRM_ERROR("No supported YCBCR output\n");
+ return DRM_HDMI_OUTPUT_INVALID;
+ }
+
+ switch (type) {
+ case DRM_HDMI_OUTPUT_YCBCR_HQ:
+ type = drm_get_highest_quality_ycbcr_supported(info, mode,
+ src_output_cap);
+ break;
+
+ case DRM_HDMI_OUTPUT_YCBCR_LQ:
+ type = drm_get_lowest_quality_ycbcr_supported(info, mode,
+ src_output_cap);
+ break;
+
+ case DRM_HDMI_OUTPUT_YCBCR420:
+ ret = drm_mode_is_420(info, mode);
+ if (!ret) {
+ DRM_ERROR("Mode %s doesn't support 420 output\n",
+ mode->name);
+ type = DRM_HDMI_OUTPUT_INVALID;
+ }
+
+ break;
+
+ /* Below cases are just to satisfy checkpatch's AI */
+ case DRM_HDMI_OUTPUT_DEFAULT_RGB:
+ case DRM_HDMI_OUTPUT_YCBCR444:
+ case DRM_HDMI_OUTPUT_YCBCR422:
+ break;
+
+ default:
+ type = DRM_HDMI_OUTPUT_INVALID;
+ }
+
+ if (type == DRM_HDMI_OUTPUT_INVALID) {
+ DRM_ERROR("Can't support mode %s in YCBCR format\n",
+ mode->name);
+ return DRM_HDMI_OUTPUT_INVALID;
+ }
+
+ /* Test if this src/sink/mode combination can support selected output */
+ ret = drm_can_support_this_ycbcr_output(info, mode, type,
+ src_output_cap);
+ if (!ret) {
+ DRM_ERROR("Output %s can't be supported\n",
+ drm_get_hdmi_output_name(type));
+ return DRM_HDMI_OUTPUT_INVALID;
+ }
+
+ DRM_DEBUG_KMS("Best supported output is: %s\n",
+ drm_get_hdmi_output_name(type));
+ return type;
+}
+EXPORT_SYMBOL(drm_find_hdmi_output_type);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 3ea833f..483f668 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -468,6 +468,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
+bool drm_valid_cea_vic(u8 vic);
bool drm_detect_hdmi_monitor(struct edid *edid);
bool drm_detect_monitor_audio(struct edid *edid);
bool drm_rgb_quant_range_selectable(struct edid *edid);
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 94ac771..3c237e1 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -451,6 +451,11 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
+enum drm_hdmi_output_type
+drm_find_hdmi_output_type(struct drm_connector *connector,
+ struct drm_display_mode *mode,
+ enum drm_hdmi_output_type type,
+ u32 src_output_cap);
struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
int hdisplay, int vdisplay, int vrefresh,
bool reduced, bool interlaced,
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
