Hi Laurent,

On Thu, 2019-12-05 at 08:39:09 -0800, Vinod Koul wrote:

Hi Laurent,

On 05-12-19, 17:04, Laurent Pinchart wrote:

quoted

quoted

quoted

+/*
+ * DPDMA descriptor placement
+ * --------------------------
+ * DPDMA descritpor life time is described with following placements:
+ *
+ * allocated_desc -> submitted_desc -> pending_desc -> active_desc -> done_list
+ *
+ * Transition is triggered as following:
+ *
+ * -> allocated_desc : a descriptor allocation
+ * allocated_desc -> submitted_desc: a descriptor submission
+ * submitted_desc -> pending_desc: request to issue pending a descriptor
+ * pending_desc -> active_desc: VSYNC intr when a desc is scheduled to DPDMA
+ * active_desc -> done_list: VSYNC intr when DPDMA switches to a new desc

Well this tells me driver is not using vchan infrastructure, the
drivers/dma/virt-dma.c is common infra which does pretty decent list
management and drivers do not need to open code this.

Please convert the driver to use virt-dma

As noted in the cover letter,

"There is one review comment that is still pending: switching to
virt-dma. I started investigating this, and it quickly appeared that
this would result in an almost complete rewrite of the driver's logic.
While the end result may be better, I wonder if this is worth it, given
that the DPDMA is tied to the DisplayPort subsystem and can't be used
with other DMA slaves. The DPDMA is thus used with very specific usage
patterns, which don't need the genericity of descriptor handling
provided by virt-dma. Vinod, what's your opinion on this ? Is virt-dma
usage a blocker to merge this driver, could we switch to it later, or is
it just overkill in this case ?"

I'd like to ask an additional question : is the dmaengine API the best
solution for this ? The DPDMA is a separate IP core, but it is tied with
the DP subsystem. I'm tempted to just fold it in the display driver. The
only reason why I'm hesitant on this is that the DPDMA also handles
audio channels, that are also part of the DP subsystem, but that could
be handled by a separate ALSA driver. Still, handling display, audio and
DMA in drivers that we pretend are independent and generic would be a
bit of a lie.

Yeah if it is _only_ going to be used in display and no other client
using it, then I really do not see any advantage of this being a
dmaengine driver. That is pretty much we have been telling folks over
the years.

In the development cycles, the IP blocks came in pieces. The DP tx driver
was developed first as one driver, with dmaengine driver other than DPDMA.
Then the ZynqMP block was added along with this DPDMA driver. Hence,
the reverse is possible, meaning some can decide to take a part of it
and harden with other blocks in next generation SoC. So there was and will
be benefit of keeping drivers modular at block level in my opinion, and
I'm not sure if it needs to put in a monolithic format, when it's already
modular.

Btw since this is xilinx and I guess everything is an IP how difficult
would it be to put this on a non display core :)

If you decide to use dmaengine I would prefer it use virt-dma that mean
rewrite yes but helps you term

I made changes using vchan[1], but it was a while ago. So it might have
been outdated, and details are vague in my head. Not sure if it was at
fully functional stage. Still, just in case it may be helpful.

[1] https://github.com/starhwk/linux-xlnx/commits/hyunk/upstreaming?after=0b0002113e7381d8a5f3119d064676af4d0953f4+34

Thanks,
-hyun