Re: [PATCH 2/2] drm/msm/hdmi: add hdmi hdcp support

[Rob Clark]

On Wed, Dec 3, 2014 at 12:16 PM,  <jilaiw@xxxxxxxxxxxxxx> wrote:
>>>> +       enum hdmi_hdcp_state hdcp_state;
>>>> +       struct mutex state_mutex;
>>>> +       struct delayed_work hdcp_reauth_work;
>>>> +       struct delayed_work hdcp_auth_part1_1_work;
>>>> +       struct delayed_work hdcp_auth_part1_2_work;
>>>> +       struct work_struct hdcp_auth_part1_3_work;
>>>> +       struct delayed_work hdcp_auth_part2_1_work;
>>>> +       struct delayed_work hdcp_auth_part2_2_work;
>>>> +       struct delayed_work hdcp_auth_part2_3_work;
>>>> +       struct delayed_work hdcp_auth_part2_4_work;
>>>> +       struct work_struct hdcp_auth_prepare_work;
>>>
>>> You shouldn't use "work" as a way to express states in your state
>>> machine.
>>> Better have 1 auth work function that does all these steps, probably
>>> having
>>> them split in functions just like you do now.
>>>
>>> That way you can have 1 function running the pass of authentication,
>>> starting
>>> by checking if you're reauthing or not then processing each step one by
>>> one,
>>> sleeping inbetween them. You can have the functions return -EAGAIN to
>>> indicate
>>> that you need to retry the current operation and so on.
>>>
>>> This would split the state machine from the state executioners and
>>> simplify
>>> your code.
>>
>> As a side note (disclaimer, I'm not an hdcp expert), but I wonder if
>> eventually some of that should be extracted out into some helpers in
>> drm core.  I guess that is something we'll figure out when a 2nd
>> driver gains upstream HDCP support.  One big work w/ msleep()'s does
>> sound like it would be easier to understand (and therefore easier to
>> refactor out into helpers).
>>
>> [snip]
>>
>
> The reason that I break the partI/PartII work into these small works
> because I want to avoid to use msleep in work.
> Otherwise cancel a HDCP work may cause long delay up to several seconds.


hmm, yeah, several seconds is long enough for user to plug/unplug
several displays ;-)

But I guess a wait_event_timeout() plus a wq that is signalled on hpd
(or whenever we need to cancel) instead of msleep might do the trick?

BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/