Re: [RFC PATCH 11/13] drm/msm/disp/dpu1: Add support for DSC in topology

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 29/05/2021 01:23, abhinavk@codeaurora.org wrote:
> On 2021-05-28 03:39, Dmitry Baryshkov wrote:
>> On 21/05/2021 15:49, Vinod Koul wrote:
>>> For DSC to work we typically need a 2,2,1 configuration. This should
>>> suffice for resolutions upto 4k. For more resolutions like 8k this won't
>>> work.
>>>
>>> Furthermore, we can use 1 DSC encoder in lesser resulutions, but that is
>>> not power efficient according to Abhinav, so it is recommended to always
>>> use 2 encoders.
>>
>> Not power efficient because the second DSC would also be powered on or
>> because single DSC enc would consume more power than two DSCs?
> 
> I havent got through the series yet but just thought of answering this,
> 
> So before coming to the power aspects of this, hard-coding was done for 
> the foll reasons:
> 
> -> We do not have a topology DTSI property in upstream and will probably 
> not have as well till
> other features are added which support all the topologies
> -> The DSC panel which is being upstreamed as part of this series is 
> working with this 2,2,1 topology
> downstream ( dual lm, dual DSC encoders, single DSI ). Other topologies 
> have not been tried on it yet
> -> There needs to be a better approach to handle all topologies once we 
> have added support for them.
> It can be either a DTSI property if others agree OR some helper API 
> which will determine the best topology
> based on various factors. Till then, since this will be the only DSC 
> panel we are adding support for
> I thought we can start with a fixed topology for now.
> 
> Coming to the power aspect, I only recommended 2-2-1 here because using 
> two mixers is better power wise
> as it will split the width/2. We can also do 2-1-1 by enabling 3D mux 
> but this panel has not been validated
> with a single DSC. So to keep things simple with what has been 
> validated, I thought we can go ahead with
> 2-2-1 for now.
> 
> So rather than giving too much importance to the power aspect of it, the 
> other reasons should also
> be highlighted here as the main reason and the commit text should give 
> these details as well.

Sounds reasonable now, thank you!


> 
>>>
>>> So for now we blindly create 2,2,1 topology when DSC is enabled
>>>
>>> Co-developed-by: Abhinav Kumar <abhinavk@codeaurora.org>
>>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 18cb1274a8bb..bffb40085c67 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -609,8 +609,22 @@ static struct msm_display_topology 
>>> dpu_encoder_get_topology(
>>>       topology.num_enc = 0;
>>>       topology.num_intf = intf_count;
>>>   +    drm_enc = &dpu_enc->base;
>>> +    priv = drm_enc->dev->dev_private;
>>> +    if (priv && priv->dsc) {
>>> +        /* In case of Display Stream Compression DSC, we would use
>>> +         * 2 encoders, 2 line mixers and 1 interface
>>> +         * this is power optimal and can drive upto (including) 4k
>>> +         * screens
>>> +         */
>>> +        topology.num_enc = 2;
>>> +        topology.num_intf = 1;
>>> +        topology.num_lm = 2;
>>> +    }
>>> +
>>>       return topology;
>>>   }
>>> +
>>>   static int dpu_encoder_virt_atomic_check(
>>>           struct drm_encoder *drm_enc,
>>>           struct drm_crtc_state *crtc_state,
>>>


-- 
With best wishes
Dmitry