mbox series

[v2,0/6] drm/msm: DSI DSC video mode fixes

Message ID 20231114225857.19702-1-jonathan@marek.ca
Headers show
Series drm/msm: DSI DSC video mode fixes | expand

Message

Jonathan Marek Nov. 14, 2023, 10:58 p.m. UTC
v2: added new patches (first two patches) to get DSC video mode running with
the upstream DPU driver (tested with the vtdr6130 panel)

Jonathan Marek (6):
  drm/msm/dpu: fix video mode DSC for DSI
  drm/msm/dsi: set video mode widebus enable bit when widebus is enabled
  drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)
  drm/msm/dsi: add a comment to explain pkt_per_line encoding
  drm/msm/dsi: support DSC configurations with slice_per_pkt > 1
  drm/msm/dsi: fix DSC for the bonded DSI case

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  2 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 11 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 13 ++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  1 +
 drivers/gpu/drm/msm/dsi/dsi.h                 |  3 +-
 drivers/gpu/drm/msm/dsi/dsi.xml.h             |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c            | 50 ++++++++++---------
 drivers/gpu/drm/msm/dsi/dsi_manager.c         |  2 +-
 include/drm/drm_mipi_dsi.h                    |  1 +
 10 files changed, 58 insertions(+), 28 deletions(-)

Comments

Dmitry Baryshkov Nov. 15, 2023, 8:53 a.m. UTC | #1
On Wed, 15 Nov 2023 at 01:00, Jonathan Marek <jonathan@marek.ca> wrote:
>
> Add necessary DPU changes for DSC to work with DSI video mode.
>
> Note this changes the logic to enable HCTL to match downstream, it will
> now be enabled for the no-DSC no-widebus case.
>
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c         |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h    |  2 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c    | 11 +++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c         | 13 ++++++++++++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h         |  1 +
>  5 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1cf7ff6caff4..d745c8678b9d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2477,7 +2477,7 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder)
>         return INTF_MODE_NONE;
>  }
>
> -unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc)
> +unsigned int dpu_encoder_helper_get_dsc(const struct dpu_encoder_phys *phys_enc)

Why?

>  {
>         struct drm_encoder *encoder = phys_enc->parent;
>         struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 6f04c3d56e77..7e27a7da0887 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -332,7 +332,7 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
>   *   used for this encoder.
>   * @phys_enc: Pointer to physical encoder structure
>   */
> -unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc);
> +unsigned int dpu_encoder_helper_get_dsc(const struct dpu_encoder_phys *phys_enc);
>
>  /**
>   * dpu_encoder_helper_split_config - split display configuration helper function
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index a01fda711883..df10800a9615 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -100,6 +100,8 @@ static void drm_mode_to_intf_timing_params(
>         }
>
>         timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> +       if (dpu_encoder_helper_get_dsc(phys_enc))
> +               timing->compression_en = true;
>
>         /*
>          * for DP, divide the horizonal parameters by 2 when
> @@ -112,6 +114,15 @@ static void drm_mode_to_intf_timing_params(
>                 timing->h_front_porch = timing->h_front_porch >> 1;
>                 timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
>         }
> +
> +       /*
> +        * for DSI, if compression is enabled, then divide the horizonal active
> +        * timing parameters by compression ratio.
> +        */
> +       if (phys_enc->hw_intf->cap->type != INTF_DP && timing->compression_en) {
> +               timing->width = timing->width / 3; /* XXX: don't assume 3:1 compression ratio */

Is this /3 from bpp / compressed_bpp?

> +               timing->xres = timing->width;
> +       }
>  }
>
>  static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params *timing)
> 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 e8b8908d3e12..d6fe45a6da2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -166,10 +166,21 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>          * video timing. It is recommended to enable it for all cases, except
>          * if compression is enabled in 1 pixel per clock mode
>          */
> +       if (!p->compression_en || p->wide_bus_en)
> +               intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> +
>         if (p->wide_bus_en)
> -               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>
>         data_width = p->width;
> +       if (p->wide_bus_en && !dp_intf)
> +               data_width = p->width >> 1;
> +
> +       if (p->compression_en)
> +               intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> +
> +       if (p->compression_en && dp_intf)
> +               DPU_ERROR("missing adjustments for DSC+DP\n");
>
>         hsync_data_start_x = hsync_start_x;
>         hsync_data_end_x =  hsync_start_x + data_width - 1;

This should go into a separate commit with the proper justification.

> 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 c539025c418b..15a5fdadd0a0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -33,6 +33,7 @@ struct dpu_hw_intf_timing_params {
>         u32 hsync_skew;
>
>         bool wide_bus_en;
> +       bool compression_en;
>  };
>
>  struct dpu_hw_intf_prog_fetch {
> --
> 2.26.1
>
Jonathan Marek Nov. 16, 2023, 6:30 p.m. UTC | #2
On 11/15/23 3:53 AM, Dmitry Baryshkov wrote:
> On Wed, 15 Nov 2023 at 01:00, Jonathan Marek <jonathan@marek.ca> wrote:
>>
>> Add necessary DPU changes for DSC to work with DSI video mode.
>>
>> Note this changes the logic to enable HCTL to match downstream, it will
>> now be enabled for the no-DSC no-widebus case.
>>
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c         |  2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h    |  2 +-
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c    | 11 +++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c         | 13 ++++++++++++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h         |  1 +
>>   5 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1cf7ff6caff4..d745c8678b9d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2477,7 +2477,7 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder)
>>          return INTF_MODE_NONE;
>>   }
>>
>> -unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc)
>> +unsigned int dpu_encoder_helper_get_dsc(const struct dpu_encoder_phys *phys_enc)
> 
> Why?
> 

drm_mode_to_intf_timing_params has "phys_enc" pointer declared as const, 
so one of them needs to change to call dpu_encoder_helper_get_dsc

>>   {
>>          struct drm_encoder *encoder = phys_enc->parent;
>>          struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index 6f04c3d56e77..7e27a7da0887 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -332,7 +332,7 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
>>    *   used for this encoder.
>>    * @phys_enc: Pointer to physical encoder structure
>>    */
>> -unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc);
>> +unsigned int dpu_encoder_helper_get_dsc(const struct dpu_encoder_phys *phys_enc);
>>
>>   /**
>>    * dpu_encoder_helper_split_config - split display configuration helper function
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index a01fda711883..df10800a9615 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -100,6 +100,8 @@ static void drm_mode_to_intf_timing_params(
>>          }
>>
>>          timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> +       if (dpu_encoder_helper_get_dsc(phys_enc))
>> +               timing->compression_en = true;
>>
>>          /*
>>           * for DP, divide the horizonal parameters by 2 when
>> @@ -112,6 +114,15 @@ static void drm_mode_to_intf_timing_params(
>>                  timing->h_front_porch = timing->h_front_porch >> 1;
>>                  timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
>>          }
>> +
>> +       /*
>> +        * for DSI, if compression is enabled, then divide the horizonal active
>> +        * timing parameters by compression ratio.
>> +        */
>> +       if (phys_enc->hw_intf->cap->type != INTF_DP && timing->compression_en) {
>> +               timing->width = timing->width / 3; /* XXX: don't assume 3:1 compression ratio */
> 
> Is this /3 from bpp / compressed_bpp?
> 

It is the compression ratio of DSC for 8bpc (24bpp) compressed to 8bpp. 
DSI driver doesn't support any other cases so this assumption should be 
OK for now (the other common ratio is 3.75 for 10bpc compressed to 8bpp 
- from downstream driver it appears this would mean a division by 3.75 
here).

>> +               timing->xres = timing->width;
>> +       }
>>   }
>>
>>   static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params *timing)
>> 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 e8b8908d3e12..d6fe45a6da2d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -166,10 +166,21 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>           * video timing. It is recommended to enable it for all cases, except
>>           * if compression is enabled in 1 pixel per clock mode
>>           */
>> +       if (!p->compression_en || p->wide_bus_en)
>> +               intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>> +
>>          if (p->wide_bus_en)
>> -               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>
>>          data_width = p->width;
>> +       if (p->wide_bus_en && !dp_intf)
>> +               data_width = p->width >> 1;
>> +
>> +       if (p->compression_en)
>> +               intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>> +
>> +       if (p->compression_en && dp_intf)
>> +               DPU_ERROR("missing adjustments for DSC+DP\n");
>>
>>          hsync_data_start_x = hsync_start_x;
>>          hsync_data_end_x =  hsync_start_x + data_width - 1;
> 
> This should go into a separate commit with the proper justification.
> 

All of it? setting the INTF_CFG2_DCE_DATA_COMPRESS flag at least doesn't 
make sense to make a separate patch. And DSI widebus is only used with 
DSC (and always used when available), so IMO also in the scope of this 
commit.

>> 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 c539025c418b..15a5fdadd0a0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -33,6 +33,7 @@ struct dpu_hw_intf_timing_params {
>>          u32 hsync_skew;
>>
>>          bool wide_bus_en;
>> +       bool compression_en;
>>   };
>>
>>   struct dpu_hw_intf_prog_fetch {
>> --
>> 2.26.1
>>
> 
>
Dmitry Baryshkov Dec. 2, 2023, 10:20 p.m. UTC | #3
On 16/11/2023 20:30, Jonathan Marek wrote:
> On 11/15/23 3:53 AM, Dmitry Baryshkov wrote:
>> On Wed, 15 Nov 2023 at 01:00, Jonathan Marek <jonathan@marek.ca> wrote:
>>>
>>> Add necessary DPU changes for DSC to work with DSI video mode.
>>>
>>> Note this changes the logic to enable HCTL to match downstream, it will
>>> now be enabled for the no-DSC no-widebus case.
>>>
>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c         |  2 +-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h    |  2 +-
>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c    | 11 +++++++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c         | 13 ++++++++++++-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h         |  1 +
>>>   5 files changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 1cf7ff6caff4..d745c8678b9d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -2477,7 +2477,7 @@ enum dpu_intf_mode 
>>> dpu_encoder_get_intf_mode(struct drm_encoder *encoder)
>>>          return INTF_MODE_NONE;
>>>   }
>>>
>>> -unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +unsigned int dpu_encoder_helper_get_dsc(const struct 
>>> dpu_encoder_phys *phys_enc)
>>
>> Why?
>>
> 
> drm_mode_to_intf_timing_params has "phys_enc" pointer declared as const, 
> so one of them needs to change to call dpu_encoder_helper_get_dsc
> 
>>>   {
>>>          struct drm_encoder *encoder = phys_enc->parent;
>>>          struct dpu_encoder_virt *dpu_enc = 
>>> to_dpu_encoder_virt(encoder);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> index 6f04c3d56e77..7e27a7da0887 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -332,7 +332,7 @@ static inline enum dpu_3d_blend_mode 
>>> dpu_encoder_helper_get_3d_blend_mode(
>>>    *   used for this encoder.
>>>    * @phys_enc: Pointer to physical encoder structure
>>>    */
>>> -unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys 
>>> *phys_enc);
>>> +unsigned int dpu_encoder_helper_get_dsc(const struct 
>>> dpu_encoder_phys *phys_enc);
>>>
>>>   /**
>>>    * dpu_encoder_helper_split_config - split display configuration 
>>> helper function
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> index a01fda711883..df10800a9615 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> @@ -100,6 +100,8 @@ static void drm_mode_to_intf_timing_params(
>>>          }
>>>
>>>          timing->wide_bus_en = 
>>> dpu_encoder_is_widebus_enabled(phys_enc->parent);
>>> +       if (dpu_encoder_helper_get_dsc(phys_enc))
>>> +               timing->compression_en = true;
>>>
>>>          /*
>>>           * for DP, divide the horizonal parameters by 2 when
>>> @@ -112,6 +114,15 @@ static void drm_mode_to_intf_timing_params(
>>>                  timing->h_front_porch = timing->h_front_porch >> 1;
>>>                  timing->hsync_pulse_width = 
>>> timing->hsync_pulse_width >> 1;
>>>          }
>>> +
>>> +       /*
>>> +        * for DSI, if compression is enabled, then divide the 
>>> horizonal active
>>> +        * timing parameters by compression ratio.
>>> +        */
>>> +       if (phys_enc->hw_intf->cap->type != INTF_DP && 
>>> timing->compression_en) {
>>> +               timing->width = timing->width / 3; /* XXX: don't 
>>> assume 3:1 compression ratio */
>>
>> Is this /3 from bpp / compressed_bpp?
>>
> 
> It is the compression ratio of DSC for 8bpc (24bpp) compressed to 8bpp. 
> DSI driver doesn't support any other cases so this assumption should be 
> OK for now (the other common ratio is 3.75 for 10bpc compressed to 8bpp 
> - from downstream driver it appears this would mean a division by 3.75 
> here).
> 
>>> +               timing->xres = timing->width;
>>> +       }
>>>   }
>>>
>>>   static u32 get_horizontal_total(const struct 
>>> dpu_hw_intf_timing_params *timing)
>>> 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 e8b8908d3e12..d6fe45a6da2d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>> @@ -166,10 +166,21 @@ static void 
>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
>>>           * video timing. It is recommended to enable it for all 
>>> cases, except
>>>           * if compression is enabled in 1 pixel per clock mode
>>>           */
>>> +       if (!p->compression_en || p->wide_bus_en)
>>> +               intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>>> +
>>>          if (p->wide_bus_en)
>>> -               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | 
>>> INTF_CFG2_DATA_HCTL_EN;
>>> +               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>>>
>>>          data_width = p->width;
>>> +       if (p->wide_bus_en && !dp_intf)
>>> +               data_width = p->width >> 1;
>>> +
>>> +       if (p->compression_en)
>>> +               intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>> +
>>> +       if (p->compression_en && dp_intf)
>>> +               DPU_ERROR("missing adjustments for DSC+DP\n");
>>>
>>>          hsync_data_start_x = hsync_start_x;
>>>          hsync_data_end_x =  hsync_start_x + data_width - 1;
>>
>> This should go into a separate commit with the proper justification.
>>
> 
> All of it? setting the INTF_CFG2_DCE_DATA_COMPRESS flag at least doesn't 
> make sense to make a separate patch. And DSI widebus is only used with 
> DSC (and always used when available), so IMO also in the scope of this 
> commit.

Excuse me for being not precise. I'm more concerned about 
INTF_CFG2_DATA_HCTL_EN change. They way you have added it doesn't have 
anything to do with the DSC support. If we ever want to revert just that 
clause to check anything, we wil have to revert the whole DSC-DSI-video 
commit, which doesn't seem correct.

> 
>>> 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 c539025c418b..15a5fdadd0a0 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>> @@ -33,6 +33,7 @@ struct dpu_hw_intf_timing_params {
>>>          u32 hsync_skew;
>>>
>>>          bool wide_bus_en;
>>> +       bool compression_en;
>>>   };
>>>
>>>   struct dpu_hw_intf_prog_fetch {
>>> -- 
>>> 2.26.1
>>>
>>
>>
Jessica Zhang Dec. 7, 2023, 8:28 p.m. UTC | #4
On 11/14/2023 2:58 PM, Jonathan Marek wrote:
> Add a dsc_slice_per_pkt field to mipi_dsi_device struct and the necessary
> changes to msm driver to support this field.
> 
> Note that the removed "pkt_per_line = slice_per_intf * slice_per_pkt"
> comment is incorrect.

Hi John,

Thanks for catching the typo.

> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 25 ++++++++++---------------
>   include/drm/drm_mipi_dsi.h         |  1 +
>   2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 842765063b1b..892a463a7e03 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -161,6 +161,7 @@ struct msm_dsi_host {
>   
>   	struct drm_display_mode *mode;
>   	struct drm_dsc_config *dsc;
> +	unsigned int dsc_slice_per_pkt;
>   
>   	/* connected device info */
>   	unsigned int channel;
> @@ -857,17 +858,10 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
>   	slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>   
>   	total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
> -	bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
> +	bytes_per_pkt = dsc->slice_chunk_size * msm_host->dsc_slice_per_pkt;
>   
>   	eol_byte_num = total_bytes_per_intf % 3;
> -
> -	/*
> -	 * Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
> -	 *
> -	 * Since the current driver only supports slice_per_pkt = 1,
> -	 * pkt_per_line will be equal to slice per intf for now.
> -	 */
> -	pkt_per_line = slice_per_intf;
> +	pkt_per_line = slice_per_intf / msm_host->dsc_slice_per_pkt;
>   
>   	if (is_cmd_mode) /* packet data type */
>   		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> @@ -1004,12 +998,8 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   		else
>   			/*
>   			 * When DSC is enabled, WC = slice_chunk_size * slice_per_pkt + 1.
> -			 * Currently, the driver only supports default value of slice_per_pkt = 1
> -			 *
> -			 * TODO: Expand mipi_dsi_device struct to hold slice_per_pkt info
> -			 *       and adjust DSC math to account for slice_per_pkt.
>   			 */
> -			wc = msm_host->dsc->slice_chunk_size + 1;
> +			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc_slice_per_pkt + 1;

Maybe we can reuse bytes_per_pkt here.

>   
>   		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
>   			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> @@ -1636,8 +1626,13 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
>   	msm_host->lanes = dsi->lanes;
>   	msm_host->format = dsi->format;
>   	msm_host->mode_flags = dsi->mode_flags;
> -	if (dsi->dsc)
> +	if (dsi->dsc) {
>   		msm_host->dsc = dsi->dsc;
> +		msm_host->dsc_slice_per_pkt = dsi->dsc_slice_per_pkt;
> +		/* for backwards compatibility, assume 1 if not set */
> +		if (!msm_host->dsc_slice_per_pkt)
> +			msm_host->dsc_slice_per_pkt = 1;
> +	}
>   
>   	/* Some gpios defined in panel DT need to be controlled by host */
>   	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index c9df0407980c..3e32fa52d94b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -193,6 +193,7 @@ struct mipi_dsi_device {
>   	unsigned long hs_rate;
>   	unsigned long lp_rate;
>   	struct drm_dsc_config *dsc;

Any reason for not putting this in drm_dsc_config?

Thanks,

Jessica Zhang

> +	unsigned int dsc_slice_per_pkt;
>   };
>   
>   #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
> -- 
> 2.26.1
>