On Wed, 21 Jun 2023 at 00:38, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote: > > > > On 6/16/2023 5:35 PM, Dmitry Baryshkov wrote: > > On 17/06/2023 00:54, Marijn Suijten wrote: > >> On 2023-06-16 14:18:39, Abhinav Kumar wrote: > >>> > >>> > >>> On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote: > >>>> On 14/06/2023 04:57, Jessica Zhang wrote: > >>>>> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the > >>>>> databus-widen mode datapath. > >>>>> > >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> > >>>>> --- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++ > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 > >>>>> ++++++++++++ > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++ > >>>>> 3 files changed, 18 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 b856c6286c85..124ba96bebda 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 > >>>>> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( > >>>>> if (intf_cfg.dsc != 0 && > >>>>> phys_enc->hw_intf->ops.enable_compression) > >>>>> > >>>>> phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf); > >>>>> + > >>>>> + if (phys_enc->hw_intf->ops.enable_widebus) > >>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf); > >>>> > >>>> No. Please provide a single function which takes necessary > >>>> configuration, including compression and wide_bus_enable. > >>>> > >>> > >>> There are two ways to look at this. Your point is coming from the > >>> perspective that its programming the same register but just a different > >>> bit. But that will also make it a bit confusing. > > > > My point is to have a high-level function that configures the INTF for > > the CMD mode. This way it can take a structure with necessary > > configuration bits. > > Hi Dmitry, > > After discussing this approach with Abhinav, we still have a few > questions about it: > > Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the > rest are reserved with no plans of being programmed in the future). Does > this still justify the use of a struct to pass in the necessary > configuration? Yes. > > In addition, it seems that video mode does all its INTF_CONFIG2 > configuration separately in dpu_hw_intf_setup_timing_engine(). If we > have a generic set_intf_config2() op, it might be good to have it as > part of a larger cleanup where we have both video and command mode use > the generic op. What are your thoughts on this? No. If I get your idea right, this would mean moving INTF_CONFIG2 knowledge to the caller, which, I think, is a bad idea. > > Thanks, > > Jessica Zhang > > > > >>> > >>> So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool > >>> data_compress) > >>> > >>> Now for the caller who wants to just enable widebus this will be > >>> > >>> intf_cfg2_xxx(....., true, false) > >>> > >>> if we want to do both > >>> > >>> intf_cfg2_xxx(...., true, true) > >>> > >>> the last one where we want to do just data_compress(highly unlikely and > >>> not recommended) > >>> > >>> intf_cfg2_xxx(...., false, true) > >>> > >>> Now someone looking at the code will have to go and find out what each > >>> bool is. > >>> > >>> Whereas with separate ops, its kind of implicit. > >> > >> That's why you never pass bools as function argument (edge-case if it is > >> the only argument, and its meaning becomes clear from the function > >> name). Use enumerations anywhere else. > >> > >> - Marijn > >> > >>> > >>> For that reason, I dont think this patch is bad at all. > > > > -- > > With best wishes > > Dmitry > > -- With best wishes Dmitry
