[PATCH 1/3] drm: Introduce scaling filter mode property

[Sharma, Shashank]

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 at intel.com>
>> ---
>>   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