[PATCH v8 11/17] drm/dp_mst: Add DSC enablement helpers to DRM

[Mikita Lipski]

On 12/6/19 7:24 PM, Lyude Paul wrote:
> Nice! All I've got is a couple of typos I noticed and one question, this looks
> great :)
> 
Thanks! I'll clean it up. The response to the question is below.

>> On Tue, 2019-12-03 at 09:35 -0500, mikita.lipski at amd.com wrote:
>> From: Mikita Lipski <mikita.lipski at amd.com>
>>
>> Adding a helper function to be called by
>> drivers outside of DRM to enable DSC on
>> the MST ports.
>>
>> Function is called to recalculate VCPI allocation
>> if DSC is enabled and raise the DSC flag to enable.
>> In case of disabling DSC the flag is set to false
>> and recalculation of VCPI slots is expected to be done
>> in encoder's atomic_check.
>>
>> v2: squash separate functions into one and call it per
>> port
>>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Cc: Lyude Paul <lyude at redhat.com>
>> Signed-off-by: Mikita Lipski <mikita.lipski at amd.com>
>> ---
>>   drivers/gpu/drm/drm_dp_mst_topology.c | 61 +++++++++++++++++++++++++++
>>   include/drm/drm_dp_mst_helper.h       |  5 +++
>>   2 files changed, 66 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index f1d883960831..5e549f48ffb8 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -4742,6 +4742,67 @@ drm_dp_mst_atomic_check_topology_state(struct
>> drm_dp_mst_topology_mgr *mgr,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * drm_dp_mst_atomic_enable_dsc - Set DSC Enable Flag to On/Off
>> + * @state: Pointer to the new drm_atomic_state
>> + * @pointer: Pointer to the affected MST Port
> Typo here
> 
>> + * @pbn: Newly recalculated bw required for link with DSC enabled
>> + * @pbn_div: Divider to calculate correct number of pbn per slot
>> + * @enable: Boolean flag enabling or disabling DSC on the port
>> + *
>> + * This function enables DSC on the given Port
>> + * by recalculating its vcpi from pbn provided
>> + * and sets dsc_enable flag to keep track of which
>> + * ports have DSC enabled
>> + *
>> + */
>> +int drm_dp_mst_atomic_enable_dsc(struct drm_atomic_state *state,
>> +				 struct drm_dp_mst_port *port,
>> +				 int pbn, int pbn_div,
>> +				 bool enable)
>> +{
>> +	struct drm_dp_mst_topology_state *mst_state;
>> +	struct drm_dp_vcpi_allocation *pos;
>> +	bool found = false;
>> +	int vcpi = 0;
>> +
>> +	mst_state = drm_atomic_get_mst_topology_state(state, port->mgr);
>> +
>> +	if (IS_ERR(mst_state))
>> +		return PTR_ERR(mst_state);
>> +
>> +	list_for_each_entry(pos, &mst_state->vcpis, next) {
>> +		if (pos->port == port) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!found) {
>> +		DRM_DEBUG_ATOMIC("[MST PORT:%p] Couldn't find VCPI allocation
>> in mst state %p\n",
>> +				 port, mst_state);
>> +		return -EINVAL;
>> +	}
> 
> Just double checking-does this handle the case where we're enabling DSC on a
> port that didn't previously have a VCPI allocation because it wasn't enabled
> previously? Or do we not need to handle that here

Because we call encoder atomic check previously to allocate VCPI slots - 
the port should have VCPI allocation before enabling DSC even if it 
wasn't enabled previously.
Therefore, I was thinking, that if encoder atomic check fails to 
allocate VCPI slots for the port - we shouldn't enable DSC on it and 
probably should fail atomic check if that is even requested.
> 
> Assuming you did the right thing here, with the small typo fixes:
> 
> Reviewed-by: Lyude Paul <lyude at redhat.com>
> 
>> +
>> +	if (pos->dsc_enabled == enable) {
>> +		DRM_DEBUG_ATOMIC("[MST PORT:%p] DSC flag is already set to %d,
>> returning %d VCPI slots\n",
>> +				 port, enable, pos->vcpi);
>> +		vcpi = pos->vcpi;
>> +	}
>> +
>> +	if (enable) {
>> +		vcpi = drm_dp_atomic_find_vcpi_slots(state, port->mgr, port,
>> pbn, pbn_div);
>> +		DRM_DEBUG_ATOMIC("[MST PORT:%p] Enabling DSC flag,
>> reallocating %d VCPI slots on the port\n",
>> +				 port, vcpi);
>> +		if (vcpi < 0)
>> +			return -EINVAL;
>> +	}
>> +
>> +	pos->dsc_enabled = enable;
>> +
>> +	return vcpi;
>> +}
>> +EXPORT_SYMBOL(drm_dp_mst_atomic_enable_dsc);
>>   /**
>>    * drm_dp_mst_atomic_check - Check that the new state of an MST topology in
>> an
>>    * atomic update is valid
>> diff --git a/include/drm/drm_dp_mst_helper.h
>> b/include/drm/drm_dp_mst_helper.h
>> index 0f813d6346aa..830c94b7f45d 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -502,6 +502,7 @@ struct drm_dp_payload {
>>   struct drm_dp_vcpi_allocation {
>>   	struct drm_dp_mst_port *port;
>>   	int vcpi;
>> +	bool dsc_enabled;
>>   	struct list_head next;
>>   };
>>   
>> @@ -773,6 +774,10 @@ drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state
>> *state,
>>   			      struct drm_dp_mst_topology_mgr *mgr,
>>   			      struct drm_dp_mst_port *port, int pbn,
>>   			      int pbn_div);
>> +int drm_dp_mst_atomic_enable_dsc(struct drm_atomic_state *state,
>> +				 struct drm_dp_mst_port *port,
>> +				 int pbn, int pbn_div,
>> +				 bool enable);
>>   int __must_check
>>   drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>>   				 struct drm_dp_mst_topology_mgr *mgr,

-- 
Thanks,
Mikita Lipski
Software Engineer, AMD
mikita.lipski at amd.com