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. > > 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@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
