2023-05-05 21:27:00

by Jessica Zhang

[permalink] [raw]
Subject: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

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.

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 <[email protected]>
---
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);
+}

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
*/
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


2023-05-07 16:29:39

by Marijn Suijten

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

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.

> 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 <[email protected]>
> ---
> 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.

> +}

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().

>
> 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.

- 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
>

2023-05-08 23:51:10

by Jessica Zhang

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode



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.
>
>> 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 <[email protected]>
>> ---
>> 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
>>

2023-05-09 00:47:46

by Jessica Zhang

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode



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 <[email protected]>
>>> ---
>>>   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
>>>

2023-05-09 06:52:23

by Marijn Suijten

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

On 2023-05-08 16:17:54, Jessica Zhang wrote:
<snip>
> >> 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?

I couldn't see the first part of the block in the diff context here, but
indeed that's a broken disaster so we will have to fix that up :(

I think it is fine to leave the latter ones as it is, as long as it is
consistent:

- Only using spaces;
- Colon directly after the word (and an @ before it, see kerneldoc
specification);
- Aligned to existing entries.

- Marijn

2023-05-09 07:07:55

by Marijn Suijten

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH v2 4/4] drm/msm/dpu: Set DATA_COMPRESS for command mode

On 2023-05-08 17:00:12, Jessica Zhang wrote:
> 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().

Sounsds good, if you can clarify something along those lines so that it
is clear that the call is valid on video mode too, and that the callback
is also available there.

e.g.
- Drop "for command mode panels" from "op to set DATA_COMPRESS
register";
- "Note: the op is currently called for command-mode encoders only,
video mode changes..."

Thanks!

- Marijn