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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.