On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
> >>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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
