diff mbox series

[RFC,3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration

Message ID 20221213232207.113607-4-marijn.suijten@somainline.org
State New
Headers
Series drm/msm: DSC Electric Boogaloo for sm8[12]50 |

Commit Message

Marijn Suijten Dec. 13, 2022, 11:22 p.m. UTC 
  Active CTLs have to configure what DSC block(s) have to be enabled, and
what DSC block(s) have to be flushed; this value was initialized to zero
resulting in the necessary register writes to never happen (or would
write zero otherwise).  This seems to have gotten lost in the DSC v4->v5
series while refactoring how the combination with merge_3d was handled.

Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  | 2 ++
 3 files changed, 4 insertions(+)

Comments

Dmitry Baryshkov Dec. 14, 2022, 6:43 p.m. UTC | #1 
On 14/12/2022 01:22, Marijn Suijten wrote:
> Active CTLs have to configure what DSC block(s) have to be enabled, and
> what DSC block(s) have to be flushed; this value was initialized to zero
> resulting in the necessary register writes to never happen (or would
> write zero otherwise).  This seems to have gotten lost in the DSC v4->v5
> series while refactoring how the combination with merge_3d was handled.
> 
> Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  | 2 ++
>   3 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index ae28b2b93e69..35791f93c33d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>   	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
>   	intf_cfg.stream_sel = cmd_enc->stream_sel;
>   	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> +	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>   	ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
>   
>   	/* setup which pp blk will connect to this intf */
> 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 0f71e8fe7be7..9ee3a7306a5f 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
> @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>   	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>   	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>   	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> +	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>   	if (phys_enc->hw_pp->merge_3d)
>   		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>   
> 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 7cbcef6efe17..92ddf9995b37 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
> @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>   
>   		intf_cfg.intf = DPU_NONE;
>   		intf_cfg.wb = hw_wb->idx;
> +		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);

We usually don't have DSC with the writeback, don't we?

>   		if (mode_3d && hw_pp && hw_pp->merge_3d)
>   			intf_cfg.merge_3d = hw_pp->merge_3d->idx;
> @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>   		intf_cfg.wb = hw_wb->idx;
>   		intf_cfg.mode_3d =
>   			dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> +		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>   		phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
>   	}
>   }
Marijn Suijten Dec. 14, 2022, 7:30 p.m. UTC | #2 
On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
> On 14/12/2022 01:22, Marijn Suijten wrote:
> > Active CTLs have to configure what DSC block(s) have to be enabled, and
> > what DSC block(s) have to be flushed; this value was initialized to zero
> > resulting in the necessary register writes to never happen (or would
> > write zero otherwise).  This seems to have gotten lost in the DSC v4->v5
> > series while refactoring how the combination with merge_3d was handled.
> > 
> > Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  | 2 ++
> >   3 files changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > index ae28b2b93e69..35791f93c33d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> >   	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
> >   	intf_cfg.stream_sel = cmd_enc->stream_sel;
> >   	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > +	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> >   	ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
> >   
> >   	/* setup which pp blk will connect to this intf */
> > 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 0f71e8fe7be7..9ee3a7306a5f 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
> > @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
> >   	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
> >   	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
> >   	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > +	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> >   	if (phys_enc->hw_pp->merge_3d)
> >   		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
> >   
> > 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 7cbcef6efe17..92ddf9995b37 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
> > @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
> >   
> >   		intf_cfg.intf = DPU_NONE;
> >   		intf_cfg.wb = hw_wb->idx;
> > +		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> 
> We usually don't have DSC with the writeback, don't we?

I am unsure so ended up adding them in writeback regardless.  Downstream
uses a separate callback to process intf_cfg.dsc instead of going
through setup_intf_cfg().

To prevent these from being missed again (in the case of copy&paste),
how about instead having some function that sets up intf_cfg with these
default values from a phys_enc?  That way most of this remains oblivious
to the caller.

On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
doesn't use the intf_cfg.dsc member anyway, but it was again added to
keep the blocks somewhat consistent (in case it ever becomes used?).

> >   		if (mode_3d && hw_pp && hw_pp->merge_3d)
> >   			intf_cfg.merge_3d = hw_pp->merge_3d->idx;
> > @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
> >   		intf_cfg.wb = hw_wb->idx;
> >   		intf_cfg.mode_3d =
> >   			dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > +		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
> >   		phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
> >   	}
> >   }

- Marijn
Dmitry Baryshkov Dec. 15, 2022, 1:08 a.m. UTC | #3 
On 14/12/2022 21:30, Marijn Suijten wrote:
> On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
>> On 14/12/2022 01:22, Marijn Suijten wrote:
>>> Active CTLs have to configure what DSC block(s) have to be enabled, and
>>> what DSC block(s) have to be flushed; this value was initialized to zero
>>> resulting in the necessary register writes to never happen (or would
>>> write zero otherwise).  This seems to have gotten lost in the DSC v4->v5
>>> series while refactoring how the combination with merge_3d was handled.
>>>
>>> Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  | 2 ++
>>>    3 files changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index ae28b2b93e69..35791f93c33d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>    	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
>>>    	intf_cfg.stream_sel = cmd_enc->stream_sel;
>>>    	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> +	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>    	ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
>>>    
>>>    	/* setup which pp blk will connect to this intf */
>>> 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 0f71e8fe7be7..9ee3a7306a5f 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
>>> @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>>    	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>>>    	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>>    	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> +	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>    	if (phys_enc->hw_pp->merge_3d)
>>>    		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>>>    
>>> 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 7cbcef6efe17..92ddf9995b37 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
>>> @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>>>    
>>>    		intf_cfg.intf = DPU_NONE;
>>>    		intf_cfg.wb = hw_wb->idx;
>>> +		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>
>> We usually don't have DSC with the writeback, don't we?
> 
> I am unsure so ended up adding them in writeback regardless.  Downstream
> uses a separate callback to process intf_cfg.dsc instead of going
> through setup_intf_cfg().
> 
> To prevent these from being missed again (in the case of copy&paste),
> how about instead having some function that sets up intf_cfg with these
> default values from a phys_enc?  That way most of this remains oblivious
> to the caller.

I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for 
the WB.

> 
> On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
> doesn't use the intf_cfg.dsc member anyway, but it was again added to
> keep the blocks somewhat consistent (in case it ever becomes used?).
> 
>>>    		if (mode_3d && hw_pp && hw_pp->merge_3d)
>>>    			intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>>> @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
>>>    		intf_cfg.wb = hw_wb->idx;
>>>    		intf_cfg.mode_3d =
>>>    			dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> +		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>    		phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
>>>    	}
>>>    }
> 
> - Marijn
Abhinav Kumar Dec. 16, 2022, 10:20 p.m. UTC | #4 
On 12/14/2022 5:08 PM, Dmitry Baryshkov wrote:
> On 14/12/2022 21:30, Marijn Suijten wrote:
>> On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
>>> On 14/12/2022 01:22, Marijn Suijten wrote:
>>>> Active CTLs have to configure what DSC block(s) have to be enabled, and
>>>> what DSC block(s) have to be flushed; this value was initialized to 
>>>> zero
>>>> resulting in the necessary register writes to never happen (or would
>>>> write zero otherwise).  This seems to have gotten lost in the DSC 
>>>> v4->v5
>>>> series while refactoring how the combination with merge_3d was handled.
>>>>
>>>> Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in 
>>>> encoder")
>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 +
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  | 2 ++
>>>>    3 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index ae28b2b93e69..35791f93c33d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>>        intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
>>>>        intf_cfg.stream_sel = cmd_enc->stream_sel;
>>>>        intf_cfg.mode_3d = 
>>>> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> +    intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>>        ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
>>>>        /* setup which pp blk will connect to this intf */
>>>> 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 0f71e8fe7be7..9ee3a7306a5f 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
>>>> @@ -274,6 +274,7 @@ static void 
>>>> dpu_encoder_phys_vid_setup_timing_engine(
>>>>        intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
>>>>        intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>>>        intf_cfg.mode_3d = 
>>>> dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> +    intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>>        if (phys_enc->hw_pp->merge_3d)
>>>>            intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>>>> 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 7cbcef6efe17..92ddf9995b37 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
>>>> @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct 
>>>> dpu_encoder_phys *phys_enc)
>>>>            intf_cfg.intf = DPU_NONE;
>>>>            intf_cfg.wb = hw_wb->idx;
>>>> +        intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>
>>> We usually don't have DSC with the writeback, don't we?
>>
>> I am unsure so ended up adding them in writeback regardless.  Downstream
>> uses a separate callback to process intf_cfg.dsc instead of going
>> through setup_intf_cfg().
>>
>> To prevent these from being missed again (in the case of copy&paste),
>> how about instead having some function that sets up intf_cfg with these
>> default values from a phys_enc?  That way most of this remains oblivious
>> to the caller.
> 
> I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for 
> the WB.
> 

Although this change is harmless because 
dpu_encoder_helper_get_dsc(phys_enc) will not return a valid DSC mask 
for the WB encoder, hence the setup_intf_cfg will just skip the DSC 
programming, I also agree that we can skip setting the intf_cfg.dsc for 
the writeback encoder in this patch.

>>
>> On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware
>> doesn't use the intf_cfg.dsc member anyway, but it was again added to
>> keep the blocks somewhat consistent (in case it ever becomes used?).
>>
>>>>            if (mode_3d && hw_pp && hw_pp->merge_3d)
>>>>                intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>>>> @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct 
>>>> dpu_encoder_phys *phys_enc)
>>>>            intf_cfg.wb = hw_wb->idx;
>>>>            intf_cfg.mode_3d =
>>>>                dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> +        intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>>            phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, 
>>>> &intf_cfg);
>>>>        }
>>>>    }
>>
>> - Marijn
>
Marijn Suijten Dec. 20, 2022, 10:32 p.m. UTC | #5 
On 2022-12-16 14:20:52, Abhinav Kumar wrote:
> 
> 
> On 12/14/2022 5:08 PM, Dmitry Baryshkov wrote:
> > On 14/12/2022 21:30, Marijn Suijten wrote:
> >> On 2022-12-14 20:43:29, Dmitry Baryshkov wrote:
> >>> On 14/12/2022 01:22, Marijn Suijten wrote:
> >>>> [..]
> >>> We usually don't have DSC with the writeback, don't we?
> >>
> >> I am unsure so ended up adding them in writeback regardless.  Downstream
> >> uses a separate callback to process intf_cfg.dsc instead of going
> >> through setup_intf_cfg().
> >>
> >> To prevent these from being missed again (in the case of copy&paste),
> >> how about instead having some function that sets up intf_cfg with these
> >> default values from a phys_enc?  That way most of this remains oblivious
> >> to the caller.
> > 
> > I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for 
> > the WB.
> > 
> 
> Although this change is harmless because 
> dpu_encoder_helper_get_dsc(phys_enc) will not return a valid DSC mask 
> for the WB encoder, hence the setup_intf_cfg will just skip the DSC 
> programming, I also agree that we can skip setting the intf_cfg.dsc for 
> the writeback encoder in this patch.

Since both of you agree that it is useless I'll drop this in V2.  Have
to confess that I know nothing about the writeback interface and haven't
even read the code; does it run in parallel to a "physical" (e.g.
DP/DSI) interface to capture screenshots (or even video) of what is
currently being shown on the screen?  By that logic the WB may have
needed to know what is going on in the HW, but it wouldn't have made any
sense regardless if the presented planes first pass through DSC before
being captured.  Something for me to read up on :)

- Marijn
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index ae28b2b93e69..35791f93c33d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -61,6 +61,7 @@  static void _dpu_encoder_phys_cmd_update_intf_cfg(
 	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
 	intf_cfg.stream_sel = cmd_enc->stream_sel;
 	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
 	ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
 
 	/* setup which pp blk will connect to this intf */
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 0f71e8fe7be7..9ee3a7306a5f 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
@@ -274,6 +274,7 @@  static void dpu_encoder_phys_vid_setup_timing_engine(
 	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID;
 	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
 	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
 	if (phys_enc->hw_pp->merge_3d)
 		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
 
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 7cbcef6efe17..92ddf9995b37 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
@@ -209,6 +209,7 @@  static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
 
 		intf_cfg.intf = DPU_NONE;
 		intf_cfg.wb = hw_wb->idx;
+		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
 
 		if (mode_3d && hw_pp && hw_pp->merge_3d)
 			intf_cfg.merge_3d = hw_pp->merge_3d->idx;
@@ -230,6 +231,7 @@  static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
 		intf_cfg.wb = hw_wb->idx;
 		intf_cfg.mode_3d =
 			dpu_encoder_helper_get_3d_blend_mode(phys_enc);
+		intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
 		phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
 	}
 }