Re: [PATCH v2 00/26] Improve DVB documentation and reduce its gap

[Honza PetrouÅ]

2017-09-04 11:06 GMT+02:00 Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>:
> Em Mon, 4 Sep 2017 09:12:49 +0200
> Honza PetrouÅ <jpetrous@xxxxxxxxx> escreveu:
>
>> 2017-09-04 2:54 GMT+02:00 Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>:
>> > Em Sun, 3 Sep 2017 22:05:23 +0200
>> > Honza PetrouÅ <jpetrous@xxxxxxxxx> escreveu:
>> >
>> >> 1) #define CA_SET_DESCR      _IOW('o', 134, ca_descr_t)
>> >> ============================================
>> >>
>> >> CA_SET_DESCR is used for feeding descrambler device
>> >> with correct keys (called here "control words") what
>> >> allows to get services unscrambled.
>> >>
>> >> The best docu is:
>> >>
>> >> "Digital Video Broadcasting (DVB);
>> >> Support for use of the DVB Scrambling Algorithm version 3
>> >> within digital broadcasting systems"
>> >>
>> >> Defined as DVB Document A125 and publicly
>> >> available here:
>> >>
>> >> https://www.dvb.org/resources/public/standards/a125_dvb-csa3.pdf
>> >>
>> >>
>> >> typedef struct ca_descr {
>> >>         unsigned int index;
>> >>         unsigned int parity;    /* 0 == even, 1 == odd */
>> >>         unsigned char cw[8];
>> >> } ca_descr_t;
>> >>
>> >> The 'index' is adress of the descrambler instance, as there exist
>> >> limited number of them (retieved by CA_GET_DESCR_INFO).
>> >
>> > Thanks for the info. If I understood well, the enclosed patch should
>> > be documenting it.
>> >
>> >
>> > Thanks,
>> > Mauro
>> >
>> > [PATCH] media: ca docs: document CA_SET_DESCR ioctl and structs
>> >
>> > The av7110 driver uses CA_SET_DESCR to store the descrambler
>> > control words at the CA descrambler slots.
>> >
>> > Document it.
>> >
>> > Thanks-to: Honza PetrouÅ <jpetrous@xxxxxxxxx>
>> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
>> >
>> > diff --git a/Documentation/media/uapi/dvb/ca-set-descr.rst b/Documentation/media/uapi/dvb/ca-set-descr.rst
>> > index 9c484317d55c..a6c47205ffd8 100644
>> > --- a/Documentation/media/uapi/dvb/ca-set-descr.rst
>> > +++ b/Documentation/media/uapi/dvb/ca-set-descr.rst
>> > @@ -28,22 +28,11 @@ Arguments
>> >  ``msg``
>> >    Pointer to struct :c:type:`ca_descr`.
>> >
>> > -.. c:type:: ca_descr
>> > -
>> > -.. code-block:: c
>> > -
>> > -    struct ca_descr {
>> > -       unsigned int index;
>> > -       unsigned int parity;
>> > -       unsigned char cw[8];
>> > -    };
>> > -
>> > -
>> >  Description
>> >  -----------
>> >
>> > -.. note:: This ioctl is undocumented. Documentation is welcome.
>> > -
>> > +CA_SET_DESCR is used for feeding descrambler CA slots with descrambling
>> > +keys (refered as control words).
>> >
>> >  Return Value
>> >  ------------
>> > diff --git a/include/uapi/linux/dvb/ca.h b/include/uapi/linux/dvb/ca.h
>> > index f66ed53f4dc7..a62ddf0cebcd 100644
>> > --- a/include/uapi/linux/dvb/ca.h
>> > +++ b/include/uapi/linux/dvb/ca.h
>> > @@ -109,9 +109,16 @@ struct ca_msg {
>> >         unsigned char msg[256];
>> >  };
>> >
>> > +/**
>> > + * struct ca_descr - CA descrambler control words info
>> > + *
>> > + * @index: CA Descrambler slot
>> > + * @parity: control words parity, where 0 means even and 1 means odd
>> > + * @cw: CA Descrambler control words
>> > + */
>> >  struct ca_descr {
>> >         unsigned int index;
>> > -       unsigned int parity;    /* 0 == even, 1 == odd */
>> > +       unsigned int parity;
>> >         unsigned char cw[8];
>> >  };
>> >
>> >
>>
>> Yeh, it should be that way.
>
> Good! I'll add this patch to the series.
>
>> BTW, the only issue I have in mind is how to link particular
>> descrambler with the PID
>> after your removal of the CA_SET_PID. And yes, I know that currently we have
>> no any user of such ioctl in our driver base :)
>
> Well, I don't think that an ioctl like CA_SET_PID would solve it.
>
> On a generic case with is quite common nowadays on embedded hardware,
> We have K demods and M CIs (where K may be different than M).
>
> Also, You may need to route N PIDs to O descramblers.

TBH that is exactly most common use-case = most Digital TV
vendors are scrambling per-service, what requires one descrambler
for all scrambled PIDs (usually only A/V PIDs are scrambled)
for particular service. So we have to add more PIDs to one descrambler

What was possible by multiple call of CA_SET_PID (I agree that much
better would be name like CA_ADD_PID)
>
> As user switch channels, the N PIDs should be unset, and another
> set of N' pids will be routed.
>
> CA_SET_PID allows to set just one PID, without identifying from
> what demod it would be received, and doesn't have a "reset"
> function to undo.

Here I can agree - it looks like the value -1 in 'index' should
do the job, but it, unfortunately, looses info from which descrambler
it should be removed (see note in struct ca_pid for value -1)

>
> So, IMHO, the interface is broken by design. Perhaps that's
> the reason why no upstream driver uses it.

I have the same feeling regarding brokenness.

>
> What seems to be a much better design would be to use the demux
> set filter ioctls and route the PIDs to the right CA.
>

I don't have access to any programmer reference documentation
for any modern DVB-enabled SoC, but I see two possible scenario
of connecting descramblers to the demuxes (most of modern SoCs
have more then one demux) - static one, when every demux has
predefined descramblers already connected to it and dynamic ones,
when any descrambler can be connected to the any demux.

>From that reason I vote to have some descrambler specific ioctl,
which allow more flexibility then if we add it to the filter set ioctl.

My 5 cents

/Honza

PS: I understand that until we get some multi-descrambler device included,
we don't need to address descrambler management.