From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: jani.nikula@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v20 02/10] drm/i915: Eliminate magic numbers "0" and "1" from color plane
Date: Thu, 2 Apr 2020 19:17:52 +0300 [thread overview]
Message-ID: <20200402161752.GY13686@intel.com> (raw)
In-Reply-To: <20200326181005.11775-3-stanislav.lisovskiy@intel.com>
On Thu, Mar 26, 2020 at 08:09:57PM +0200, Stanislav Lisovskiy wrote:
> According to many computer science sources - magic values
> in code _are_ _bad_. For many reasons: the reason is that "0"
> or "1" or whatever magic values confuses and doesn't give any
> info why this parameter is this value and what it's meaning
> is.
> I renamed "0" to COLOR_PLANE_Y and "1" to COLOR_PLANE_UV,
> because we in fact already use this naming in many other places
> and function names, when dealing with color planes.
And now it's incosistent with all the rest of the codebase :(
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> .../drm/i915/display/intel_display_types.h | 5 +++
> drivers/gpu/drm/i915/intel_pm.c | 40 +++++++++----------
> 2 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 176ab5f1e867..523e0444b373 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -682,6 +682,11 @@ struct skl_plane_wm {
> bool is_planar;
> };
>
> +enum color_plane {
> + COLOR_PLANE_Y,
> + COLOR_PLANE_UV
> +};
> +
> struct skl_pipe_wm {
> struct skl_plane_wm planes[I915_MAX_PLANES];
> };
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b632b6bb9c3e..9e9a4612d842 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4013,7 +4013,7 @@ static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> int width, const struct drm_format_info *format,
> u64 modifier, unsigned int rotation,
> u32 plane_pixel_rate, struct skl_wm_params *wp,
> - int color_plane);
> + enum color_plane);
> static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> int level,
> unsigned int latency,
> @@ -4035,7 +4035,7 @@ skl_cursor_allocation(const struct intel_crtc_state *crtc_state,
> drm_format_info(DRM_FORMAT_ARGB8888),
> DRM_FORMAT_MOD_LINEAR,
> DRM_MODE_ROTATE_0,
> - crtc_state->pixel_rate, &wp, 0);
> + crtc_state->pixel_rate, &wp, COLOR_PLANE_Y);
> drm_WARN_ON(&dev_priv->drm, ret);
>
> for (level = 0; level <= max_level; level++) {
> @@ -4431,7 +4431,7 @@ static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
> static u64
> skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state,
> - int color_plane)
> + enum color_plane color_plane)
> {
> struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> const struct drm_framebuffer *fb = plane_state->hw.fb;
> @@ -4446,7 +4446,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> if (plane->id == PLANE_CURSOR)
> return 0;
>
> - if (color_plane == 1 &&
> + if (color_plane == COLOR_PLANE_UV &&
> !intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
> return 0;
>
> @@ -4459,7 +4459,7 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> height = drm_rect_height(&plane_state->uapi.src) >> 16;
>
> /* UV plane does 1/2 pixel sub-sampling */
> - if (color_plane == 1) {
> + if (color_plane == COLOR_PLANE_UV) {
> width /= 2;
> height /= 2;
> }
> @@ -4489,12 +4489,12 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
> u64 rate;
>
> /* packed/y */
> - rate = skl_plane_relative_data_rate(crtc_state, plane_state, 0);
> + rate = skl_plane_relative_data_rate(crtc_state, plane_state, COLOR_PLANE_Y);
> plane_data_rate[plane_id] = rate;
> total_data_rate += rate;
>
> /* uv-plane */
> - rate = skl_plane_relative_data_rate(crtc_state, plane_state, 1);
> + rate = skl_plane_relative_data_rate(crtc_state, plane_state, COLOR_PLANE_UV);
> uv_plane_data_rate[plane_id] = rate;
> total_data_rate += rate;
> }
> @@ -4516,7 +4516,7 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
> u64 rate;
>
> if (!plane_state->planar_linked_plane) {
> - rate = skl_plane_relative_data_rate(crtc_state, plane_state, 0);
> + rate = skl_plane_relative_data_rate(crtc_state, plane_state, COLOR_PLANE_Y);
> plane_data_rate[plane_id] = rate;
> total_data_rate += rate;
> } else {
> @@ -4533,12 +4533,12 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
> continue;
>
> /* Y plane rate is calculated on the slave */
> - rate = skl_plane_relative_data_rate(crtc_state, plane_state, 0);
> + rate = skl_plane_relative_data_rate(crtc_state, plane_state, COLOR_PLANE_Y);
> y_plane_id = plane_state->planar_linked_plane->id;
> plane_data_rate[y_plane_id] = rate;
> total_data_rate += rate;
>
> - rate = skl_plane_relative_data_rate(crtc_state, plane_state, 1);
> + rate = skl_plane_relative_data_rate(crtc_state, plane_state, COLOR_PLANE_UV);
> plane_data_rate[plane_id] = rate;
> total_data_rate += rate;
> }
> @@ -4854,14 +4854,14 @@ skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> int width, const struct drm_format_info *format,
> u64 modifier, unsigned int rotation,
> u32 plane_pixel_rate, struct skl_wm_params *wp,
> - int color_plane)
> + enum color_plane color_plane)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> u32 interm_pbpl;
>
> /* only planar format has two planes */
> - if (color_plane == 1 &&
> + if (color_plane == COLOR_PLANE_UV &&
> !intel_format_info_is_yuv_semiplanar(format, modifier)) {
> drm_dbg_kms(&dev_priv->drm,
> "Non planar format have single plane\n");
> @@ -4878,7 +4878,7 @@ skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> wp->is_planar = intel_format_info_is_yuv_semiplanar(format, modifier);
>
> wp->width = width;
> - if (color_plane == 1 && wp->is_planar)
> + if (color_plane == COLOR_PLANE_UV && wp->is_planar)
> wp->width /= 2;
>
> wp->cpp = format->cpp[color_plane];
> @@ -4945,7 +4945,7 @@ skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> static int
> skl_compute_plane_wm_params(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state,
> - struct skl_wm_params *wp, int color_plane)
> + struct skl_wm_params *wp, enum color_plane color_plane)
> {
> const struct drm_framebuffer *fb = plane_state->hw.fb;
> int width;
> @@ -5187,7 +5187,7 @@ static void skl_compute_transition_wm(const struct intel_crtc_state *crtc_state,
>
> static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state,
> - enum plane_id plane_id, int color_plane)
> + enum plane_id plane_id, enum color_plane color_plane)
> {
> struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> struct skl_wm_params wm_params;
> @@ -5216,7 +5216,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
>
> /* uv plane watermarks must also be validated for NV12/Planar */
> ret = skl_compute_plane_wm_params(crtc_state, plane_state,
> - &wm_params, 1);
> + &wm_params, COLOR_PLANE_UV);
> if (ret)
> return ret;
>
> @@ -5237,7 +5237,7 @@ static int skl_build_plane_wm(struct intel_crtc_state *crtc_state,
> return 0;
>
> ret = skl_build_plane_wm_single(crtc_state, plane_state,
> - plane_id, 0);
> + plane_id, COLOR_PLANE_Y);
> if (ret)
> return ret;
>
> @@ -5270,17 +5270,17 @@ static int icl_build_plane_wm(struct intel_crtc_state *crtc_state,
> fb->format->num_planes == 1);
>
> ret = skl_build_plane_wm_single(crtc_state, plane_state,
> - y_plane_id, 0);
> + y_plane_id, COLOR_PLANE_Y);
> if (ret)
> return ret;
>
> ret = skl_build_plane_wm_single(crtc_state, plane_state,
> - plane_id, 1);
> + plane_id, COLOR_PLANE_UV);
> if (ret)
> return ret;
> } else if (intel_wm_plane_visible(crtc_state, plane_state)) {
> ret = skl_build_plane_wm_single(crtc_state, plane_state,
> - plane_id, 0);
> + plane_id, COLOR_PLANE_Y);
> if (ret)
> return ret;
> }
> --
> 2.24.1.485.gad05a3d8e5
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-04-02 16:17 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-26 18:09 [Intel-gfx] [PATCH v20 00/10] SAGV support for Gen12+ Stanislav Lisovskiy
2020-03-26 18:09 ` [Intel-gfx] [PATCH v20 01/10] drm/i915: Start passing latency as parameter Stanislav Lisovskiy
2020-03-26 18:09 ` [Intel-gfx] [PATCH v20 02/10] drm/i915: Eliminate magic numbers "0" and "1" from color plane Stanislav Lisovskiy
2020-04-02 16:17 ` Ville Syrjälä [this message]
2020-04-02 16:28 ` Lisovskiy, Stanislav
2020-04-02 16:41 ` Ville Syrjälä
2020-04-03 15:41 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-03-26 18:09 ` [Intel-gfx] [PATCH v20 03/10] drm/i915: Introduce skl_plane_wm_level accessor Stanislav Lisovskiy
2020-03-26 18:09 ` [Intel-gfx] [PATCH v20 04/10] drm/i915: Add intel_atomic_get_bw_*_state helpers Stanislav Lisovskiy
2020-04-02 16:20 ` Ville Syrjälä
2020-04-02 16:49 ` Ville Syrjälä
2020-04-03 6:11 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-04-03 6:15 ` Stanislav Lisovskiy
2020-03-26 18:10 ` [Intel-gfx] [PATCH v20 05/10] drm/i915: Extract gen specific functions from intel_can_enable_sagv Stanislav Lisovskiy
2020-04-02 16:44 ` Ville Syrjälä
2020-04-03 6:20 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-04-07 19:01 ` Ville Syrjälä
2020-04-08 7:58 ` Lisovskiy, Stanislav
2020-04-08 14:55 ` Ville Syrjälä
2020-04-08 15:54 ` Lisovskiy, Stanislav
2020-04-08 16:18 ` Lisovskiy, Stanislav
2020-04-09 15:58 ` Ville Syrjälä
2020-03-26 18:10 ` [Intel-gfx] [PATCH v20 06/10] drm/i915: Add proper SAGV support for TGL+ Stanislav Lisovskiy
2020-03-26 18:39 ` Stanislav Lisovskiy
2020-04-02 17:22 ` Ville Syrjälä
2020-04-03 6:29 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-04-03 15:43 ` Stanislav Lisovskiy
2020-03-26 18:10 ` [Intel-gfx] [PATCH v20 07/10] drm/i915: Added required new PCode commands Stanislav Lisovskiy
2020-04-02 17:27 ` Ville Syrjälä
2020-04-03 6:32 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-03-26 18:10 ` [Intel-gfx] [PATCH v20 08/10] drm/i915: Rename bw_state to new_bw_state Stanislav Lisovskiy
2020-04-02 17:30 ` Ville Syrjälä
2020-04-03 6:34 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-03-26 18:10 ` [Intel-gfx] [PATCH v20 09/10] drm/i915: Restrict qgv points which don't have enough bandwidth Stanislav Lisovskiy
2020-03-26 18:36 ` Stanislav Lisovskiy
2020-04-02 17:50 ` Ville Syrjälä
2020-04-03 6:37 ` [Intel-gfx] [PATCH v21 " Stanislav Lisovskiy
2020-04-03 16:41 ` Stanislav Lisovskiy
2020-03-26 18:10 ` [Intel-gfx] [PATCH v20 10/10] drm/i915: Enable SAGV support for Gen12 Stanislav Lisovskiy
2020-03-26 20:59 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for SAGV support for Gen12+ (rev3) Patchwork
2020-03-26 21:26 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-03-27 11:58 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-04-03 6:19 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for SAGV support for Gen12+ (rev4) Patchwork
2020-04-03 6:22 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for SAGV support for Gen12+ (rev5) Patchwork
2020-04-03 6:50 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for SAGV support for Gen12+ (rev10) Patchwork
2020-04-03 6:54 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-04-03 7:16 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-04-03 16:29 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for SAGV support for Gen12+ (rev12) Patchwork
2020-04-03 16:40 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for SAGV support for Gen12+ (rev10) Patchwork
2020-04-03 17:05 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for SAGV support for Gen12+ (rev13) Patchwork
2020-04-03 17:29 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-04-04 2:33 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200402161752.GY13686@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=stanislav.lisovskiy@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox