Re: [RFC][PATCH 2/2] drm/arm: Add support for Mali Display Processors

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 12, 2016 at 06:13:49PM +0100, Liviu Dudau wrote:
> On Tue, Apr 12, 2016 at 05:47:57PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 01, 2016 at 05:21:52PM +0100, Liviu Dudau wrote:
> > > +static int malidp_enable_vblank(struct drm_device *drm, unsigned int crtc)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static void malidp_disable_vblank(struct drm_device *drm, unsigned int pipe)
> > > +{
> > > +}
> > 
> > Might be worth it to create a patch for drm_irq.c to make
> > enable/disable_vblank functions optional. Otoh does your chip really keep
> > on generating vblank irqs all the time, with no way to shut it up? That
> > would be terrible for power consumption ... Especially since you have no
> > hw counter either.
> 
> Initially I had code here that was turning off the vblank irq, then I've read
> the comment in drmP.h that the routine should be a no-op when hardware counters
> are missing, hence this version. As for the display processor: it will generate
> an interrupt for every finished scanout cycle, but it has support for variable
> vsync. Interrupt can be disabled, but I've read in the drmP.h that it is required
> for timestamping support when one doesn't have hw counters.
> 
> I'm OK with fixing drm_irq.c to not require enable/disable_vblank but then the
> comments in drmP.h will also have to change?

Nah, you bring up a good point actually - you really can't disable vblank
if there's no hw counter. At least not right now. I think dummy functions
in drm_irq.c like drm_vblank_get/set_no_hw_counter to make this clear
would be nice. Or maybe just a comment here.

The other option would be to finally fake this using high-precision
timestamps, since a lot of mobile hw seems to have forgotten to add a
proper vblank counter. But that has issues (it can drift), and probably
better done separately.

> > > +static void malidp_de_plane_disable(struct drm_plane *plane,
> > > +				    struct drm_plane_state *state)
> > > +{
> > > +	struct malidp_plane *mp = to_malidp_plane(plane);
> > > +
> > > +	/* ToDo: figure out the attached framebuffer lifecycle */
> > 
> > You don't need to figure this out, atomic helpers will take care of the fb
> > for you.
> 
> It is more in line with un/pinning the framebuffer and making sure that the
> framebuffer has been scanned out before unref-ing it.

That should be taken care of by the vblank wait the helpers do for you.
Again happans all automatically (except you need to keep that in mind for
the async work).

> Thanks again for finding time to review the code.

No problem.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch