diff mbox series

[01/17] drm/msm/dpu: allow dpu_encoder_helper_phys_setup_cdm to work for DP

Message ID 20240125193834.7065-2-quic_parellan@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add support for CDM over DP | expand

Commit Message

Paloma Arellano Jan. 25, 2024, 7:38 p.m. UTC
Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.

Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
 2 files changed, 18 insertions(+), 17 deletions(-)

Comments

Dmitry Baryshkov Jan. 25, 2024, 9:14 p.m. UTC | #1
On 25/01/2024 21:38, Paloma Arellano wrote:
> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
> 
> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> ---
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
>   2 files changed, 18 insertions(+), 17 deletions(-)
> 
> 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 993f263433314..37ac385727c3b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>    * @hw_intf:		Hardware interface to the intf registers
>    * @hw_wb:		Hardware interface to the wb registers
>    * @hw_cdm:		Hardware interface to the CDM registers
> + * @cdm_cfg:	CDM block config needed to store WB/DP block's CDM configuration

Please realign the description.

>    * @dpu_kms:		Pointer to the dpu_kms top level
>    * @cached_mode:	DRM mode cached at mode_set time, acted on in enable
>    * @vblank_ctl_lock:	Vblank ctl mutex lock to protect vblank_refcount
> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>   	struct dpu_hw_intf *hw_intf;
>   	struct dpu_hw_wb *hw_wb;
>   	struct dpu_hw_cdm *hw_cdm;
> +	struct dpu_hw_cdm_cfg cdm_cfg;

It might be slightly better to move it after all the pointers, so after 
the dpu_kms.

>   	struct dpu_kms *dpu_kms;
>   	struct drm_display_mode cached_mode;
>   	struct mutex vblank_ctl_lock;
> @@ -213,7 +215,6 @@ static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>    * @wbirq_refcount:     Reference count of writeback interrupt
>    * @wb_done_timeout_cnt: number of wb done irq timeout errors
>    * @wb_cfg:  writeback block config to store fb related details
> - * @cdm_cfg: cdm block config needed to store writeback block's CDM configuration
>    * @wb_conn: backpointer to writeback connector
>    * @wb_job: backpointer to current writeback job
>    * @dest:   dpu buffer layout for current writeback output buffer
> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>   	atomic_t wbirq_refcount;
>   	int wb_done_timeout_cnt;
>   	struct dpu_hw_wb_cfg wb_cfg;
> -	struct dpu_hw_cdm_cfg cdm_cfg;
>   	struct drm_writeback_connector *wb_conn;
>   	struct drm_writeback_job *wb_job;
>   	struct dpu_hw_fmt_layout dest;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index 4cd2d9e3131a4..072fc6950e496 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -269,28 +269,21 @@ static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>    *                                     This API does not handle DPU_CHROMA_H1V2.
>    * @phys_enc:Pointer to physical encoder
>    */
> -static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> +static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc,
> +					      const struct dpu_format *dpu_fmt,
> +					      u32 output_type)
>   {
>   	struct dpu_hw_cdm *hw_cdm;
>   	struct dpu_hw_cdm_cfg *cdm_cfg;
>   	struct dpu_hw_pingpong *hw_pp;
> -	struct dpu_encoder_phys_wb *wb_enc;
> -	const struct msm_format *format;
> -	const struct dpu_format *dpu_fmt;
> -	struct drm_writeback_job *wb_job;
>   	int ret;
>   
>   	if (!phys_enc)
>   		return;
>   
> -	wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> -	cdm_cfg = &wb_enc->cdm_cfg;
> +	cdm_cfg = &phys_enc->cdm_cfg;
>   	hw_pp = phys_enc->hw_pp;
>   	hw_cdm = phys_enc->hw_cdm;
> -	wb_job = wb_enc->wb_job;
> -
> -	format = msm_framebuffer_format(wb_enc->wb_job->fb);
> -	dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, wb_job->fb->modifier);
>   
>   	if (!hw_cdm)
>   		return;
> @@ -306,10 +299,10 @@ static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>   
>   	memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>   
> -	cdm_cfg->output_width = wb_job->fb->width;
> -	cdm_cfg->output_height = wb_job->fb->height;
> +	cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
> +	cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;

This is a semantic change. Instead of passing the FB size, this passes 
the mode dimensions. They are not guaranteed to be the same, especially 
for the WB case.

>   	cdm_cfg->output_fmt = dpu_fmt;
> -	cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB;
> +	cdm_cfg->output_type = output_type;
>   	cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ?
>   			CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT;
>   	cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l;
> @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup(
>   	struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>   	struct drm_display_mode mode = phys_enc->cached_mode;
>   	struct drm_framebuffer *fb = NULL;
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +	struct drm_writeback_job *wb_job;
> +	const struct msm_format *format;
> +	const struct dpu_format *dpu_fmt;
> +
> +	wb_job = wb_enc->wb_job;
> +	format = msm_framebuffer_format(wb_enc->wb_job->fb);
> +	dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, wb_job->fb->modifier);
>   
>   	DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>   			hw_wb->idx - WB_0, mode.name,
> @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup(
>   
>   	dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>   
> -	dpu_encoder_helper_phys_setup_cdm(phys_enc);
> +	dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB);
>   
>   	dpu_encoder_phys_wb_setup_ctl(phys_enc);
>   }
Paloma Arellano Jan. 27, 2024, 12:39 a.m. UTC | #2
On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
> On 25/01/2024 21:38, Paloma Arellano wrote:
>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>
>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>> ---
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
>>   2 files changed, 18 insertions(+), 17 deletions(-)
>>
>> 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 993f263433314..37ac385727c3b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>    * @hw_intf:        Hardware interface to the intf registers
>>    * @hw_wb:        Hardware interface to the wb registers
>>    * @hw_cdm:        Hardware interface to the CDM registers
>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM 
>> configuration
>
> Please realign the description.
Ack
>
>>    * @dpu_kms:        Pointer to the dpu_kms top level
>>    * @cached_mode:    DRM mode cached at mode_set time, acted on in 
>> enable
>>    * @vblank_ctl_lock:    Vblank ctl mutex lock to protect 
>> vblank_refcount
>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>       struct dpu_hw_intf *hw_intf;
>>       struct dpu_hw_wb *hw_wb;
>>       struct dpu_hw_cdm *hw_cdm;
>> +    struct dpu_hw_cdm_cfg cdm_cfg;
>
> It might be slightly better to move it after all the pointers, so 
> after the dpu_kms.
Ack
>
>>       struct dpu_kms *dpu_kms;
>>       struct drm_display_mode cached_mode;
>>       struct mutex vblank_ctl_lock;
>> @@ -213,7 +215,6 @@ static inline int 
>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>    * @wbirq_refcount:     Reference count of writeback interrupt
>>    * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>    * @wb_cfg:  writeback block config to store fb related details
>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM 
>> configuration
>>    * @wb_conn: backpointer to writeback connector
>>    * @wb_job: backpointer to current writeback job
>>    * @dest:   dpu buffer layout for current writeback output buffer
>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>>       atomic_t wbirq_refcount;
>>       int wb_done_timeout_cnt;
>>       struct dpu_hw_wb_cfg wb_cfg;
>> -    struct dpu_hw_cdm_cfg cdm_cfg;
>>       struct drm_writeback_connector *wb_conn;
>>       struct drm_writeback_job *wb_job;
>>       struct dpu_hw_fmt_layout dest;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> index 4cd2d9e3131a4..072fc6950e496 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> @@ -269,28 +269,21 @@ static void 
>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>    *                                     This API does not handle 
>> DPU_CHROMA_H1V2.
>>    * @phys_enc:Pointer to physical encoder
>>    */
>> -static void dpu_encoder_helper_phys_setup_cdm(struct 
>> dpu_encoder_phys *phys_enc)
>> +static void dpu_encoder_helper_phys_setup_cdm(struct 
>> dpu_encoder_phys *phys_enc,
>> +                          const struct dpu_format *dpu_fmt,
>> +                          u32 output_type)
>>   {
>>       struct dpu_hw_cdm *hw_cdm;
>>       struct dpu_hw_cdm_cfg *cdm_cfg;
>>       struct dpu_hw_pingpong *hw_pp;
>> -    struct dpu_encoder_phys_wb *wb_enc;
>> -    const struct msm_format *format;
>> -    const struct dpu_format *dpu_fmt;
>> -    struct drm_writeback_job *wb_job;
>>       int ret;
>>         if (!phys_enc)
>>           return;
>>   -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>> -    cdm_cfg = &wb_enc->cdm_cfg;
>> +    cdm_cfg = &phys_enc->cdm_cfg;
>>       hw_pp = phys_enc->hw_pp;
>>       hw_cdm = phys_enc->hw_cdm;
>> -    wb_job = wb_enc->wb_job;
>> -
>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, 
>> wb_job->fb->modifier);
>>         if (!hw_cdm)
>>           return;
>> @@ -306,10 +299,10 @@ static void 
>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>         memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>>   -    cdm_cfg->output_width = wb_job->fb->width;
>> -    cdm_cfg->output_height = wb_job->fb->height;
>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>
> This is a semantic change. Instead of passing the FB size, this passes 
> the mode dimensions. They are not guaranteed to be the same, 
> especially for the WB case.
>
>>       cdm_cfg->output_fmt = dpu_fmt;
>> -    cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB;
>> +    cdm_cfg->output_type = output_type;
>>       cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ?
>>               CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT;
>>       cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l;
>> @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup(
>>       struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>       struct drm_display_mode mode = phys_enc->cached_mode;
>>       struct drm_framebuffer *fb = NULL;
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +    struct drm_writeback_job *wb_job;
>> +    const struct msm_format *format;
>> +    const struct dpu_format *dpu_fmt;
>> +
>> +    wb_job = wb_enc->wb_job;
>> +    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>> +    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, 
>> wb_job->fb->modifier);
>>         DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>>               hw_wb->idx - WB_0, mode.name,
>> @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup(
>>         dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>>   -    dpu_encoder_helper_phys_setup_cdm(phys_enc);
>> +    dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, 
>> CDM_CDWN_OUTPUT_WB);
>>         dpu_encoder_phys_wb_setup_ctl(phys_enc);
>>   }
>
Abhinav Kumar Jan. 29, 2024, 3:06 a.m. UTC | #3
On 1/26/2024 4:39 PM, Paloma Arellano wrote:
> 
> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>>
>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>> ---
>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
>>>   2 files changed, 18 insertions(+), 17 deletions(-)
>>>
>>> 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 993f263433314..37ac385727c3b 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>>    * @hw_intf:        Hardware interface to the intf registers
>>>    * @hw_wb:        Hardware interface to the wb registers
>>>    * @hw_cdm:        Hardware interface to the CDM registers
>>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM 
>>> configuration
>>
>> Please realign the description.
> Ack
>>
>>>    * @dpu_kms:        Pointer to the dpu_kms top level
>>>    * @cached_mode:    DRM mode cached at mode_set time, acted on in 
>>> enable
>>>    * @vblank_ctl_lock:    Vblank ctl mutex lock to protect 
>>> vblank_refcount
>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>>       struct dpu_hw_intf *hw_intf;
>>>       struct dpu_hw_wb *hw_wb;
>>>       struct dpu_hw_cdm *hw_cdm;
>>> +    struct dpu_hw_cdm_cfg cdm_cfg;
>>
>> It might be slightly better to move it after all the pointers, so 
>> after the dpu_kms.
> Ack
>>
>>>       struct dpu_kms *dpu_kms;
>>>       struct drm_display_mode cached_mode;
>>>       struct mutex vblank_ctl_lock;
>>> @@ -213,7 +215,6 @@ static inline int 
>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>    * @wbirq_refcount:     Reference count of writeback interrupt
>>>    * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>>    * @wb_cfg:  writeback block config to store fb related details
>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM 
>>> configuration
>>>    * @wb_conn: backpointer to writeback connector
>>>    * @wb_job: backpointer to current writeback job
>>>    * @dest:   dpu buffer layout for current writeback output buffer
>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>>>       atomic_t wbirq_refcount;
>>>       int wb_done_timeout_cnt;
>>>       struct dpu_hw_wb_cfg wb_cfg;
>>> -    struct dpu_hw_cdm_cfg cdm_cfg;
>>>       struct drm_writeback_connector *wb_conn;
>>>       struct drm_writeback_job *wb_job;
>>>       struct dpu_hw_fmt_layout dest;
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> index 4cd2d9e3131a4..072fc6950e496 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> @@ -269,28 +269,21 @@ static void 
>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>>    *                                     This API does not handle 
>>> DPU_CHROMA_H1V2.
>>>    * @phys_enc:Pointer to physical encoder
>>>    */
>>> -static void dpu_encoder_helper_phys_setup_cdm(struct 
>>> dpu_encoder_phys *phys_enc)
>>> +static void dpu_encoder_helper_phys_setup_cdm(struct 
>>> dpu_encoder_phys *phys_enc,
>>> +                          const struct dpu_format *dpu_fmt,
>>> +                          u32 output_type)
>>>   {
>>>       struct dpu_hw_cdm *hw_cdm;
>>>       struct dpu_hw_cdm_cfg *cdm_cfg;
>>>       struct dpu_hw_pingpong *hw_pp;
>>> -    struct dpu_encoder_phys_wb *wb_enc;
>>> -    const struct msm_format *format;
>>> -    const struct dpu_format *dpu_fmt;
>>> -    struct drm_writeback_job *wb_job;
>>>       int ret;
>>>         if (!phys_enc)
>>>           return;
>>>   -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>>> -    cdm_cfg = &wb_enc->cdm_cfg;
>>> +    cdm_cfg = &phys_enc->cdm_cfg;
>>>       hw_pp = phys_enc->hw_pp;
>>>       hw_cdm = phys_enc->hw_cdm;
>>> -    wb_job = wb_enc->wb_job;
>>> -
>>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, 
>>> wb_job->fb->modifier);
>>>         if (!hw_cdm)
>>>           return;
>>> @@ -306,10 +299,10 @@ static void 
>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>>         memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>>>   -    cdm_cfg->output_width = wb_job->fb->width;
>>> -    cdm_cfg->output_height = wb_job->fb->height;
>>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>
>> This is a semantic change. Instead of passing the FB size, this passes 
>> the mode dimensions. They are not guaranteed to be the same, 
>> especially for the WB case.
>>

The WB job is storing the output FB of WB. I cannot think of a use-case 
where this cannot match the current mode programmed to the WB encoder.

Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the 
plane can scale the contents but here thats not the case. Here its the 
output FB of WB.

>>>       cdm_cfg->output_fmt = dpu_fmt;
>>> -    cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB;
>>> +    cdm_cfg->output_type = output_type;
>>>       cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ?
>>>               CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT;
>>>       cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l;
>>> @@ -462,6 +455,14 @@ static void dpu_encoder_phys_wb_setup(
>>>       struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>>       struct drm_display_mode mode = phys_enc->cached_mode;
>>>       struct drm_framebuffer *fb = NULL;
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +    struct drm_writeback_job *wb_job;
>>> +    const struct msm_format *format;
>>> +    const struct dpu_format *dpu_fmt;
>>> +
>>> +    wb_job = wb_enc->wb_job;
>>> +    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>> +    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, 
>>> wb_job->fb->modifier);
>>>         DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>>>               hw_wb->idx - WB_0, mode.name,
>>> @@ -475,7 +476,7 @@ static void dpu_encoder_phys_wb_setup(
>>>         dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>>>   -    dpu_encoder_helper_phys_setup_cdm(phys_enc);
>>> +    dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, 
>>> CDM_CDWN_OUTPUT_WB);
>>>         dpu_encoder_phys_wb_setup_ctl(phys_enc);
>>>   }
>>
Dmitry Baryshkov Jan. 29, 2024, 3:23 a.m. UTC | #4
On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
> >
> > On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
> >> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
> >>>
> >>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>> ---
> >>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
> >>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
> >>>   2 files changed, 18 insertions(+), 17 deletions(-)
> >>>
> >>> 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 993f263433314..37ac385727c3b 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
> >>>    * @hw_intf:        Hardware interface to the intf registers
> >>>    * @hw_wb:        Hardware interface to the wb registers
> >>>    * @hw_cdm:        Hardware interface to the CDM registers
> >>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM
> >>> configuration
> >>
> >> Please realign the description.
> > Ack
> >>
> >>>    * @dpu_kms:        Pointer to the dpu_kms top level
> >>>    * @cached_mode:    DRM mode cached at mode_set time, acted on in
> >>> enable
> >>>    * @vblank_ctl_lock:    Vblank ctl mutex lock to protect
> >>> vblank_refcount
> >>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
> >>>       struct dpu_hw_intf *hw_intf;
> >>>       struct dpu_hw_wb *hw_wb;
> >>>       struct dpu_hw_cdm *hw_cdm;
> >>> +    struct dpu_hw_cdm_cfg cdm_cfg;
> >>
> >> It might be slightly better to move it after all the pointers, so
> >> after the dpu_kms.
> > Ack
> >>
> >>>       struct dpu_kms *dpu_kms;
> >>>       struct drm_display_mode cached_mode;
> >>>       struct mutex vblank_ctl_lock;
> >>> @@ -213,7 +215,6 @@ static inline int
> >>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
> >>>    * @wbirq_refcount:     Reference count of writeback interrupt
> >>>    * @wb_done_timeout_cnt: number of wb done irq timeout errors
> >>>    * @wb_cfg:  writeback block config to store fb related details
> >>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
> >>> configuration
> >>>    * @wb_conn: backpointer to writeback connector
> >>>    * @wb_job: backpointer to current writeback job
> >>>    * @dest:   dpu buffer layout for current writeback output buffer
> >>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
> >>>       atomic_t wbirq_refcount;
> >>>       int wb_done_timeout_cnt;
> >>>       struct dpu_hw_wb_cfg wb_cfg;
> >>> -    struct dpu_hw_cdm_cfg cdm_cfg;
> >>>       struct drm_writeback_connector *wb_conn;
> >>>       struct drm_writeback_job *wb_job;
> >>>       struct dpu_hw_fmt_layout dest;
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>> index 4cd2d9e3131a4..072fc6950e496 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>> @@ -269,28 +269,21 @@ static void
> >>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
> >>>    *                                     This API does not handle
> >>> DPU_CHROMA_H1V2.
> >>>    * @phys_enc:Pointer to physical encoder
> >>>    */
> >>> -static void dpu_encoder_helper_phys_setup_cdm(struct
> >>> dpu_encoder_phys *phys_enc)
> >>> +static void dpu_encoder_helper_phys_setup_cdm(struct
> >>> dpu_encoder_phys *phys_enc,
> >>> +                          const struct dpu_format *dpu_fmt,
> >>> +                          u32 output_type)
> >>>   {
> >>>       struct dpu_hw_cdm *hw_cdm;
> >>>       struct dpu_hw_cdm_cfg *cdm_cfg;
> >>>       struct dpu_hw_pingpong *hw_pp;
> >>> -    struct dpu_encoder_phys_wb *wb_enc;
> >>> -    const struct msm_format *format;
> >>> -    const struct dpu_format *dpu_fmt;
> >>> -    struct drm_writeback_job *wb_job;
> >>>       int ret;
> >>>         if (!phys_enc)
> >>>           return;
> >>>   -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> >>> -    cdm_cfg = &wb_enc->cdm_cfg;
> >>> +    cdm_cfg = &phys_enc->cdm_cfg;
> >>>       hw_pp = phys_enc->hw_pp;
> >>>       hw_cdm = phys_enc->hw_cdm;
> >>> -    wb_job = wb_enc->wb_job;
> >>> -
> >>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
> >>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
> >>> wb_job->fb->modifier);
> >>>         if (!hw_cdm)
> >>>           return;
> >>> @@ -306,10 +299,10 @@ static void
> >>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> >>>         memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
> >>>   -    cdm_cfg->output_width = wb_job->fb->width;
> >>> -    cdm_cfg->output_height = wb_job->fb->height;
> >>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
> >>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
> >>
> >> This is a semantic change. Instead of passing the FB size, this passes
> >> the mode dimensions. They are not guaranteed to be the same,
> >> especially for the WB case.
> >>
>
> The WB job is storing the output FB of WB. I cannot think of a use-case
> where this cannot match the current mode programmed to the WB encoder.
>
> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
> plane can scale the contents but here thats not the case. Here its the
> output FB of WB.

Is it a part of WB uAPI, to have the FB dimensions equal to mode
dimensions? Or is it just our current limitation? I can easily imagine
WB outputting data to a part of the FB (just like we can clip FB using
plane's clip rectangle).

This boils down to a question, whether CDM should be setup in terms of
actual output date or the physical memory buffer parameters. I suspect
the former is the case (which makes this change correct). But it
either should be described in the commit message or (even better)
split to a separate commit.
Abhinav Kumar Jan. 29, 2024, 4 a.m. UTC | #5
On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
>>>
>>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>>>>
>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>> ---
>>>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>>>>>    .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
>>>>>    2 files changed, 18 insertions(+), 17 deletions(-)
>>>>>
>>>>> 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 993f263433314..37ac385727c3b 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>>>>     * @hw_intf:        Hardware interface to the intf registers
>>>>>     * @hw_wb:        Hardware interface to the wb registers
>>>>>     * @hw_cdm:        Hardware interface to the CDM registers
>>>>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM
>>>>> configuration
>>>>
>>>> Please realign the description.
>>> Ack
>>>>
>>>>>     * @dpu_kms:        Pointer to the dpu_kms top level
>>>>>     * @cached_mode:    DRM mode cached at mode_set time, acted on in
>>>>> enable
>>>>>     * @vblank_ctl_lock:    Vblank ctl mutex lock to protect
>>>>> vblank_refcount
>>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>>>>        struct dpu_hw_intf *hw_intf;
>>>>>        struct dpu_hw_wb *hw_wb;
>>>>>        struct dpu_hw_cdm *hw_cdm;
>>>>> +    struct dpu_hw_cdm_cfg cdm_cfg;
>>>>
>>>> It might be slightly better to move it after all the pointers, so
>>>> after the dpu_kms.
>>> Ack
>>>>
>>>>>        struct dpu_kms *dpu_kms;
>>>>>        struct drm_display_mode cached_mode;
>>>>>        struct mutex vblank_ctl_lock;
>>>>> @@ -213,7 +215,6 @@ static inline int
>>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>>>     * @wbirq_refcount:     Reference count of writeback interrupt
>>>>>     * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>>>>     * @wb_cfg:  writeback block config to store fb related details
>>>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
>>>>> configuration
>>>>>     * @wb_conn: backpointer to writeback connector
>>>>>     * @wb_job: backpointer to current writeback job
>>>>>     * @dest:   dpu buffer layout for current writeback output buffer
>>>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>>>>>        atomic_t wbirq_refcount;
>>>>>        int wb_done_timeout_cnt;
>>>>>        struct dpu_hw_wb_cfg wb_cfg;
>>>>> -    struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>        struct drm_writeback_connector *wb_conn;
>>>>>        struct drm_writeback_job *wb_job;
>>>>>        struct dpu_hw_fmt_layout dest;
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>> index 4cd2d9e3131a4..072fc6950e496 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>> @@ -269,28 +269,21 @@ static void
>>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>>>>     *                                     This API does not handle
>>>>> DPU_CHROMA_H1V2.
>>>>>     * @phys_enc:Pointer to physical encoder
>>>>>     */
>>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>> dpu_encoder_phys *phys_enc)
>>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>> dpu_encoder_phys *phys_enc,
>>>>> +                          const struct dpu_format *dpu_fmt,
>>>>> +                          u32 output_type)
>>>>>    {
>>>>>        struct dpu_hw_cdm *hw_cdm;
>>>>>        struct dpu_hw_cdm_cfg *cdm_cfg;
>>>>>        struct dpu_hw_pingpong *hw_pp;
>>>>> -    struct dpu_encoder_phys_wb *wb_enc;
>>>>> -    const struct msm_format *format;
>>>>> -    const struct dpu_format *dpu_fmt;
>>>>> -    struct drm_writeback_job *wb_job;
>>>>>        int ret;
>>>>>          if (!phys_enc)
>>>>>            return;
>>>>>    -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>>>>> -    cdm_cfg = &wb_enc->cdm_cfg;
>>>>> +    cdm_cfg = &phys_enc->cdm_cfg;
>>>>>        hw_pp = phys_enc->hw_pp;
>>>>>        hw_cdm = phys_enc->hw_cdm;
>>>>> -    wb_job = wb_enc->wb_job;
>>>>> -
>>>>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>>>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
>>>>> wb_job->fb->modifier);
>>>>>          if (!hw_cdm)
>>>>>            return;
>>>>> @@ -306,10 +299,10 @@ static void
>>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>>>>          memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>>>>>    -    cdm_cfg->output_width = wb_job->fb->width;
>>>>> -    cdm_cfg->output_height = wb_job->fb->height;
>>>>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>>>>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>>>
>>>> This is a semantic change. Instead of passing the FB size, this passes
>>>> the mode dimensions. They are not guaranteed to be the same,
>>>> especially for the WB case.
>>>>
>>
>> The WB job is storing the output FB of WB. I cannot think of a use-case
>> where this cannot match the current mode programmed to the WB encoder.
>>
>> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
>> plane can scale the contents but here thats not the case. Here its the
>> output FB of WB.
> 
> Is it a part of WB uAPI, to have the FB dimensions equal to mode
> dimensions? Or is it just our current limitation? I can easily imagine
> WB outputting data to a part of the FB (just like we can clip FB using
> plane's clip rectangle).
> 
> This boils down to a question, whether CDM should be setup in terms of
> actual output date or the physical memory buffer parameters. I suspect
> the former is the case (which makes this change correct). But it
> either should be described in the commit message or (even better)
> split to a separate commit.
> 

I would say its a combination of both today.

The way I would look at it is even if WB crops a certain section of FB, 
that will not change the FB size. FB size of WB should match the rest of 
the DRM pipeline (the mode programmed to the CRTC/encoder). If WB 
decides to write to only a small section of FB (cropping), then we need 
another WB property like CROP_ROI so that we can program the WB to only 
write to a small section of the programmed FB. So in some sense, there 
is no such support in DRM UAPI today. Hence the FB of WB is the full 
mode of the WB.

CDM is before WB so follows the rest of the pipeline that is whatever 
the data feeding it was programmed to.

>
Dmitry Baryshkov Jan. 29, 2024, 4:12 a.m. UTC | #6
On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
> > On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
> >>>
> >>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
> >>>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
> >>>>>
> >>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>>> ---
> >>>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
> >>>>>    .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
> >>>>>    2 files changed, 18 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> 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 993f263433314..37ac385727c3b 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
> >>>>>     * @hw_intf:        Hardware interface to the intf registers
> >>>>>     * @hw_wb:        Hardware interface to the wb registers
> >>>>>     * @hw_cdm:        Hardware interface to the CDM registers
> >>>>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM
> >>>>> configuration
> >>>>
> >>>> Please realign the description.
> >>> Ack
> >>>>
> >>>>>     * @dpu_kms:        Pointer to the dpu_kms top level
> >>>>>     * @cached_mode:    DRM mode cached at mode_set time, acted on in
> >>>>> enable
> >>>>>     * @vblank_ctl_lock:    Vblank ctl mutex lock to protect
> >>>>> vblank_refcount
> >>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
> >>>>>        struct dpu_hw_intf *hw_intf;
> >>>>>        struct dpu_hw_wb *hw_wb;
> >>>>>        struct dpu_hw_cdm *hw_cdm;
> >>>>> +    struct dpu_hw_cdm_cfg cdm_cfg;
> >>>>
> >>>> It might be slightly better to move it after all the pointers, so
> >>>> after the dpu_kms.
> >>> Ack
> >>>>
> >>>>>        struct dpu_kms *dpu_kms;
> >>>>>        struct drm_display_mode cached_mode;
> >>>>>        struct mutex vblank_ctl_lock;
> >>>>> @@ -213,7 +215,6 @@ static inline int
> >>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
> >>>>>     * @wbirq_refcount:     Reference count of writeback interrupt
> >>>>>     * @wb_done_timeout_cnt: number of wb done irq timeout errors
> >>>>>     * @wb_cfg:  writeback block config to store fb related details
> >>>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
> >>>>> configuration
> >>>>>     * @wb_conn: backpointer to writeback connector
> >>>>>     * @wb_job: backpointer to current writeback job
> >>>>>     * @dest:   dpu buffer layout for current writeback output buffer
> >>>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
> >>>>>        atomic_t wbirq_refcount;
> >>>>>        int wb_done_timeout_cnt;
> >>>>>        struct dpu_hw_wb_cfg wb_cfg;
> >>>>> -    struct dpu_hw_cdm_cfg cdm_cfg;
> >>>>>        struct drm_writeback_connector *wb_conn;
> >>>>>        struct drm_writeback_job *wb_job;
> >>>>>        struct dpu_hw_fmt_layout dest;
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>> index 4cd2d9e3131a4..072fc6950e496 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>> @@ -269,28 +269,21 @@ static void
> >>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
> >>>>>     *                                     This API does not handle
> >>>>> DPU_CHROMA_H1V2.
> >>>>>     * @phys_enc:Pointer to physical encoder
> >>>>>     */
> >>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
> >>>>> dpu_encoder_phys *phys_enc)
> >>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
> >>>>> dpu_encoder_phys *phys_enc,
> >>>>> +                          const struct dpu_format *dpu_fmt,
> >>>>> +                          u32 output_type)
> >>>>>    {
> >>>>>        struct dpu_hw_cdm *hw_cdm;
> >>>>>        struct dpu_hw_cdm_cfg *cdm_cfg;
> >>>>>        struct dpu_hw_pingpong *hw_pp;
> >>>>> -    struct dpu_encoder_phys_wb *wb_enc;
> >>>>> -    const struct msm_format *format;
> >>>>> -    const struct dpu_format *dpu_fmt;
> >>>>> -    struct drm_writeback_job *wb_job;
> >>>>>        int ret;
> >>>>>          if (!phys_enc)
> >>>>>            return;
> >>>>>    -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> >>>>> -    cdm_cfg = &wb_enc->cdm_cfg;
> >>>>> +    cdm_cfg = &phys_enc->cdm_cfg;
> >>>>>        hw_pp = phys_enc->hw_pp;
> >>>>>        hw_cdm = phys_enc->hw_cdm;
> >>>>> -    wb_job = wb_enc->wb_job;
> >>>>> -
> >>>>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
> >>>>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
> >>>>> wb_job->fb->modifier);
> >>>>>          if (!hw_cdm)
> >>>>>            return;
> >>>>> @@ -306,10 +299,10 @@ static void
> >>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> >>>>>          memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
> >>>>>    -    cdm_cfg->output_width = wb_job->fb->width;
> >>>>> -    cdm_cfg->output_height = wb_job->fb->height;
> >>>>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
> >>>>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
> >>>>
> >>>> This is a semantic change. Instead of passing the FB size, this passes
> >>>> the mode dimensions. They are not guaranteed to be the same,
> >>>> especially for the WB case.
> >>>>
> >>
> >> The WB job is storing the output FB of WB. I cannot think of a use-case
> >> where this cannot match the current mode programmed to the WB encoder.
> >>
> >> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
> >> plane can scale the contents but here thats not the case. Here its the
> >> output FB of WB.
> >
> > Is it a part of WB uAPI, to have the FB dimensions equal to mode
> > dimensions? Or is it just our current limitation? I can easily imagine
> > WB outputting data to a part of the FB (just like we can clip FB using
> > plane's clip rectangle).
> >
> > This boils down to a question, whether CDM should be setup in terms of
> > actual output date or the physical memory buffer parameters. I suspect
> > the former is the case (which makes this change correct). But it
> > either should be described in the commit message or (even better)
> > split to a separate commit.
> >
>
> I would say its a combination of both today.
>
> The way I would look at it is even if WB crops a certain section of FB,
> that will not change the FB size. FB size of WB should match the rest of
> the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
> decides to write to only a small section of FB (cropping), then we need
> another WB property like CROP_ROI so that we can program the WB to only
> write to a small section of the programmed FB. So in some sense, there
> is no such support in DRM UAPI today. Hence the FB of WB is the full
> mode of the WB.

 I'd say, CROP_ROI can refer to cropping of the image source (esp. in
the cloned output case). For writing to the part of the FB there can
be DST_X/_Y/_W/_H properties. But this becomes off-topic.

> CDM is before WB so follows the rest of the pipeline that is whatever
> the data feeding it was programmed to.

Yes. So the change is correct, but it should be split or documented
properly. I prefer the first option.
Abhinav Kumar Jan. 29, 2024, 4:33 a.m. UTC | #7
On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
>>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
>>>>>
>>>>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>>>>>>
>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>>>> ---
>>>>>>>     .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>>>>>>>     .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
>>>>>>>     2 files changed, 18 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> 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 993f263433314..37ac385727c3b 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>>>>>>      * @hw_intf:        Hardware interface to the intf registers
>>>>>>>      * @hw_wb:        Hardware interface to the wb registers
>>>>>>>      * @hw_cdm:        Hardware interface to the CDM registers
>>>>>>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM
>>>>>>> configuration
>>>>>>
>>>>>> Please realign the description.
>>>>> Ack
>>>>>>
>>>>>>>      * @dpu_kms:        Pointer to the dpu_kms top level
>>>>>>>      * @cached_mode:    DRM mode cached at mode_set time, acted on in
>>>>>>> enable
>>>>>>>      * @vblank_ctl_lock:    Vblank ctl mutex lock to protect
>>>>>>> vblank_refcount
>>>>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>>>>>>         struct dpu_hw_intf *hw_intf;
>>>>>>>         struct dpu_hw_wb *hw_wb;
>>>>>>>         struct dpu_hw_cdm *hw_cdm;
>>>>>>> +    struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>>
>>>>>> It might be slightly better to move it after all the pointers, so
>>>>>> after the dpu_kms.
>>>>> Ack
>>>>>>
>>>>>>>         struct dpu_kms *dpu_kms;
>>>>>>>         struct drm_display_mode cached_mode;
>>>>>>>         struct mutex vblank_ctl_lock;
>>>>>>> @@ -213,7 +215,6 @@ static inline int
>>>>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>>>>>      * @wbirq_refcount:     Reference count of writeback interrupt
>>>>>>>      * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>>>>>>      * @wb_cfg:  writeback block config to store fb related details
>>>>>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
>>>>>>> configuration
>>>>>>>      * @wb_conn: backpointer to writeback connector
>>>>>>>      * @wb_job: backpointer to current writeback job
>>>>>>>      * @dest:   dpu buffer layout for current writeback output buffer
>>>>>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>>>>>>>         atomic_t wbirq_refcount;
>>>>>>>         int wb_done_timeout_cnt;
>>>>>>>         struct dpu_hw_wb_cfg wb_cfg;
>>>>>>> -    struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>>>         struct drm_writeback_connector *wb_conn;
>>>>>>>         struct drm_writeback_job *wb_job;
>>>>>>>         struct dpu_hw_fmt_layout dest;
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> index 4cd2d9e3131a4..072fc6950e496 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>> @@ -269,28 +269,21 @@ static void
>>>>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>>>>>>      *                                     This API does not handle
>>>>>>> DPU_CHROMA_H1V2.
>>>>>>>      * @phys_enc:Pointer to physical encoder
>>>>>>>      */
>>>>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>>>> dpu_encoder_phys *phys_enc)
>>>>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>>>> dpu_encoder_phys *phys_enc,
>>>>>>> +                          const struct dpu_format *dpu_fmt,
>>>>>>> +                          u32 output_type)
>>>>>>>     {
>>>>>>>         struct dpu_hw_cdm *hw_cdm;
>>>>>>>         struct dpu_hw_cdm_cfg *cdm_cfg;
>>>>>>>         struct dpu_hw_pingpong *hw_pp;
>>>>>>> -    struct dpu_encoder_phys_wb *wb_enc;
>>>>>>> -    const struct msm_format *format;
>>>>>>> -    const struct dpu_format *dpu_fmt;
>>>>>>> -    struct drm_writeback_job *wb_job;
>>>>>>>         int ret;
>>>>>>>           if (!phys_enc)
>>>>>>>             return;
>>>>>>>     -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>>>>>>> -    cdm_cfg = &wb_enc->cdm_cfg;
>>>>>>> +    cdm_cfg = &phys_enc->cdm_cfg;
>>>>>>>         hw_pp = phys_enc->hw_pp;
>>>>>>>         hw_cdm = phys_enc->hw_cdm;
>>>>>>> -    wb_job = wb_enc->wb_job;
>>>>>>> -
>>>>>>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>>>>>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
>>>>>>> wb_job->fb->modifier);
>>>>>>>           if (!hw_cdm)
>>>>>>>             return;
>>>>>>> @@ -306,10 +299,10 @@ static void
>>>>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>>>>>>           memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>>>>>>>     -    cdm_cfg->output_width = wb_job->fb->width;
>>>>>>> -    cdm_cfg->output_height = wb_job->fb->height;
>>>>>>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>>>>>>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>>>>>
>>>>>> This is a semantic change. Instead of passing the FB size, this passes
>>>>>> the mode dimensions. They are not guaranteed to be the same,
>>>>>> especially for the WB case.
>>>>>>
>>>>
>>>> The WB job is storing the output FB of WB. I cannot think of a use-case
>>>> where this cannot match the current mode programmed to the WB encoder.
>>>>
>>>> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
>>>> plane can scale the contents but here thats not the case. Here its the
>>>> output FB of WB.
>>>
>>> Is it a part of WB uAPI, to have the FB dimensions equal to mode
>>> dimensions? Or is it just our current limitation? I can easily imagine
>>> WB outputting data to a part of the FB (just like we can clip FB using
>>> plane's clip rectangle).
>>>
>>> This boils down to a question, whether CDM should be setup in terms of
>>> actual output date or the physical memory buffer parameters. I suspect
>>> the former is the case (which makes this change correct). But it
>>> either should be described in the commit message or (even better)
>>> split to a separate commit.
>>>
>>
>> I would say its a combination of both today.
>>
>> The way I would look at it is even if WB crops a certain section of FB,
>> that will not change the FB size. FB size of WB should match the rest of
>> the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
>> decides to write to only a small section of FB (cropping), then we need
>> another WB property like CROP_ROI so that we can program the WB to only
>> write to a small section of the programmed FB. So in some sense, there
>> is no such support in DRM UAPI today. Hence the FB of WB is the full
>> mode of the WB.
> 
>   I'd say, CROP_ROI can refer to cropping of the image source (esp. in
> the cloned output case). For writing to the part of the FB there can
> be DST_X/_Y/_W/_H properties. But this becomes off-topic.
> 
>> CDM is before WB so follows the rest of the pipeline that is whatever
>> the data feeding it was programmed to.
> 
> Yes. So the change is correct, but it should be split or documented
> properly. I prefer the first option.
> 

Ok just to clarify you prefer below part of the change to be moved to 
its own commit right?

-    cdm_cfg->output_width = wb_job->fb->width;
-    cdm_cfg->output_height = wb_job->fb->height;
+    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
+    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;

If so, ack.

>
Dmitry Baryshkov Jan. 29, 2024, 5:12 a.m. UTC | #8
On Mon, 29 Jan 2024 at 06:33, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote:
> > On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
> >>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
> >>>>>
> >>>>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
> >>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
> >>>>>>>
> >>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>>>>> ---
> >>>>>>>     .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
> >>>>>>>     .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
> >>>>>>>     2 files changed, 18 insertions(+), 17 deletions(-)
> >>>>>>>
> >>>>>>> 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 993f263433314..37ac385727c3b 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
> >>>>>>>      * @hw_intf:        Hardware interface to the intf registers
> >>>>>>>      * @hw_wb:        Hardware interface to the wb registers
> >>>>>>>      * @hw_cdm:        Hardware interface to the CDM registers
> >>>>>>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM
> >>>>>>> configuration
> >>>>>>
> >>>>>> Please realign the description.
> >>>>> Ack
> >>>>>>
> >>>>>>>      * @dpu_kms:        Pointer to the dpu_kms top level
> >>>>>>>      * @cached_mode:    DRM mode cached at mode_set time, acted on in
> >>>>>>> enable
> >>>>>>>      * @vblank_ctl_lock:    Vblank ctl mutex lock to protect
> >>>>>>> vblank_refcount
> >>>>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
> >>>>>>>         struct dpu_hw_intf *hw_intf;
> >>>>>>>         struct dpu_hw_wb *hw_wb;
> >>>>>>>         struct dpu_hw_cdm *hw_cdm;
> >>>>>>> +    struct dpu_hw_cdm_cfg cdm_cfg;
> >>>>>>
> >>>>>> It might be slightly better to move it after all the pointers, so
> >>>>>> after the dpu_kms.
> >>>>> Ack
> >>>>>>
> >>>>>>>         struct dpu_kms *dpu_kms;
> >>>>>>>         struct drm_display_mode cached_mode;
> >>>>>>>         struct mutex vblank_ctl_lock;
> >>>>>>> @@ -213,7 +215,6 @@ static inline int
> >>>>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
> >>>>>>>      * @wbirq_refcount:     Reference count of writeback interrupt
> >>>>>>>      * @wb_done_timeout_cnt: number of wb done irq timeout errors
> >>>>>>>      * @wb_cfg:  writeback block config to store fb related details
> >>>>>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
> >>>>>>> configuration
> >>>>>>>      * @wb_conn: backpointer to writeback connector
> >>>>>>>      * @wb_job: backpointer to current writeback job
> >>>>>>>      * @dest:   dpu buffer layout for current writeback output buffer
> >>>>>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
> >>>>>>>         atomic_t wbirq_refcount;
> >>>>>>>         int wb_done_timeout_cnt;
> >>>>>>>         struct dpu_hw_wb_cfg wb_cfg;
> >>>>>>> -    struct dpu_hw_cdm_cfg cdm_cfg;
> >>>>>>>         struct drm_writeback_connector *wb_conn;
> >>>>>>>         struct drm_writeback_job *wb_job;
> >>>>>>>         struct dpu_hw_fmt_layout dest;
> >>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>>>> index 4cd2d9e3131a4..072fc6950e496 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> >>>>>>> @@ -269,28 +269,21 @@ static void
> >>>>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
> >>>>>>>      *                                     This API does not handle
> >>>>>>> DPU_CHROMA_H1V2.
> >>>>>>>      * @phys_enc:Pointer to physical encoder
> >>>>>>>      */
> >>>>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
> >>>>>>> dpu_encoder_phys *phys_enc)
> >>>>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
> >>>>>>> dpu_encoder_phys *phys_enc,
> >>>>>>> +                          const struct dpu_format *dpu_fmt,
> >>>>>>> +                          u32 output_type)
> >>>>>>>     {
> >>>>>>>         struct dpu_hw_cdm *hw_cdm;
> >>>>>>>         struct dpu_hw_cdm_cfg *cdm_cfg;
> >>>>>>>         struct dpu_hw_pingpong *hw_pp;
> >>>>>>> -    struct dpu_encoder_phys_wb *wb_enc;
> >>>>>>> -    const struct msm_format *format;
> >>>>>>> -    const struct dpu_format *dpu_fmt;
> >>>>>>> -    struct drm_writeback_job *wb_job;
> >>>>>>>         int ret;
> >>>>>>>           if (!phys_enc)
> >>>>>>>             return;
> >>>>>>>     -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> >>>>>>> -    cdm_cfg = &wb_enc->cdm_cfg;
> >>>>>>> +    cdm_cfg = &phys_enc->cdm_cfg;
> >>>>>>>         hw_pp = phys_enc->hw_pp;
> >>>>>>>         hw_cdm = phys_enc->hw_cdm;
> >>>>>>> -    wb_job = wb_enc->wb_job;
> >>>>>>> -
> >>>>>>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
> >>>>>>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
> >>>>>>> wb_job->fb->modifier);
> >>>>>>>           if (!hw_cdm)
> >>>>>>>             return;
> >>>>>>> @@ -306,10 +299,10 @@ static void
> >>>>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
> >>>>>>>           memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
> >>>>>>>     -    cdm_cfg->output_width = wb_job->fb->width;
> >>>>>>> -    cdm_cfg->output_height = wb_job->fb->height;
> >>>>>>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
> >>>>>>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
> >>>>>>
> >>>>>> This is a semantic change. Instead of passing the FB size, this passes
> >>>>>> the mode dimensions. They are not guaranteed to be the same,
> >>>>>> especially for the WB case.
> >>>>>>
> >>>>
> >>>> The WB job is storing the output FB of WB. I cannot think of a use-case
> >>>> where this cannot match the current mode programmed to the WB encoder.
> >>>>
> >>>> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
> >>>> plane can scale the contents but here thats not the case. Here its the
> >>>> output FB of WB.
> >>>
> >>> Is it a part of WB uAPI, to have the FB dimensions equal to mode
> >>> dimensions? Or is it just our current limitation? I can easily imagine
> >>> WB outputting data to a part of the FB (just like we can clip FB using
> >>> plane's clip rectangle).
> >>>
> >>> This boils down to a question, whether CDM should be setup in terms of
> >>> actual output date or the physical memory buffer parameters. I suspect
> >>> the former is the case (which makes this change correct). But it
> >>> either should be described in the commit message or (even better)
> >>> split to a separate commit.
> >>>
> >>
> >> I would say its a combination of both today.
> >>
> >> The way I would look at it is even if WB crops a certain section of FB,
> >> that will not change the FB size. FB size of WB should match the rest of
> >> the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
> >> decides to write to only a small section of FB (cropping), then we need
> >> another WB property like CROP_ROI so that we can program the WB to only
> >> write to a small section of the programmed FB. So in some sense, there
> >> is no such support in DRM UAPI today. Hence the FB of WB is the full
> >> mode of the WB.
> >
> >   I'd say, CROP_ROI can refer to cropping of the image source (esp. in
> > the cloned output case). For writing to the part of the FB there can
> > be DST_X/_Y/_W/_H properties. But this becomes off-topic.
> >
> >> CDM is before WB so follows the rest of the pipeline that is whatever
> >> the data feeding it was programmed to.
> >
> > Yes. So the change is correct, but it should be split or documented
> > properly. I prefer the first option.
> >
>
> Ok just to clarify you prefer below part of the change to be moved to
> its own commit right?
>
> -    cdm_cfg->output_width = wb_job->fb->width;
> -    cdm_cfg->output_height = wb_job->fb->height;
> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>
> If so, ack.

Yes.
Paloma Arellano Jan. 29, 2024, 11:06 p.m. UTC | #9
On 1/28/2024 9:12 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 06:33, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>> On 1/28/2024 8:12 PM, Dmitry Baryshkov wrote:
>>> On Mon, 29 Jan 2024 at 06:01, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>> On 1/28/2024 7:23 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, 29 Jan 2024 at 05:06, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 1/26/2024 4:39 PM, Paloma Arellano wrote:
>>>>>>> On 1/25/2024 1:14 PM, Dmitry Baryshkov wrote:
>>>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>>>>>> Generalize dpu_encoder_helper_phys_setup_cdm to be compatible with DP.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>>>>>> ---
>>>>>>>>>      .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  4 +--
>>>>>>>>>      .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 31 ++++++++++---------
>>>>>>>>>      2 files changed, 18 insertions(+), 17 deletions(-)
>>>>>>>>>
>>>>>>>>> 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 993f263433314..37ac385727c3b 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>>>>> @@ -153,6 +153,7 @@ enum dpu_intr_idx {
>>>>>>>>>       * @hw_intf:        Hardware interface to the intf registers
>>>>>>>>>       * @hw_wb:        Hardware interface to the wb registers
>>>>>>>>>       * @hw_cdm:        Hardware interface to the CDM registers
>>>>>>>>> + * @cdm_cfg:    CDM block config needed to store WB/DP block's CDM
>>>>>>>>> configuration
>>>>>>>> Please realign the description.
>>>>>>> Ack
>>>>>>>>>       * @dpu_kms:        Pointer to the dpu_kms top level
>>>>>>>>>       * @cached_mode:    DRM mode cached at mode_set time, acted on in
>>>>>>>>> enable
>>>>>>>>>       * @vblank_ctl_lock:    Vblank ctl mutex lock to protect
>>>>>>>>> vblank_refcount
>>>>>>>>> @@ -183,6 +184,7 @@ struct dpu_encoder_phys {
>>>>>>>>>          struct dpu_hw_intf *hw_intf;
>>>>>>>>>          struct dpu_hw_wb *hw_wb;
>>>>>>>>>          struct dpu_hw_cdm *hw_cdm;
>>>>>>>>> +    struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>>>> It might be slightly better to move it after all the pointers, so
>>>>>>>> after the dpu_kms.
>>>>>>> Ack
>>>>>>>>>          struct dpu_kms *dpu_kms;
>>>>>>>>>          struct drm_display_mode cached_mode;
>>>>>>>>>          struct mutex vblank_ctl_lock;
>>>>>>>>> @@ -213,7 +215,6 @@ static inline int
>>>>>>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>>>>>>>       * @wbirq_refcount:     Reference count of writeback interrupt
>>>>>>>>>       * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>>>>>>>>       * @wb_cfg:  writeback block config to store fb related details
>>>>>>>>> - * @cdm_cfg: cdm block config needed to store writeback block's CDM
>>>>>>>>> configuration
>>>>>>>>>       * @wb_conn: backpointer to writeback connector
>>>>>>>>>       * @wb_job: backpointer to current writeback job
>>>>>>>>>       * @dest:   dpu buffer layout for current writeback output buffer
>>>>>>>>> @@ -223,7 +224,6 @@ struct dpu_encoder_phys_wb {
>>>>>>>>>          atomic_t wbirq_refcount;
>>>>>>>>>          int wb_done_timeout_cnt;
>>>>>>>>>          struct dpu_hw_wb_cfg wb_cfg;
>>>>>>>>> -    struct dpu_hw_cdm_cfg cdm_cfg;
>>>>>>>>>          struct drm_writeback_connector *wb_conn;
>>>>>>>>>          struct drm_writeback_job *wb_job;
>>>>>>>>>          struct dpu_hw_fmt_layout dest;
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>>>> index 4cd2d9e3131a4..072fc6950e496 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>>>>>> @@ -269,28 +269,21 @@ static void
>>>>>>>>> dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
>>>>>>>>>       *                                     This API does not handle
>>>>>>>>> DPU_CHROMA_H1V2.
>>>>>>>>>       * @phys_enc:Pointer to physical encoder
>>>>>>>>>       */
>>>>>>>>> -static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>>>>>> dpu_encoder_phys *phys_enc)
>>>>>>>>> +static void dpu_encoder_helper_phys_setup_cdm(struct
>>>>>>>>> dpu_encoder_phys *phys_enc,
>>>>>>>>> +                          const struct dpu_format *dpu_fmt,
>>>>>>>>> +                          u32 output_type)
>>>>>>>>>      {
>>>>>>>>>          struct dpu_hw_cdm *hw_cdm;
>>>>>>>>>          struct dpu_hw_cdm_cfg *cdm_cfg;
>>>>>>>>>          struct dpu_hw_pingpong *hw_pp;
>>>>>>>>> -    struct dpu_encoder_phys_wb *wb_enc;
>>>>>>>>> -    const struct msm_format *format;
>>>>>>>>> -    const struct dpu_format *dpu_fmt;
>>>>>>>>> -    struct drm_writeback_job *wb_job;
>>>>>>>>>          int ret;
>>>>>>>>>            if (!phys_enc)
>>>>>>>>>              return;
>>>>>>>>>      -    wb_enc = to_dpu_encoder_phys_wb(phys_enc);
>>>>>>>>> -    cdm_cfg = &wb_enc->cdm_cfg;
>>>>>>>>> +    cdm_cfg = &phys_enc->cdm_cfg;
>>>>>>>>>          hw_pp = phys_enc->hw_pp;
>>>>>>>>>          hw_cdm = phys_enc->hw_cdm;
>>>>>>>>> -    wb_job = wb_enc->wb_job;
>>>>>>>>> -
>>>>>>>>> -    format = msm_framebuffer_format(wb_enc->wb_job->fb);
>>>>>>>>> -    dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format,
>>>>>>>>> wb_job->fb->modifier);
>>>>>>>>>            if (!hw_cdm)
>>>>>>>>>              return;
>>>>>>>>> @@ -306,10 +299,10 @@ static void
>>>>>>>>> dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
>>>>>>>>>            memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
>>>>>>>>>      -    cdm_cfg->output_width = wb_job->fb->width;
>>>>>>>>> -    cdm_cfg->output_height = wb_job->fb->height;
>>>>>>>>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>>>>>>>>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>>>>>>> This is a semantic change. Instead of passing the FB size, this passes
>>>>>>>> the mode dimensions. They are not guaranteed to be the same,
>>>>>>>> especially for the WB case.
>>>>>>>>
>>>>>> The WB job is storing the output FB of WB. I cannot think of a use-case
>>>>>> where this cannot match the current mode programmed to the WB encoder.
>>>>>>
>>>>>> Yes, if it was the drm_plane's FB, then it cannot be guaranteed as the
>>>>>> plane can scale the contents but here thats not the case. Here its the
>>>>>> output FB of WB.
>>>>> Is it a part of WB uAPI, to have the FB dimensions equal to mode
>>>>> dimensions? Or is it just our current limitation? I can easily imagine
>>>>> WB outputting data to a part of the FB (just like we can clip FB using
>>>>> plane's clip rectangle).
>>>>>
>>>>> This boils down to a question, whether CDM should be setup in terms of
>>>>> actual output date or the physical memory buffer parameters. I suspect
>>>>> the former is the case (which makes this change correct). But it
>>>>> either should be described in the commit message or (even better)
>>>>> split to a separate commit.
>>>>>
>>>> I would say its a combination of both today.
>>>>
>>>> The way I would look at it is even if WB crops a certain section of FB,
>>>> that will not change the FB size. FB size of WB should match the rest of
>>>> the DRM pipeline (the mode programmed to the CRTC/encoder). If WB
>>>> decides to write to only a small section of FB (cropping), then we need
>>>> another WB property like CROP_ROI so that we can program the WB to only
>>>> write to a small section of the programmed FB. So in some sense, there
>>>> is no such support in DRM UAPI today. Hence the FB of WB is the full
>>>> mode of the WB.
>>>    I'd say, CROP_ROI can refer to cropping of the image source (esp. in
>>> the cloned output case). For writing to the part of the FB there can
>>> be DST_X/_Y/_W/_H properties. But this becomes off-topic.
>>>
>>>> CDM is before WB so follows the rest of the pipeline that is whatever
>>>> the data feeding it was programmed to.
>>> Yes. So the change is correct, but it should be split or documented
>>> properly. I prefer the first option.
>>>
>> Ok just to clarify you prefer below part of the change to be moved to
>> its own commit right?
>>
>> -    cdm_cfg->output_width = wb_job->fb->width;
>> -    cdm_cfg->output_height = wb_job->fb->height;
>> +    cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
>> +    cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
>>
>> If so, ack.
> Yes.
Ack will change in next version.
>
>
diff mbox series

Patch

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 993f263433314..37ac385727c3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -153,6 +153,7 @@  enum dpu_intr_idx {
  * @hw_intf:		Hardware interface to the intf registers
  * @hw_wb:		Hardware interface to the wb registers
  * @hw_cdm:		Hardware interface to the CDM registers
+ * @cdm_cfg:	CDM block config needed to store WB/DP block's CDM configuration
  * @dpu_kms:		Pointer to the dpu_kms top level
  * @cached_mode:	DRM mode cached at mode_set time, acted on in enable
  * @vblank_ctl_lock:	Vblank ctl mutex lock to protect vblank_refcount
@@ -183,6 +184,7 @@  struct dpu_encoder_phys {
 	struct dpu_hw_intf *hw_intf;
 	struct dpu_hw_wb *hw_wb;
 	struct dpu_hw_cdm *hw_cdm;
+	struct dpu_hw_cdm_cfg cdm_cfg;
 	struct dpu_kms *dpu_kms;
 	struct drm_display_mode cached_mode;
 	struct mutex vblank_ctl_lock;
@@ -213,7 +215,6 @@  static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
  * @wbirq_refcount:     Reference count of writeback interrupt
  * @wb_done_timeout_cnt: number of wb done irq timeout errors
  * @wb_cfg:  writeback block config to store fb related details
- * @cdm_cfg: cdm block config needed to store writeback block's CDM configuration
  * @wb_conn: backpointer to writeback connector
  * @wb_job: backpointer to current writeback job
  * @dest:   dpu buffer layout for current writeback output buffer
@@ -223,7 +224,6 @@  struct dpu_encoder_phys_wb {
 	atomic_t wbirq_refcount;
 	int wb_done_timeout_cnt;
 	struct dpu_hw_wb_cfg wb_cfg;
-	struct dpu_hw_cdm_cfg cdm_cfg;
 	struct drm_writeback_connector *wb_conn;
 	struct drm_writeback_job *wb_job;
 	struct dpu_hw_fmt_layout dest;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 4cd2d9e3131a4..072fc6950e496 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -269,28 +269,21 @@  static void dpu_encoder_phys_wb_setup_ctl(struct dpu_encoder_phys *phys_enc)
  *                                     This API does not handle DPU_CHROMA_H1V2.
  * @phys_enc:Pointer to physical encoder
  */
-static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
+static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc,
+					      const struct dpu_format *dpu_fmt,
+					      u32 output_type)
 {
 	struct dpu_hw_cdm *hw_cdm;
 	struct dpu_hw_cdm_cfg *cdm_cfg;
 	struct dpu_hw_pingpong *hw_pp;
-	struct dpu_encoder_phys_wb *wb_enc;
-	const struct msm_format *format;
-	const struct dpu_format *dpu_fmt;
-	struct drm_writeback_job *wb_job;
 	int ret;
 
 	if (!phys_enc)
 		return;
 
-	wb_enc = to_dpu_encoder_phys_wb(phys_enc);
-	cdm_cfg = &wb_enc->cdm_cfg;
+	cdm_cfg = &phys_enc->cdm_cfg;
 	hw_pp = phys_enc->hw_pp;
 	hw_cdm = phys_enc->hw_cdm;
-	wb_job = wb_enc->wb_job;
-
-	format = msm_framebuffer_format(wb_enc->wb_job->fb);
-	dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, wb_job->fb->modifier);
 
 	if (!hw_cdm)
 		return;
@@ -306,10 +299,10 @@  static void dpu_encoder_helper_phys_setup_cdm(struct dpu_encoder_phys *phys_enc)
 
 	memset(cdm_cfg, 0, sizeof(struct dpu_hw_cdm_cfg));
 
-	cdm_cfg->output_width = wb_job->fb->width;
-	cdm_cfg->output_height = wb_job->fb->height;
+	cdm_cfg->output_width = phys_enc->cached_mode.hdisplay;
+	cdm_cfg->output_height = phys_enc->cached_mode.vdisplay;
 	cdm_cfg->output_fmt = dpu_fmt;
-	cdm_cfg->output_type = CDM_CDWN_OUTPUT_WB;
+	cdm_cfg->output_type = output_type;
 	cdm_cfg->output_bit_depth = DPU_FORMAT_IS_DX(dpu_fmt) ?
 			CDM_CDWN_OUTPUT_10BIT : CDM_CDWN_OUTPUT_8BIT;
 	cdm_cfg->csc_cfg = &dpu_csc10_rgb2yuv_601l;
@@ -462,6 +455,14 @@  static void dpu_encoder_phys_wb_setup(
 	struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
 	struct drm_display_mode mode = phys_enc->cached_mode;
 	struct drm_framebuffer *fb = NULL;
+	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
+	struct drm_writeback_job *wb_job;
+	const struct msm_format *format;
+	const struct dpu_format *dpu_fmt;
+
+	wb_job = wb_enc->wb_job;
+	format = msm_framebuffer_format(wb_enc->wb_job->fb);
+	dpu_fmt = dpu_get_dpu_format_ext(format->pixel_format, wb_job->fb->modifier);
 
 	DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
 			hw_wb->idx - WB_0, mode.name,
@@ -475,7 +476,7 @@  static void dpu_encoder_phys_wb_setup(
 
 	dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
 
-	dpu_encoder_helper_phys_setup_cdm(phys_enc);
+	dpu_encoder_helper_phys_setup_cdm(phys_enc, dpu_fmt, CDM_CDWN_OUTPUT_WB);
 
 	dpu_encoder_phys_wb_setup_ctl(phys_enc);
 }