[PATCH 1/5] drm: Add drm_fixed_16_16 helper

[PATCH 1/5] drm: Add drm_fixed_16_16 helper

[Alyssa Rosenzweig]

This constructs a fixed 16.16 rational, useful to specify the minimum
and maximum scaling in drm_atomic_helper_check_plane_state. It is
open-coded as a macro in multiple drivers, so let's share the helper.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 include/drm/drm_fixed.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 553210c02ee0..df1f369b4918 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -208,4 +208,9 @@ static inline s64 drm_fixp_exp(s64 x)
 	return sum;
 }
 
+static inline int drm_fixed_16_16(s32 mult, s32 div)
+{
+	return (mult << 16) / div;
+}
+
 #endif
-- 
2.30.2

[PATCH 2/5] drm/meson: Use common drm_fixed_16_16 helper

[Alyssa Rosenzweig]

Replace our open-coded FRAC_16_16 with the common drm_fixed_16_16
helper to reduce code duplication between drivers.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: linux-amlogic@lists.infradead.org
---
 drivers/gpu/drm/meson/meson_overlay.c | 7 +++----
 drivers/gpu/drm/meson/meson_plane.c   | 5 ++---
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_overlay.c b/drivers/gpu/drm/meson/meson_overlay.c
index dfef8afcc245..d8fc6bbb332f 100644
--- a/drivers/gpu/drm/meson/meson_overlay.c
+++ b/drivers/gpu/drm/meson/meson_overlay.c
@@ -15,6 +15,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_fixed.h>
 
 #include "meson_overlay.h"
 #include "meson_registers.h"
@@ -162,8 +163,6 @@ struct meson_overlay {
 };
 #define to_meson_overlay(x) container_of(x, struct meson_overlay, base)
 
-#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
-
 static int meson_overlay_atomic_check(struct drm_plane *plane,
 				      struct drm_atomic_state *state)
 {
@@ -181,8 +180,8 @@ static int meson_overlay_atomic_check(struct drm_plane *plane,
 
 	return drm_atomic_helper_check_plane_state(new_plane_state,
 						   crtc_state,
-						   FRAC_16_16(1, 5),
-						   FRAC_16_16(5, 1),
+						   drm_fixed_16_16(1, 5),
+						   drm_fixed_16_16(5, 1),
 						   true, true);
 }
 
diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index 8640a8a8a469..4fae9ebbf178 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -19,6 +19,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_fixed.h>
 
 #include "meson_plane.h"
 #include "meson_registers.h"
@@ -68,8 +69,6 @@ struct meson_plane {
 };
 #define to_meson_plane(x) container_of(x, struct meson_plane, base)
 
-#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
-
 static int meson_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_atomic_state *state)
 {
@@ -92,7 +91,7 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
 	 */
 	return drm_atomic_helper_check_plane_state(new_plane_state,
 						   crtc_state,
-						   FRAC_16_16(1, 5),
+						   drm_fixed_16_16(1, 5),
 						   DRM_PLANE_HELPER_NO_SCALING,
 						   false, true);
 }
-- 
2.30.2

[PATCH 3/5] drm/msm: Use common drm_fixed_16_16 helper

[Alyssa Rosenzweig]

Replace our open-coded FRAC_16_16 with the common drm_fixed_16_16
helper to reduce code duplication between drivers.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: linux-arm-msm@vger.kernel.org
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c  | 2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 8 ++++----
 drivers/gpu/drm/msm/msm_drv.h              | 3 +--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index c989621209aa..fc9a9f544110 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -964,7 +964,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
 		crtc_state = drm_atomic_get_new_crtc_state(state,
 							   new_plane_state->crtc);
 
-	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxupscale);
+	min_scale = drm_fixed_16_16(1, pdpu->pipe_sblk->maxupscale);
 	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
 						  min_scale,
 						  pdpu->pipe_sblk->maxdwnscale << 16,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index c6b69afcbac8..079b0662ee3c 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -199,8 +199,8 @@ static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state *crtc_state,
 		return -ERANGE;
 	}
 
-	min_scale = FRAC_16_16(1, 8);
-	max_scale = FRAC_16_16(8, 1);
+	min_scale = drm_fixed_16_16(1, 8);
+	max_scale = drm_fixed_16_16(8, 1);
 
 	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
 						  min_scale, max_scale,
@@ -381,8 +381,8 @@ static int mdp5_plane_atomic_async_check(struct drm_plane *plane,
 	    plane->state->fb != new_plane_state->fb)
 		return -EINVAL;
 
-	min_scale = FRAC_16_16(1, 8);
-	max_scale = FRAC_16_16(8, 1);
+	min_scale = drm_fixed_16_16(1, 8);
+	max_scale = drm_fixed_16_16(8, 1);
 
 	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
 						  min_scale, max_scale,
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 8b005d1ac899..b5aa94024a42 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -32,6 +32,7 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/msm_drm.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_fixed.h>
 
 struct msm_kms;
 struct msm_gpu;
@@ -51,8 +52,6 @@ struct msm_disp_state;
 #define MAX_BRIDGES    8
 #define MAX_CONNECTORS 8
 
-#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
-
 struct msm_file_private {
 	rwlock_t queuelock;
 	struct list_head submitqueues;
-- 
2.30.2

[PATCH 4/5] drm/rockchip: Use common drm_fixed_16_16 helper

[Alyssa Rosenzweig]

Replace our open-coded FRAC_16_16 with the common drm_fixed_16_16
helper to reduce code duplication between drivers.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: linux-rockchip@lists.infradead.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 9 +++++----
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 1 -
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index ba9e14da41b4..9428fcba400f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -29,6 +29,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_self_refresh_helper.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_fixed.h>
 
 #ifdef CONFIG_DRM_ANALOGIX_DP
 #include <drm/bridge/analogix_dp.h>
@@ -789,9 +790,9 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
 	struct vop_win *vop_win = to_vop_win(plane);
 	const struct vop_win_data *win = vop_win->data;
 	int ret;
-	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
+	int min_scale = win->phy->scl ? drm_fixed_16_16(1, 8) :
 					DRM_PLANE_HELPER_NO_SCALING;
-	int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
+	int max_scale = win->phy->scl ? drm_fixed_16_16(8, 1) :
 					DRM_PLANE_HELPER_NO_SCALING;
 
 	if (!crtc || WARN_ON(!fb))
@@ -1037,9 +1038,9 @@ static int vop_plane_atomic_async_check(struct drm_plane *plane,
 										 plane);
 	struct vop_win *vop_win = to_vop_win(plane);
 	const struct vop_win_data *win = vop_win->data;
-	int min_scale = win->phy->scl ? FRAC_16_16(1, 8) :
+	int min_scale = win->phy->scl ? drm_fixed_16_16(1, 8) :
 					DRM_PLANE_HELPER_NO_SCALING;
-	int max_scale = win->phy->scl ? FRAC_16_16(8, 1) :
+	int max_scale = win->phy->scl ? drm_fixed_16_16(8, 1) :
 					DRM_PLANE_HELPER_NO_SCALING;
 	struct drm_crtc_state *crtc_state;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 857d97cdc67c..cada12e653cc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -335,7 +335,6 @@ enum vop_pol {
 	DEN_NEGATIVE   = 2
 };
 
-#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
 #define SCL_FT_DEFAULT_FIXPOINT_SHIFT	12
 #define SCL_MAX_VSKIPLINES		4
 #define MIN_SCL_FT_AFTER_VSKIP		1
-- 
2.30.2

[PATCH 5/5] drm/zte: Use common drm_fixed_16_16 helper

[Alyssa Rosenzweig]

Replace our open-coded FRAC_16_16 with the common drm_fixed_16_16
helper to reduce code duplication between drivers.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/gpu/drm/zte/zx_plane.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/zte/zx_plane.c b/drivers/gpu/drm/zte/zx_plane.c
index 93bcca428e35..80f61d79be83 100644
--- a/drivers/gpu/drm/zte/zx_plane.c
+++ b/drivers/gpu/drm/zte/zx_plane.c
@@ -11,6 +11,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_fixed.h>
 
 #include "zx_common_regs.h"
 #include "zx_drm_drv.h"
@@ -43,8 +44,6 @@ static const uint32_t vl_formats[] = {
 	 */
 };
 
-#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
-
 static int zx_vl_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_atomic_state *state)
 {
@@ -53,8 +52,8 @@ static int zx_vl_plane_atomic_check(struct drm_plane *plane,
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_crtc *crtc = plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
-	int min_scale = FRAC_16_16(1, 8);
-	int max_scale = FRAC_16_16(8, 1);
+	int min_scale = drm_fixed_16_16(1, 8);
+	int max_scale = drm_fixed_16_16(8, 1);
 
 	if (!crtc || WARN_ON(!fb))
 		return 0;
-- 
2.30.2

Re: [PATCH 1/5] drm: Add drm_fixed_16_16 helper

[Laurent Pinchart]

Hi Alyssa,

Thank you for the patch.

On Wed, Sep 01, 2021 at 01:54:27PM -0400, Alyssa Rosenzweig wrote:
> This constructs a fixed 16.16 rational, useful to specify the minimum
> and maximum scaling in drm_atomic_helper_check_plane_state. It is
> open-coded as a macro in multiple drivers, so let's share the helper.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  include/drm/drm_fixed.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 553210c02ee0..df1f369b4918 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -208,4 +208,9 @@ static inline s64 drm_fixp_exp(s64 x)
>  	return sum;
>  }
>  

Missing documentation :-)

> +static inline int drm_fixed_16_16(s32 mult, s32 div)

You should return a s32.

The function name isn't very explicit, and departs from the naming
scheme of other functions in the same file. As fixed-point numbers are
stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the
drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The
function should probably be named drm_fixp_s16_16 from_fraction() then,
but then the same logic should possibly be replicated to ensure optimal
precision. I wonder if it wouldn't be best to simply use
drm_fixp_from_fraction() and shift the result right by 16 bits.

> +{
> +	return (mult << 16) / div;
> +}
> +
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] drm: Add drm_fixed_16_16 helper

[Alyssa Rosenzweig]

> Missing documentation :-)

Ack.

> > +static inline int drm_fixed_16_16(s32 mult, s32 div)
> 
> You should return a s32.

Ack.

> The function name isn't very explicit, and departs from the naming
> scheme of other functions in the same file. As fixed-point numbers are
> stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the
> drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The
> function should probably be named drm_fixp_s16_16 from_fraction() then,
> but then the same logic should possibly be replicated to ensure optimal
> precision. I wonder if it wouldn't be best to simply use
> drm_fixp_from_fraction() and shift the result right by 16 bits.

Sure, I'm not attached to the naming ... will wait to hear what colours
everyone else wants the bikehed painted.

As for the implementation, I just went with what was used across
multiple drivers already (no chance of regressions that way) but could
reuse other helpers if it's better..? If the behaviour changes this goes
from a trivial cleanup to a much more invasive changeset. I don't own
half of the hardware here.

Re: [PATCH 1/5] drm: Add drm_fixed_16_16 helper

[Laurent Pinchart]

On Wed, Sep 01, 2021 at 09:35:40PM -0400, Alyssa Rosenzweig wrote:
> > Missing documentation :-)
> 
> Ack.
> 
> > > +static inline int drm_fixed_16_16(s32 mult, s32 div)
> > 
> > You should return a s32.
> 
> Ack.
> 
> > The function name isn't very explicit, and departs from the naming
> > scheme of other functions in the same file. As fixed-point numbers are
> > stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the
> > drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The
> > function should probably be named drm_fixp_s16_16 from_fraction() then,
> > but then the same logic should possibly be replicated to ensure optimal
> > precision. I wonder if it wouldn't be best to simply use
> > drm_fixp_from_fraction() and shift the result right by 16 bits.
> 
> Sure, I'm not attached to the naming ... will wait to hear what colours
> everyone else wants the bikehed painted.
> 
> As for the implementation, I just went with what was used across
> multiple drivers already (no chance of regressions that way) but could
> reuse other helpers if it's better..? If the behaviour changes this goes
> from a trivial cleanup to a much more invasive changeset. I don't own
> half of the hardware here.

But except for msm which may need testing, all other drivers use the
existing FRAC_16_16() macro with constant arguments, so it shouldn't be
difficult to ensure that the new function gives the same results.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] drm/meson: Use common drm_fixed_16_16 helper

[Neil Armstrong]

On 01/09/2021 19:54, Alyssa Rosenzweig wrote:
> Replace our open-coded FRAC_16_16 with the common drm_fixed_16_16
> helper to reduce code duplication between drivers.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: linux-amlogic@lists.infradead.org
> ---
>  drivers/gpu/drm/meson/meson_overlay.c | 7 +++----
>  drivers/gpu/drm/meson/meson_plane.c   | 5 ++---
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_overlay.c b/drivers/gpu/drm/meson/meson_overlay.c
> index dfef8afcc245..d8fc6bbb332f 100644
> --- a/drivers/gpu/drm/meson/meson_overlay.c
> +++ b/drivers/gpu/drm/meson/meson_overlay.c
> @@ -15,6 +15,7 @@
>  #include <drm/drm_gem_atomic_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_fixed.h>
>  
>  #include "meson_overlay.h"
>  #include "meson_registers.h"
> @@ -162,8 +163,6 @@ struct meson_overlay {
>  };
>  #define to_meson_overlay(x) container_of(x, struct meson_overlay, base)
>  
> -#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
> -
>  static int meson_overlay_atomic_check(struct drm_plane *plane,
>  				      struct drm_atomic_state *state)
>  {
> @@ -181,8 +180,8 @@ static int meson_overlay_atomic_check(struct drm_plane *plane,
>  
>  	return drm_atomic_helper_check_plane_state(new_plane_state,
>  						   crtc_state,
> -						   FRAC_16_16(1, 5),
> -						   FRAC_16_16(5, 1),
> +						   drm_fixed_16_16(1, 5),
> +						   drm_fixed_16_16(5, 1),
>  						   true, true);
>  }
>  
> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> index 8640a8a8a469..4fae9ebbf178 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -19,6 +19,7 @@
>  #include <drm/drm_gem_atomic_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_fixed.h>
>  
>  #include "meson_plane.h"
>  #include "meson_registers.h"
> @@ -68,8 +69,6 @@ struct meson_plane {
>  };
>  #define to_meson_plane(x) container_of(x, struct meson_plane, base)
>  
> -#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
> -
>  static int meson_plane_atomic_check(struct drm_plane *plane,
>  				    struct drm_atomic_state *state)
>  {
> @@ -92,7 +91,7 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
>  	 */
>  	return drm_atomic_helper_check_plane_state(new_plane_state,
>  						   crtc_state,
> -						   FRAC_16_16(1, 5),
> +						   drm_fixed_16_16(1, 5),
>  						   DRM_PLANE_HELPER_NO_SCALING,
>  						   false, true);
>  }
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>