Op 03-03-2020 om 17:35 schreef Anshuman Gupta:
> On 2020-03-03 at 15:36:37 +0100, Maarten Lankhorst wrote:
>> Op 05-02-2020 om 06:07 schreef Anshuman Gupta:
>>> On 2020-01-28 at 21:45:45 +0530, Anshuman Gupta wrote:
>>> Hi Jani ,
>>> As per my understanding intel_hdcp_atomic_check() is not sufficient to
>>> fix the broken hdcp uapi state, as the state fixup required in case
>>> of modeset.
>>> If you do not have any concern, can we continue with the patch.
>>> Thanks,
>>> Anshuman Gupta.
>> Hey,
>>
> Thanks martin for review.
> As full modeset DDI disable sequence (encoder->disable()->intel_hdcp_disable()) can cause HDCP to
> disable without user space knowledge i.e. when Content Protetion state is not UNDESIRED, in those cases
> we want to fix the HDCP Content Protection state.
You can get to crtc_state from the connector_state->crtc, should be easy to fix up this case.
>> In case of a modeset, don't we always call atomic_check() on the connector, either before or after?
> yes it calls drm_atomic_helper_check_modeset()->intel_digital_connector_atomic_check()->intel_hdcp_atomic_check(),
> but if we fix HDCP state in intel_hdcp_atomic_check(), there may be a case at later point that fastset
> check is true, which disable need_modeset and enable update_pipe due to which encoder->update_pipe()->intel_hdcp_update_pipe()
> may endup enabling HDCP again when HDCP is already enabled, which is wrong.
Seems that if you look at the crtc_state carefully, you can prevent that. :)
~Maarten
>> Should be fine to fixup there then?
> Therefore we want to fixup the HDCP state only when full modeset is required, when it is going
> to disable DDI.
>
> Thanks ,
> Anshuman Gupta.
>>>> On 2020-01-28 at 21:14:44 +0530, Anshuman Gupta wrote:
>>>>> On 2020-01-28 at 16:19:31 +0200, Jani Nikula wrote:
>>>>>> On Tue, 28 Jan 2020, Anshuman Gupta <anshuman.gupta@xxxxxxxxx> wrote:
>>>>>>> Content Protection property should be updated as per the kernel
>>>>>>> internal state. Let's say if Content protection is disabled
>>>>>>> by userspace, CP property should be set to UNDESIRED so that
>>>>>>> reauthentication will not happen until userspace request it again,
>>>>>>> but when kernel disables the HDCP due to any DDI disabling sequences
>>>>>>> like modeset/DPMS operation, kernel should set the property to
>>>>>>> DESIRED, so that when opportunity arises, kernel will start the
>>>>>>> HDCP authentication on its own.
>>>>>>>
>>>>>>> Somewhere in the line, state machine to set content protection to
>>>>>>> DESIRED from kernel was broken and IGT coverage was missing for it.
>>>>>>> This patch fixes it.
>>>>>>> IGT patch to catch further regression on this features is being
>>>>>>> worked upon.
>>>>>>>
>>>>>>> CC: Ramalingam C <ramalingam.c@xxxxxxxxx>
>>>>>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/i915/display/intel_display.c | 4 +++
>>>>>>> drivers/gpu/drm/i915/display/intel_hdcp.c | 26 ++++++++++++++++++++
>>>>>>> drivers/gpu/drm/i915/display/intel_hdcp.h | 2 ++
>>>>>>> 3 files changed, 32 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>>>>> index da5266e76738..934cdf1f1858 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>>>>> @@ -14595,6 +14595,10 @@ static int intel_atomic_check(struct drm_device *dev,
>>>>>>> goto fail;
>>>>>>>
>>>>>>> if (any_ms) {
>>>>>>> + /*
>>>>>>> + * When there is modeset fix the hdcp uapi CP state.
>>>>>>> + */
>>>>>>> + intel_hdcp_post_need_modeset_check(state);
>>>>>>> ret = intel_modeset_checks(state);
>>>>>>> if (ret)
>>>>>>> goto fail;
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>>>> index 0fdbd39f6641..be083136eee2 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
>>>>>>> @@ -2074,6 +2074,32 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>>>>>>> crtc_state->mode_changed = true;
>>>>>>> }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * intel_hdcp_post_need_modeset_check.
>>>>>>> + * @state: intel atomic state.
>>>>>>> + *
>>>>>>> + * This function fix the HDCP uapi state when hdcp disabling initiated from
>>>>>>> + * modeset DDI disabling sequence. It updates uapi CP state from ENABLED to
>>>>>>> + * DESIRED so that HDCP uapi state can be restored as per HDCP Auth state.
>>>>>>> + * This function should be called only in case of in case of modeset.
>>>>>>> + * FIXME: As per HDCP content protection property uapi doc, an uevent()
>>>>>>> + * need to be sent if there is transition from ENABLED->DESIRED.
>>>>>>> + */
>>>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state)
>>>>>>> +{
>>>>>>> + struct drm_connector *connector;
>>>>>>> + struct drm_connector_state *old_state;
>>>>>>> + struct drm_connector_state *new_state;
>>>>>>> + int i;
>>>>>>> +
>>>>>>> + for_each_oldnew_connector_in_state(&state->base, connector, old_state,
>>>>>>> + new_state, i) {
>>>>>>> + if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
>>>>>>> + new_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>>>>>>> + new_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>> Why does this feel like duplication of what you already have in
>>>>>> intel_hdcp_atomic_check()?
>>>>> intel_hdcp_atomic_check() have checks that for disconnected connector and it doesn't look for
>>>> typo here, "intel_hdcp_atomic_check() checks that for disconnected connector and it doesn't check for new state shouldn't be UNDESIRED"
>>>>> old state, that is not sufficient to fix the hdcp CP uapi state, it need to be fix only in case of
>>>>> modeset, Later on a fastset check can disable the modeset and we would endup calling intel_hdcp_enable
>>>>> while hdcp is already enabled. That is the reason i think we would require a new API to
>>>>> fix the uapi state.
>>>>> Thanks ,
>>>>> Anshuman Gupta.
>>>>>> BR,
>>>>>> Jani.
>>>>>>
>>>>>>
>>>>>>> /* Handles the CP_IRQ raised from the DP HDCP sink */
>>>>>>> void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
>>>>>>> {
>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>>>> index f3c3272e712a..7bf46bc3c348 100644
>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>> struct drm_connector;
>>>>>>> struct drm_connector_state;
>>>>>>> struct drm_i915_private;
>>>>>>> +struct intel_atomic_state;
>>>>>>> struct intel_connector;
>>>>>>> struct intel_hdcp_shim;
>>>>>>> enum port;
>>>>>>> @@ -21,6 +22,7 @@ enum transcoder;
>>>>>>> void intel_hdcp_atomic_check(struct drm_connector *connector,
>>>>>>> struct drm_connector_state *old_state,
>>>>>>> struct drm_connector_state *new_state);
>>>>>>> +void intel_hdcp_post_need_modeset_check(struct intel_atomic_state *state);
>>>>>>> int intel_hdcp_init(struct intel_connector *connector,
>>>>>>> const struct intel_hdcp_shim *hdcp_shim);
>>>>>>> int intel_hdcp_enable(struct intel_connector *connector,
>>>>>> --
>>>>>> Jani Nikula, Intel Open Source Graphics Center
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
