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

[Ville Syrjälä]

On Tue, Oct 22, 2019 at 02:06:22PM +0000, 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.

Yeah, even linear is somewhat questionable since we don't have clear
need for it. Although given that it is well defined it's much less
of a problem than a bunch of the other proposed filters.

-- 
Ville Syrjälä
Intel