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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: [email protected]
---
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
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
> 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.
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 <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>