[U-Boot] [PATCH 3/5] rockchip: video: rk3399: enable HDMI output (from the rk_vop) for the RK3399

[Simon Glass]

Hi Philipp,

On 30 April 2017 at 06:31, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
> Hi Simon,
>
> On 30 Apr 2017, at 05:49, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Philipp,
>
> On 28 April 2017 at 09:53, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>
> This commit enables RK3399 support for HDMI through the following
> changes:
> - adds a driverdata structure to mirror some subtle version
>  differences between the RK3399 VOPs and those in the RK3288
>  (e.g. the pin-polarity configuration)
> - configures the VOP to output 32bpp for HDMI
> - handles whether a VOP can output 10BIT data or not (i.e. RGBaaa vs.
> RGB888)
>  using the driverdata structure
>
> And as we touch this file anyway, we also increase the size of the
> framebuffer to a slightly overzealous 4K2K at 32bpp.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> ---
>
> arch/arm/include/asm/arch-rockchip/vop_rk3288.h |  11 ++
> drivers/video/rockchip/rk_vop.c                 | 180
> ++++++++++++++++++++----
> 2 files changed, 161 insertions(+), 30 deletions(-)
>
>
> I'd like to somehow keep the SoC-specific code out of this driver.
> You've done a nice job separating it out, but I wonder if we can go a
> bit further.
>
> I'm thinking of perhaps having two vop drivers, one for rk3288 and one
> for rk3399. They can share most of the operations (e.g. bind()) which
> can stay in the existing rk_vop.c file. For probe() you can rename the
> existing probe() function to something like rockchip_vop_probe(), and
> pass it the device and a rkvop_driverdata *.
>
> Does that make sense? Then when we add more chips we'll have a small
> extra file with the SoC-specific functionality.
>
>
> I had considered (for any future work on this) to go into the opposite
> direction
> and to port drivers/gpu/drm/rockchip/rockchip_vop_reg.[ch] from Linux (and
> possibly even drivers/gpu/drm/rockchip/rockchip_drm_vop.c … those treat
> the various variants (3288,3328,  3366, 3368, 3399-lit, 3399-big) as a
> single
> hardware block that has a different version and sometimes has the capability
> of supporting optional features (e.g. 10bit RGB output)
>
> Instead of splitting things up it would thus put them into a single driver
> and
> then use driverdata to model all the differences.  And to make things easier
> for the long-term maintenance (and avoid mistakes in the first place), I
> would
> rather try and reuse (large parts of) rockchip_vop_reg.[ch] verbatim in
> U-Boot.
>
> Keeping things aligned with the Linux driver would be my preferred long-term
> solution, if I can get a consensus for it…

I do understand this desire, but I worry that we will end up with
monolithic drivers where adding video support will cost a lot in code
space. Linux has essentially unlimited code space so the trade-offs
are different.

If you do end up porting these things more directly from Linux then
I'd like to see how things can be broken up so that we can scale the
code size functionality a bit.

You are effectively creating a new API layer within the driver to
handle hardware differences. I really like that approach, but I feel
that putting all the implementations in this one file is unwieldy.

Having said that, from the example you gave I doubt there is a very
large difference in code size?

Regards,
Simon