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

[Sharma, Shashank]

Hello Harry,

Thanks for your comments, please find mine inline.

On 10/22/2019 7:36 PM, Harry Wentland wrote:
>
> On 2019-10-22 8:20 a.m., 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.
>>
>>> +	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.
>>
>>> +	DRM_SCALING_FILTER_EDGE_ENHANCE,
>>> +	DRM_SCALING_FILTER_INVALID,
>> That invalid enum value seems entirely pointless.
>>
>> 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.
>>
> FWIW, AMD HW allows us to specify a number of horizontal and vertical
> taps for scaling. Number of taps are limited by our linebuffer size. The
> default is 4 in each dimension but you could have 2 v_taps and 4 h_taps
> if your're running a large horizontal resolution on some ASICs.
>
> I'm not sure it makes sense to define filters here that aren't used. It
> sounds like default and nearest neighbour would suffice for now in order
> to support integer scaling.

Agree, this seems to be a consistent feedback from the community, I will 
probably start with a smaller set of most popular/used ones.

- Shashank

>
> Harry
>
>>> +};
>>> +
>>> +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. The documentation should probably
>> point out that this applies to full crtc scaling only, not plane
>> scaling.
>>
>>> +
>>>   	/**
>>>   	 * @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