| Message ID | 20191022095946.29354-2-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add scaling filters in DRM layer | expand |
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@intel.com> Two things: - needs real property docs for this in the kms property chapter - would be good if we could somehow subsume the panel fitter prop into this? Or at least document possible interactions with that. Plus userspace ofc, recommended best practices is to add a Link: tag pointing at the userspace patch series/merge request directly to the patch that adds the new uapi. Cheers, Daniel > --- > 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, > + DRM_SCALING_FILTER_NN_IS_ONLY, > + DRM_SCALING_FILTER_EDGE_ENHANCE, > + DRM_SCALING_FILTER_INVALID, > +}; > + > +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; > + > /** > * @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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hey Daniel, > -----Original Message----- > From: Daniel Vetter <daniel.vetter@ffwll.ch> On Behalf Of Daniel Vetter > Sent: Tuesday, October 22, 2019 3:39 PM > To: Sharma, Shashank <shashank.sharma@intel.com> > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/3] drm: Introduce scaling filter mode property > > 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@intel.com> > > Two things: > > - needs real property docs for this in the kms property chapter Got it, > - would be good if we could somehow subsume the panel fitter prop into > this? Or at least document possible interactions with that. > Sure, let me see what can I do here. > Plus userspace ofc, recommended best practices is to add a Link: tag pointing at the > userspace patch series/merge request directly to the patch that adds the new uapi. > Yep, WIP, will soon drop a link here. - Shashank > Cheers, Daniel > > --- > > 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, > > + DRM_SCALING_FILTER_NN_IS_ONLY, > > + DRM_SCALING_FILTER_EDGE_ENHANCE, > > + DRM_SCALING_FILTER_INVALID, > > +}; > > + > > +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; > > + > > /** > > * @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 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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@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. > +}; > + > +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
On Tue, Oct 22, 2019 at 10:12:29AM +0000, Sharma, Shashank wrote: > Hey Daniel, > > > -----Original Message----- > > From: Daniel Vetter <daniel.vetter@ffwll.ch> On Behalf Of Daniel Vetter > > Sent: Tuesday, October 22, 2019 3:39 PM > > To: Sharma, Shashank <shashank.sharma@intel.com> > > Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/3] drm: Introduce scaling filter mode property > > > > 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@intel.com> > > > > Two things: > > > > - needs real property docs for this in the kms property chapter > Got it, > > - would be good if we could somehow subsume the panel fitter prop into > > this? Or at least document possible interactions with that. > > > Sure, let me see what can I do here. The scaling mode prop has nothing really to do with the filter being used. The scaling mode prop just provides a bad mechanism for configuring the destination coordinates of the scaler. Trying to shoehorn these things into one prop would be a mistake IMO.
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@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, > + DRM_SCALING_FILTER_NN_IS_ONLY, > + DRM_SCALING_FILTER_EDGE_ENHANCE, > + DRM_SCALING_FILTER_INVALID, > +}; > + > +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; > + > /** > * @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; This needs to per-crtc/plane. > + > /* cursor size */ > uint32_t cursor_width, cursor_height; > > -- > 2.17.1
Hi Shashank, On Tuesday, 22 October 2019 10:59:44 BST 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@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, > + DRM_SCALING_FILTER_NN_IS_ONLY, > + DRM_SCALING_FILTER_EDGE_ENHANCE, This one looks to be missing a stringified version below. Just wanted to jump in early and suggest dropping it from the scaling filter enum. Edge enhancement is orthogonal to the mode used for scaling, at least on komeda HW, so we wouldn't be able to make the best use of the property in its current form. > + DRM_SCALING_FILTER_INVALID, > +}; > + > +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; > + > /** > * @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 A scaling filter option can also be useful for scaling individual planes, do you have any plans to extend the property's applications in that direction? > + * while scaling in panel fitter mode. > + */ > + struct drm_property *scaling_filter_property; > + > /* cursor size */ > uint32_t cursor_width, cursor_height; > > -- Mihail IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Tuesday, 22 October 2019 14:26:38 BST Mihail Atanassov wrote: > Hi Shashank, > > On Tuesday, 22 October 2019 10:59:44 BST 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@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, > > + DRM_SCALING_FILTER_NN_IS_ONLY, > > + DRM_SCALING_FILTER_EDGE_ENHANCE, > > This one looks to be missing a stringified version below. Just wanted > to jump in early and suggest dropping it from the scaling filter enum. > Edge enhancement is orthogonal to the mode used for scaling, at least > on komeda HW, so we wouldn't be able to make the best use of the > property in its current form. > > > + DRM_SCALING_FILTER_INVALID, > > +}; > > + > > +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; > > + > > /** > > * @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 > > A scaling filter option can also be useful for scaling individual > planes, do you have any plans to extend the property's applications > in that direction? > > > + * while scaling in panel fitter mode. > > + */ > > + struct drm_property *scaling_filter_property; > > + > > /* cursor size */ > > uint32_t cursor_width, cursor_height; > > > > > > > -- > Mihail > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. Sorry about that notice, not intentional :-( > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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@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. 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 >
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@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
On 10/22/2019 5:56 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@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, >> + DRM_SCALING_FILTER_NN_IS_ONLY, >> + DRM_SCALING_FILTER_EDGE_ENHANCE, >> + DRM_SCALING_FILTER_INVALID, >> +}; >> + >> +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; >> + >> /** >> * @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; > This needs to per-crtc/plane. I am not getting this, why ? It's not different than any other CRTC property like gamma/CSC etc, where we just attach them to corresponding CRTC, isn't it ? - Shashank >> + >> /* cursor size */ >> uint32_t cursor_width, cursor_height; >> >> -- >> 2.17.1
Hello Mihail, Thanks for your review, my comments inline. On 10/22/2019 6:56 PM, Mihail Atanassov wrote: > Hi Shashank, > > On Tuesday, 22 October 2019 10:59:44 BST 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@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, >> + DRM_SCALING_FILTER_NN_IS_ONLY, >> + DRM_SCALING_FILTER_EDGE_ENHANCE, > This one looks to be missing a stringified version below. Oh yes, it did. Thanks for pointing this out. > Just wanted > to jump in early and suggest dropping it from the scaling filter enum. > Edge enhancement is orthogonal to the mode used for scaling, at least > on komeda HW, so we wouldn't be able to make the best use of the > property in its current form. Yes, Ville also suggested similar, I guess we can start with the smaller set. > >> + DRM_SCALING_FILTER_INVALID, >> +}; >> + >> +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; >> + >> /** >> * @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 > A scaling filter option can also be useful for scaling individual > planes, do you have any plans to extend the property's applications > in that direction? Yes, the second stage of development should contain the plane level filtering too, but as you know, that would be a complex thing to handle, as planes apply filter pre-blending. We need to discus that (in a parallel thread :)) how should that look like. - Shashank > >> + * while scaling in panel fitter mode. >> + */ >> + struct drm_property *scaling_filter_property; >> + >> /* cursor size */ >> uint32_t cursor_width, cursor_height; >> >> > > -- > Mihail > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
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@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
On Tue, Oct 22, 2019 at 08:51:20PM +0530, Sharma, Shashank wrote: > > On 10/22/2019 5:56 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@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, > >> + DRM_SCALING_FILTER_NN_IS_ONLY, > >> + DRM_SCALING_FILTER_EDGE_ENHANCE, > >> + DRM_SCALING_FILTER_INVALID, > >> +}; > >> + > >> +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; > >> + > >> /** > >> * @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; > > This needs to per-crtc/plane. > > I am not getting this, why ? It's not different than any other CRTC > property like gamma/CSC etc, where we just attach them to corresponding > CRTC, isn't it ? Different crtcs/planes can have different capabilities, and so not all of them may want to expose all the possible filters.
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@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.
On Tue, 22 Oct 2019 20:48:02 +0530 "Sharma, Shashank" <shashank.sharma@intel.com> wrote: > 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@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/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. Hi, how could the kernel have more information or knowledge of intent than a display server? I cannot see how, because everything the kernel gets comes through the display server. I agree with Ville here. Thanks, pq
On Wed, Oct 23, 2019 at 10:34:05AM +0300, Pekka Paalanen wrote: > On Tue, 22 Oct 2019 20:48:02 +0530 > "Sharma, Shashank" <shashank.sharma@intel.com> wrote: > > > 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@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/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. > > Hi, > > how could the kernel have more information or knowledge of intent than > a display server? I cannot see how, because everything the kernel gets > comes through the display server. The only exception is due to that annoying scaling mode property. Currently userspace just tells the kernel "center vs. fullscreen vs. aspect" so in theory only the kernel knows the actual output size (though userspace should be able to calculate it as well). But maybe we can just avoid that little issue by also exposing the "TV" margin properties on local panels, at which point userspace can be more explicit about what it wants.
On Tue, 22 Oct 2019, Daniel Vetter <daniel@ffwll.ch> 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@intel.com> > > Two things: > > - needs real property docs for this in the kms property chapter > - would be good if we could somehow subsume the panel fitter prop into > this? Or at least document possible interactions with that. > > Plus userspace ofc, recommended best practices is to add a Link: tag > pointing at the userspace patch series/merge request directly to the patch > that adds the new uapi. I think Link: should only be used for referencing the patch that became the commit? References: perhaps? > > Cheers, Daniel >> --- >> 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, >> + DRM_SCALING_FILTER_NN_IS_ONLY, >> + DRM_SCALING_FILTER_EDGE_ENHANCE, >> + DRM_SCALING_FILTER_INVALID, >> +}; >> + >> +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; >> + >> /** >> * @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 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 23, 2019 at 03:44:17PM +0300, Jani Nikula wrote: > On Tue, 22 Oct 2019, Daniel Vetter <daniel@ffwll.ch> 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@intel.com> > > > > Two things: > > > > - needs real property docs for this in the kms property chapter > > - would be good if we could somehow subsume the panel fitter prop into > > this? Or at least document possible interactions with that. > > > > Plus userspace ofc, recommended best practices is to add a Link: tag > > pointing at the userspace patch series/merge request directly to the patch > > that adds the new uapi. > > I think Link: should only be used for referencing the patch that became > the commit? Dave brought up the Link: bikeshed in his uapi rfc, I'm just following the herd here. But yeah maybe we want something else. > > References: perhaps? Or Userspace: to make it real obvious? -Daniel > > > > > Cheers, Daniel > >> --- > >> 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, > >> + DRM_SCALING_FILTER_NN_IS_ONLY, > >> + DRM_SCALING_FILTER_EDGE_ENHANCE, > >> + DRM_SCALING_FILTER_INVALID, > >> +}; > >> + > >> +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; > >> + > >> /** > >> * @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 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 24 Oct 2019, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Oct 23, 2019 at 03:44:17PM +0300, Jani Nikula wrote: >> On Tue, 22 Oct 2019, Daniel Vetter <daniel@ffwll.ch> 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@intel.com> >> > >> > Two things: >> > >> > - needs real property docs for this in the kms property chapter >> > - would be good if we could somehow subsume the panel fitter prop into >> > this? Or at least document possible interactions with that. >> > >> > Plus userspace ofc, recommended best practices is to add a Link: tag >> > pointing at the userspace patch series/merge request directly to the patch >> > that adds the new uapi. >> >> I think Link: should only be used for referencing the patch that became >> the commit? > > Dave brought up the Link: bikeshed in his uapi rfc, I'm just following the > herd here. But yeah maybe we want something else. >> >> References: perhaps? > > Or Userspace: to make it real obvious? Works for me, as long as we don't conflate Link. (Maybe Link: should've been Patch: or Submission: or whatever.) BR, Jani. > -Daniel > >> >> > >> > Cheers, Daniel >> >> --- >> >> 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, >> >> + DRM_SCALING_FILTER_NN_IS_ONLY, >> >> + DRM_SCALING_FILTER_EDGE_ENHANCE, >> >> + DRM_SCALING_FILTER_INVALID, >> >> +}; >> >> + >> >> +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; >> >> + >> >> /** >> >> * @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 >> >> >> >> _______________________________________________ >> >> Intel-gfx mailing list >> >> Intel-gfx@lists.freedesktop.org >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Thu, Oct 24, 2019 at 03:12:07PM +0300, Jani Nikula wrote: > On Thu, 24 Oct 2019, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Oct 23, 2019 at 03:44:17PM +0300, Jani Nikula wrote: > >> On Tue, 22 Oct 2019, Daniel Vetter <daniel@ffwll.ch> 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@intel.com> > >> > > >> > Two things: > >> > > >> > - needs real property docs for this in the kms property chapter > >> > - would be good if we could somehow subsume the panel fitter prop into > >> > this? Or at least document possible interactions with that. > >> > > >> > Plus userspace ofc, recommended best practices is to add a Link: tag > >> > pointing at the userspace patch series/merge request directly to the patch > >> > that adds the new uapi. > >> > >> I think Link: should only be used for referencing the patch that became > >> the commit? > > > > Dave brought up the Link: bikeshed in his uapi rfc, I'm just following the > > herd here. But yeah maybe we want something else. > >> > >> References: perhaps? > > > > Or Userspace: to make it real obvious? > > Works for me, as long as we don't conflate Link. (Maybe Link: should've > been Patch: or Submission: or whatever.) The Powers That Be (kernel summit) decided that it's Link for reference to a http link of an archive of some sort that contains the msg-id. Aside: I asked k.o admins to add dri-devel/amdgpu-dev/intel-gfx to lore.k.o. They'll backfill the entire archive too. Imo still better we point at patchwork, since that's where hopefully the CI result hangs out. -Daniel > > BR, > Jani. > > > > -Daniel > > > >> > >> > > >> > Cheers, Daniel > >> >> --- > >> >> 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, > >> >> + DRM_SCALING_FILTER_NN_IS_ONLY, > >> >> + DRM_SCALING_FILTER_EDGE_ENHANCE, > >> >> + DRM_SCALING_FILTER_INVALID, > >> >> +}; > >> >> + > >> >> +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; > >> >> + > >> >> /** > >> >> * @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 > >> >> > >> >> _______________________________________________ > >> >> Intel-gfx mailing list > >> >> Intel-gfx@lists.freedesktop.org > >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> > >> -- > >> Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center
On Thu, 24 Oct 2019, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Oct 24, 2019 at 03:12:07PM +0300, Jani Nikula wrote: >> On Thu, 24 Oct 2019, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Wed, Oct 23, 2019 at 03:44:17PM +0300, Jani Nikula wrote: >> >> On Tue, 22 Oct 2019, Daniel Vetter <daniel@ffwll.ch> 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@intel.com> >> >> > >> >> > Two things: >> >> > >> >> > - needs real property docs for this in the kms property chapter >> >> > - would be good if we could somehow subsume the panel fitter prop into >> >> > this? Or at least document possible interactions with that. >> >> > >> >> > Plus userspace ofc, recommended best practices is to add a Link: tag >> >> > pointing at the userspace patch series/merge request directly to the patch >> >> > that adds the new uapi. >> >> >> >> I think Link: should only be used for referencing the patch that became >> >> the commit? >> > >> > Dave brought up the Link: bikeshed in his uapi rfc, I'm just following the >> > herd here. But yeah maybe we want something else. >> >> >> >> References: perhaps? >> > >> > Or Userspace: to make it real obvious? >> >> Works for me, as long as we don't conflate Link. (Maybe Link: should've >> been Patch: or Submission: or whatever.) > > The Powers That Be (kernel summit) decided that it's Link for reference to > a http link of an archive of some sort that contains the msg-id. Oh totally aware of that. But hopefully they also assert that Link is not also for something else. > Aside: I asked k.o admins to add dri-devel/amdgpu-dev/intel-gfx to > lore.k.o. They'll backfill the entire archive too. Imo still better we > point at patchwork, since that's where hopefully the CI result hangs out. Thanks, appreciated. Agreed on patchwork. BR, Jani. > -Daniel > >> >> BR, >> Jani. >> >> >> > -Daniel >> > >> >> >> >> > >> >> > Cheers, Daniel >> >> >> --- >> >> >> 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, >> >> >> + DRM_SCALING_FILTER_NN_IS_ONLY, >> >> >> + DRM_SCALING_FILTER_EDGE_ENHANCE, >> >> >> + DRM_SCALING_FILTER_INVALID, >> >> >> +}; >> >> >> + >> >> >> +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; >> >> >> + >> >> >> /** >> >> >> * @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 >> >> >> >> >> >> _______________________________________________ >> >> >> Intel-gfx mailing list >> >> >> Intel-gfx@lists.freedesktop.org >> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> >> Jani Nikula, Intel Open Source Graphics Center >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
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, + DRM_SCALING_FILTER_NN_IS_ONLY, + DRM_SCALING_FILTER_EDGE_ENHANCE, + DRM_SCALING_FILTER_INVALID, +}; + +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; + /** * @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;
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@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(+)