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

[Venkateshwar Rao Gannavarapu]

Hi Laurent,

Thanks for the comment.

>-----Original Message-----
>From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>Sent: Wednesday, June 17, 2020 3:18 AM
>To: Venkateshwar Rao Gannavarapu <VGANNAVA@xxxxxxxxxx>
>Cc: Hyun Kwon <hyunk@xxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
>airlied@xxxxxxxx; daniel@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sandip Kothari
><sandipk@xxxxxxxxxx>; Vishal Sagar <vsagar@xxxxxxxxxx>
>Subject: Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem
>
>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.

Thanks for the details.
I can understand it as SoC requirement where crtc is fixed, but as a FPGA product
encoder driver should work with any of crtc driver.  And I still not see bridge implementation as hard requirement.
Could you please explain what problem we face, if implemented as encoder driver.
>
>> 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

Regards,
GVRao