Message-ID: <cab0b98e-89fe-4ead-ba10-c399250013c3@nxp.com>
Date: Mon, 22 Sep 2025 11:55:37 +0800
From: Liu Ying <victor.liu@....com>
To: Frank Li <Frank.li@....com>
Cc: Philipp Zabel <p.zabel@...gutronix.de>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Dmitry Baryshkov <lumag@...nel.org>,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 14/14] drm/imx: dc: Use prefetch engine

On 09/19/2025, Frank Li wrote:
> On Fri, Jul 04, 2025 at 05:04:01PM +0800, Liu Ying wrote:
>> One prefetch engine consists of one DPR channel and one or two PRGs.
>> Each PRG handles one planar in a pixel format.  Every FetchUnit used
>> by KMS may attach to a PRG and hence use a prefetch engine.  So, to
>> simplify driver code, always use prefetch engines for FetchUnits in
>> KMS driver and avoid supporting bypassing them.  Aside from configuring
>> and disabling a prefetch engine along with a FetchUnit for atomic
>> commits, properly disable the prefetch engine at boot and adapt burst
>> size/stride fixup requirements from PRG in FetchUnit driver.
>>
>> Signed-off-by: Liu Ying <victor.liu@....com>
>> ---
>>  drivers/gpu/drm/imx/dc/dc-crtc.c  | 139 +++++++++++++++++++++++++++++++++++---
>>  drivers/gpu/drm/imx/dc/dc-fu.c    |  27 +++++++-
>>  drivers/gpu/drm/imx/dc/dc-fu.h    |   2 +-
>>  drivers/gpu/drm/imx/dc/dc-kms.h   |   5 ++
>>  drivers/gpu/drm/imx/dc/dc-plane.c |  46 +++++++++++--
>>  5 files changed, 197 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
>> index 9e9e86cd5202bcb0bb4d5627dbcefcc3f4e2ead0..4c7aab360616cb1c84c31c83f16df703b1c2c6d7 100644
>> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
>> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
>> @@ -25,6 +25,7 @@
>>  #include <drm/drm_vblank.h>
>>
>>  #include "dc-de.h"
>> +#include "dc-dprc.h"
>>  #include "dc-drv.h"
>>  #include "dc-kms.h"
>>  #include "dc-pe.h"
>> @@ -204,7 +205,13 @@ dc_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state)
>>  				drm_atomic_get_old_crtc_state(state, crtc);
>>  	struct drm_crtc_state *new_crtc_state =
>>  				drm_atomic_get_new_crtc_state(state, crtc);
>> +	struct drm_plane_state *old_plane_state =
>> +			drm_atomic_get_old_plane_state(state, crtc->primary);
>> +	struct drm_plane_state *new_plane_state =
>> +			drm_atomic_get_new_plane_state(state, crtc->primary);
>> +	struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
>>  	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
>> +	bool disabling_plane;
>>  	int idx;
>>
>>  	if (drm_atomic_crtc_needs_modeset(new_crtc_state) ||
>> @@ -216,13 +223,40 @@ dc_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state)
>>
>>  	enable_irq(dc_crtc->irq_ed_cont_shdload);
>>
>> -	/* flush plane update out to display */
>> -	dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
>> +	disabling_plane = drm_atomic_plane_disabling(old_plane_state,
>> +						     new_plane_state);
>> +
>> +	if (disabling_plane) {
>> +		unsigned long flags;
>> +
>> +		dc_crtc_dbg(crtc, "disabling plane\n");
>> +
>> +		/*
>> +		 * Don't relinquish CPU until DPRC REPEAT_EN is disabled and
>> +		 * sync is triggered.
>> +		 */
>> +		local_irq_save(flags);
>> +		preempt_disable();
>> +
>> +		DC_CRTC_WAIT_FOR_FRAMEGEN_FRAME_INDEX_MOVING(dc_crtc->fg);
>> +		dc_dprc_disable_repeat_en(dc_plane->fu->dprc);
>> +		/* flush plane update out to display */
>> +		dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
>> +
>> +		local_irq_restore(flags);
>> +		preempt_enable();
> 
> preempt_enable();
> local_irq_restore(flags);

Like I replied to your comment on patch 8, see
__raw_spin_lock_irqsave() and __raw_spin_unlock_irqrestore().

> 
> look symmetry() with enter this sections.

Sorry, I don't understand your point here.
Function symmetry()?  Where is it defined?

> 
>> +	} else {
>> +		/* flush plane update out to display */
>> +		dc_ed_pec_sync_trigger(dc_crtc->ed_cont);
>> +	}
>>
>>  	DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(ed_cont_shdload_done);
>>
>>  	disable_irq(dc_crtc->irq_ed_cont_shdload);
>>
>> +	if (disabling_plane)
>> +		dc_dprc_disable(dc_plane->fu->dprc);
>> +
>>  	DC_CRTC_CHECK_FRAMEGEN_FIFO(dc_crtc->fg);
>>
>>  	drm_dev_exit(idx);
>> @@ -320,14 +354,33 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
>>  	dc_crtc_queue_state_event(new_crtc_state);
>>  }
>>
> 
> ...
> 
>> +	struct drm_crtc_state *old_crtc_state =
>> +				drm_atomic_get_old_crtc_state(state, crtc);
>>  	struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
>> +	struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
>>  	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
>>  	int idx, ret;
>>
>>  	if (!drm_dev_enter(crtc->dev, &idx))
>>  		goto out;
>>
>> -	__dc_crtc_disable_fg(crtc);
>> +	enable_irq(dc_crtc->irq_dec_seqcomplete);
>> +
>> +	if (old_crtc_state->plane_mask)
>> +		__dc_crtc_disable_fg_along_with_dprc_repeat_en(crtc);
>> +	else
>> +		dc_fg_disable(dc_crtc->fg);
>> +
>> +	DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
>> +	disable_irq(dc_crtc->irq_dec_seqcomplete);
> 
> Are you sure irq is disabled when call this function?

Same here, I don't get your point/question.

If you mean IRQ dc_crtc->irq_dec_seqcomplete, then it's disabled after
calling this function.

> 
> Frank
>> +
>> +	if (old_crtc_state->plane_mask)
>> +		dc_dprc_disable(dc_plane->fu->dprc);
>> +
>>  	dc_fg_disable_clock(dc_crtc->fg);
>>
>>  	/* request pixel engine power-off as plane is off too */
>> @@ -373,7 +441,10 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
>>  void dc_crtc_disable_at_boot(struct drm_crtc *crtc)
>>  {
>>  	struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
>> +	struct dc_plane *dc_plane = to_dc_plane(crtc->primary);
>>  	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
>> +	enum dc_link_id ed_src, lb_sec;
>> +	bool disable_dprc = false;
>>  	int ret;
>>
>>  	ret = pm_runtime_resume_and_get(dc_crtc->de->dev);
> ...
> 
>>  	fu_ops->set_src_buf_dimensions(fu, DC_FETCHUNIT_FRAC0, src_w, src_h);
>>  	fu_ops->set_fmt(fu, DC_FETCHUNIT_FRAC0, fb->format);
>>  	fu_ops->set_framedimensions(fu, src_w, src_h);
>> @@ -161,6 +190,9 @@ dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state)
>>
>>  	dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu));
>>
>> +	dc_dprc_configure(fu->dprc, new_state->crtc->index, src_w, src_h,
>> +			  fb->pitches[0], fb->format, baseaddr, prefetch_start);
>> +
>>  	dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf));
>>  	dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu));
>>  	dc_lb_mode(lb, LB_BLEND);
>>
>> --
>> 2.34.1
>>


-- 
Regards,
Liu Ying

Powered by blists - more mailing lists