Re: [RFC 0/1] drm/pl111: Initial drm/kms driver for pl111

On Fri, Jul 26, 2013 at 10:06 AM, Pawel Moll [off-list ref] wrote:

On Thu, 2013-07-25 at 19:21 +0100, Rob Clark wrote:

quoted

quoted

 * Only supports 640x480 mode, which is hard-coded. We intend to
   rebase on top of CDF once it is merged, which hopefully will
   handle a lot of the EDID parsing & mode setting for us (once
   Pawel's CDF patches for VExpress also land).

note that drm core already handles EDID parsing quite nicely.. I'm not
entirely sure why CDF would be needed for this?

What is the dependency on CDF?  Is there an external encoder/bridge
that could be shared between platforms?

By all means. The platform we have here at ARM - Versatile Express - has
a pretty generic idea of "multimedia bus", carrying a set of RGB pixel
data and digital audio. There are three possible source of those: an
FPGA on the motherboard and two daughterboards which can take random
combination of FPGAs or test chips. In other words: the pixel data can
be generated by anything. And some of those things can be driven by
existing fbdev drivers. Now Tom is trying to make the most modern driver
for the oldest of the things ;-)

So that's about sources. All of them are connected to yet another FPGA
which acts as a muxer (switch if you wish). And from there things are
fed to SiI9022 HDMI/DVI formatter which is pretty common in the
"embedded world". In other words: loads of other fbdev-driven display
controllers are driving them as well (googling for sii9022 reveals at
least 3 different more or less custom drivers for it). And this chip is
also a reason Tom mentioned EDID. In order to get (just access, not even
parse) it we must jump through hoops - the chip acts as a I2C master
itself and must be kindly asked to switch into a sort of I2C bridge
mode.

btw, you might want to have a look at some of the existing i2c slave
encoders, since that sounds a lot like what the si9022 is.  There is
already drivers/gpu/drm/i2c/sil164_drv.c for some other SI part.  And
tda998x is another more recently added hdmi encoder/formatter (the
edid probing code in there might serve as a reasonable source of
inspiration)

So yes, the display pipeline is exactly straight-forward...

quoted

btw, I think that having some share-able KMS objects is a good idea.
I'm not entirely sure that the directions that the current CDF
proposals are headed is necessarily the right way forward.  I'd prefer
to see small/incremental evolution of KMS (ie. add drm_bridge and
drm_panel, and refactor the existing encoder-slave).  Keeping it
inside drm means that we can evolve it more easily, and avoid layers
of glue code for no good reason.

To keep the story short - I'm a great enthusiast of CDF because it
caters for both DRM *and* fbdev. Today. I'm looking forward to the day
when the last fbdev driver is kicked off the kernel, but it doesn't look
like happening soon. At the same time I couldn't care less about naming,
so if you prefer to call it drm_panel (but keep the API abstract enough
so non-DRM drivers can use them as well) - it's fine for me :-) There
are already generic mode/timing structures and DT bidnings available
(with helpers for the interesting parties), so this doesn't seem like a
unreasonable request?

Well, if you have something complex enough to benefit from CDF, then
you probably ought to be looking at drm/kms.  If someone wants to
somehow come up with some glue to re-use shareable drm/kms components
from fbdev, well.. I guess I don't really care about what goes in
fbdev.  My main concern is that we have more than enough work to do,
and have more than enough complexity.  If we end up w/ bunch of glue
to tie things in to drm properties, how drm helpers sequence things
during modeset, locking, etc.. I just don't see that turning out well.
 And also, just on the logistical side, having something shared across
subsystems makes changes more of a pain during the merge window.  Not
to mention backports to stable kernels, distro kernels, etc.  It all
just seems like it will be a lot of unnecessary headache.

Cheers!

Pawel