Re: [PATCH v2 1/1] drm/mediatek: Filter modes according to hardware capability

[Shawn Sung (宋孝謙)]

Hi CK,

On Fri, 2024-02-16 at 09:38 +0000, CK Hu (胡俊光) wrote:
> Hi, Hsiao-chien:
> 
> On Wed, 2024-02-07 at 10:15 +0800, Hsiao Chien Sung wrote:
> > From: Hsiao Chien Sung <shawn.sung@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> > >
> > 
> > We found a stability issue on MT8188 when connecting an external
> > monitor
> > in 2560x1440@144Hz mode. Checked with the designer, there is a
> > function
> > called "prefetch" which is working during VBP (triggered by VSYNC).
> > If the duration of VBP is too short, the throughput requirement
> > could
> > increase more than 3 times and lead to stability issues.
> > 
> > The mode settings that VDOSYS supports are mainly affected by clock
> > rate and throughput, display driver should filter these settings
> > according to the SoC's limitation to avoid unstable conditions.
> > 
> > Since currently the mode filter is only available on MT8195 and
> > MT8188
> > and they share the same compatible name, the reference number
> > (8250)
> > is hard coded instead of in the driver data.
> > 
> > Signed-off-by: Hsiao Chien Sung <
> > shawn.sung@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |  4 ++
> >  drivers/gpu/drm/mediatek/mtk_disp_merge.c     | 56
> > +++++++++++++++++++
> >  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   | 17 ++++++
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 17 ++++++
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   | 12 ++++
> >  6 files changed, 107 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index eb738f14f09e3..4a5661334fb1a 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > @@ -72,6 +72,8 @@ void mtk_merge_advance_config(struct device *dev,
> > unsigned int l_w, unsigned int
> >  			      struct cmdq_pkt *cmdq_pkt);
> >  void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt
> > *cmdq_pkt);
> >  void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt
> > *cmdq_pkt);
> > +enum drm_mode_status mtk_merge_mode_valid(struct device *dev,
> > +					  const struct drm_display_mode
> > *mode);
> >  
> >  void mtk_ovl_bgclr_in_on(struct device *dev);
> >  void mtk_ovl_bgclr_in_off(struct device *dev);
> > @@ -130,6 +132,8 @@ unsigned int mtk_ovl_adaptor_layer_nr(struct
> > device *dev);
> >  struct device *mtk_ovl_adaptor_dma_dev_get(struct device *dev);
> >  const u32 *mtk_ovl_adaptor_get_formats(struct device *dev);
> >  size_t mtk_ovl_adaptor_get_num_formats(struct device *dev);
> > +enum drm_mode_status mtk_ovl_adaptor_mode_valid(struct device
> > *dev,
> > +						const struct
> > drm_display_mode *mode);
> >  
> >  void mtk_rdma_bypass_shadow(struct device *dev);
> >  int mtk_rdma_clk_enable(struct device *dev);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > index c19fb1836034d..6b065ee254455 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > @@ -223,6 +223,62 @@ void mtk_merge_clk_disable(struct device *dev)
> >  	clk_disable_unprepare(priv->clk);
> >  }
> >  
> > +enum drm_mode_status mtk_merge_mode_valid(struct device *dev,
> > +					  const struct drm_display_mode
> > *mode)
> > +{
> > +	struct mtk_disp_merge *priv = dev_get_drvdata(dev);
> > +	unsigned long rate = 0;
> > +
> > +	rate = clk_get_rate(priv->clk);
> > +
> > +	/* Convert to KHz and round the number */
> > +	rate = (rate + 500) / 1000;
> > +
> > +	if (rate && mode->clock > rate) {
> > +		dev_dbg(dev, "invalid clock: %d (>%lu)\n", mode-
> > >clock, 
> > rate);
> > +		return MODE_CLOCK_HIGH;
> > +	}
> > +
> > +	/*
> > +	 * Measure the bandwidth requirement of hardware prefetch (per
> > frame)
> > +	 *
> > +	 * let N = prefetch buffer size in lines
> > +	 *         (ex. N=3, then prefetch buffer size = 3 lines)
> > +	 *
> > +	 * prefetch size = htotal * N (pixels)
> > +	 * time per line = 1 / fps / vtotal (seconds)
> > +	 * duration      = vbp * time per line
> > +	 *               = vbp / fps / vtotal
> > +	 *
> > +	 * data rate = prefetch size / duration
> > +	 *           = htotal * N / (vbp / fps / vtotal)
> > +	 *           = htotal * vtotal * fps * N / vbp
> > +	 *           = clk * N / vbp (pixels per second)
> > +	 *
> > +	 * Say 4K60 (CAE-861) is the maximum mode supported by the SoC
> > +	 * data rate = 594000K * N / 72 = 8250 (standard)
> > +	 * (remove K*N because of the same unit)
> 
> Is N constant? For example, when 4K, N=3. When 2560x1440, N is still
> 3?
> I think the buffer size is constant, if N is still 3 when 2560x1440,
> the buffer is not full and some space is wasted.
> 

Yes, 'N' is a constant in the proposed formula, and indeed, in this
case, it seems there is some space wasted in the prefetch buffer.
However, if the throughput exceeds the expectation when only 3 lines of
the data is considered, then it must be insufficient for the whole
prefetch buffer.

In order to have as many common factors in different modes so they can
be eliminated when comparing with each other, here we just use the same
'N' for all kinds of resolutions.

Regards,
Shawn