Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f99c5891-5d46-b39e-929a-00aedb068fb5@quicinc.com>
Date:   Mon, 8 May 2023 17:00:12 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [Freedreno] [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for
 command mode
Content-Language: en-US
From:   Jessica Zhang <quic_jesszhan@quicinc.com>
To:     Marijn Suijten <marijn.suijten@somainline.org>
CC:     <freedreno@lists.freedesktop.org>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        <linux-arm-msm@vger.kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Sean Paul <sean@poorly.run>
References: <20230405-add-dsc-support-v2-0-1072c70e9786@quicinc.com>
 <20230405-add-dsc-support-v2-4-1072c70e9786@quicinc.com>
 <j5wa45g4v6swvsiakl23azu7qgxtdllf2gav5wdc7s7zukxe4c@jkcu2wnyn6rn>
 <baa25817-4a0d-551d-a351-21cc1525a932@quicinc.com>
In-Reply-To: <baa25817-4a0d-551d-a351-21cc1525a932@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 5/8/2023 4:17 PM, Jessica Zhang wrote:
> 
> 
> On 5/7/2023 9:06 AM, Marijn Suijten wrote:
>> On 2023-05-05 14:23:51, Jessica Zhang wrote:
>>> Add a DPU INTF op to set DATA_COMPRESS register for command mode 
>>> panels if
>>> the DPU_INTF_DATA_COMPRESS feature flag is set. This flag needs to be
>>> enabled in order for DSC v1.2 to work.
>>>
>>> Note: These changes are for command mode only. Video mode changes will
>>> be posted along with the DSC v1.2 support for DP.
>>
>> Nit: the "command mode" parts of both paragraphs only apply to the call
>> in dpu_encoder_phys_cmd, right?  If so, and the INTF op remains the same
>> for video mode (but only the call needs to be added to the
>> dpu_encoder_phy_vid), make this a bit more clear in your commit message.

(Sorry, forgot to address this comment in my initial reply)

The op will be available for video mode to use, but most likely video 
mode will set DATA_COMPRESS (or call dpu_hw_intf_enable_compression()) 
directly in dpu_hw_intf_setup_timing_engine().

Thanks,

Jessica Zhang

>>
>>> Changes in v2:
>>> - Fixed whitespace issue in macro definition
>>> - Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
>>> - Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
>>> - Removed `inline` from dpu_hw_intf_enable_compression declaration
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 11 +++++++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          |  2 ++
>>>   3 files changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index d8ed85a238af..1a4c20f02312 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -68,6 +68,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>                   phys_enc->hw_intf,
>>>                   true,
>>>                   phys_enc->hw_pp->idx);
>>> +
>>> +    if (phys_enc->hw_intf->ops.enable_compression)
>>> +        phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>   }
>>>   static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int 
>>> irq_idx)
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> index 6485500eedb8..322c55a5042c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -91,6 +91,14 @@
>>>   #define INTF_CFG2_DATABUS_WIDEN    BIT(0)
>>>   #define INTF_CFG2_DATA_HCTL_EN    BIT(4)
>>> +#define INTF_CFG2_DCE_DATA_COMPRESS     BIT(12)
>>> +
>>> +static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>> +{
>>> +    u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
>>> +
>>> +    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2 | 
>>> INTF_CFG2_DCE_DATA_COMPRESS);
>>
>> I'm not sure if it's more idiomatic to write:
>>
>>      intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>
>> On a separate line.
> 
> Hi Marijn,
> 
> Sounds good.
> 
>>
>>> +}
>>
>> Move the function close to the bottom of this file.  Right now all the
>> functions are defined approximately in the same order as they're listed
>> in the header and assigned in _setup_intf_ops().
> 
> Acked.
> 
>>
>>>   static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>>           const struct intf_timing_params *p,
>>> @@ -542,6 +550,9 @@ static void _setup_intf_ops(struct 
>>> dpu_hw_intf_ops *ops,
>>>           ops->vsync_sel = dpu_hw_intf_vsync_sel;
>>>           ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
>>>       }
>>> +
>>> +    if (cap & BIT(DPU_INTF_DATA_COMPRESS))
>>> +        ops->enable_compression = dpu_hw_intf_enable_compression;
>>>   }
>>>   struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> index 73b0885918f8..a8def68a5ec2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> @@ -70,6 +70,7 @@ struct intf_status {
>>>    * @get_autorefresh:            Retrieve autorefresh config from 
>>> hardware
>>>    *                              Return: 0 on success, -ETIMEDOUT on 
>>> timeout
>>>    * @vsync_sel:                  Select vsync signal for tear-effect 
>>> configuration
>>> + * @enable_compression: Enable data compression
>>
>> Indent to match above.
> 
> Sure, is the plan to correct the whitespace in the first half of the 
> comment block in the future?
> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>> - Marijn
>>
>>>    */
>>>   struct dpu_hw_intf_ops {
>>>       void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>> @@ -107,6 +108,7 @@ struct dpu_hw_intf_ops {
>>>        * Disable autorefresh if enabled
>>>        */
>>>       void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t 
>>> encoder_id, u16 vdisplay);
>>> +    void (*enable_compression)(struct dpu_hw_intf *intf);
>>>   };
>>>   struct dpu_hw_intf {
>>>
>>> -- 
>>> 2.40.1
>>>