Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Sent: Wednesday, January 10, 2024 7:39 PM > Subject: Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support > > Hi Biju, > > On Thu, Jan 04, 2024 at 02:17:39PM +0000, Biju Das wrote: > > On Friday, December 15, 2023 2:56 PM, Biju Das wrote: > > > On Friday, December 15, 2023 2:18 PM, Maxime Ripard wrote: > > > > On Fri, Dec 15, 2023 at 01:52:28PM +0000, Biju Das wrote: > > > > > > > > > > > +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### > > > > > > > > Sorry, my question was why aren't you unmasking and masking them > > > > in the enable/disable_vblank hooks of the CRTC. > > > > > > I have n't tried that. Will try and provide feedback. > > > > > > Currently the IRQ source belongs to VSPD in media subsystem. > > > So I need to export an API though vsp1_drm and test it. > > > > + linux-media > > > > Laurent, are you ok with the below RZ/G2L specific patch[1] for > > enabling/disabling frame end interrupt in VSP driver? > > Note: > > I need to add a quirk for handling this only for RZ/G2L family as > > other SoCs have Vblank specific interrupt available in DU. > > The DU driver on Gen3 handles vblank exactly as in your patch. What's the > problem with that ? There is no issue. Maxime Ripard is checking whether is it possible to mask/unmask Interrupt associated with vblank reporting during vblank{enable, disable}? That is the reason I produced the below patch[1] for suggestions. > > > [1] > > > > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c > > b/drivers/media/platform/renesas/vsp1/vsp1_drm.c > > index 9b087bd8df7d..39347c16bb27 100644 > > --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c > > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c > > @@ -936,6 +936,14 @@ void vsp1_du_unmap_sg(struct device *dev, struct > > sg_table *sgt) } EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg); > > > > +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask) > > +{ > > + struct vsp1_device *vsp1 = dev_get_drvdata(dev); > > + > > + vsp1_write(vsp1, VI6_WPF_IRQ_ENB(0), mask ? 0 : > > + VI6_WPF_IRQ_ENB_DFEE); > > That will break everything. As soon as you turn of vblank reporting, the > VSP will stop processing frames and the display will freeze. OK. I am not able to reproduce this issue on RZ/G2L with limited testing compared to R-Car. As per your suggestion, I will drop this change. I hope it is ok for everyone. Cheers, Biju > > > +} > > +EXPORT_SYMBOL_GPL(vsp1_du_mask_frame_end_interrupt); > > + > > /* -------------------------------------------------------------------- > --------- > > * Initialization > > */ > > diff --git a/include/media/vsp1.h b/include/media/vsp1.h index > > 48f4a5023d81..ccac48a6bdd2 100644 > > --- a/include/media/vsp1.h > > +++ b/include/media/vsp1.h > > @@ -117,4 +117,6 @@ void vsp1_du_atomic_flush(struct device *dev, > > unsigned int pipe_index, int vsp1_du_map_sg(struct device *dev, > > struct sg_table *sgt); void vsp1_du_unmap_sg(struct device *dev, > > struct sg_table *sgt); > > > > +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask); > > + > > #endif /* __MEDIA_VSP1_H__ */ > > -- > Regards, > > Laurent Pinchart