On 5/8/23 05:18, Daniel Vetter wrote:
> On Mon, 8 May 2023 at 10:58, Simon Ser <contact@xxxxxxxxxxx> wrote:
>>
>> On Friday, May 5th, 2023 at 21:53, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxx> 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;
> };
>
This would be about as much as we would want for a 'node' struct, for
reasons that others already outlined. In short, a good API for a color
pipeline needs to do a good job communicating the constraints. Hence the
"next" pointer needs to be live in a colorop struct, whether it's a
drm_private_obj or its own thing.
I'm not quite seeing much benefits with a drm_mode_node other than being
able to have a GET_NODE IOCTL instead of a GET_COLOROP, the former being
able to be re-used for future scenarios that might need a "node." I feel
this adds a layer of confusion to the API.
Harry
> 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
