RE: [RFC v3 5/9] cec: add new driver for cec support.

[Kamil Debski <k.debski@samsung.com>]

Hi Hans,

From: linux-media-owner@vger.kernel.org [mailto:linux-media-
owner@vger.kernel.org] On Behalf Of Hans Verkuil
Sent: Friday, April 17, 2015 2:17 PM
> 
> On 04/13/2015 03:19 PM, Kamil Debski wrote:
> > Hi Hans,
> >
> > Thank you so much for the review.
> >
> > From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> > owner@vger.kernel.org] On Behalf Of Hans Verkuil
> > Sent: Friday, March 20, 2015 7:08 PM
> >>
> 
> <snip>
> 
> >>> +In order for a CEC adapter to be configured it needs a physical
> >> address.
> >>> +This is normally assigned by the driver. It is either 0.0.0.0 for
> a
> >> TV (aka
> >>> +video receiver) or it is derived from the EDID that the source
> >> received
> >>> +from the sink. This is normally set by the driver before enabling
> >> the CEC
> >>> +adapter, or it is set from userspace in the case of CEC USB
> dongles
> >> (although
> >>> +embedded systems might also want to set this manually).
> >>
> >> I would actually expect that USB dongles read out the EDID from the
> >> source.
> >> I might be wrong, though.
> >
> > EDID is communicated to the device by the TV on a different bus than
> > CEC, it is DDC. It is possible that the dongle also reads DDC
> messages.
> > My initial understanding was that a CEC USB dongle handles only CEC
> > messages and is passing through all other signals, such as DDC.
> 
> I checked against the libcec code (see link here: http://libcec.pulse-
> eight.com/) for my usb-cec dongle and it turns out the library reads
> out the edid from the monitor using xrandr (I think, see
> src/libcec/platform/X11/randr-edid.cpp) in order to get the physical
> address. So it is not using the dongle itself for that. Makes sense.
> 
> >
> >>> +
> >>> +After enabling the CEC adapter it has to be configured. The CEC
> >> adapter has
> >>> +to be informed for which CEC device types a logical address has to
> >> be found.
> >>
> >> I would say: 'a free (unused) logical address'.
> >>
> >>> +The CEC framework will attempt to find such logical addresses. If
> >> none are
> >>
> >> And here: 'find and claim'
> >>
> >>> +found, then it will fall back to logical address Unregistered (15).
> >>
> >> You probably need to add some documentation regarding
> >> cec_claim_log_addrs()
> >> and how drivers can use it. Also, while logical addresses are being
> >> claimed, are drivers or userspace allowed to transmit/receive other
> >> messages? Or just stall until this is finished?
> >
> > When sending a message the user space is free to set any source and
> > destination address. Hence, I see no need to wait until the logical
> > address is claimed.
> >
> > If the user space is not waiting until the address and is sending
> > messages, then I guess it is done with full responsibility on the
> user
> > space.
> >
> > Regarding receiving, I guess it should be possible to receive
> > broadcast messages.
> >
> > What do you think?
> 
> Fair enough, it just needs to be documented.

Ok, will do.

> 
> <snip>
> 
> >>> +Promiscuous mode
> >>> +----------------
> >>> +
> >>> +The promiscuous mode enables the userspace applications to read
> all
> >>> +messages on the CEC bus. This is similar to the promiscuous mode
> in
> >>> +network devices. In the normal mode messages not directed to the
> >> device
> >>> +(differentiated by the logical address of the CEC device) are not
> >>> +forwarded to the userspace. Same rule applies to the messages
> >> contailning
> >>> +remote control key codes. When promiscuous mode is enabled all
> >> messages
> >>> +can be read by userspace. Processing of the messages is still done,
> >> thus
> >>> +key codes will be both interpreted by the framework and available
> >>> +as
> >> an
> >>> +input device, but also raw messages containing these codes are
> sent
> >> to
> >>> +the userspace.
> >>
> >> Will messages that are processed by the driver or cec framework also
> >> be relayed to userspace in promiscuous mode? Will userspace be able
> >> to tell that it has been processed already?
> >
> > All messages will be relayed to the user space and no there is no
> > possibility to check whether the message was processed by the kernel
> > already.
> 
> Should we add that? To be honest, I'm not sure about that myself.

The promiscuous mode is useful mainly for debug reasons. I would leave it,
however it is not a deal breaker for me. It could be added at a later time.

> Once thing I notice is that there are no reserved fields at the end of
> struct cec_msg. We should add that. Same with the other structs. It
> served us well with v4l2, and we should do the same with the cec API.

This is indeed a good idea. Thanks :)

> 
> Another upcoming problem is the use of struct timespec: this will have
> to change in the near future to one that is year 2038-safe.
> Unfortunately, there is no public 'struct timespec64' type yet. This
> mailinglist might provide answers w.r.t. the precise plans with
> timespec:
> http://lwn.net/Articles/640284/
> 
> Also, we don't have 32-bit compat code for CEC. I wonder if it is a
> good idea to improve the layout of the structs to minimize 64/32-bit
> layout differences. I never paid attention to that when I made these
> structs as I always planned to do that at the end.

It's good that you mentioned this, will do.

> 
> Regards,
> 
> 	Hans

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland