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

[Biju Das]

Hi Maxime Ripard,

> -----Original Message-----
> From: Maxime Ripard <mripard at kernel.org>
> Sent: Friday, December 15, 2023 10:24 AM
> To: Biju Das <biju.das.jz at 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 at kernel.org>
> > > Sent: Wednesday, December 13, 2023 3:47 PM
> > > To: Biju Das <biju.das.jz at 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);
}

Cheers,
Biju