Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support

From: Maxime Ripard <mripard@kernel.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Fabrizio Castro <fabrizio.castro.jz@renesas.com>,
	"biju.das.au" <biju.das.au@gmail.com>,
	Magnus Damm <magnus.damm@gmail.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>,
	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Date: Fri, 15 Dec 2023 15:23:02 +0100	[thread overview]
Message-ID: <5xmkpbymmvsqdfjxgkrjf7r4b7sgxdd7gq7qmohg5id6ljjg7z@rznod4o464iy> (raw)
In-Reply-To: <TYCPR01MB11269A92241B3469FAC06AF398693A@TYCPR01MB11269.jpnprd01.prod.outlook.com>

[-- Attachment #1: Type: text/plain, Size: 4610 bytes --]

On Fri, Dec 15, 2023 at 02:19:25PM +0000, Biju Das wrote:
> Hi Maxime Ripard,
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: Friday, December 15, 2023 1:52 PM
> > Subject: RE: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > 
> > Hi Maxime Ripard,
> > 
> > > -----Original Message-----
> > > From: Maxime Ripard <mripard@kernel.org>
> > > Sent: Friday, December 15, 2023 12:58 PM
> > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > >
> > > On Fri, Dec 15, 2023 at 11:37:07AM +0000, Biju Das wrote:
> > > > Hi Maxime Ripard,
> > > >
> > > > > -----Original Message-----
> > > > > From: Maxime Ripard <mripard@kernel.org>
> > > > > Sent: Friday, December 15, 2023 10:24 AM
> > > > > To: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > > > >
> > > > > On Thu, Dec 14, 2023 at 03:24:17PM +0000, Biju Das wrote:
> > > > > > Hi Maxime Ripard,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Maxime Ripard <mripard@kernel.org>
> > > > > > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > > > > > To: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU
> > > > > > > Support
> > > > > > >
> > > > > > > > +	 */
> > > > > > > > +	rzg2l_du_crtc_get(rcrtc);
> > > > > > >
> > > > > > > That's a bit suspicious. Have you looked at
> > > > > > > drm_atomic_helper_commit_tail_rpm() ?
> > > > > >
> > > > > > Ok, I will drop this and start using
> > > > > > drm_atomic_helper_commit_tail_rpm()
> > > > > > instead of rzg2l_du_atomic_commit_tail().
> > > > >
> > > > > It was more of a suggestion, really. I'm not sure whether it works
> > > > > for you, but it usually addresses similar issues in drivers.
> > > >
> > > > I confirm, it is working in my case, even after removing
> > > > rzg2l_du_crtc_get() and using the helper function
> > > drm_atomic_helper_commit_tail_rpm().
> > > >
> > > > >
> > > > > > >
> > > > > > > > +static int rzg2l_du_crtc_enable_vblank(struct drm_crtc *crtc)
> > {
> > > > > > > > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > +
> > > > > > > > +	rcrtc->vblank_enable = true;
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void rzg2l_du_crtc_disable_vblank(struct drm_crtc
> > > > > > > > +*crtc)
> > > {
> > > > > > > > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > +
> > > > > > > > +	rcrtc->vblank_enable = false; }
> > > > > > >
> > > > > > > You should enable / disable your interrupts here
> > > > > >
> > > > > > We don't have dedicated vblank IRQ for enabling/disabling vblank.
> > > > > >
> > > > > > vblank is handled by vspd.
> > > > > >
> > > > > > vspd is directly rendering images to display,
> > > > > > rcar_du_crtc_finish_page_flip() and drm_crtc_handle_vblank()
> > > > > > called in vspd's pageflip context.
> > > > > >
> > > > > > See rzg2l_du_vsp_complete()in rzg2l_du_vsp.c
> > > > >
> > > > > Sorry, I couldn't really get how the interrupt flow / vblank
> > > > > reporting is going to work. Could you explain it a bit more?
> > > >
> > > > We just need to handle vertical blanking in the VSP frame end handler.
> > > > See the code below.
> > > >
> > > > static void rzg2l_du_vsp_complete(void *private, unsigned int
> > > > status,
> > > > u32 crc) {
> > > > 	struct rzg2l_du_crtc *crtc = private;
> > > >
> > > > 	if (crtc->vblank_enable)
> > > > 		drm_crtc_handle_vblank(&crtc->crtc);
> > > >
> > > > 	if (status & VSP1_DU_STATUS_COMPLETE)
> > > > 		rzg2l_du_crtc_finish_page_flip(crtc);
> > > >
> > > > 	drm_crtc_add_crc_entry(&crtc->crtc, false, 0, &crc); }
> > >
> > > Then we're back to the same question :)
> > >
> > > Why can't you mask the frame end interrupt?
> > 
> > We are masking interrupts.
> > 
> > [   70.639139] #######rzg2l_du_crtc_disable_vblank#######
> > [   70.650243] #########rzg2l_du_vsp_disable ############
> > [   70.652003] ########## vsp1_wpf_stop###
> > 
> > Unmask is,
> > 
> > [ 176.354520] #######rzg2l_du_crtc_enable_vblank#######
> > [  176.354922] #########rzg2l_du_vsp_atomic_flush ############
> > [  176.355198] ########## wpf_configure_stream###
> > 
> 
> Shall I send V16 now as I am going for xmas vacation or
> Do you prefer to wait?

Waiting is fine, most of us are going to be in holidays too so you won't
get any reviews either :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]