Re: [PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.

[Rob Herring]

On Wed, Aug 26, 2015 at 7:09 AM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Wed, Aug 26, 2015 at 01:52:29PM +0200, Daniel Vetter wrote:
>> On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote:
>> > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@xxxxxxxxx> wrote:
>> > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@xxxxxxxxxx> wrote:
>> > >> Stephen Warren <swarren@xxxxxxxxxxxxx> writes:
>> > >>
>> > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote:
>> > >>>> Signed-off-by: Eric Anholt <eric@xxxxxxxxxx>
>> > >>>
>> > >>> This one definitely needs a patch description, since someone might not
>> > >>> know what a VC4 is, and "git log" won't show the text from the binding
>> > >>> doc itself. I'd suggest adding the initial paragraph of the binding doc
>> > >>> as the patch description, or more.
>> > >>>
>> > >>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt
>> > >>
>> > >>>> +- hvss:             List of references to HVS video scalers
>> > >>>> +- encoders: List of references to output encoders (HDMI, SDTV)
>> > >>>
>> > >>> Would it make sense to make all those nodes child node of the vc4
>> > >>> object. That way, there's no need to have these lists of objects; they
>> > >>> can be automatically built up as the DT is enumerated. I know that e.g.
>> > >>> the NVIDIA Tegra host1x binding works this way, and I think it may have
>> > >>> been inspired by other similar cases.
>> > >>
>> > >> I've looked at tegra, and the component system used by msm appears to be
>> > >> nicer than it.  To follow tegra's model, it looks like I need to build
>> > >> this extra bus thing corresponding to host1x that is effectively the
>> > >> drivers/base/component.c code, so that I can get at vc4's structure from
>> > >> the component drivers.
>> > >>
>> > >>> Of course, this is only appropriate if the HW modules really are
>> > >>> logically children of the VC4 HW module. Perhaps they aren't. If they
>> > >>> aren't though, I wonder what this "vc4" module actually represents in HW?
>> > >>
>> > >> It's the subsystem, same as we use a subsystem node for msm, sti,
>> > >> rockchip, imx, and exynos.  This appears to be the common model of how
>> > >> the collection of graphics-related components is represented in the DT.
>> > >
>> > > I think most of these bindings are wrong. They are grouped together
>> > > because that is what DRM wants not because that reflects the h/w. So
>> > > convince me this is one block, not that it is what other people do.
>> >
>> > I think, when it comes to more complex driver subsystems (like drm in
>> > particular) we have a bit of mismatch between how things look from the
>> > "pure hw ignoring sw" perspective, and the "how sw and in particular
>> > userspace expects things" perspective.  Maybe it is less a problem in
>> > other subsystems, where bindings map to things that are only visible
>> > in the kernel, or well defined devices like uart or sata controller.
>> > But when given the choice, I'm going to err on the side of not
>> > confusing userspace and the large software stack that sits above
>> > drm/kms, over dt purity.
>> >
>> > Maybe it would be nice to have a sort of dt overlay that adds the bits
>> > needed to tie together hw blocks that should be assembled into a
>> > single logical device for linux and userspace (but maybe not some
>> > other hypothetical operating system).  But so far that doesn't exist.
>> > All we have is a hammer (devicetree), everything looks like a nail.
>> > End result is we end up adding some things in the bindings which
>> > aren't purely about the hw.  Until someone invents a screwdriver, I'm
>> > not sure what else to do.  In the end, other hypothetical OS is free
>> > to ignore those extra fields in the bindings if it doesn't need them.
>> > So meh?
>>
>> I thought we agreed a while back that these kind of "pull everything for
>> the logical device together" dt nodes which just have piles of phandles
>> are totally accepted? At least that's the point behind the component
>> helpers, and Eric even suggested to create dt-specific component helpers
>> to cut down a bit on the usual boilerplate. dt maintainers are also fine
>> with this approach afaik. From my understanding tegra with the host1x bus
>> really is the odd one out and not the norm.
>
> I agree that in many aspects Tegra is somewhat special. But the same
> principles that the host1x infrastructure uses could be implemented in a
> similar way for other DRM drivers. You can easily collect information
> about subdevices by walking the device tree and matching on known
> compatible strings. And you can also instantiate the top-level device
> from driver code rather than have it in DT. It should still be possible
> to make this work without an artificial device node in DT. The component
> and master infrastructure is largely orthogonal to that, and as far as I
> remember the only blocker is the need for a top-level device. I wonder
> if perhaps this could be made to work by binding the master to the top-
> level SoC device.
>
> Obviously adding the node in DT is easier, but to my knowledge easy has
> never been a good excuse for mangling DT. Perhaps that's different these
> days...

I agree we should avoid the virtual node if possible. It is certainly
possible as I started out with one and removed it. At least in my
case, it essentially required the drm_device and crtc to be a single
driver rather than 2 components. However, I'm more concerned that we
are consistent from platform to platform where it makes sense than
whether we have a somewhat questionable node or not.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/