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

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

[Shawn Sung (宋孝謙)]

Hi Michał,

On Fri, 2024-11-29 at 17:13 +0100, Michał Kopeć wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi Shawn, thanks for replying!

Thank you for bringing this issue to our attention.

Since every email from outside of the company is first moved to an
isolation folder, I replied your mail before moving it out and noticed
that there is a "[SPAM]" tag in the subject line.
I apologize for what happened and have fixed it this time.

> 
>  > As the reference number 8250 = 594000 / 72:
>  > 594000 is KHz of the clock rate in mode setting
>  > 72 is lines during VBP (Vertical Back Porch)
>  > Can you share your reference number or the EDID in 3440x1440@144
> with
>  > us?
> 
> Sure, here's the mode defined in the EDID:
> 
> DTD: 3440x1440 143.922761 Hz 0:0 217.323 kHz 799.750000 MHz (aspect
> undefined, no 3D stereo, preferred)
>            Hfront 48 Hsync 32 Hback 160 Hpol P
>            Vfront 3 Vsync 10 Vback 57 Vpol N
> 
> 799750/57 is 14030 so definitely much higher than supported
> according to the code.
> 
> There's also other modes:
> 
> DTD: 3440x1440 119.999642 Hz 0:0 182.999 kHz 670.510000 MHz (aspect
> undefined, no 3D stereo)
>            Hfront 144 Hsync 32 Hback 48 Hpol P
>            Vfront 3 Vsync 4 Vback 78 Vpol N
> 
> DTD 2: 3440x1440 99.981604 Hz 43:18 150.972 kHz 543.500000 MHz (800
> mm
> x 335 mm)
>              Hfront 48 Hsync 32 Hback 80 Hpol P
>              Vfront 3 Vsync 4 Vback 63 Vpol N
> 
> 670510/78 = 8596
> 543500/63 = 8626
> 
> So the 120Hz and 100Hz modes look like they might almost work :) The
> manufacturer neglected to add a 60Hz mode which I suspect might have
> worked without issue.

Yes, these two numbers seems to work as they are just less than 5%
above the spec.

> 
>  > The reference number 8250 is established based on our hardware
>  > specifications. Numbers higher than that may function, but could
> be
>  > unstable. For example, the monitor may seem to work properly at
> first,
>  > but you could encounter black screens or glitching from time to
> time.
> 
> Understood. I was just wondering if it might be dependent on other
> factors than Vbp and pixel clock.
> 
>  > May I confirm if you are going to implement the patch just for
> local
>  > testing or for upstream? As 8250 is calculated based on the
>  > specifications and was verified through a series of procedures.
> Any
>  > changes to it may require thorough verifications before being
> accepted.
> 
> I was going to initially try to implement something for personal use
> but
> I wanted to eventually try to upstream it. However, from what I
> understand,
> it might not be easy or possible.
> 
> If I develop a patch that works reliably at least for me, would you
> be
> open to me adding a kernel module parameter to allow unverified data
> rates, with clear description that it may cause unstable behavior?

As I also have a high refresh rate monitor, I can completely understand
how you feel. Could you please change 8250 to `8250 * 1.05` and see if
it works in your situation?

Since the official configuration usually prefer a more stable setting,
we have initiated an internal discussion about this request.

> 
>  > Regards,
>  > Shawn
> 
> Regards,
> Michał
> 

Regards,
Shawn

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

[Jason-JH Lin (林睿祥)]

Hi Michał,

[snip]

> >  > The reference number 8250 is established based on our hardware
> >  > specifications. Numbers higher than that may function, but could
> > be
> >  > unstable. For example, the monitor may seem to work properly at
> > first,
> >  > but you could encounter black screens or glitching from time to
> > time.
> > 
> > Understood. I was just wondering if it might be dependent on other
> > factors than Vbp and pixel clock.
> > 
> >  > May I confirm if you are going to implement the patch just for
> > local
> >  > testing or for upstream? As 8250 is calculated based on the
> >  > specifications and was verified through a series of procedures.
> > Any
> >  > changes to it may require thorough verifications before being
> > accepted.
> > 
> > I was going to initially try to implement something for personal
> > use
> > but
> > I wanted to eventually try to upstream it. However, from what I
> > understand,
> > it might not be easy or possible.
> > 
> > If I develop a patch that works reliably at least for me, would you
> > be
> > open to me adding a kernel module parameter to allow unverified
> > data
> > rates, with clear description that it may cause unstable behavior?
> 
> As I also have a high refresh rate monitor, I can completely
> understand
> how you feel. Could you please change 8250 to `8250 * 1.05` and see
> if
> it works in your situation?
> 
> Since the official configuration usually prefer a more stable
> setting,
> we have initiated an internal discussion about this request.
> 

Here is the reply by hardware designer after an internal discussion
with us:

Due to internal regulations, we are unable to disclose the actual
hardware specifications.

The current approach has been validated against the hardware
limitations:
1. pixel clock rate < 594000 KHz
2. pixel clock rate / VBP  < 8250

While it is possible that exceeding these limitations might still work,
it could potentially go beyond the guarantees provided by the DRAM
controller and Hardware Real-Time (HRT) capabilities, leading to a risk
of encountering unstable underflow issues.

I think this scaling patch (without hardware guarantees) is more
suitable for the specific product lines applied to Chrome, not
upstream.

Regards,
Jason-JH Lin

> > 
> >  > Regards,
> >  > Shawn
> > 
> > Regards,
> > Michał
> > 
> 
> Regards,
> Shawn
> 

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

[Michał Kopeć]

(Re-sending because Shawn's @google email is inactive)

Hi Shawn,

I'm a new owner of the Chromebook Ciri (MT8188) and have run into an 
issue which I believe is related to this patch.

I'm attempting to use a 3440x1440 144Hz monitor over DisplayPort, but 
the available resolutions are limited to 2560x1440@120Hz. I found this 
patch, then I confirmed with the monitor's EDID that i am indeed 
reaching data rates above 8250 (though minimally) in higher resolution 
modes.

I wanted to ask about this comment:

 > The proposed formula is only one way to estimate whether our SoC
 > supports the mode setting. The basic idea behind it is just to check
 > if the data rate requirement is too high (directly proportional to
 > pixel clock, inversely proportional to vbp). Please adjust the
 > function if it doesn't fit your situation in the future.

- Can you confirm if the reference number 8250 is the hard limit of the 
hardware?
- Could you suggest other formulae that could work for my use case? I'd 
be happy to implement and test on my end but as I don't have access to 
MTK documentation, I don't know the actual limits of the hardware and 
how to deal with them

Regards,
Michał Kopeć

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

[CK Hu (胡俊光)]

+ Jason

On Wed, 2024-11-27 at 15:28 +0100, Michał Kopeć wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> (Re-sending because Shawn's @google email is inactive)
> 
> Hi Shawn,
> 
> I'm a new owner of the Chromebook Ciri (MT8188) and have run into an
> issue which I believe is related to this patch.
> 
> I'm attempting to use a 3440x1440 144Hz monitor over DisplayPort, but
> the available resolutions are limited to 2560x1440@120Hz. I found this
> patch, then I confirmed with the monitor's EDID that i am indeed
> reaching data rates above 8250 (though minimally) in higher resolution
> modes.
> 
> I wanted to ask about this comment:
> 
>  > The proposed formula is only one way to estimate whether our SoC
>  > supports the mode setting. The basic idea behind it is just to check
>  > if the data rate requirement is too high (directly proportional to
>  > pixel clock, inversely proportional to vbp). Please adjust the
>  > function if it doesn't fit your situation in the future.
> 
> - Can you confirm if the reference number 8250 is the hard limit of the
> hardware?
> - Could you suggest other formulae that could work for my use case? I'd
> be happy to implement and test on my end but as I don't have access to
> MTK documentation, I don't know the actual limits of the hardware and
> how to deal with them
> 
> Regards,
> Michał Kopeć
> 
> 
> 
> 

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

[Michał Kopeć]

Hi Shawn,

I'm a new owner of the Chromebook Ciri (MT8188) and have run into an 
issue which I believe is caused by this patch.

I'm attempting to use a 3440x1440 144Hz monitor over DisplayPort, but 
the available resolutions are limited to 2560x1440@120Hz. I found this 
patch, then I confirmed with the monitor's EDID that i am indeed 
reaching data rates above 8250 (though minimally) in higher resolution 
modes.

I wanted to ask about this comment:

 > The proposed formula is only one way to estimate whether our SoC
 > supports the mode setting. The basic idea behind it is just to check
 > if the data rate requirement is too high (directly proportional to
 > pixel clock, inversely proportional to vbp). Please adjust the
 > function if it doesn't fit your situation in the future.

- Can you confirm if the reference number 8250 is the hard limit of the 
hardware?
- Could you suggest other formulae that could work for my use case? I'd 
be happy to implement and test on my end but as I don't have access to 
MTK documentation, I don't know the actual limits of the hardware and 
how to deal with them

Regards,
Michał Kopeć

[PATCH v2 0/1] Filter modes according to hardware capability

[Hsiao Chien Sung]

From: Hsiao Chien Sung <shawn.sung@mediatek.corp-partner.google.com>

This series is based on mediatek-drm-next branch of
kernel/git/chunkuang.hu/linux.git.

Changes in v2:
- Refined the filtering formula
- Add the .mode_valid hook to the hardware that causes the limitation

Hsiao Chien Sung (1):
  drm/mediatek: Filter modes according to hardware capability

 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(+)

--
2.18.0

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

[Hsiao Chien Sung]

From: Hsiao Chien Sung <shawn.sung@mediatek.corp-partner.google.com>

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@mediatek.corp-partner.google.com>
---
 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)
+	 *
+	 * For 2560x1440@144 (clk=583600K, vbp=17):
+	 * rate = 583600 / 17 ~= 34329 > 8250 (NG)
+	 *
+	 * For 2560x1440@120 (clk=497760K, vbp=77):
+	 * rate = 497760 / 77 ~= 6464 < 8250 (OK)
+	 *
+	 * Bandwidth requirement of hardware prefetch increases significantly
+	 * when the VBP decreases (more than 4x in this example).
+	 */
+	rate = mode->clock / (mode->vtotal - mode->vsync_end);
+
+	if (rate > 8250) {
+		dev_dbg(dev, "invalid rate: %lu (>8250): " DRM_MODE_FMT "\n",
+			rate, DRM_MODE_ARG(mode));
+		return MODE_BAD;
+	}
+
+	return MODE_OK;
+}
+
 static int mtk_disp_merge_bind(struct device *dev, struct device *master,
 			       void *data)
 {
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
index 10d23e76acaa9..6d4334955e3d3 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
@@ -88,6 +88,7 @@ static const struct mtk_ddp_comp_funcs ethdr = {
 static const struct mtk_ddp_comp_funcs merge = {
 	.clk_enable = mtk_merge_clk_enable,
 	.clk_disable = mtk_merge_clk_disable,
+	.mode_valid = mtk_merge_mode_valid,
 };
 
 static const struct mtk_ddp_comp_funcs padding = {
@@ -341,6 +342,22 @@ void mtk_ovl_adaptor_clk_disable(struct device *dev)
 	}
 }
 
+enum drm_mode_status mtk_ovl_adaptor_mode_valid(struct device *dev,
+						const struct drm_display_mode *mode)
+
+{
+	int i;
+	struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev);
+
+	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
+		dev = ovl_adaptor->ovl_adaptor_comp[i];
+		if (!dev || !comp_matches[i].funcs->mode_valid)
+			continue;
+		return comp_matches[i].funcs->mode_valid(dev, mode);
+	}
+	return MODE_OK;
+}
+
 unsigned int mtk_ovl_adaptor_layer_nr(struct device *dev)
 {
 	return MTK_OVL_ADAPTOR_LAYER_NUM;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 1dac8d0fbc669..14cf75fa217f9 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -212,6 +212,22 @@ static void mtk_drm_crtc_destroy_state(struct drm_crtc *crtc,
 	kfree(to_mtk_crtc_state(state));
 }
 
+static enum drm_mode_status
+mtk_drm_crtc_mode_valid(struct drm_crtc *crtc,
+			const struct drm_display_mode *mode)
+{
+	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
+	enum drm_mode_status status = MODE_OK;
+	int i;
+
+	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
+		status = mtk_ddp_comp_mode_valid(mtk_crtc->ddp_comp[i], mode);
+		if (status != MODE_OK)
+			break;
+	}
+	return status;
+}
+
 static bool mtk_drm_crtc_mode_fixup(struct drm_crtc *crtc,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adjusted_mode)
@@ -830,6 +846,7 @@ static const struct drm_crtc_funcs mtk_crtc_funcs = {
 static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
 	.mode_fixup	= mtk_drm_crtc_mode_fixup,
 	.mode_set_nofb	= mtk_drm_crtc_mode_set_nofb,
+	.mode_valid	= mtk_drm_crtc_mode_valid,
 	.atomic_begin	= mtk_drm_crtc_atomic_begin,
 	.atomic_flush	= mtk_drm_crtc_atomic_flush,
 	.atomic_enable	= mtk_drm_crtc_atomic_enable,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index 9633e860cc3ce..94590227c56a9 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -416,6 +416,7 @@ static const struct mtk_ddp_comp_funcs ddp_ovl_adaptor = {
 	.remove = mtk_ovl_adaptor_remove_comp,
 	.get_formats = mtk_ovl_adaptor_get_formats,
 	.get_num_formats = mtk_ovl_adaptor_get_num_formats,
+	.mode_valid = mtk_ovl_adaptor_mode_valid,
 };
 
 static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index f8c7e8d8ddc12..215b7234ff13c 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -12,6 +12,8 @@
 #include <linux/soc/mediatek/mtk-mmsys.h>
 #include <linux/soc/mediatek/mtk-mutex.h>
 
+#include <drm/drm_modes.h>
+
 struct device;
 struct device_node;
 struct drm_crtc;
@@ -84,6 +86,7 @@ struct mtk_ddp_comp_funcs {
 	void (*add)(struct device *dev, struct mtk_mutex *mutex);
 	void (*remove)(struct device *dev, struct mtk_mutex *mutex);
 	unsigned int (*encoder_index)(struct device *dev);
+	enum drm_mode_status (*mode_valid)(struct device *dev, const struct drm_display_mode *mode);
 };
 
 struct mtk_ddp_comp {
@@ -125,6 +128,15 @@ static inline void mtk_ddp_comp_clk_disable(struct mtk_ddp_comp *comp)
 		comp->funcs->clk_disable(comp->dev);
 }
 
+static inline
+enum drm_mode_status mtk_ddp_comp_mode_valid(struct mtk_ddp_comp *comp,
+					     const struct drm_display_mode *mode)
+{
+	if (comp && comp->funcs && comp->funcs->mode_valid)
+		return comp->funcs->mode_valid(comp->dev, mode);
+	return MODE_OK;
+}
+
 static inline void mtk_ddp_comp_config(struct mtk_ddp_comp *comp,
 				       unsigned int w, unsigned int h,
 				       unsigned int vrefresh, unsigned int bpc,
-- 
2.18.0

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

[CK Hu (胡俊光)]

Hi, Hsiao-chien:

On Wed, 2024-02-07 at 10:15 +0800, Hsiao Chien Sung wrote:
> From: Hsiao Chien Sung <shawn.sung@mediatek.corp-partner.google.com>
> 
> 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@mediatek.corp-partner.google.com>
> ---
>  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.

Regards,
CK

> +	 *
> +	 * For 2560x1440@144 (clk=583600K, vbp=17):
> +	 * rate = 583600 / 17 ~= 34329 > 8250 (NG)
> +	 *
> +	 * For 2560x1440@120 (clk=497760K, vbp=77):
> +	 * rate = 497760 / 77 ~= 6464 < 8250 (OK)
> +	 *
> +	 * Bandwidth requirement of hardware prefetch increases
> significantly
> +	 * when the VBP decreases (more than 4x in this example).
> +	 */
> +	rate = mode->clock / (mode->vtotal - mode->vsync_end);
> +
> +	if (rate > 8250) {
> +		dev_dbg(dev, "invalid rate: %lu (>8250): " DRM_MODE_FMT
> "\n",
> +			rate, DRM_MODE_ARG(mode));
> +		return MODE_BAD;
> +	}
> +
> +	return MODE_OK;
> +}
> +
>  static int mtk_disp_merge_bind(struct device *dev, struct device
> *master,
>  			       void *data)
>  {
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> index 10d23e76acaa9..6d4334955e3d3 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -88,6 +88,7 @@ static const struct mtk_ddp_comp_funcs ethdr = {
>  static const struct mtk_ddp_comp_funcs merge = {
>  	.clk_enable = mtk_merge_clk_enable,
>  	.clk_disable = mtk_merge_clk_disable,
> +	.mode_valid = mtk_merge_mode_valid,
>  };
>  
>  static const struct mtk_ddp_comp_funcs padding = {
> @@ -341,6 +342,22 @@ void mtk_ovl_adaptor_clk_disable(struct device
> *dev)
>  	}
>  }
>  
> +enum drm_mode_status mtk_ovl_adaptor_mode_valid(struct device *dev,
> +						const struct
> drm_display_mode *mode)
> +
> +{
> +	int i;
> +	struct mtk_disp_ovl_adaptor *ovl_adaptor =
> dev_get_drvdata(dev);
> +
> +	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> +		dev = ovl_adaptor->ovl_adaptor_comp[i];
> +		if (!dev || !comp_matches[i].funcs->mode_valid)
> +			continue;
> +		return comp_matches[i].funcs->mode_valid(dev, mode);
> +	}
> +	return MODE_OK;
> +}
> +
>  unsigned int mtk_ovl_adaptor_layer_nr(struct device *dev)
>  {
>  	return MTK_OVL_ADAPTOR_LAYER_NUM;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 1dac8d0fbc669..14cf75fa217f9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -212,6 +212,22 @@ static void mtk_drm_crtc_destroy_state(struct
> drm_crtc *crtc,
>  	kfree(to_mtk_crtc_state(state));
>  }
>  
> +static enum drm_mode_status
> +mtk_drm_crtc_mode_valid(struct drm_crtc *crtc,
> +			const struct drm_display_mode *mode)
> +{
> +	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> +	enum drm_mode_status status = MODE_OK;
> +	int i;
> +
> +	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> +		status = mtk_ddp_comp_mode_valid(mtk_crtc->ddp_comp[i], 
> mode);
> +		if (status != MODE_OK)
> +			break;
> +	}
> +	return status;
> +}
> +
>  static bool mtk_drm_crtc_mode_fixup(struct drm_crtc *crtc,
>  				    const struct drm_display_mode
> *mode,
>  				    struct drm_display_mode
> *adjusted_mode)
> @@ -830,6 +846,7 @@ static const struct drm_crtc_funcs mtk_crtc_funcs
> = {
>  static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
>  	.mode_fixup	= mtk_drm_crtc_mode_fixup,
>  	.mode_set_nofb	= mtk_drm_crtc_mode_set_nofb,
> +	.mode_valid	= mtk_drm_crtc_mode_valid,
>  	.atomic_begin	= mtk_drm_crtc_atomic_begin,
>  	.atomic_flush	= mtk_drm_crtc_atomic_flush,
>  	.atomic_enable	= mtk_drm_crtc_atomic_enable,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 9633e860cc3ce..94590227c56a9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -416,6 +416,7 @@ static const struct mtk_ddp_comp_funcs
> ddp_ovl_adaptor = {
>  	.remove = mtk_ovl_adaptor_remove_comp,
>  	.get_formats = mtk_ovl_adaptor_get_formats,
>  	.get_num_formats = mtk_ovl_adaptor_get_num_formats,
> +	.mode_valid = mtk_ovl_adaptor_mode_valid,
>  };
>  
>  static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] =
> {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index f8c7e8d8ddc12..215b7234ff13c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -12,6 +12,8 @@
>  #include <linux/soc/mediatek/mtk-mmsys.h>
>  #include <linux/soc/mediatek/mtk-mutex.h>
>  
> +#include <drm/drm_modes.h>
> +
>  struct device;
>  struct device_node;
>  struct drm_crtc;
> @@ -84,6 +86,7 @@ struct mtk_ddp_comp_funcs {
>  	void (*add)(struct device *dev, struct mtk_mutex *mutex);
>  	void (*remove)(struct device *dev, struct mtk_mutex *mutex);
>  	unsigned int (*encoder_index)(struct device *dev);
> +	enum drm_mode_status (*mode_valid)(struct device *dev, const
> struct drm_display_mode *mode);
>  };
>  
>  struct mtk_ddp_comp {
> @@ -125,6 +128,15 @@ static inline void
> mtk_ddp_comp_clk_disable(struct mtk_ddp_comp *comp)
>  		comp->funcs->clk_disable(comp->dev);
>  }
>  
> +static inline
> +enum drm_mode_status mtk_ddp_comp_mode_valid(struct mtk_ddp_comp
> *comp,
> +					     const struct
> drm_display_mode *mode)
> +{
> +	if (comp && comp->funcs && comp->funcs->mode_valid)
> +		return comp->funcs->mode_valid(comp->dev, mode);
> +	return MODE_OK;
> +}
> +
>  static inline void mtk_ddp_comp_config(struct mtk_ddp_comp *comp,
>  				       unsigned int w, unsigned int h,
>  				       unsigned int vrefresh, unsigned
> int bpc,

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@mediatek.corp-partner.google.com
> > >
> > 
> > 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@mediatek.corp-partner.google.com>
> > ---
> >  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