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
--
With best wishes
Dmitry
