Re: [RFC] Plane color pipeline KMS uAPI

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, 8 May 2023 at 10:58, Simon Ser <contact@emersion.fr> wrote:
>
> On Friday, May 5th, 2023 at 21:53, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Fri, May 05, 2023 at 04:06:26PM +0000, Simon Ser wrote:
> > > On Friday, May 5th, 2023 at 17:28, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > Ok no comments from me on the actual color operations and semantics of all
> > > > that, because I have simply nothing to bring to that except confusion :-)
> > > >
> > > > Some higher level thoughts instead:
> > > >
> > > > - I really like that we just go with graph nodes here. I think that was
> > > >   bound to happen sooner or later with kms (we almost got there with
> > > >   writeback, and with hindsight maybe should have).
> > >
> > > I'd really rather not do graphs here. We only need linked lists as Sebastian
> > > said. Graphs would significantly add more complexity to this proposal, and
> > > I don't think that's a good idea unless there is a strong use-case.
> >
> > You have a graph, because a graph is just nodes + links. I did _not_
> > propose a full generic graph structure, the link pointer would be in the
> > class/type specific structure only. Like how we have the plane->crtc or
> > connector->crtc links already like that (which already _is_ is full blown
> > graph).
>
> I really don't get why a pointer in a struct makes plane->crtc a full-blown
> graph. There is only a single parent-child link. A plane has a reference to a
> CRTC, and nothing more.
>
> You could say that anything is a graph. Yes, even an isolated struct somewhere
> is a graph: one with a single node and no link. But I don't follow what's the
> point of explaining everything with a graph when we only need a much simpler
> subset of the concept of graphs?
>
> Putting the graph thing aside, what are you suggesting exactly from a concrete
> uAPI point-of-view? Introducing a new struct type? Would it be a colorop
> specific struct, or a more generic one? What would be the fields? Why do you
> think that's necessary and better than the current proposal?
>
> My understanding so far is that you're suggesting introducing something like
> this at the uAPI level:
>
>     struct drm_mode_node {
>         uint32_t id;
>
>         uint32_t children_count;
>         uint32_t *children; // list of child object IDs
>     };

Already too much I think

struct drm_mode_node {
    struct drm_mode_object base;
    struct drm_private_obj atomic_base;
    enum drm_mode_node_enum type;
};

The actual graph links would be in the specific type's state
structure, like they are for everything else. And the limits would be
on the property type, we probably need a new DRM_MODE_PROP_OBJECT_ENUM
to make the new limitations work correctly, since the current
DRM_MODE_PROP_OBJECT only limits to a specific type of object, not an
explicit list of drm_mode_object.id.

You might not even need a node subclass for the state stuff, that
would directly be a drm_color_op_state that only embeds
drm_private_state.

Another uapi difference is that the new kms objects would be of type
DRM_MODE_OBJECT_NODE, and would always have a "class" property.

> I don't think this is a good idea for multiple reasons. First, this is
> overkill: we don't need this complexity, and this complexity will make it more
> difficult to reason about the color pipeline. This is a premature abstraction,
> one we don't need right now, and one I heaven't heard a potential future
> use-case for. Sure, one can kill an ant with a sledgehammer if they'd like, but
> that's not the right tool for the job.
>
> Second, this will make user-space miserable. User-space already has a tricky
> task to achieve to translate its abstract descriptive color pipeline to our
> proposed simple list of color operations. If we expose a full-blown graph, then
> the user-space logic will need to handle arbitrary graphs. This will have a
> significant cost (on implementation and testing), which we will be paying in
> terms of time spent and in terms of bugs.

The color op pipeline would still be linear. I did not ask for a non-linear one.

> Last, this kind of generic "node" struct is at odds with existing KMS object
> types. So far, KMS objects are concrete like CRTC, connector, plane, etc.
> "Node" is abstract. This is inconsistent.

Yeah I think I think we should change that. That's essentially the
full extend of my proposal. The classes + possible_foo mask approach
just always felt rather brittle to me (and there's plenty of userspace
out there to prove that's the case), going more explicit with the
links with enumerated combos feels better. Plus it should allow
building a bit cleaner interfaces for drivers to construct the correct
graphs, because drivers _also_ rather consistently got the entire
possible_foo mask business wrong.

> Please let me know whether the above is what you have in mind. If not, please
> explain what exactly you mean by "graphs" in terms of uAPI, and please explain
> why we need it and what real-world use-cases it would solve.

_Way_ too much graph compared to what I'm proposing :-)

Also I guess what's not clear: This is 100% a bikeshed with no impact
on the actual color handling pipeline in any semantic way. At all. If
you think it is, it's not what I mean.

I guess the misunderstanding started out with me asking for "graph
nodes" and you thinking "full blown graph structure with mandatory
flexibility". I really only wanted to bring up the slightly more
generic "node" think, and you can totally think of them as "list
nodes" in the context of color op pipelines.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch