From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Lisovskiy, Stanislav" <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:41:17 +0300 [thread overview]
Message-ID: <20200402164117.GA13686@intel.com> (raw)
In-Reply-To: <20200402162806.GA25052@intel.com>
On Thu, Apr 02, 2020 at 07:28:43PM +0300, Lisovskiy, Stanislav wrote:
> On Thu, Apr 02, 2020 at 07:17:52PM +0300, Ville Syrjälä wrote:
> > 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 :(
>
> ]I think functions like func(0,1,2) is more than ugly.
>
> I can find dozens of sources from credible people stating why it's
> wrong.
>
> If the existing code base is inspiring people to do it that way
> - then it's wrong.
>
> And I honestly don't even know what is more stupid here:
> passing magic numbers into functions, making it look non-obvious
> or arguing about that. Kind of competition.
>
> I think we have also enum values passed into functions in our
> code and that seems fine(may be because that code wasn't written by me?).
>
> For instance you do use PIPE_A, PIPE_B instead of 0,1,2 right?
color_plane is just an index into the fb metadata. It's not a specific
named thing as such. Again, if we want to introduce some new convention
for this stuff we should do it across the whole driver (and probably
also drm core) otherewise you're just introducing inconsistencies
between different parts of the codebase. Different conventions between
different parts just increases the load on the poor brain when you have
to convert between thenm to figure out what's going on.
>
> Or sure I can find another examples.
>
>
> >
> > >
> > > 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
--
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:41 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ä
2020-04-02 16:28 ` Lisovskiy, Stanislav
2020-04-02 16:41 ` Ville Syrjälä [this message]
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=20200402164117.GA13686@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.