[4/4] drm/msm/dpu: Enable compression for command mode

  Add a dpu_hw_intf op to enable data compression.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
 3 files changed, 13 insertions(+)
  

On 2023-05-02 18:19:15, Jessica Zhang wrote:
> Add a dpu_hw_intf op to enable data compression.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>  3 files changed, 13 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 74470d068622..4321a1aba17f 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

Can we have INTF DCE on video-mode encoders as well?

> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>  				phys_enc->hw_intf,
>  				true,
>  				phys_enc->hw_pp->idx);
> +
> +	if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&

As per my suggestion on patch 3/4, drop the flag and check above and
only check if the function is NULL (below).

> +			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 671048a78801..4ce7ffdd7a05 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -64,10 +64,16 @@
>  
>  #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>  #define INTF_CFG2_DATA_HCTL_EN	BIT(4)

These should probably be reindented to match the below... And the rest
of the defines use spaces instead of tabs.

> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>  
>  #define INTF_MISR_CTRL			0x180
>  #define INTF_MISR_SIGNATURE		0x184

This does not seem to apply on top of:
https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/

>  
> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)

Why inline?  This is used as a pointer callback.

> +{
> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);

dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
double-buffered, or is that config **always** unused when DSI CMD mode
is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
the bitflag into the register, or write the whole thing at once in
dpu_hw_intf_setup_timing_engine()?

> +}
> +
>  static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>  		const struct intf_timing_params *p,
>  		const struct dpu_format *fmt)
> @@ -325,6 +331,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>  		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
>  	ops->setup_misr = dpu_hw_intf_setup_misr;
>  	ops->collect_misr = dpu_hw_intf_collect_misr;
> +	ops->enable_compression = dpu_hw_intf_enable_compression;

And per the same suggestion on patch 3/4, this is then wrapped in:

    if (cap & BIT(DPU_INTF_DATA_COMPRESS))

(or similary named) flag check.

>  }

This also doesn't seem to apply on top of the INTF TE [1] support
series, even though it depends on DSC 1.2 DPU support(s?) [2] which
mentions it was rebase(d) on top of that.

[1]: https://patchwork.freedesktop.org/series/112332/
[2]: https://patchwork.freedesktop.org/series/116789/

- Marijn

>  
>  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 102c4f0e812b..99528c735368 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -60,6 +60,7 @@ struct intf_status {
>   *                     feed pixels to this interface
>   * @setup_misr: enable/disable MISR
>   * @collect_misr: read MISR signature
> + * @enable_compression: Enable data compression
>   */
>  struct dpu_hw_intf_ops {
>  	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -82,6 +83,7 @@ struct dpu_hw_intf_ops {
>  			const enum dpu_pingpong pp);
>  	void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
>  	int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
> +	void (*enable_compression)(struct dpu_hw_intf *intf);
>  };
>  
>  struct dpu_hw_intf {
> 
> -- 
> 2.40.1
>
  

On 5/3/2023 12:28 AM, Marijn Suijten wrote:
> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>> Add a dpu_hw_intf op to enable data compression.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>   3 files changed, 13 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 74470d068622..4321a1aba17f 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
> 
> Can we have INTF DCE on video-mode encoders as well?

Hi Marijn,

Currently, there's no way to validate DSC for video mode as I've only 
made changes to support DSI for command mode. We are planning to post 
changes to support DSC over DP, which will include changes for video mode.

> 
>> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>   				phys_enc->hw_intf,
>>   				true,
>>   				phys_enc->hw_pp->idx);
>> +
>> +	if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
> 
> As per my suggestion on patch 3/4, drop the flag and check above and
> only check if the function is NULL (below).

Acked.

> 
>> +			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 671048a78801..4ce7ffdd7a05 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -64,10 +64,16 @@
>>   
>>   #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>>   #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
> 
> These should probably be reindented to match the below... And the rest
> of the defines use spaces instead of tabs.

Fair point, though I think fixing the whitespace for these 2 macros 
specifically might be better in a more relevant series.

With that being said, I'll change the spacing of the DATA_COMPRESS bit 
to spaces instead of tabs.

> 
>> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>>   
>>   #define INTF_MISR_CTRL			0x180
>>   #define INTF_MISR_SIGNATURE		0x184
> 
> This does not seem to apply on top of:
> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/

Seems like I'm missing some patches from that series on my working 
branch. Will rebase on top of the full series for the v2.

> 
>>   
>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> 
> Why inline?  This is used as a pointer callback.

Acked, will remove the inline.

> 
>> +{
>> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
> 
> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
> double-buffered, or is that config **always** unused when DSI CMD mode
> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
> the bitflag into the register, or write the whole thing at once in
> dpu_hw_intf_setup_timing_engine()?

For command mode, INTF_CONFIG2 is unused aside from setting 
DATA_COMPRESS for DSC.

Since setup_timing_engine() is only used for video mode, the 
corresponding changes will be made in the DSC v1.2 for DP changes.

> 
>> +}
>> +
>>   static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>   		const struct intf_timing_params *p,
>>   		const struct dpu_format *fmt)
>> @@ -325,6 +331,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>>   		ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
>>   	ops->setup_misr = dpu_hw_intf_setup_misr;
>>   	ops->collect_misr = dpu_hw_intf_collect_misr;
>> +	ops->enable_compression = dpu_hw_intf_enable_compression;
> 
> And per the same suggestion on patch 3/4, this is then wrapped in:
> 
>      if (cap & BIT(DPU_INTF_DATA_COMPRESS))
> 
> (or similary named) flag check.

Acked.

Thanks,

Jessica Zhang

> 
>>   }
> 
> This also doesn't seem to apply on top of the INTF TE [1] support
> series, even though it depends on DSC 1.2 DPU support(s?) [2] which
> mentions it was rebase(d) on top of that.
> 
> [1]: https://patchwork.freedesktop.org/series/112332/
> [2]: https://patchwork.freedesktop.org/series/116789/
> 
> - Marijn
> 
>>   
>>   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 102c4f0e812b..99528c735368 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -60,6 +60,7 @@ struct intf_status {
>>    *                     feed pixels to this interface
>>    * @setup_misr: enable/disable MISR
>>    * @collect_misr: read MISR signature
>> + * @enable_compression: Enable data compression
>>    */
>>   struct dpu_hw_intf_ops {
>>   	void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>> @@ -82,6 +83,7 @@ struct dpu_hw_intf_ops {
>>   			const enum dpu_pingpong pp);
>>   	void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32 frame_count);
>>   	int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
>> +	void (*enable_compression)(struct dpu_hw_intf *intf);
>>   };
>>   
>>   struct dpu_hw_intf {
>>
>> -- 
>> 2.40.1
>>
  

On 03/05/2023 22:04, Jessica Zhang wrote:
> 
> 
> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>> Add a dpu_hw_intf op to enable data compression.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>   3 files changed, 13 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 74470d068622..4321a1aba17f 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
>>
>> Can we have INTF DCE on video-mode encoders as well?
> 
> Hi Marijn,
> 
> Currently, there's no way to validate DSC for video mode as I've only 
> made changes to support DSI for command mode. We are planning to post 
> changes to support DSC over DP, which will include changes for video mode.

If I remember correctly, HDK8350 panel should support DSC for both 
command and video modes.

> 
>>
>>> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>                   phys_enc->hw_intf,
>>>                   true,
>>>                   phys_enc->hw_pp->idx);
>>> +
>>> +    if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
>>
>> As per my suggestion on patch 3/4, drop the flag and check above and
>> only check if the function is NULL (below).
> 
> Acked.
> 
>>
>>> +            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 671048a78801..4ce7ffdd7a05 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -64,10 +64,16 @@
>>>   #define INTF_CFG2_DATABUS_WIDEN    BIT(0)
>>>   #define INTF_CFG2_DATA_HCTL_EN    BIT(4)
>>
>> These should probably be reindented to match the below... And the rest
>> of the defines use spaces instead of tabs.
> 
> Fair point, though I think fixing the whitespace for these 2 macros 
> specifically might be better in a more relevant series.
> 
> With that being said, I'll change the spacing of the DATA_COMPRESS bit 
> to spaces instead of tabs.
> 
>>
>>> +#define INTF_CFG2_DCE_DATA_COMPRESS    BIT(12)
>>>   #define INTF_MISR_CTRL            0x180
>>>   #define INTF_MISR_SIGNATURE        0x184
>>
>> This does not seem to apply on top of:
>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
> 
> Seems like I'm missing some patches from that series on my working 
> branch. Will rebase on top of the full series for the v2.
> 
>>
>>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf 
>>> *ctx)
>>
>> Why inline?  This is used as a pointer callback.
> 
> Acked, will remove the inline.
> 
>>
>>> +{
>>> +    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
>>
>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>> double-buffered, or is that config **always** unused when DSI CMD mode
>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>> the bitflag into the register, or write the whole thing at once in
>> dpu_hw_intf_setup_timing_engine()?
> 
> For command mode, INTF_CONFIG2 is unused aside from setting 
> DATA_COMPRESS for DSC.
> 
> Since setup_timing_engine() is only used for video mode, the 
> corresponding changes will be made in the DSC v1.2 for DP changes.

So, for command mode panels is this the only bit that should be set in 
INTF_CFG2?
  

Hi Jessica,

On 2023-05-03 12:04:59, Jessica Zhang wrote:
> 
> 
> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
> > On 2023-05-02 18:19:15, Jessica Zhang wrote:
> >> Add a dpu_hw_intf op to enable data compression.
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
> >>   3 files changed, 13 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 74470d068622..4321a1aba17f 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
> > 
> > Can we have INTF DCE on video-mode encoders as well?
> 
> Hi Marijn,
> 
> Currently, there's no way to validate DSC for video mode as I've only 
> made changes to support DSI for command mode. We are planning to post 
> changes to support DSC over DP, which will include changes for video mode.

Okay, but then mention so in the patch description (which is rather
short in this revision).

<snip>

> >>   #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
> >>   #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
> > 
> > These should probably be reindented to match the below... And the rest
> > of the defines use spaces instead of tabs.
> 
> Fair point, though I think fixing the whitespace for these 2 macros 
> specifically might be better in a more relevant series.

Yes, I have many patches to start cleaning these up, as well as all the
broken kerneldoc comments, but it's an uphill battle.  Not sure if I'll
get to it any time soon if at all.

> With that being said, I'll change the spacing of the DATA_COMPRESS bit 
> to spaces instead of tabs.

Thanks, that seems to be the most common format.

> >> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
> >>   
> >>   #define INTF_MISR_CTRL			0x180
> >>   #define INTF_MISR_SIGNATURE		0x184
> > 
> > This does not seem to apply on top of:
> > https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
> 
> Seems like I'm missing some patches from that series on my working 
> branch. Will rebase on top of the full series for the v2.

Thanks, but do discuss with Abhinav/Dmitry which series will land first.

> >> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> > 
> > Why inline?  This is used as a pointer callback.
> 
> Acked, will remove the inline.
> 
> > 
> >> +{
> >> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
> > 
> > dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
> > double-buffered, or is that config **always** unused when DSI CMD mode
> > is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
> > the bitflag into the register, or write the whole thing at once in
> > dpu_hw_intf_setup_timing_engine()?
> 
> For command mode, INTF_CONFIG2 is unused aside from setting 
> DATA_COMPRESS for DSC.
> 
> Since setup_timing_engine() is only used for video mode, the 
> corresponding changes will be made in the DSC v1.2 for DP changes.

Ack, that makes sense.  However, is this a guarantee that nothing else
will write INTF_CONFIG2 in the future, or will we solve that problem
when it happens?  I'm afraid more config-bits get added to this register
in the future and might possibly race/overwrite each other.

- Marijn

<snip>
  

On 5/3/2023 12:51 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 22:04, Jessica Zhang wrote:
>>
>>
>> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>>> Add a dpu_hw_intf op to enable data compression.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>>   3 files changed, 13 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 74470d068622..4321a1aba17f 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
>>>
>>> Can we have INTF DCE on video-mode encoders as well?
>>
>> Hi Marijn,
>>
>> Currently, there's no way to validate DSC for video mode as I've only 
>> made changes to support DSI for command mode. We are planning to post 
>> changes to support DSC over DP, which will include changes for video 
>> mode.
> 
> If I remember correctly, HDK8350 panel should support DSC for both 
> command and video modes.

Hi Dmitry,

Correct, however we are planning to submit the video mode changes with 
the DP DSC v1.2 changes.

My current panel driver/dt changes are for command mode, so we would 
have to spent time to also add video mode support. It would be faster to 
land the video mode changes with DP support as that's already a work in 
progress.

> 
>>
>>>
>>>> @@ -72,6 +72,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>>                   phys_enc->hw_intf,
>>>>                   true,
>>>>                   phys_enc->hw_pp->idx);
>>>> +
>>>> +    if (phys_enc->dpu_kms->catalog->caps->has_data_compress &&
>>>
>>> As per my suggestion on patch 3/4, drop the flag and check above and
>>> only check if the function is NULL (below).
>>
>> Acked.
>>
>>>
>>>> +            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 671048a78801..4ce7ffdd7a05 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -64,10 +64,16 @@
>>>>   #define INTF_CFG2_DATABUS_WIDEN    BIT(0)
>>>>   #define INTF_CFG2_DATA_HCTL_EN    BIT(4)
>>>
>>> These should probably be reindented to match the below... And the rest
>>> of the defines use spaces instead of tabs.
>>
>> Fair point, though I think fixing the whitespace for these 2 macros 
>> specifically might be better in a more relevant series.
>>
>> With that being said, I'll change the spacing of the DATA_COMPRESS bit 
>> to spaces instead of tabs.
>>
>>>
>>>> +#define INTF_CFG2_DCE_DATA_COMPRESS    BIT(12)
>>>>   #define INTF_MISR_CTRL            0x180
>>>>   #define INTF_MISR_SIGNATURE        0x184
>>>
>>> This does not seem to apply on top of:
>>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
>>
>> Seems like I'm missing some patches from that series on my working 
>> branch. Will rebase on top of the full series for the v2.
>>
>>>
>>>> +static inline void dpu_hw_intf_enable_compression(struct 
>>>> dpu_hw_intf *ctx)
>>>
>>> Why inline?  This is used as a pointer callback.
>>
>> Acked, will remove the inline.
>>
>>>
>>>> +{
>>>> +    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, 
>>>> INTF_CFG2_DCE_DATA_COMPRESS);
>>>
>>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>>> double-buffered, or is that config **always** unused when DSI CMD mode
>>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>>> the bitflag into the register, or write the whole thing at once in
>>> dpu_hw_intf_setup_timing_engine()?
>>
>> For command mode, INTF_CONFIG2 is unused aside from setting 
>> DATA_COMPRESS for DSC.
>>
>> Since setup_timing_engine() is only used for video mode, the 
>> corresponding changes will be made in the DSC v1.2 for DP changes.
> 
> So, for command mode panels is this the only bit that should be set in 
> INTF_CFG2?

Yep, outside of the changes in this patch, INTF_CONFIG2 is only used in 
the video mode setup_timing_engine() method.

Thanks,

Jessica Zhang

> -- 
> With best wishes
> Dmitry
>
  

On 5/3/2023 4:00 PM, Marijn Suijten wrote:
> Hi Jessica,
> 
> On 2023-05-03 12:04:59, Jessica Zhang wrote:
>>
>>
>> On 5/3/2023 12:28 AM, Marijn Suijten wrote:
>>> On 2023-05-02 18:19:15, Jessica Zhang wrote:
>>>> Add a dpu_hw_intf op to enable data compression.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 ++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c          | 7 +++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h          | 2 ++
>>>>    3 files changed, 13 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 74470d068622..4321a1aba17f 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
>>>
>>> Can we have INTF DCE on video-mode encoders as well?
>>
>> Hi Marijn,
>>
>> Currently, there's no way to validate DSC for video mode as I've only
>> made changes to support DSI for command mode. We are planning to post
>> changes to support DSC over DP, which will include changes for video mode.
> 
> Okay, but then mention so in the patch description (which is rather
> short in this revision).

Acked.

> 
> <snip>
> 
>>>>    #define INTF_CFG2_DATABUS_WIDEN	BIT(0)
>>>>    #define INTF_CFG2_DATA_HCTL_EN	BIT(4)
>>>
>>> These should probably be reindented to match the below... And the rest
>>> of the defines use spaces instead of tabs.
>>
>> Fair point, though I think fixing the whitespace for these 2 macros
>> specifically might be better in a more relevant series.
> 
> Yes, I have many patches to start cleaning these up, as well as all the
> broken kerneldoc comments, but it's an uphill battle.  Not sure if I'll
> get to it any time soon if at all.
> 
>> With that being said, I'll change the spacing of the DATA_COMPRESS bit
>> to spaces instead of tabs.
> 
> Thanks, that seems to be the most common format.
> 
>>>> +#define INTF_CFG2_DCE_DATA_COMPRESS	BIT(12)
>>>>    
>>>>    #define INTF_MISR_CTRL			0x180
>>>>    #define INTF_MISR_SIGNATURE		0x184
>>>
>>> This does not seem to apply on top of:
>>> https://lore.kernel.org/linux-arm-msm/20230411-dpu-intf-te-v4-10-27ce1a5ab5c6@somainline.org/
>>
>> Seems like I'm missing some patches from that series on my working
>> branch. Will rebase on top of the full series for the v2.
> 
> Thanks, but do discuss with Abhinav/Dmitry which series will land first.
> 
>>>> +static inline void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
>>>
>>> Why inline?  This is used as a pointer callback.
>>
>> Acked, will remove the inline.
>>
>>>
>>>> +{
>>>> +	DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, INTF_CFG2_DCE_DATA_COMPRESS);
>>>
>>> dpu_hw_intf_setup_timing_engine() also programs INTF_CONFIG2.  Is it
>>> double-buffered, or is that config **always** unused when DSI CMD mode
>>> is used in conjunction with DSC/DCE?  Otherwise this should perhaps OR
>>> the bitflag into the register, or write the whole thing at once in
>>> dpu_hw_intf_setup_timing_engine()?
>>
>> For command mode, INTF_CONFIG2 is unused aside from setting
>> DATA_COMPRESS for DSC.
>>
>> Since setup_timing_engine() is only used for video mode, the
>> corresponding changes will be made in the DSC v1.2 for DP changes.
> 
> Ack, that makes sense.  However, is this a guarantee that nothing else
> will write INTF_CONFIG2 in the future, or will we solve that problem
> when it happens?  I'm afraid more config-bits get added to this register
> in the future and might possibly race/overwrite each other.

That's a fair point. There's no guarantee that nothing else will set 
INTF_CONFIG2 for command mode in the future. I think it would be better 
to add a register read now instead of having to fix that issue in a 
future change.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
> <snip>