| Message ID | 20240614173911.3743172-2-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Dump DSC state to dmesg/debugfs | expand |
On Fri, 14 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: > Add helpers to convert between x16 fixed point and integer/fraction > values. Also add the format/argument macros required to printk x16 > fixed point variables. > > These are needed by later patches dumping the Display Stream Compression > configuration in DRM core and in the i915 driver to replace the > corresponding bpp_x16 helpers defined locally in the driver. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/display/drm_dp_helper.c | 5 +++-- > include/drm/drm_fixed.h | 23 +++++++++++++++++++++++ > 2 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > index 79a615667aab1..806f9c9764995 100644 > --- a/drivers/gpu/drm/display/drm_dp_helper.c > +++ b/drivers/gpu/drm/display/drm_dp_helper.c > @@ -35,6 +35,7 @@ > #include <drm/display/drm_dp_helper.h> > #include <drm/display/drm_dp_mst_helper.h> > #include <drm/drm_edid.h> > +#include <drm/drm_fixed.h> > #include <drm/drm_print.h> > #include <drm/drm_vblank.h> > #include <drm/drm_panel.h> > @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive, > int symbol_cycles; > > if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) { > - DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n", > + DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n", > lane_count, hactive, > - bpp_x16 >> 4, (bpp_x16 & 0xf) * 625); > + DRM_X16_ARGS(bpp_x16)); > return 0; > } > > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h > index 81572d32db0c2..0fe2a7f50d54e 100644 > --- a/include/drm/drm_fixed.h > +++ b/include/drm/drm_fixed.h > @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x) > return sum; > } > > +static inline int drm_x16_from_int(int val_int) > +{ > + return val_int << 4; > +} > + > +static inline int drm_x16_to_int(int val_x16) > +{ > + return val_x16 >> 4; > +} > + > +static inline int drm_x16_to_int_roundup(int val_x16) > +{ > + return (val_x16 + 0xf) >> 4; > +} > + > +static inline int drm_x16_to_frac(int val_x16) > +{ > + return val_x16 & 0xf; > +} Sad trombone about the completely different naming scheme compared to the rest of the file. Not saying the existing naming is great, but neither is this. And there's no way to unify except by renaming *both* afterwards. We could devise a scheme now that could be used for the existing stuff later, without renaming the new stuff. *shrug* BR, Jani. > + > +#define DRM_X16_FMT "%d.%04d" > +#define DRM_X16_ARGS(val_x16) drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625) > + > #endif
On Wed, Jun 19, 2024 at 01:10:09PM +0300, Jani Nikula wrote: > On Fri, 14 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: > > Add helpers to convert between x16 fixed point and integer/fraction > > values. Also add the format/argument macros required to printk x16 > > fixed point variables. > > > > These are needed by later patches dumping the Display Stream Compression > > configuration in DRM core and in the i915 driver to replace the > > corresponding bpp_x16 helpers defined locally in the driver. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/display/drm_dp_helper.c | 5 +++-- > > include/drm/drm_fixed.h | 23 +++++++++++++++++++++++ > > 2 files changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > > index 79a615667aab1..806f9c9764995 100644 > > --- a/drivers/gpu/drm/display/drm_dp_helper.c > > +++ b/drivers/gpu/drm/display/drm_dp_helper.c > > @@ -35,6 +35,7 @@ > > #include <drm/display/drm_dp_helper.h> > > #include <drm/display/drm_dp_mst_helper.h> > > #include <drm/drm_edid.h> > > +#include <drm/drm_fixed.h> > > #include <drm/drm_print.h> > > #include <drm/drm_vblank.h> > > #include <drm/drm_panel.h> > > @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive, > > int symbol_cycles; > > > > if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) { > > - DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n", > > + DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n", > > lane_count, hactive, > > - bpp_x16 >> 4, (bpp_x16 & 0xf) * 625); > > + DRM_X16_ARGS(bpp_x16)); > > return 0; > > } > > > > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h > > index 81572d32db0c2..0fe2a7f50d54e 100644 > > --- a/include/drm/drm_fixed.h > > +++ b/include/drm/drm_fixed.h > > @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x) > > return sum; > > } > > > > +static inline int drm_x16_from_int(int val_int) > > +{ > > + return val_int << 4; > > +} > > + > > +static inline int drm_x16_to_int(int val_x16) > > +{ > > + return val_x16 >> 4; > > +} > > + > > +static inline int drm_x16_to_int_roundup(int val_x16) > > +{ > > + return (val_x16 + 0xf) >> 4; > > +} > > + > > +static inline int drm_x16_to_frac(int val_x16) > > +{ > > + return val_x16 & 0xf; > > +} > > Sad trombone about the completely different naming scheme compared to > the rest of the file. > > Not saying the existing naming is great, but neither is this. And > there's no way to unify except by renaming *both* afterwards. > > We could devise a scheme now that could be used for the existing stuff > later, without renaming the new stuff. Based on [1]'s short variant, we could have: dfixed*(fixed20_12 v) -> drm_uq12*(drm_uq20_12_t v) drm_fixp*(s64 v) -> drm_q32*(s64 v) drm_x16*(int v) -> drm_q4*(int v) Or instead of uq12/q32/q4 using ufp12/fp32/fp4. [1] https://en.wikipedia.org/wiki/Q_(number_format) > *shrug* > > BR, > Jani. > > > > > + > > +#define DRM_X16_FMT "%d.%04d" > > +#define DRM_X16_ARGS(val_x16) drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625) > > + > > #endif > > -- > Jani Nikula, Intel
On Wed, Jun 19, 2024 at 03:01:03PM +0300, Imre Deak wrote: > On Wed, Jun 19, 2024 at 01:10:09PM +0300, Jani Nikula wrote: > > On Fri, 14 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: > > > Add helpers to convert between x16 fixed point and integer/fraction > > > values. Also add the format/argument macros required to printk x16 > > > fixed point variables. > > > > > > These are needed by later patches dumping the Display Stream Compression > > > configuration in DRM core and in the i915 driver to replace the > > > corresponding bpp_x16 helpers defined locally in the driver. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/display/drm_dp_helper.c | 5 +++-- > > > include/drm/drm_fixed.h | 23 +++++++++++++++++++++++ > > > 2 files changed, 26 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > > > index 79a615667aab1..806f9c9764995 100644 > > > --- a/drivers/gpu/drm/display/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/display/drm_dp_helper.c > > > @@ -35,6 +35,7 @@ > > > #include <drm/display/drm_dp_helper.h> > > > #include <drm/display/drm_dp_mst_helper.h> > > > #include <drm/drm_edid.h> > > > +#include <drm/drm_fixed.h> > > > #include <drm/drm_print.h> > > > #include <drm/drm_vblank.h> > > > #include <drm/drm_panel.h> > > > @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive, > > > int symbol_cycles; > > > > > > if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) { > > > - DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n", > > > + DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n", > > > lane_count, hactive, > > > - bpp_x16 >> 4, (bpp_x16 & 0xf) * 625); > > > + DRM_X16_ARGS(bpp_x16)); > > > return 0; > > > } > > > > > > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h > > > index 81572d32db0c2..0fe2a7f50d54e 100644 > > > --- a/include/drm/drm_fixed.h > > > +++ b/include/drm/drm_fixed.h > > > @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x) > > > return sum; > > > } > > > > > > +static inline int drm_x16_from_int(int val_int) > > > +{ > > > + return val_int << 4; > > > +} > > > + > > > +static inline int drm_x16_to_int(int val_x16) > > > +{ > > > + return val_x16 >> 4; > > > +} > > > + > > > +static inline int drm_x16_to_int_roundup(int val_x16) > > > +{ > > > + return (val_x16 + 0xf) >> 4; > > > +} > > > + > > > +static inline int drm_x16_to_frac(int val_x16) > > > +{ > > > + return val_x16 & 0xf; > > > +} > > > > Sad trombone about the completely different naming scheme compared to > > the rest of the file. > > > > Not saying the existing naming is great, but neither is this. And > > there's no way to unify except by renaming *both* afterwards. > > > > We could devise a scheme now that could be used for the existing stuff > > later, without renaming the new stuff. > > Based on [1]'s short variant, we could have: > > dfixed*(fixed20_12 v) -> drm_uq12*(drm_uq20_12_t v) > drm_fixp*(s64 v) -> drm_q32*(s64 v) > drm_x16*(int v) -> drm_q4*(int v) > > Or instead of uq12/q32/q4 using ufp12/fp32/fp4. Jani, any objection to using the drm_fp4_from_int() etc. names? > [1] https://en.wikipedia.org/wiki/Q_(number_format) > > > *shrug* > > > > BR, > > Jani. > > > > > > > > > + > > > +#define DRM_X16_FMT "%d.%04d" > > > +#define DRM_X16_ARGS(val_x16) drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625) > > > + > > > #endif > > > > -- > > Jani Nikula, Intel
On Wed, 19 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: > On Wed, Jun 19, 2024 at 01:10:09PM +0300, Jani Nikula wrote: >> On Fri, 14 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: >> > Add helpers to convert between x16 fixed point and integer/fraction >> > values. Also add the format/argument macros required to printk x16 >> > fixed point variables. >> > >> > These are needed by later patches dumping the Display Stream Compression >> > configuration in DRM core and in the i915 driver to replace the >> > corresponding bpp_x16 helpers defined locally in the driver. >> > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> >> > --- >> > drivers/gpu/drm/display/drm_dp_helper.c | 5 +++-- >> > include/drm/drm_fixed.h | 23 +++++++++++++++++++++++ >> > 2 files changed, 26 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c >> > index 79a615667aab1..806f9c9764995 100644 >> > --- a/drivers/gpu/drm/display/drm_dp_helper.c >> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c >> > @@ -35,6 +35,7 @@ >> > #include <drm/display/drm_dp_helper.h> >> > #include <drm/display/drm_dp_mst_helper.h> >> > #include <drm/drm_edid.h> >> > +#include <drm/drm_fixed.h> >> > #include <drm/drm_print.h> >> > #include <drm/drm_vblank.h> >> > #include <drm/drm_panel.h> >> > @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive, >> > int symbol_cycles; >> > >> > if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) { >> > - DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n", >> > + DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n", >> > lane_count, hactive, >> > - bpp_x16 >> 4, (bpp_x16 & 0xf) * 625); >> > + DRM_X16_ARGS(bpp_x16)); >> > return 0; >> > } >> > >> > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h >> > index 81572d32db0c2..0fe2a7f50d54e 100644 >> > --- a/include/drm/drm_fixed.h >> > +++ b/include/drm/drm_fixed.h >> > @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x) >> > return sum; >> > } >> > >> > +static inline int drm_x16_from_int(int val_int) >> > +{ >> > + return val_int << 4; >> > +} >> > + >> > +static inline int drm_x16_to_int(int val_x16) >> > +{ >> > + return val_x16 >> 4; >> > +} >> > + >> > +static inline int drm_x16_to_int_roundup(int val_x16) >> > +{ >> > + return (val_x16 + 0xf) >> 4; >> > +} >> > + >> > +static inline int drm_x16_to_frac(int val_x16) >> > +{ >> > + return val_x16 & 0xf; >> > +} >> >> Sad trombone about the completely different naming scheme compared to >> the rest of the file. >> >> Not saying the existing naming is great, but neither is this. And >> there's no way to unify except by renaming *both* afterwards. >> >> We could devise a scheme now that could be used for the existing stuff >> later, without renaming the new stuff. > > Based on [1]'s short variant, we could have: > > dfixed*(fixed20_12 v) -> drm_uq12*(drm_uq20_12_t v) > drm_fixp*(s64 v) -> drm_q32*(s64 v) > drm_x16*(int v) -> drm_q4*(int v) I like it. If you're brave, add them with a generic prefix instead of drm_ from the start, say fp_q4. ;) BR, Jani. > > Or instead of uq12/q32/q4 using ufp12/fp32/fp4. > > [1] https://en.wikipedia.org/wiki/Q_(number_format) > >> *shrug* >> >> BR, >> Jani. >> >> >> >> > + >> > +#define DRM_X16_FMT "%d.%04d" >> > +#define DRM_X16_ARGS(val_x16) drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625) >> > + >> > #endif >> >> -- >> Jani Nikula, Intel
On Thu, Jun 27, 2024 at 06:41:46PM +0300, Jani Nikula wrote: > On Wed, 19 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: > > On Wed, Jun 19, 2024 at 01:10:09PM +0300, Jani Nikula wrote: > >> On Fri, 14 Jun 2024, Imre Deak <imre.deak@intel.com> wrote: > >> > Add helpers to convert between x16 fixed point and integer/fraction > >> > values. Also add the format/argument macros required to printk x16 > >> > fixed point variables. > >> > > >> > These are needed by later patches dumping the Display Stream Compression > >> > configuration in DRM core and in the i915 driver to replace the > >> > corresponding bpp_x16 helpers defined locally in the driver. > >> > > >> > Signed-off-by: Imre Deak <imre.deak@intel.com> > >> > --- > >> > drivers/gpu/drm/display/drm_dp_helper.c | 5 +++-- > >> > include/drm/drm_fixed.h | 23 +++++++++++++++++++++++ > >> > 2 files changed, 26 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c > >> > index 79a615667aab1..806f9c9764995 100644 > >> > --- a/drivers/gpu/drm/display/drm_dp_helper.c > >> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c > >> > @@ -35,6 +35,7 @@ > >> > #include <drm/display/drm_dp_helper.h> > >> > #include <drm/display/drm_dp_mst_helper.h> > >> > #include <drm/drm_edid.h> > >> > +#include <drm/drm_fixed.h> > >> > #include <drm/drm_print.h> > >> > #include <drm/drm_vblank.h> > >> > #include <drm/drm_panel.h> > >> > @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive, > >> > int symbol_cycles; > >> > > >> > if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) { > >> > - DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n", > >> > + DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n", > >> > lane_count, hactive, > >> > - bpp_x16 >> 4, (bpp_x16 & 0xf) * 625); > >> > + DRM_X16_ARGS(bpp_x16)); > >> > return 0; > >> > } > >> > > >> > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h > >> > index 81572d32db0c2..0fe2a7f50d54e 100644 > >> > --- a/include/drm/drm_fixed.h > >> > +++ b/include/drm/drm_fixed.h > >> > @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x) > >> > return sum; > >> > } > >> > > >> > +static inline int drm_x16_from_int(int val_int) > >> > +{ > >> > + return val_int << 4; > >> > +} > >> > + > >> > +static inline int drm_x16_to_int(int val_x16) > >> > +{ > >> > + return val_x16 >> 4; > >> > +} > >> > + > >> > +static inline int drm_x16_to_int_roundup(int val_x16) > >> > +{ > >> > + return (val_x16 + 0xf) >> 4; > >> > +} > >> > + > >> > +static inline int drm_x16_to_frac(int val_x16) > >> > +{ > >> > + return val_x16 & 0xf; > >> > +} > >> > >> Sad trombone about the completely different naming scheme compared to > >> the rest of the file. > >> > >> Not saying the existing naming is great, but neither is this. And > >> there's no way to unify except by renaming *both* afterwards. > >> > >> We could devise a scheme now that could be used for the existing stuff > >> later, without renaming the new stuff. > > > > Based on [1]'s short variant, we could have: > > > > dfixed*(fixed20_12 v) -> drm_uq12*(drm_uq20_12_t v) > > drm_fixp*(s64 v) -> drm_q32*(s64 v) > > drm_x16*(int v) -> drm_q4*(int v) > > I like it. If you're brave, add them with a generic prefix instead of > drm_ from the start, say fp_q4. ;) Ok will do that, but using fxp_q4 is clearer I think. > > BR, > Jani. > > > > > Or instead of uq12/q32/q4 using ufp12/fp32/fp4. > > > > [1] https://en.wikipedia.org/wiki/Q_(number_format) > > > >> *shrug* > >> > >> BR, > >> Jani. > >> > >> > >> > >> > + > >> > +#define DRM_X16_FMT "%d.%04d" > >> > +#define DRM_X16_ARGS(val_x16) drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625) > >> > + > >> > #endif > >> > >> -- > >> Jani Nikula, Intel > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c index 79a615667aab1..806f9c9764995 100644 --- a/drivers/gpu/drm/display/drm_dp_helper.c +++ b/drivers/gpu/drm/display/drm_dp_helper.c @@ -35,6 +35,7 @@ #include <drm/display/drm_dp_helper.h> #include <drm/display/drm_dp_mst_helper.h> #include <drm/drm_edid.h> +#include <drm/drm_fixed.h> #include <drm/drm_print.h> #include <drm/drm_vblank.h> #include <drm/drm_panel.h> @@ -4151,9 +4152,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive, int symbol_cycles; if (lane_count == 0 || hactive == 0 || bpp_x16 == 0) { - DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 %d.%04d\n", + DRM_DEBUG_KMS("Invalid BW overhead params: lane_count %d, hactive %d, bpp_x16 " DRM_X16_FMT "\n", lane_count, hactive, - bpp_x16 >> 4, (bpp_x16 & 0xf) * 625); + DRM_X16_ARGS(bpp_x16)); return 0; } diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h index 81572d32db0c2..0fe2a7f50d54e 100644 --- a/include/drm/drm_fixed.h +++ b/include/drm/drm_fixed.h @@ -214,4 +214,27 @@ static inline s64 drm_fixp_exp(s64 x) return sum; } +static inline int drm_x16_from_int(int val_int) +{ + return val_int << 4; +} + +static inline int drm_x16_to_int(int val_x16) +{ + return val_x16 >> 4; +} + +static inline int drm_x16_to_int_roundup(int val_x16) +{ + return (val_x16 + 0xf) >> 4; +} + +static inline int drm_x16_to_frac(int val_x16) +{ + return val_x16 & 0xf; +} + +#define DRM_X16_FMT "%d.%04d" +#define DRM_X16_ARGS(val_x16) drm_x16_to_int(val_x16), (drm_x16_to_frac(val_x16) * 625) + #endif
Add helpers to convert between x16 fixed point and integer/fraction values. Also add the format/argument macros required to printk x16 fixed point variables. These are needed by later patches dumping the Display Stream Compression configuration in DRM core and in the i915 driver to replace the corresponding bpp_x16 helpers defined locally in the driver. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/display/drm_dp_helper.c | 5 +++-- include/drm/drm_fixed.h | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)