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 <[email protected]>
> > ---
> > 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.
