| Message ID | 20230525-add-widebus-support-v1-2-c7069f2efca1@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for databus widen mode | expand |
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@quicinc.com> > --- > 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. Also note, that we already have dpu_encoder_is_widebus_enabled() and the rest of support code. Please stick to it too. > } > > static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx) > 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 5b0f6627e29b..03ba3a1c7a46 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > @@ -513,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf, > > } > > +static void dpu_hw_intf_enable_widebus(struct dpu_hw_intf *ctx) > +{ > + u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2); > + > + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN; > + > + DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2); > +} > + > static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx) > { > u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2); > @@ -545,6 +554,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, > > if (cap & BIT(DPU_INTF_DATA_COMPRESS)) > ops->enable_compression = dpu_hw_intf_enable_compression; > + > + if (cap & BIT(DPU_INTF_DATABUS_WIDEN)) > + ops->enable_widebus = dpu_hw_intf_enable_widebus; > } > > struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > index 99e21c4137f9..64a17b99d3d1 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h > @@ -71,6 +71,7 @@ struct intf_status { > * Return: 0 on success, -ETIMEDOUT on timeout > * @vsync_sel: Select vsync signal for tear-effect configuration > * @enable_compression: Enable data compression > + * @enable_widebus: Enable widebus > */ > struct dpu_hw_intf_ops { > void (*setup_timing_gen)(struct dpu_hw_intf *intf, > @@ -109,6 +110,8 @@ struct dpu_hw_intf_ops { > void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay); > > void (*enable_compression)(struct dpu_hw_intf *intf); > + > + void (*enable_widebus)(struct dpu_hw_intf *intf); > }; > > struct dpu_hw_intf { >
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@quicinc.com> >> --- >> 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. 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. For that reason, I dont think this patch is bad at all.
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@quicinc.com> > >> --- > >> 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. > > 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.
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@quicinc.com> >>>> --- >>>> 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. >> >> 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.
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@quicinc.com> >>>>> --- >>>>> 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? 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? 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 >
On Wed, 21 Jun 2023 at 00:38, Jessica Zhang <quic_jesszhan@quicinc.com> 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@quicinc.com> > >>>>> --- > >>>>> 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 > >
On 2023-06-20 14:38:34, Jessica Zhang wrote: <snip> > >>>>> + 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? No. The point Dmitry is making is **not** about this concidentally using the same register, but about adding a common codepath to enable compression on this hw_intf (regardless of the registers it needs to touch). Similar to how dpu_hw_intf_setup_timing_engine() programs the hw_intf - including widebus! - for video-mode. Or even more generically, have a struct similar to intf_timing_params that says how the intf needs to be configured - without the caller knowing about INTF_CONFIG2. struct dpu_hw_intf_cfg is a very good example of how we can use a single struct and a single callback to configure multiple registers at once based on some input parameters. > 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? Not in that way, but if there is a generic enable_compression() or configure_compression() callback (or even more generic, similar to setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and command-mode, maybe that is beneficial. - Marijn
On 21/06/2023 18:17, Marijn Suijten wrote: > On 2023-06-20 14:38:34, Jessica Zhang wrote: > <snip> >>>>>>> + 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? > > No. The point Dmitry is making is **not** about this concidentally > using the same register, but about adding a common codepath to enable > compression on this hw_intf (regardless of the registers it needs to > touch). Actually to setup INTF for CMD stream (which is equal to setting up compression at this point). > Similar to how dpu_hw_intf_setup_timing_engine() programs the > hw_intf - including widebus! - for video-mode. > > Or even more generically, have a struct similar to intf_timing_params > that says how the intf needs to be configured - without the caller > knowing about INTF_CONFIG2. > > struct dpu_hw_intf_cfg is a very good example of how we can use a single > struct and a single callback to configure multiple registers at once > based on some input parameters. > >> 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? > > Not in that way, but if there is a generic enable_compression() or > configure_compression() callback (or even more generic, similar to > setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and > command-mode, maybe that is beneficial. I'd rather not do this. Let's just 'setup timing enging' vs 'setup CMD'. For example, it might also include setting up other INTF parameters for CMD mode (if anything is required later on). > > - Marijn
On 2023-06-21 19:36:37, Dmitry Baryshkov wrote: > On 21/06/2023 18:17, Marijn Suijten wrote: > > On 2023-06-20 14:38:34, Jessica Zhang wrote: > > <snip> > >>>>>>> + 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? > > > > No. The point Dmitry is making is **not** about this concidentally > > using the same register, but about adding a common codepath to enable > > compression on this hw_intf (regardless of the registers it needs to > > touch). > > Actually to setup INTF for CMD stream (which is equal to setting up > compression at this point). Yup, thaty is what I suggested below ("or even more generically"). > > Similar to how dpu_hw_intf_setup_timing_engine() programs the > > hw_intf - including widebus! - for video-mode. > > > > Or even more generically, have a struct similar to intf_timing_params > > that says how the intf needs to be configured - without the caller > > knowing about INTF_CONFIG2. > > > > struct dpu_hw_intf_cfg is a very good example of how we can use a single > > struct and a single callback to configure multiple registers at once > > based on some input parameters. > > > >> 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? > > > > Not in that way, but if there is a generic enable_compression() or > > configure_compression() callback (or even more generic, similar to > > setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and > > command-mode, maybe that is beneficial. > > I'd rather not do this. Let's just 'setup timing enging' vs 'setup CMD'. > For example, it might also include setting up other INTF parameters for > CMD mode (if anything is required later on). Sure, sounds good. hw_intf internally could even have a static function that deduplicates these "setup" function if there is any. We could rename setup_timing_engine to setup_video_mode to be more clear to the reader? - Marijn
On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: > On 21/06/2023 18:17, Marijn Suijten wrote: >> On 2023-06-20 14:38:34, Jessica Zhang wrote: >> <snip> >>>>>>>> + 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? >> >> No. The point Dmitry is making is **not** about this concidentally >> using the same register, but about adding a common codepath to enable >> compression on this hw_intf (regardless of the registers it needs to >> touch). > > Actually to setup INTF for CMD stream (which is equal to setting up > compression at this point). > Yes it should be setup intf for cmd and not enable compression. Widebus and compression are different features and we should be able to control them independently. We just enable them together for DSI. So a separation is necessary. But I am still not totally convinced we even need to go down the path for having an op called setup_intf_cmd() which takes in a struct like struct dpu_cmd_intf_cfg { bool data_compress; bool widebus_en; }; As we have agreed that we will not touch the video mode timing engine path, it leaves us with only two bits. And like I said, its not that these two bits always go together. We want to be able to control them independently which means that its not necessary both bits program the same register one by one. We might just end up programming one of them if we just use widebus. Thats why I am still leaning on keeping this approach. >> Similar to how dpu_hw_intf_setup_timing_engine() programs the >> hw_intf - including widebus! - for video-mode. >> >> Or even more generically, have a struct similar to intf_timing_params >> that says how the intf needs to be configured - without the caller >> knowing about INTF_CONFIG2. >> >> struct dpu_hw_intf_cfg is a very good example of how we can use a single >> struct and a single callback to configure multiple registers at once >> based on some input parameters. >> >>> 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? >> >> Not in that way, but if there is a generic enable_compression() or >> configure_compression() callback (or even more generic, similar to >> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and >> command-mode, maybe that is beneficial. > > I'd rather not do this. Let's just 'setup timing enging' vs 'setup CMD'. > For example, it might also include setting up other INTF parameters for > CMD mode (if anything is required later on). > Agreed on setup CMD but I dont know whether we need a setup CMD at all. Seems like an overkill. >> >> - Marijn >
On 22/06/2023 02:01, Abhinav Kumar wrote: > > > On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >> On 21/06/2023 18:17, Marijn Suijten wrote: >>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>> <snip> >>>>>>>>> + 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? >>> >>> No. The point Dmitry is making is **not** about this concidentally >>> using the same register, but about adding a common codepath to enable >>> compression on this hw_intf (regardless of the registers it needs to >>> touch). >> >> Actually to setup INTF for CMD stream (which is equal to setting up >> compression at this point). >> > > Yes it should be setup intf for cmd and not enable compression. > > Widebus and compression are different features and we should be able to > control them independently. > > We just enable them together for DSI. So a separation is necessary. > > But I am still not totally convinced we even need to go down the path > for having an op called setup_intf_cmd() which takes in a struct like > > struct dpu_cmd_intf_cfg { > bool data_compress; > bool widebus_en; > }; > > As we have agreed that we will not touch the video mode timing engine > path, it leaves us with only two bits. > > And like I said, its not that these two bits always go together. We want > to be able to control them independently which means that its not > necessary both bits program the same register one by one. We might just > end up programming one of them if we just use widebus. > > Thats why I am still leaning on keeping this approach. I do not like the idea of having small functions being called between modules. So, yes there will a config of two booleans, but it is preferable (and more scalable) compared to separate callbacks. Not to mention that it allows us to program required registers directly (by setting values) rather than using RMW cycles and thus depending on the value being previously programmed to these registers. > >>> Similar to how dpu_hw_intf_setup_timing_engine() programs the >>> hw_intf - including widebus! - for video-mode. >>> >>> Or even more generically, have a struct similar to intf_timing_params >>> that says how the intf needs to be configured - without the caller >>> knowing about INTF_CONFIG2. >>> >>> struct dpu_hw_intf_cfg is a very good example of how we can use a single >>> struct and a single callback to configure multiple registers at once >>> based on some input parameters. >>> >>>> 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? >>> >>> Not in that way, but if there is a generic enable_compression() or >>> configure_compression() callback (or even more generic, similar to >>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and >>> command-mode, maybe that is beneficial. >> >> I'd rather not do this. Let's just 'setup timing enging' vs 'setup >> CMD'. For example, it might also include setting up other INTF >> parameters for CMD mode (if anything is required later on). >> > > Agreed on setup CMD but I dont know whether we need a setup CMD at all. > Seems like an overkill. > >>> >>> - Marijn >>
On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote: > On 22/06/2023 02:01, Abhinav Kumar wrote: >> >> >> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >>> On 21/06/2023 18:17, Marijn Suijten wrote: >>>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>>> <snip> >>>>>>>>>> + 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? >>>> >>>> No. The point Dmitry is making is **not** about this concidentally >>>> using the same register, but about adding a common codepath to enable >>>> compression on this hw_intf (regardless of the registers it needs to >>>> touch). >>> >>> Actually to setup INTF for CMD stream (which is equal to setting up >>> compression at this point). >>> >> >> Yes it should be setup intf for cmd and not enable compression. >> >> Widebus and compression are different features and we should be able >> to control them independently. >> >> We just enable them together for DSI. So a separation is necessary. >> >> But I am still not totally convinced we even need to go down the path >> for having an op called setup_intf_cmd() which takes in a struct like >> >> struct dpu_cmd_intf_cfg { >> bool data_compress; >> bool widebus_en; >> }; >> >> As we have agreed that we will not touch the video mode timing engine >> path, it leaves us with only two bits. >> >> And like I said, its not that these two bits always go together. We >> want to be able to control them independently which means that its not >> necessary both bits program the same register one by one. We might >> just end up programming one of them if we just use widebus. >> >> Thats why I am still leaning on keeping this approach. > > I do not like the idea of having small functions being called between > modules. So, yes there will a config of two booleans, but it is > preferable (and more scalable) compared to separate callbacks. > I definitely agree with the scalable part and I even cross checked that the number of usable bitfields of this register is going up from one chipset to the other although once again that depends on whether we will use those features. For that reason I am not opposed to the struct idea. But there is also another pattern i am seeing which worries me. Usable bitfields sometimes even reduce. For those cases, if we go with a pre-defined struct it ends up with redundant members as those bitfields go away. With the function op based approach, we just call the op if the feature bit / core revision. So I wanted to check once more about the fact that we should consider not just expansion but also reduction. > Not to mention that it allows us to program required registers directly > (by setting values) rather than using RMW cycles and thus depending on > the value being previously programmed to these registers. > This will not change. We will still have to use RMW cycles to preserve the reset values of some of the fields as those are the right values for the registers and shouldnt be touched. >> >>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the >>>> hw_intf - including widebus! - for video-mode. >>>> >>>> Or even more generically, have a struct similar to intf_timing_params >>>> that says how the intf needs to be configured - without the caller >>>> knowing about INTF_CONFIG2. >>>> >>>> struct dpu_hw_intf_cfg is a very good example of how we can use a >>>> single >>>> struct and a single callback to configure multiple registers at once >>>> based on some input parameters. >>>> >>>>> 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? >>>> >>>> Not in that way, but if there is a generic enable_compression() or >>>> configure_compression() callback (or even more generic, similar to >>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and >>>> command-mode, maybe that is beneficial. >>> >>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup >>> CMD'. For example, it might also include setting up other INTF >>> parameters for CMD mode (if anything is required later on). >>> >> >> Agreed on setup CMD but I dont know whether we need a setup CMD at all. >> Seems like an overkill. >> >>>> >>>> - Marijn >>> >
On 23/06/2023 01:37, Abhinav Kumar wrote: > > > On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote: >> On 22/06/2023 02:01, Abhinav Kumar wrote: >>> >>> >>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >>>> On 21/06/2023 18:17, Marijn Suijten wrote: >>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>>>> <snip> >>>>>>>>>>> + 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? >>>>> >>>>> No. The point Dmitry is making is **not** about this concidentally >>>>> using the same register, but about adding a common codepath to enable >>>>> compression on this hw_intf (regardless of the registers it needs to >>>>> touch). >>>> >>>> Actually to setup INTF for CMD stream (which is equal to setting up >>>> compression at this point). >>>> >>> >>> Yes it should be setup intf for cmd and not enable compression. >>> >>> Widebus and compression are different features and we should be able >>> to control them independently. >>> >>> We just enable them together for DSI. So a separation is necessary. >>> >>> But I am still not totally convinced we even need to go down the path >>> for having an op called setup_intf_cmd() which takes in a struct like >>> >>> struct dpu_cmd_intf_cfg { >>> bool data_compress; >>> bool widebus_en; >>> }; >>> >>> As we have agreed that we will not touch the video mode timing engine >>> path, it leaves us with only two bits. >>> >>> And like I said, its not that these two bits always go together. We >>> want to be able to control them independently which means that its >>> not necessary both bits program the same register one by one. We >>> might just end up programming one of them if we just use widebus. >>> >>> Thats why I am still leaning on keeping this approach. >> >> I do not like the idea of having small functions being called between >> modules. So, yes there will a config of two booleans, but it is >> preferable (and more scalable) compared to separate callbacks. >> > > I definitely agree with the scalable part and I even cross checked that > the number of usable bitfields of this register is going up from one > chipset to the other although once again that depends on whether we will > use those features. > > For that reason I am not opposed to the struct idea. > > But there is also another pattern i am seeing which worries me. Usable > bitfields sometimes even reduce. For those cases, if we go with a > pre-defined struct it ends up with redundant members as those bitfields > go away. > > With the function op based approach, we just call the op if the feature > bit / core revision. > > So I wanted to check once more about the fact that we should consider > not just expansion but also reduction. As we have to support all generations, there is no actual reduction. Newer SoCs do not have particular feature/bit, but older ones do. By having multiple optional ops we just move this knowledge from ops->complex_callback() to _setup_block_ops(). But more importantly the caller gets more complicated. Instead of just calling ops->set_me_up(), it has to check all the optional callbacks and call the one by one. > >> Not to mention that it allows us to program required registers >> directly (by setting values) rather than using RMW cycles and thus >> depending on the value being previously programmed to these registers. >> > > This will not change. We will still have to use RMW cycles to preserve > the reset values of some of the fields as those are the right values for > the registers and shouldnt be touched. I'd like to point to the dpu_hw_intf_setup_timing_engine(), a close rival callback, setting up the INTF for video mode. It does not do RMW cycles, it just writes all the registers. In the worst case, there will be a single RMW instead of having multiple of them. > >>> >>>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the >>>>> hw_intf - including widebus! - for video-mode. >>>>> >>>>> Or even more generically, have a struct similar to intf_timing_params >>>>> that says how the intf needs to be configured - without the caller >>>>> knowing about INTF_CONFIG2. >>>>> >>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a >>>>> single >>>>> struct and a single callback to configure multiple registers at once >>>>> based on some input parameters. >>>>> >>>>>> 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? >>>>> >>>>> Not in that way, but if there is a generic enable_compression() or >>>>> configure_compression() callback (or even more generic, similar to >>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and >>>>> command-mode, maybe that is beneficial. >>>> >>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup >>>> CMD'. For example, it might also include setting up other INTF >>>> parameters for CMD mode (if anything is required later on). >>>> >>> >>> Agreed on setup CMD but I dont know whether we need a setup CMD at all. >>> Seems like an overkill. >>> >>>>> >>>>> - Marijn >>>> >>
On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote: > On 23/06/2023 01:37, Abhinav Kumar wrote: >> >> >> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote: >>> On 22/06/2023 02:01, Abhinav Kumar wrote: >>>> >>>> >>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >>>>> On 21/06/2023 18:17, Marijn Suijten wrote: >>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>>>>> <snip> >>>>>>>>>>>> + 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? >>>>>> >>>>>> No. The point Dmitry is making is **not** about this concidentally >>>>>> using the same register, but about adding a common codepath to enable >>>>>> compression on this hw_intf (regardless of the registers it needs to >>>>>> touch). >>>>> >>>>> Actually to setup INTF for CMD stream (which is equal to setting up >>>>> compression at this point). >>>>> >>>> >>>> Yes it should be setup intf for cmd and not enable compression. >>>> >>>> Widebus and compression are different features and we should be able >>>> to control them independently. >>>> >>>> We just enable them together for DSI. So a separation is necessary. >>>> >>>> But I am still not totally convinced we even need to go down the >>>> path for having an op called setup_intf_cmd() which takes in a >>>> struct like >>>> >>>> struct dpu_cmd_intf_cfg { >>>> bool data_compress; >>>> bool widebus_en; >>>> }; >>>> >>>> As we have agreed that we will not touch the video mode timing >>>> engine path, it leaves us with only two bits. >>>> >>>> And like I said, its not that these two bits always go together. We >>>> want to be able to control them independently which means that its >>>> not necessary both bits program the same register one by one. We >>>> might just end up programming one of them if we just use widebus. >>>> >>>> Thats why I am still leaning on keeping this approach. >>> >>> I do not like the idea of having small functions being called between >>> modules. So, yes there will a config of two booleans, but it is >>> preferable (and more scalable) compared to separate callbacks. >>> >> >> I definitely agree with the scalable part and I even cross checked >> that the number of usable bitfields of this register is going up from >> one chipset to the other although once again that depends on whether >> we will use those features. >> >> For that reason I am not opposed to the struct idea. >> >> But there is also another pattern i am seeing which worries me. Usable >> bitfields sometimes even reduce. For those cases, if we go with a >> pre-defined struct it ends up with redundant members as those >> bitfields go away. >> >> With the function op based approach, we just call the op if the >> feature bit / core revision. >> >> So I wanted to check once more about the fact that we should consider >> not just expansion but also reduction. > > As we have to support all generations, there is no actual reduction. > Newer SoCs do not have particular feature/bit, but older ones do. By > having multiple optional ops we just move this knowledge from > ops->complex_callback() to _setup_block_ops(). But more importantly the > caller gets more complicated. Instead of just calling ops->set_me_up(), > it has to check all the optional callbacks and call the one by one. > Alright, I am thinking that perhaps because this register is kind of unique that its actually controlling a specific setting in the datapath, downstream also has separate ops for this. But thats fine, we can go ahead with the struct based approach. >> >>> Not to mention that it allows us to program required registers >>> directly (by setting values) rather than using RMW cycles and thus >>> depending on the value being previously programmed to these registers. >>> >> >> This will not change. We will still have to use RMW cycles to preserve >> the reset values of some of the fields as those are the right values >> for the registers and shouldnt be touched. > > I'd like to point to the dpu_hw_intf_setup_timing_engine(), a close > rival callback, setting up the INTF for video mode. It does not do RMW > cycles, it just writes all the registers. > Yes as it ends up ORing pretty much all the usable bits. So it works fine. > In the worst case, there will be a single RMW instead of having multiple > of them. > > >> >>>> >>>>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the >>>>>> hw_intf - including widebus! - for video-mode. >>>>>> >>>>>> Or even more generically, have a struct similar to intf_timing_params >>>>>> that says how the intf needs to be configured - without the caller >>>>>> knowing about INTF_CONFIG2. >>>>>> >>>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a >>>>>> single >>>>>> struct and a single callback to configure multiple registers at once >>>>>> based on some input parameters. >>>>>> >>>>>>> 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? >>>>>> >>>>>> Not in that way, but if there is a generic enable_compression() or >>>>>> configure_compression() callback (or even more generic, similar to >>>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and >>>>>> command-mode, maybe that is beneficial. >>>>> >>>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup >>>>> CMD'. For example, it might also include setting up other INTF >>>>> parameters for CMD mode (if anything is required later on). >>>>> >>>> >>>> Agreed on setup CMD but I dont know whether we need a setup CMD at all. >>>> Seems like an overkill. >>>> >>>>>> >>>>>> - Marijn >>>>> >>> >
On 6/22/2023 4:37 PM, Abhinav Kumar wrote: > > > On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote: >> On 23/06/2023 01:37, Abhinav Kumar wrote: >>> >>> >>> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote: >>>> On 22/06/2023 02:01, Abhinav Kumar wrote: >>>>> >>>>> >>>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >>>>>> On 21/06/2023 18:17, Marijn Suijten wrote: >>>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>>>>>> <snip> >>>>>>>>>>>>> + 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? >>>>>>> >>>>>>> No. The point Dmitry is making is **not** about this concidentally >>>>>>> using the same register, but about adding a common codepath to >>>>>>> enable >>>>>>> compression on this hw_intf (regardless of the registers it needs to >>>>>>> touch). >>>>>> >>>>>> Actually to setup INTF for CMD stream (which is equal to setting >>>>>> up compression at this point). >>>>>> >>>>> >>>>> Yes it should be setup intf for cmd and not enable compression. >>>>> >>>>> Widebus and compression are different features and we should be >>>>> able to control them independently. >>>>> >>>>> We just enable them together for DSI. So a separation is necessary. >>>>> >>>>> But I am still not totally convinced we even need to go down the >>>>> path for having an op called setup_intf_cmd() which takes in a >>>>> struct like >>>>> >>>>> struct dpu_cmd_intf_cfg { >>>>> bool data_compress; >>>>> bool widebus_en; >>>>> }; >>>>> >>>>> As we have agreed that we will not touch the video mode timing >>>>> engine path, it leaves us with only two bits. >>>>> >>>>> And like I said, its not that these two bits always go together. We >>>>> want to be able to control them independently which means that its >>>>> not necessary both bits program the same register one by one. We >>>>> might just end up programming one of them if we just use widebus. >>>>> >>>>> Thats why I am still leaning on keeping this approach. >>>> >>>> I do not like the idea of having small functions being called >>>> between modules. So, yes there will a config of two booleans, but it >>>> is preferable (and more scalable) compared to separate callbacks. >>>> >>> >>> I definitely agree with the scalable part and I even cross checked >>> that the number of usable bitfields of this register is going up from >>> one chipset to the other although once again that depends on whether >>> we will use those features. >>> >>> For that reason I am not opposed to the struct idea. >>> >>> But there is also another pattern i am seeing which worries me. >>> Usable bitfields sometimes even reduce. For those cases, if we go >>> with a pre-defined struct it ends up with redundant members as those >>> bitfields go away. >>> >>> With the function op based approach, we just call the op if the >>> feature bit / core revision. >>> >>> So I wanted to check once more about the fact that we should consider >>> not just expansion but also reduction. >> >> As we have to support all generations, there is no actual reduction. >> Newer SoCs do not have particular feature/bit, but older ones do. By >> having multiple optional ops we just move this knowledge from >> ops->complex_callback() to _setup_block_ops(). But more importantly >> the caller gets more complicated. Instead of just calling >> ops->set_me_up(), it has to check all the optional callbacks and call >> the one by one. >> > > Alright, I am thinking that perhaps because this register is kind of > unique that its actually controlling a specific setting in the datapath, > downstream also has separate ops for this. > > But thats fine, we can go ahead with the struct based approach. > As data_compress has already landed, let me introduced the struct along with the core_revision based approach in the core_revision series and this series will expand that struct to include widebus too.
On 6/29/2023 10:26 AM, Abhinav Kumar wrote: > > > On 6/22/2023 4:37 PM, Abhinav Kumar wrote: >> >> >> On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote: >>> On 23/06/2023 01:37, Abhinav Kumar wrote: >>>> >>>> >>>> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote: >>>>> On 22/06/2023 02:01, Abhinav Kumar wrote: >>>>>> >>>>>> >>>>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >>>>>>> On 21/06/2023 18:17, Marijn Suijten wrote: >>>>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>>>>>>> <snip> >>>>>>>>>>>>>> + 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? >>>>>>>> >>>>>>>> No. The point Dmitry is making is **not** about this concidentally >>>>>>>> using the same register, but about adding a common codepath to >>>>>>>> enable >>>>>>>> compression on this hw_intf (regardless of the registers it >>>>>>>> needs to >>>>>>>> touch). >>>>>>> >>>>>>> Actually to setup INTF for CMD stream (which is equal to setting >>>>>>> up compression at this point). >>>>>>> >>>>>> >>>>>> Yes it should be setup intf for cmd and not enable compression. >>>>>> >>>>>> Widebus and compression are different features and we should be >>>>>> able to control them independently. >>>>>> >>>>>> We just enable them together for DSI. So a separation is necessary. >>>>>> >>>>>> But I am still not totally convinced we even need to go down the >>>>>> path for having an op called setup_intf_cmd() which takes in a >>>>>> struct like >>>>>> >>>>>> struct dpu_cmd_intf_cfg { >>>>>> bool data_compress; >>>>>> bool widebus_en; >>>>>> }; >>>>>> >>>>>> As we have agreed that we will not touch the video mode timing >>>>>> engine path, it leaves us with only two bits. >>>>>> >>>>>> And like I said, its not that these two bits always go together. >>>>>> We want to be able to control them independently which means that >>>>>> its not necessary both bits program the same register one by one. >>>>>> We might just end up programming one of them if we just use widebus. >>>>>> >>>>>> Thats why I am still leaning on keeping this approach. >>>>> >>>>> I do not like the idea of having small functions being called >>>>> between modules. So, yes there will a config of two booleans, but >>>>> it is preferable (and more scalable) compared to separate callbacks. >>>>> >>>> >>>> I definitely agree with the scalable part and I even cross checked >>>> that the number of usable bitfields of this register is going up >>>> from one chipset to the other although once again that depends on >>>> whether we will use those features. >>>> >>>> For that reason I am not opposed to the struct idea. >>>> >>>> But there is also another pattern i am seeing which worries me. >>>> Usable bitfields sometimes even reduce. For those cases, if we go >>>> with a pre-defined struct it ends up with redundant members as those >>>> bitfields go away. >>>> >>>> With the function op based approach, we just call the op if the >>>> feature bit / core revision. >>>> >>>> So I wanted to check once more about the fact that we should >>>> consider not just expansion but also reduction. >>> >>> As we have to support all generations, there is no actual reduction. >>> Newer SoCs do not have particular feature/bit, but older ones do. By >>> having multiple optional ops we just move this knowledge from >>> ops->complex_callback() to _setup_block_ops(). But more importantly >>> the caller gets more complicated. Instead of just calling >>> ops->set_me_up(), it has to check all the optional callbacks and call >>> the one by one. >>> >> >> Alright, I am thinking that perhaps because this register is kind of >> unique that its actually controlling a specific setting in the >> datapath, downstream also has separate ops for this. >> >> But thats fine, we can go ahead with the struct based approach. >> > > As data_compress has already landed, let me introduced the struct along > with the core_revision based approach in the core_revision series and > this series will expand that struct to include widebus too. Acked. Will rebase on top of the core_revision series and add widebus to the config struct. Thanks, Jessica Zhang
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); } static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx) 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 5b0f6627e29b..03ba3a1c7a46 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -513,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf, } +static void dpu_hw_intf_enable_widebus(struct dpu_hw_intf *ctx) +{ + u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2); + + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN; + + DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2); +} + static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx) { u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2); @@ -545,6 +554,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops, if (cap & BIT(DPU_INTF_DATA_COMPRESS)) ops->enable_compression = dpu_hw_intf_enable_compression; + + if (cap & BIT(DPU_INTF_DATABUS_WIDEN)) + ops->enable_widebus = dpu_hw_intf_enable_widebus; } struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h index 99e21c4137f9..64a17b99d3d1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h @@ -71,6 +71,7 @@ struct intf_status { * Return: 0 on success, -ETIMEDOUT on timeout * @vsync_sel: Select vsync signal for tear-effect configuration * @enable_compression: Enable data compression + * @enable_widebus: Enable widebus */ struct dpu_hw_intf_ops { void (*setup_timing_gen)(struct dpu_hw_intf *intf, @@ -109,6 +110,8 @@ struct dpu_hw_intf_ops { void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay); void (*enable_compression)(struct dpu_hw_intf *intf); + + void (*enable_widebus)(struct dpu_hw_intf *intf); }; struct dpu_hw_intf {
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@quicinc.com> --- 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(+)