Re: [PATCH 5/6] drm/i915/wm: convert tiling mode check in slk_compute_plane_wm() to a switch-case

[Luca Coelho <luca@coelho.fi>]

On Mon, 2025-09-08 at 09:51 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2025-09-08 04:35:34-03:00)
> > Make the code a bit clearer by using a switch-case to check the tiling
> > mode in skl_compute_plane_wm(), because all the possible states and
> > the calculations they use are explicitly handled.
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/skl_watermark.c | 24 +++++++++++++++++---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index dd4bed02c3c0..21f8d52ec1d2 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -1833,21 +1833,39 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> >                                  latency,
> >                                  wp->plane_blocks_per_line);
> > 
> > -        if (wp->tiling == WM_TILING_Y_FAMILY) {
> > +        switch (wp->tiling) {
> > +        case WM_TILING_Y_FAMILY:
> >                 selected_result = max_fixed16(method2, wp->y_tile_minimum);
> > -        } else {
> > +                break;
> > +
> > +        case WM_TILING_LINEAR:
> > +        case WM_TILING_X_TILED:
> > +                /*
> > +                 * Special case for unrealistically small horizontal
> > +                 * total with plane downscaling.
> > +                 */
> >                 if ((wp->cpp * crtc_state->hw.pipe_mode.crtc_htotal /
> >                      wp->dbuf_block_size < 1) &&
> > -                     (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
> > +                    (wp->plane_bytes_per_line / wp->dbuf_block_size < 1)) {
> >                         selected_result = method2;
> >                 } else if (latency >= wp->linetime_us) {
> > +                        /*
> > +                         * With display version 9, we use the minimum
> > +                         * of both methods.
> > +                         */
> 
> Hm... Isn't this saying what is already clear in the code below?

Very true.  I had more text here, describing the method 1 case below
too, but after removing that, this became mostly irrelevant.  I'll
remove it.


> 
> >                         if (DISPLAY_VER(display) == 9)
> >                                 selected_result = min_fixed16(method1, method2);
> >                         else
> >                                 selected_result = method2;
> >                 } else {
> > +                        /* everything else with linear/X-tiled uses method 1 */
> >                         selected_result = method1;
> >                 }
> > +                break;
> > +
> > +        default:
> > +                drm_err(display->drm, "Invalid tiling mode\n", wp->tiling);
> > +                break;
> 
> If we decide to go with the enumeration solution, I think we should
> change this into a warning and use some default behavior here (perhaps
> WM_TILING_LINEAR?). Otherwise, selected_result would be used
> uninitialized.

Right, I moved this to a fallthrough in a later patch.

--
Cheers,
Luca.