Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Laurent Pinchart]

Hi GVRao,

Sorry for the delayed reply.

On Tue, Jun 09, 2020 at 02:48:25AM +0000, Venkateshwar Rao Gannavarapu wrote:
> Hi Laurent,
> 
> Thanks for the review. 
> Please see my comments about D-PHY and bridge driver implementation.
> 
> On Sunday, June 7, 2020 7:55 AM, Laurent Pinchart wrote:
> > On Sun, May 31, 2020 at 05:41:50PM +0000, Venkateshwar Rao Gannavarapu wrote:
> >> On Sunday, May 24, 2020 8:38 AM, Laurent Pinchart wrote:
> >>> On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> >>>> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> >>>>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> >>>>> data from AXI-4 stream interface.
> >>>>>
> >>>>> It supports upto 4 lanes, optional register interface for the
> >>>>> DPHY,
> >>>>
> >>>> I don't see the register interface for dphy support.
> >>>
> >>> I think the D-PHY should be supported through a PHY driver, as it
> >>> seems to be shared between different subsystems.
> >>
> >> IP has the provision to read DPHY register for debug purpose only.
> >> No programming of DPHY is required in subsystem.
> >
> > Do you know if this is the same D-PHY as used in the CSI2-RX subsystem ?
>  
> Same D-PHY core has been used in MIPI CSI2 RXSS, but with different configuration.
>
> >>>>> multiple RGB color formats, command mode and video mode.
> >>>>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> >>>>> This driver also helps to communicate with its panel using panel
> >>>>> framework.
> >>>>>
> >>>>> Signed-off-by: Venkateshwar Rao Gannavarapu
> >>>>> <venkateshwar.rao.gannavarapu@xxxxxxxxxx>
> >>>>> ---
> >>>>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> >>>>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> >>>>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> >>>
> >>> Daniel Vetter has recently expressed his opiion that bridge drivers
> >>> should go to drivers/gpu/drm/bridge/. It would then be
> >>> drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> 
> The DSI-TX subsystem IP block is not a bridge driver.
> The DSI-TX subsystem IP block itself contains all the drm encoder/connector
> functionality and itâs the last node in display pipe line.

The DSI-TX subsystem IP block is indeed an encoder from a hardware point
of view, but it's not necessarily the last block in the display
pipeline. While the output of the IP core goes of the the SoC, tt would
be entirely feasible to connect it to a DP to HDMI bridge on the board,
such as the ANX7737 ([1]) for instance. This is why we're pushing for
all encoder (from a hardware point of view) drivers to be implemented as
DRM bridge, in order to make them usable in different display pipelines,
without hardcoding the assumption they will be the last encoder in the
pipeline.

> I didn't see any hard
> requirement to implement it into bridge driver and I see many DSI drivers are
> implemented as encoder drivers.
> Xilinx PL DRM encoder drivers are implemented in modular approach so that
> they can work with any CRTC driver which handles the DMA calls.
> So, at this stage we want to upstream as encoder driver only.
>
> >>>>>  3 files changed, 768 insertions(+)  create mode 100644
> >>>>> drivers/gpu/drm/xlnx/xlnx_dsi.c

[1] https://www.analogix.com/en/products/convertersbridges/anx7737

-- 
Regards,

Laurent Pinchart