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@xxxxxxxxx>
> > > ---
> > > .../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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
