Hello Ville,
Thanks for the comments, mine inline.
On 10/22/2019 5:50 PM, Ville Syrjälä wrote:
On Tue, Oct 22, 2019 at 03:29:44PM +0530, Shashank Sharma wrote:
This patch adds a scaling filter mode porperty
to allow:
- A driver/HW to showcase it's scaling filter capabilities.
- A userspace to pick a desired effect while scaling.
This option will be particularly useful in the scenarios where
Integer mode scaling is possible, and a UI client wants to pick
filters like Nearest-neighbor applied for non-blurry outputs.
There was a RFC patch series published, to discus the request to enable
Integer mode scaling by some of the gaming communities, which can be
found here:
https://patchwork.freedesktop.org/series/66175/
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
---
drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
include/drm/drm_crtc.h | 26 ++++++++++++++++++++++++++
include/drm/drm_mode_config.h | 6 ++++++
3 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 0d466d3b0809..883329453a86 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -435,6 +435,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
return ret;
} else if (property == config->prop_vrr_enabled) {
state->vrr_enabled = val;
+ } else if (property == config->scaling_filter_property) {
+ state->scaling_filter = val;
} else if (property == config->degamma_lut_property) {
ret = drm_atomic_replace_property_blob_from_id(dev,
&state->degamma_lut,
@@ -503,6 +505,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
else if (property == config->prop_out_fence_ptr)
*val = 0;
+ else if (property == config->scaling_filter_property)
+ *val = state->scaling_filter;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, val);
else
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5e9b15a0e8c5..94c5509474a8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -58,6 +58,25 @@ struct device_node;
struct dma_fence;
struct edid;
+enum drm_scaling_filters {
+ DRM_SCALING_FILTER_DEFAULT,
+ DRM_SCALING_FILTER_MEDIUM,
+ DRM_SCALING_FILTER_BILINEAR,
+ DRM_SCALING_FILTER_NN,
Please use real words.
Yes, I saw that coming. It was getting difficult with the 80 char stuff,
it was even more difficult while using it :). But let me see how better
can I do here.
+ DRM_SCALING_FILTER_NN_IS_ONLY,
Not a big fan of this. I'd just add the explicit nearest filter
and leave the decision whether to use it to userspace.
Agree, that's also one option. I was thinking to make it convenient for
userspace, For example if a compositor just want to checkout NN only
when its beneficial (like old gaming scenarios) but doesn't have enough
information (or intent), it can leave it to kernel too. But I agree,
this can cause corner cases. Let's discuss and see if we need it at all,
as you mentioned.
+ DRM_SCALING_FILTER_EDGE_ENHANCE,
+ DRM_SCALING_FILTER_INVALID,
That invalid enum value seems entirely pointless.
I was thinking about loops and all, but yeah, we can remove it.
This set of filters is pretty arbitrary. It doesn't even cover all
Intel hw. I would probably just leave it at "default+linear+nearest"
initially. Exposing more vague hw specific choices needs more though,
and I'd prefer not to spend those brain cells until a real use case
emerges.
Sure, lets start with smaller set.
+};
+
+static const struct drm_prop_enum_list drm_scaling_filter_enum_list[] = {
+ { DRM_SCALING_FILTER_DEFAULT, "Default" },
+ { DRM_SCALING_FILTER_MEDIUM, "Medium" },
+ { DRM_SCALING_FILTER_BILINEAR, "Bilinear" },
+ { DRM_SCALING_FILTER_NN, "Nearest Neighbor" },
+ { DRM_SCALING_FILTER_NN_IS_ONLY, "Integer Mode Scaling" },
+ { DRM_SCALING_FILTER_INVALID, "Invalid" },
+};
+
static inline int64_t U642I64(uint64_t val)
{
return (int64_t)*((int64_t *)&val);
@@ -283,6 +302,13 @@ struct drm_crtc_state {
*/
u32 target_vblank;
+ /**
+ * @scaling_filter:
+ *
+ * Scaling filter mode to be applied
+ */
+ u32 scaling_filter;
We have an enum so should use it.
Got it.
The documentation should probably
point out that this applies to full crtc scaling only, not plane
scaling.
The comment is actually with the property, Here I think its clear that
it's for CRTC scaling, as its a part of CRTC state.
+
/**
* @async_flip:
*
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 3bcbe30339f0..efd6fd55770f 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -914,6 +914,12 @@ struct drm_mode_config {
*/
struct drm_property *modifiers_property;
+ /**
+ * @scaling_filter_property: CRTC property to apply a particular filter
+ * while scaling in panel fitter mode.
+ */
+ struct drm_property *scaling_filter_property;
+
/* cursor size */
uint32_t cursor_width, cursor_height;
--
2.17.1
- Shashank
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
