Re: [Intel-gfx] [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
On Thu, Dec 20, 2018 at 03:55:27PM +0100, Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 02:28:55PM +, Winkler, Tomas wrote:
> >
> >
> > > On Wed, 19 Dec 2018, "Winkler, Tomas" wrote:
> > > >>
> > > >> On Wed, 19 Dec 2018, "C, Ramalingam" wrote:
> > > >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> > > >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> > > >> >>> struct intel_hdcp {
> > > >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> > > >> >>> */
> > > >> >>>u8 content_type;
> > > >> >>>struct hdcp_port_data port_data;
> > > >> >>> +
> > > >> >>> + u8 is_paired;
> > > >> >>> + u8 is_repeater;
> > > >> >> Make these two bool, will simplify the code a bunch.
> > > >> >
> > > >> > Seems there is a movement for not to use the bool in structures.
> > > >>
> > > >> No. Please use bools in structs when it makes sense. Avoid bools in
> > > >> structs when you need to care about memory footprint or alignment or
> > > >> packing or the like. This is not one of those cases.
> > > >>
> > > >> > Thats why I have changed these from bool to u8 from v8 onwards.
> > > >> > Checkpatch also complains on this
> > > >>
> > > >> Sorry to say, checkpatch is not the authority although we do send out
> > > >> automated checkpatch results.
> > > >
> > > > I believe it was Linus' call to not use bool in structs at all
> > > > https://lkml.org/lkml/2017/11/21/384
> > >
> > > I don't care. That's a valid judgement in the context referenced, but the
> > > conclusion "no bools in structs at all" isn't. In this case, I think
> > > bools are the
> > > better option, and anything else makes the code worse.
> >
> > The solution was to use bit fields,
> > unsinged int is_paired:1;
> > unsinged int is_repeter:1
>
> This doesn't work with READ_ONCE/WRITE_ONCE, and it generates terrible
> assembly (at least gcc is well known for struggling with these, compared
> to open-coded bitops). So depending upon what you want to do, and where
> youre space/performance tradeoff lies, doing this unconditionally is just
> wrong.
Another annoying downside with non-bool bitfields:
unsigned int foo:1;
foo = 2;
-> foo == 0
So you'll need to remmber to convert everything to
0/1 before the assignment.
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
On Thu, Dec 20, 2018 at 02:28:55PM +, Winkler, Tomas wrote:
>
>
> > On Wed, 19 Dec 2018, "Winkler, Tomas" wrote:
> > >>
> > >> On Wed, 19 Dec 2018, "C, Ramalingam" wrote:
> > >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> > >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> > >> >>> struct intel_hdcp {
> > >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> > >> >>> */
> > >> >>> u8 content_type;
> > >> >>> struct hdcp_port_data port_data;
> > >> >>> +
> > >> >>> +u8 is_paired;
> > >> >>> +u8 is_repeater;
> > >> >> Make these two bool, will simplify the code a bunch.
> > >> >
> > >> > Seems there is a movement for not to use the bool in structures.
> > >>
> > >> No. Please use bools in structs when it makes sense. Avoid bools in
> > >> structs when you need to care about memory footprint or alignment or
> > >> packing or the like. This is not one of those cases.
> > >>
> > >> > Thats why I have changed these from bool to u8 from v8 onwards.
> > >> > Checkpatch also complains on this
> > >>
> > >> Sorry to say, checkpatch is not the authority although we do send out
> > >> automated checkpatch results.
> > >
> > > I believe it was Linus' call to not use bool in structs at all
> > > https://lkml.org/lkml/2017/11/21/384
> >
> > I don't care. That's a valid judgement in the context referenced, but the
> > conclusion "no bools in structs at all" isn't. In this case, I think bools
> > are the
> > better option, and anything else makes the code worse.
>
> The solution was to use bit fields,
> unsinged int is_paired:1;
> unsinged int is_repeter:1
This doesn't work with READ_ONCE/WRITE_ONCE, and it generates terrible
assembly (at least gcc is well known for struggling with these, compared
to open-coded bitops). So depending upon what you want to do, and where
youre space/performance tradeoff lies, doing this unconditionally is just
wrong.
It was the right thing for the patch Linus commented on though.
-Daniel
> There is a strong point in consistency so there are no mistakes.
>
> But frankly I don't really have strong feelings about it.
>
> Thanks
> Tomas
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
> On Wed, 19 Dec 2018, "Winkler, Tomas" wrote:
> >>
> >> On Wed, 19 Dec 2018, "C, Ramalingam" wrote:
> >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> >> >>> struct intel_hdcp {
> >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> >> >>> */
> >> >>>u8 content_type;
> >> >>>struct hdcp_port_data port_data;
> >> >>> +
> >> >>> + u8 is_paired;
> >> >>> + u8 is_repeater;
> >> >> Make these two bool, will simplify the code a bunch.
> >> >
> >> > Seems there is a movement for not to use the bool in structures.
> >>
> >> No. Please use bools in structs when it makes sense. Avoid bools in
> >> structs when you need to care about memory footprint or alignment or
> >> packing or the like. This is not one of those cases.
> >>
> >> > Thats why I have changed these from bool to u8 from v8 onwards.
> >> > Checkpatch also complains on this
> >>
> >> Sorry to say, checkpatch is not the authority although we do send out
> >> automated checkpatch results.
> >
> > I believe it was Linus' call to not use bool in structs at all
> > https://lkml.org/lkml/2017/11/21/384
>
> I don't care. That's a valid judgement in the context referenced, but the
> conclusion "no bools in structs at all" isn't. In this case, I think bools
> are the
> better option, and anything else makes the code worse.
The solution was to use bit fields,
unsinged int is_paired:1;
unsinged int is_repeter:1
There is a strong point in consistency so there are no mistakes.
But frankly I don't really have strong feelings about it.
Thanks
Tomas
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
On Wed, 19 Dec 2018, "Winkler, Tomas" wrote:
>>
>> On Wed, 19 Dec 2018, "C, Ramalingam" wrote:
>> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
>> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
>> >>> struct intel_hdcp {
>> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
>> >>> */
>> >>> u8 content_type;
>> >>> struct hdcp_port_data port_data;
>> >>> +
>> >>> +u8 is_paired;
>> >>> +u8 is_repeater;
>> >> Make these two bool, will simplify the code a bunch.
>> >
>> > Seems there is a movement for not to use the bool in structures.
>>
>> No. Please use bools in structs when it makes sense. Avoid bools in structs
>> when you need to care about memory footprint or alignment or packing or
>> the like. This is not one of those cases.
>>
>> > Thats why I have changed these from bool to u8 from v8 onwards.
>> > Checkpatch also complains on this
>>
>> Sorry to say, checkpatch is not the authority although we do send out
>> automated checkpatch results.
>
> I believe it was Linus' call to not use bool in structs at all
> https://lkml.org/lkml/2017/11/21/384
I don't care. That's a valid judgement in the context referenced, but
the conclusion "no bools in structs at all" isn't. In this case, I think
bools are the better option, and anything else makes the code worse.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
>
> On Wed, 19 Dec 2018, "C, Ramalingam" wrote:
> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> >>> struct intel_hdcp {
> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> >>>*/
> >>> u8 content_type;
> >>> struct hdcp_port_data port_data;
> >>> +
> >>> + u8 is_paired;
> >>> + u8 is_repeater;
> >> Make these two bool, will simplify the code a bunch.
> >
> > Seems there is a movement for not to use the bool in structures.
>
> No. Please use bools in structs when it makes sense. Avoid bools in structs
> when you need to care about memory footprint or alignment or packing or
> the like. This is not one of those cases.
>
> > Thats why I have changed these from bool to u8 from v8 onwards.
> > Checkpatch also complains on this
>
> Sorry to say, checkpatch is not the authority although we do send out
> automated checkpatch results.
I believe it was Linus' call to not use bool in structs at all
https://lkml.org/lkml/2017/11/21/384
Thanks
Tomas
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 10/39] drm/i915: Implement HDCP2.2 receiver authentication
On Wed, 19 Dec 2018, "C, Ramalingam" wrote:
> On 12/19/2018 8:05 PM, Daniel Vetter wrote:
>> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
>>> struct intel_hdcp {
>>> @@ -414,6 +430,24 @@ struct intel_hdcp {
>>> */
>>> u8 content_type;
>>> struct hdcp_port_data port_data;
>>> +
>>> + u8 is_paired;
>>> + u8 is_repeater;
>> Make these two bool, will simplify the code a bunch.
>
> Seems there is a movement for not to use the bool in structures.
No. Please use bools in structs when it makes sense. Avoid bools in
structs when you need to care about memory footprint or alignment or
packing or the like. This is not one of those cases.
> Thats why I have changed these from bool to u8 from v8
> onwards. Checkpatch also complains on this
Sorry to say, checkpatch is not the authority although we do send out
automated checkpatch results.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
