diff mbox series

[07/17] drm/msm/dpu: disallow widebus en in INTF_CONFIG2 when DP is YUV420

Message ID 20240125193834.7065-8-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
INTF_CONFIG2 register cannot have widebus enabled when DP format is
YUV420. Therefore, program the INTF to send 1 ppc.

Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Dmitry Baryshkov Jan. 25, 2024, 9:26 p.m. UTC | #1
On 25/01/2024 21:38, Paloma Arellano wrote:
> INTF_CONFIG2 register cannot have widebus enabled when DP format is
> YUV420. Therefore, program the INTF to send 1 ppc.

I think this is handled in the DP driver, where we disallow wide bus for 
YUV 4:2:0 modes.

> 
> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 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 6bba531d6dc41..bfb93f02fe7c1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -168,7 +168,9 @@ 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->wide_bus_en)
> +	if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> +		intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> +	else if (p->wide_bus_en)
>   		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>   
>   	data_width = p->width;
Dmitry Baryshkov Jan. 27, 2024, 5:42 a.m. UTC | #2
On Thu, 25 Jan 2024 at 23:26, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 25/01/2024 21:38, Paloma Arellano wrote:
> > INTF_CONFIG2 register cannot have widebus enabled when DP format is
> > YUV420. Therefore, program the INTF to send 1 ppc.
>
> I think this is handled in the DP driver, where we disallow wide bus for
> YUV 4:2:0 modes.

Maybe this needs some explanation from my side:
I think it will be better to have separate conditionals for setting
HCTL_EN and for DATABUS_WIDEN.


>
> >
> > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > 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 6bba531d6dc41..bfb93f02fe7c1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > @@ -168,7 +168,9 @@ 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->wide_bus_en)
> > +     if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> > +             intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> > +     else if (p->wide_bus_en)
> >               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> >
> >       data_width = p->width;
>
> --
> With best wishes
> Dmitry
>


--
With best wishes
Dmitry
Paloma Arellano Jan. 28, 2024, 5:16 a.m. UTC | #3
On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
> On 25/01/2024 21:38, Paloma Arellano wrote:
>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
>> YUV420. Therefore, program the INTF to send 1 ppc.
>
> I think this is handled in the DP driver, where we disallow wide bus 
> for YUV 4:2:0 modes.
Yes we do disallow wide bus for YUV420 modes, but we still need to 
program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add 
this check.
>
>>
>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>> +    else if (p->wide_bus_en)
>>           intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>>         data_width = p->width;
>
Dmitry Baryshkov Jan. 28, 2024, 5:33 a.m. UTC | #4
On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
>
> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
> > On 25/01/2024 21:38, Paloma Arellano wrote:
> >> INTF_CONFIG2 register cannot have widebus enabled when DP format is
> >> YUV420. Therefore, program the INTF to send 1 ppc.
> >
> > I think this is handled in the DP driver, where we disallow wide bus
> > for YUV 4:2:0 modes.
> Yes we do disallow wide bus for YUV420 modes, but we still need to
> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
> this check.

As I wrote in my second email, I'd prefer to have one if which guards
HCTL_EN and another one for WIDEN

> >
> >>
> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> 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 6bba531d6dc41..bfb93f02fe7c1 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >> @@ -168,7 +168,9 @@ 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->wide_bus_en)
> >> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> >> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> >> +    else if (p->wide_bus_en)
> >>           intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> >>         data_width = p->width;
> >
Abhinav Kumar Jan. 29, 2024, 11:51 p.m. UTC | #5
On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>
>>
>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
>>>> YUV420. Therefore, program the INTF to send 1 ppc.
>>>
>>> I think this is handled in the DP driver, where we disallow wide bus
>>> for YUV 4:2:0 modes.
>> Yes we do disallow wide bus for YUV420 modes, but we still need to
>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
>> this check.
> 
> As I wrote in my second email, I'd prefer to have one if which guards
> HCTL_EN and another one for WIDEN
> 
Its hard to separate out the conditions just for HCTL_EN . Its more 
about handling the various pixel per clock combinations.

But, here is how I can best summarize it.

Lets consider DSI and DP separately:

1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).

This is same the same condition as widebus today in 
msm_dsi_host_is_wide_bus_enabled().

Hence no changes needed for DSI.

2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
as they are independent cases. We dont support YUV420 + DSC case.

There are other cases which fall outside of this bucket but they are 
optional ones. We only follow the "required" ones.

With this summary in mind, I am fine with what we have except perhaps 
better documentation above this block.

When DSC over DP gets added, I am expecting no changes to this block as 
it will fall under the widebus_en case.

With this information, how else would you like the check?

>>>
>>>>
>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>>>> +    else if (p->wide_bus_en)
>>>>            intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>>>>          data_width = p->width;
>>>
> 
> 
>
Dmitry Baryshkov Jan. 30, 2024, 12:03 a.m. UTC | #6
On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
> > On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
> >>
> >>
> >> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
> >>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
> >>>> YUV420. Therefore, program the INTF to send 1 ppc.
> >>>
> >>> I think this is handled in the DP driver, where we disallow wide bus
> >>> for YUV 4:2:0 modes.
> >> Yes we do disallow wide bus for YUV420 modes, but we still need to
> >> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
> >> this check.
> >
> > As I wrote in my second email, I'd prefer to have one if which guards
> > HCTL_EN and another one for WIDEN
> >
> Its hard to separate out the conditions just for HCTL_EN . Its more
> about handling the various pixel per clock combinations.
>
> But, here is how I can best summarize it.
>
> Lets consider DSI and DP separately:
>
> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
>
> This is same the same condition as widebus today in
> msm_dsi_host_is_wide_bus_enabled().
>
> Hence no changes needed for DSI.

Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
enabled, while you have written that HCTL_EN should be set in all
cases on a corresponding platform.

>
> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
> as they are independent cases. We dont support YUV420 + DSC case.
>
> There are other cases which fall outside of this bucket but they are
> optional ones. We only follow the "required" ones.
>
> With this summary in mind, I am fine with what we have except perhaps
> better documentation above this block.
>
> When DSC over DP gets added, I am expecting no changes to this block as
> it will fall under the widebus_en case.
>
> With this information, how else would you like the check?

What does this bit really change?

>
> >>>
> >>>>
> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
> >>>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
> >>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> >>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> >>>> +    else if (p->wide_bus_en)
> >>>>            intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> >>>>          data_width = p->width;
> >>>
> >
> >
> >
Abhinav Kumar Jan. 30, 2024, 1:07 a.m. UTC | #7
On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
>>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>>>
>>>>
>>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
>>>>>> YUV420. Therefore, program the INTF to send 1 ppc.
>>>>>
>>>>> I think this is handled in the DP driver, where we disallow wide bus
>>>>> for YUV 4:2:0 modes.
>>>> Yes we do disallow wide bus for YUV420 modes, but we still need to
>>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
>>>> this check.
>>>
>>> As I wrote in my second email, I'd prefer to have one if which guards
>>> HCTL_EN and another one for WIDEN
>>>
>> Its hard to separate out the conditions just for HCTL_EN . Its more
>> about handling the various pixel per clock combinations.
>>
>> But, here is how I can best summarize it.
>>
>> Lets consider DSI and DP separately:
>>
>> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
>>
>> This is same the same condition as widebus today in
>> msm_dsi_host_is_wide_bus_enabled().
>>
>> Hence no changes needed for DSI.
> 
> Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
> enabled, while you have written that HCTL_EN should be set in all
> cases on a corresponding platform.
> 

Agreed. This is true, we should enable HCTL_EN for DSI irrespective of 
widebus for the versions I wrote.

Basically for the non-compressed case.

I will write something up to fix this for DSI. I think this can go as a 
bug fix.

But that does not change the DP conditions OR in other words, I dont see 
anything wrong with this patch yet.

>>
>> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
>> as they are independent cases. We dont support YUV420 + DSC case.
>>
>> There are other cases which fall outside of this bucket but they are
>> optional ones. We only follow the "required" ones.
>>
>> With this summary in mind, I am fine with what we have except perhaps
>> better documentation above this block.
>>
>> When DSC over DP gets added, I am expecting no changes to this block as
>> it will fall under the widebus_en case.
>>
>> With this information, how else would you like the check?
> 
> What does this bit really change?
> 

This bit basically just tells that the data sent per line is programmed 
with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.

         if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
                 DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
                 DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, 
display_data_hctl);
                 DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
         }

Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.

>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
>>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
>>>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
>>>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>>>>>> +    else if (p->wide_bus_en)
>>>>>>             intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>>>>>>           data_width = p->width;
>>>>>
>>>
>>>
>>>
> 
> 
>
Dmitry Baryshkov Jan. 30, 2024, 1:43 a.m. UTC | #8
On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
> >>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
> >>>>
> >>>>
> >>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
> >>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
> >>>>>> YUV420. Therefore, program the INTF to send 1 ppc.
> >>>>>
> >>>>> I think this is handled in the DP driver, where we disallow wide bus
> >>>>> for YUV 4:2:0 modes.
> >>>> Yes we do disallow wide bus for YUV420 modes, but we still need to
> >>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
> >>>> this check.
> >>>
> >>> As I wrote in my second email, I'd prefer to have one if which guards
> >>> HCTL_EN and another one for WIDEN
> >>>
> >> Its hard to separate out the conditions just for HCTL_EN . Its more
> >> about handling the various pixel per clock combinations.
> >>
> >> But, here is how I can best summarize it.
> >>
> >> Lets consider DSI and DP separately:
> >>
> >> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
> >>
> >> This is same the same condition as widebus today in
> >> msm_dsi_host_is_wide_bus_enabled().
> >>
> >> Hence no changes needed for DSI.
> >
> > Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
> > enabled, while you have written that HCTL_EN should be set in all
> > cases on a corresponding platform.
> >
>
> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of
> widebus for the versions I wrote.
>
> Basically for the non-compressed case.
>
> I will write something up to fix this for DSI. I think this can go as a
> bug fix.
>
> But that does not change the DP conditions OR in other words, I dont see
> anything wrong with this patch yet.
>
> >>
> >> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
> >> as they are independent cases. We dont support YUV420 + DSC case.
> >>
> >> There are other cases which fall outside of this bucket but they are
> >> optional ones. We only follow the "required" ones.
> >>
> >> With this summary in mind, I am fine with what we have except perhaps
> >> better documentation above this block.
> >>
> >> When DSC over DP gets added, I am expecting no changes to this block as
> >> it will fall under the widebus_en case.
> >>
> >> With this information, how else would you like the check?
> >
> > What does this bit really change?
> >
>
> This bit basically just tells that the data sent per line is programmed
> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.
>
>          if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
>                  DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
>                  DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL,
> display_data_hctl);
>                  DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>          }
>
> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.

Can we enable it unconditionally for DPU >= 5.0?

>
> >>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
> >>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
> >>>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> >>>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> >>>>>> +    else if (p->wide_bus_en)
> >>>>>>             intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> >>>>>>           data_width = p->width;
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
Abhinav Kumar Jan. 30, 2024, 4:10 a.m. UTC | #9
On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
>>> On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
>>>>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
>>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
>>>>>>>> YUV420. Therefore, program the INTF to send 1 ppc.
>>>>>>>
>>>>>>> I think this is handled in the DP driver, where we disallow wide bus
>>>>>>> for YUV 4:2:0 modes.
>>>>>> Yes we do disallow wide bus for YUV420 modes, but we still need to
>>>>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
>>>>>> this check.
>>>>>
>>>>> As I wrote in my second email, I'd prefer to have one if which guards
>>>>> HCTL_EN and another one for WIDEN
>>>>>
>>>> Its hard to separate out the conditions just for HCTL_EN . Its more
>>>> about handling the various pixel per clock combinations.
>>>>
>>>> But, here is how I can best summarize it.
>>>>
>>>> Lets consider DSI and DP separately:
>>>>
>>>> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
>>>>
>>>> This is same the same condition as widebus today in
>>>> msm_dsi_host_is_wide_bus_enabled().
>>>>
>>>> Hence no changes needed for DSI.
>>>
>>> Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
>>> enabled, while you have written that HCTL_EN should be set in all
>>> cases on a corresponding platform.
>>>
>>
>> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of
>> widebus for the versions I wrote.
>>
>> Basically for the non-compressed case.
>>
>> I will write something up to fix this for DSI. I think this can go as a
>> bug fix.
>>
>> But that does not change the DP conditions OR in other words, I dont see
>> anything wrong with this patch yet.
>>
>>>>
>>>> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
>>>> as they are independent cases. We dont support YUV420 + DSC case.
>>>>
>>>> There are other cases which fall outside of this bucket but they are
>>>> optional ones. We only follow the "required" ones.
>>>>
>>>> With this summary in mind, I am fine with what we have except perhaps
>>>> better documentation above this block.
>>>>
>>>> When DSC over DP gets added, I am expecting no changes to this block as
>>>> it will fall under the widebus_en case.
>>>>
>>>> With this information, how else would you like the check?
>>>
>>> What does this bit really change?
>>>
>>
>> This bit basically just tells that the data sent per line is programmed
>> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.
>>
>>           if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
>>                   DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
>>                   DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL,
>> display_data_hctl);
>>                   DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>>           }
>>
>> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.
> 
> Can we enable it unconditionally for DPU >= 5.0?
> 

There is a corner-case that we should not enable it when compression is 
enabled without widebus as per the docs :(

For DP there will not be a case like that because compression and 
widebus go together but for DSI, it is possible.

So I found that the reset value of this register does cover all cases 
for DPU >= 7.0 so below fix will address the DSI concern and will fix 
the issue even for YUV420 cases such as this one for DPU >= 7.0

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 6bba531d6dc4..cbd5ebd516cd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -168,6 +168,8 @@ 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
          */
+
+       intf_cfg2 = DPU_REG_READ(c, INTF_CONFIG2);
         if (p->wide_bus_en)
                 intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | 
INTF_CFG2_DATA_HCTL_EN;


But, this does not still work for DPU < 7.0 such as sc7180 if we try 
YUV420 over DP on that because its DPU version is 6.2 so we will have to 
keep this patch for those cases.

>>
>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
>>>>>>>>      1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
>>>>>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
>>>>>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>>>>>>>> +    else if (p->wide_bus_en)
>>>>>>>>              intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>>>>>>>>            data_width = p->width;
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>
Dmitry Baryshkov Jan. 30, 2024, 5:28 a.m. UTC | #10
On Tue, 30 Jan 2024 at 06:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
> >>>>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
> >>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
> >>>>>>>> YUV420. Therefore, program the INTF to send 1 ppc.
> >>>>>>>
> >>>>>>> I think this is handled in the DP driver, where we disallow wide bus
> >>>>>>> for YUV 4:2:0 modes.
> >>>>>> Yes we do disallow wide bus for YUV420 modes, but we still need to
> >>>>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
> >>>>>> this check.
> >>>>>
> >>>>> As I wrote in my second email, I'd prefer to have one if which guards
> >>>>> HCTL_EN and another one for WIDEN
> >>>>>
> >>>> Its hard to separate out the conditions just for HCTL_EN . Its more
> >>>> about handling the various pixel per clock combinations.
> >>>>
> >>>> But, here is how I can best summarize it.
> >>>>
> >>>> Lets consider DSI and DP separately:
> >>>>
> >>>> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
> >>>>
> >>>> This is same the same condition as widebus today in
> >>>> msm_dsi_host_is_wide_bus_enabled().
> >>>>
> >>>> Hence no changes needed for DSI.
> >>>
> >>> Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
> >>> enabled, while you have written that HCTL_EN should be set in all
> >>> cases on a corresponding platform.
> >>>
> >>
> >> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of
> >> widebus for the versions I wrote.
> >>
> >> Basically for the non-compressed case.
> >>
> >> I will write something up to fix this for DSI. I think this can go as a
> >> bug fix.
> >>
> >> But that does not change the DP conditions OR in other words, I dont see
> >> anything wrong with this patch yet.
> >>
> >>>>
> >>>> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
> >>>> as they are independent cases. We dont support YUV420 + DSC case.
> >>>>
> >>>> There are other cases which fall outside of this bucket but they are
> >>>> optional ones. We only follow the "required" ones.
> >>>>
> >>>> With this summary in mind, I am fine with what we have except perhaps
> >>>> better documentation above this block.
> >>>>
> >>>> When DSC over DP gets added, I am expecting no changes to this block as
> >>>> it will fall under the widebus_en case.
> >>>>
> >>>> With this information, how else would you like the check?
> >>>
> >>> What does this bit really change?
> >>>
> >>
> >> This bit basically just tells that the data sent per line is programmed
> >> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.
> >>
> >>           if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
> >>                   DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
> >>                   DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL,
> >> display_data_hctl);
> >>                   DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
> >>           }
> >>
> >> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.
> >
> > Can we enable it unconditionally for DPU >= 5.0?
> >
>
> There is a corner-case that we should not enable it when compression is
> enabled without widebus as per the docs :(

What about explicitly disabling it in such a case?
I mean something like:

if (dpu_core_rev >= 5.0 && !(enc->hw_dsc && !enc->wide_bus_en))
   intf_cfg |= INTF_CFG2_HCTL_EN;


>
> For DP there will not be a case like that because compression and
> widebus go together but for DSI, it is possible.
>
> So I found that the reset value of this register does cover all cases
> for DPU >= 7.0 so below fix will address the DSI concern and will fix
> the issue even for YUV420 cases such as this one for DPU >= 7.0
>
> 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 6bba531d6dc4..cbd5ebd516cd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -168,6 +168,8 @@ 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
>           */
> +
> +       intf_cfg2 = DPU_REG_READ(c, INTF_CONFIG2);
>          if (p->wide_bus_en)
>                  intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN |
> INTF_CFG2_DATA_HCTL_EN;
>
>
> But, this does not still work for DPU < 7.0 such as sc7180 if we try
> YUV420 over DP on that because its DPU version is 6.2 so we will have to
> keep this patch for those cases.
>
> >>
> >>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
> >>>>>>>>      1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
> >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> >>>>>>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
> >>>>>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
> >>>>>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> >>>>>>>> +    else if (p->wide_bus_en)
> >>>>>>>>              intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> >>>>>>>>            data_width = p->width;
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
Abhinav Kumar Jan. 30, 2024, 6:03 a.m. UTC | #11
On 1/29/2024 9:28 PM, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 06:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote:
>>> On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
>>>>>>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>>>>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is
>>>>>>>>>> YUV420. Therefore, program the INTF to send 1 ppc.
>>>>>>>>>
>>>>>>>>> I think this is handled in the DP driver, where we disallow wide bus
>>>>>>>>> for YUV 4:2:0 modes.
>>>>>>>> Yes we do disallow wide bus for YUV420 modes, but we still need to
>>>>>>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add
>>>>>>>> this check.
>>>>>>>
>>>>>>> As I wrote in my second email, I'd prefer to have one if which guards
>>>>>>> HCTL_EN and another one for WIDEN
>>>>>>>
>>>>>> Its hard to separate out the conditions just for HCTL_EN . Its more
>>>>>> about handling the various pixel per clock combinations.
>>>>>>
>>>>>> But, here is how I can best summarize it.
>>>>>>
>>>>>> Lets consider DSI and DP separately:
>>>>>>
>>>>>> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ).
>>>>>>
>>>>>> This is same the same condition as widebus today in
>>>>>> msm_dsi_host_is_wide_bus_enabled().
>>>>>>
>>>>>> Hence no changes needed for DSI.
>>>>>
>>>>> Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being
>>>>> enabled, while you have written that HCTL_EN should be set in all
>>>>> cases on a corresponding platform.
>>>>>
>>>>
>>>> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of
>>>> widebus for the versions I wrote.
>>>>
>>>> Basically for the non-compressed case.
>>>>
>>>> I will write something up to fix this for DSI. I think this can go as a
>>>> bug fix.
>>>>
>>>> But that does not change the DP conditions OR in other words, I dont see
>>>> anything wrong with this patch yet.
>>>>
>>>>>>
>>>>>> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case
>>>>>> as they are independent cases. We dont support YUV420 + DSC case.
>>>>>>
>>>>>> There are other cases which fall outside of this bucket but they are
>>>>>> optional ones. We only follow the "required" ones.
>>>>>>
>>>>>> With this summary in mind, I am fine with what we have except perhaps
>>>>>> better documentation above this block.
>>>>>>
>>>>>> When DSC over DP gets added, I am expecting no changes to this block as
>>>>>> it will fall under the widebus_en case.
>>>>>>
>>>>>> With this information, how else would you like the check?
>>>>>
>>>>> What does this bit really change?
>>>>>
>>>>
>>>> This bit basically just tells that the data sent per line is programmed
>>>> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting.
>>>>
>>>>            if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) {
>>>>                    DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
>>>>                    DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL,
>>>> display_data_hctl);
>>>>                    DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
>>>>            }
>>>>
>>>> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function.
>>>
>>> Can we enable it unconditionally for DPU >= 5.0?
>>>
>>
>> There is a corner-case that we should not enable it when compression is
>> enabled without widebus as per the docs :(
> 
> What about explicitly disabling it in such a case?
> I mean something like:
> 
> if (dpu_core_rev >= 5.0 && !(enc->hw_dsc && !enc->wide_bus_en))
>     intf_cfg |= INTF_CFG2_HCTL_EN;
> 

Condition is correct now. But we dont have enc or dpu version in this 
function.

We need to pass a new parameter called compression_en to 
dpu_hw_intf_timing_params and set it when dsc is used and then do this. 
We have widebus_en already in dpu_hw_intf_timing_params.

This was anyway part of the DSC over DP but we can add that here and 
then the if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) is indicative 
of dpu version >=5 so we can move this setup there.

> 
>>
>> For DP there will not be a case like that because compression and
>> widebus go together but for DSI, it is possible.
>>
>> So I found that the reset value of this register does cover all cases
>> for DPU >= 7.0 so below fix will address the DSI concern and will fix
>> the issue even for YUV420 cases such as this one for DPU >= 7.0
>>
>> 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 6bba531d6dc4..cbd5ebd516cd 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -168,6 +168,8 @@ 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
>>            */
>> +
>> +       intf_cfg2 = DPU_REG_READ(c, INTF_CONFIG2);
>>           if (p->wide_bus_en)
>>                   intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN |
>> INTF_CFG2_DATA_HCTL_EN;
>>
>>
>> But, this does not still work for DPU < 7.0 such as sc7180 if we try
>> YUV420 over DP on that because its DPU version is 6.2 so we will have to
>> keep this patch for those cases.
>>
>>>>
>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++-
>>>>>>>>>>       1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> 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 6bba531d6dc41..bfb93f02fe7c1 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>>>>>>>> @@ -168,7 +168,9 @@ 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->wide_bus_en)
>>>>>>>>>> +    if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
>>>>>>>>>> +        intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
>>>>>>>>>> +    else if (p->wide_bus_en)
>>>>>>>>>>               intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
>>>>>>>>>>             data_width = p->width;
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>
diff mbox series

Patch

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 6bba531d6dc41..bfb93f02fe7c1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -168,7 +168,9 @@  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->wide_bus_en)
+	if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420)
+		intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
+	else if (p->wide_bus_en)
 		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
 
 	data_width = p->width;