On Mon, Nov 19, 2018 at 02:34:53PM -0800, chandanu@xxxxxxxxxxxxxx wrote:
> On 2018-10-23 09:28, Sean Paul wrote:
> > On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote:
> > > Add the needed displayPort files to enable DP driver
> > > on msm target.
> > >
> > > "dp_display" module is the main module that calls into
> > > other sub-modules. "dp_drm" file represents the interface
> > > between DRM framework and DP driver.
> > >
> >
>
> Hello Sean,
> I had few queries regarding your comments. Shared my queries below. Please
> share your feedback.
/snip
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> > > b/drivers/gpu/drm/msm/dp/dp_aux.c
> > > new file mode 100644
> > > index 0000000..00b82c2
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > > @@ -0,0 +1,570 @@
> >
> > /snip
> >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h
> > > b/drivers/gpu/drm/msm/dp/dp_aux.h
> > > new file mode 100644
> > > index 0000000..0c300ed
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> >
> > /snip
> >
> > I didn't really review the dp_aux.* files as-is. Please convert to using
> > drm_dp_aux and I'll take another look.
> >
>
> Please correct me if I am wrong. Looks like you are suggesting to use most
> of the drm_dp_aux struct variable
> instead of what we have defined locally.
>
Correct
/snip
> > > + struct dp_catalog_ctrl ctrl = {
> > > + .state_ctrl = dp_catalog_ctrl_state_ctrl,
> > > + .config_ctrl = dp_catalog_ctrl_config_ctrl,
> > > + .lane_mapping = dp_catalog_ctrl_lane_mapping,
> > > + .mainlink_ctrl = dp_catalog_ctrl_mainlink_ctrl,
> > > + .config_misc = dp_catalog_ctrl_config_misc,
> > > + .config_msa = dp_catalog_ctrl_config_msa,
> > > + .set_pattern = dp_catalog_ctrl_set_pattern,
> > > + .reset = dp_catalog_ctrl_reset,
> > > + .usb_reset = dp_catalog_ctrl_usb_reset,
> > > + .mainlink_ready = dp_catalog_ctrl_mainlink_ready,
> > > + .enable_irq = dp_catalog_ctrl_enable_irq,
> > > + .hpd_config = dp_catalog_ctrl_hpd_config,
> > > + .phy_reset = dp_catalog_ctrl_phy_reset,
> > > + .phy_lane_cfg = dp_catalog_ctrl_phy_lane_cfg,
> > > + .update_vx_px = dp_catalog_ctrl_update_vx_px,
> > > + .get_interrupt = dp_catalog_ctrl_get_interrupt,
> > > + .update_transfer_unit = dp_catalog_ctrl_update_transfer_unit,
> > > + .send_phy_pattern = dp_catalog_ctrl_send_phy_pattern,
> > > + .read_phy_pattern = dp_catalog_ctrl_read_phy_pattern,
> >
> > So, what's the benefit of adding the catalog abstractions? Do you ever
> > have
> > multiple/alternate implementations of a catalog? It doesn't seem like
> > it. You
> > should just remove all of the catalog overhead and call the functions
> > directly.
>
> We use catalog abstractions for two different reasons:
> 1. To support multiple hardware since these implementations are hardware
> specific.
> 2. To virtualize DP hardware.
>
> Our internal hardware already supports multiple chips. We want to keep these
> abstractions to
> make the code consistent between upstream and internal implementation.
>
I'm not sure how this differs from having a common header/function declarations
and multiple definitions in a .c gated by CONFIG_*? Ideally even this wouldn't be
necessary since hw changes are either incremental and can be done more precisely
than wholesale changing the implementation.
If you want to virtualize, you can do the CONFIG_* thing. However since that's
not going upstream, you can carry the Kconfig/Makefile change in your
downstream.
All of these abstractions add thousands of lines of code, extra files, and multiple
levels of indirection. IMO, I don't think the benefits outweigh the costs.
/snip
> > > +static void dp_ctrl_calc_tu_parameters(struct dp_ctrl_private *ctrl,
> > > + struct dp_vc_tu_mapping_table *tu_table)
> > > +{
> > > + u32 multiplier = 1000000;
> > > + u64 pclk, lclk;
> > > + u8 bpp, ln_cnt;
> > > + int run_idx = 0;
> > > + u32 lwidth, h_blank;
> > > + u32 fifo_empty = 0;
> > > + u32 ratio_scale = 1001;
> > > + u64 temp, ratio, original_ratio;
> > > + u64 temp2, reminder;
> > > + u64 temp3, temp4, result = 0;
> > > +
> > > + u64 err = multiplier;
> > > + u64 n_err = 0, n_n_err = 0;
> > > + bool n_err_neg, nn_err_neg;
> > > + u8 hblank_margin = 16;
> > > +
> > > + u8 tu_size, tu_size_desired = 0, tu_size_minus1;
> > > + int valid_boundary_link;
> > > + u64 resulting_valid;
> > > + u64 total_valid;
> > > + u64 effective_valid;
> > > + u64 effective_valid_recorded;
> > > + int n_tus;
> > > + int n_tus_per_lane;
> > > + int paired_tus;
> > > + int remainder_tus;
> > > + int remainder_tus_upper, remainder_tus_lower;
> > > + int extra_bytes;
> > > + int filler_size;
> > > + int delay_start_link;
> > > + int boundary_moderation_en = 0;
> > > + int upper_bdry_cnt = 0;
> > > + int lower_bdry_cnt = 0;
> > > + int i_upper_bdry_cnt = 0;
> > > + int i_lower_bdry_cnt = 0;
> > > + int valid_lower_boundary_link = 0;
> > > + int even_distribution_bf = 0;
> > > + int even_distribution_legacy = 0;
> > > + int even_distribution = 0;
> > > + int min_hblank = 0;
> > > + int extra_pclk_cycles;
> > > + u8 extra_pclk_cycle_delay = 4;
> > > + int extra_pclk_cycles_in_link_clk;
> > > + u64 ratio_by_tu;
> > > + u64 average_valid2;
> > > + u64 extra_buffer_margin;
> > > + int new_valid_boundary_link;
> > > +
> > > + u64 resulting_valid_tmp;
> > > + u64 ratio_by_tu_tmp;
> > > + int n_tus_tmp;
> > > + int extra_pclk_cycles_tmp;
> > > + int extra_pclk_cycles_in_lclk_tmp;
> > > + int extra_req_bytes_new_tmp;
> > > + int filler_size_tmp;
> > > + int lower_filler_size_tmp;
> > > + int delay_start_link_tmp;
> > > + int min_hblank_tmp = 0;
> > > + bool extra_req_bytes_is_neg = false;
> > > + struct dp_panel_info *pinfo = &ctrl->panel->pinfo;
> > > +
> > > + u8 dp_brute_force = 1;
> > > + u64 brute_force_threshold = 10;
> > > + u64 diff_abs;
> >
> >
> > 76 stack vars and ~450 lines must be a new record somewhere :)
> >
> > I'm going to skip reviewing this function and let you
> > split/simplify/reduce it.
> > Please just make it more readable (comments, docs, helper functions,
> > etc).
>
> Yes, will try to split this function as modular as possible.
> JFYI, this function is IP/hardware specific implementation. It might not be
> possible
> to provide lot of comments to make is understandable.
Ok, please do what you can to simplify/split and add a comment that this is
magic from the HW team.
/snip
> > > +int msm_dp_modeset_init(struct msm_dp *dp_display, struct
> > > drm_device *dev,
> > > + struct drm_encoder *encoder)
> >
> > Where is this called?
>
> This is called from dpu_kms.c. The changes from DPU will be dependent on
> these changes.
>
Hmm, is that code missing? I don't see this function being called anywhere in
this patch.
/snip
> > > +static const struct drm_bridge_funcs dp_bridge_ops = {
> > > + .mode_fixup = dp_bridge_mode_fixup,
> > > + .pre_enable = dp_bridge_pre_enable,
> > > + .enable = dp_bridge_enable,
> > > + .disable = dp_bridge_disable,
> > > + .post_disable = dp_bridge_post_disable,
> > > + .mode_set = dp_bridge_mode_set,
> > > +};
> >
> > I'm not convinced you need any of this. The only advantage a bridge gets
> > you is
> > to be involved in modeset. However the only thing you do in modeset is
> > save the
> > mode (which is only used in enable). So why not just use the mode from
> > the
> > crtc's state when encoder->enable is called?
> >
> > That allows you to delete all of the bridge stuff here.
> >
> Will keep enable, disable and mode_set func ops and remove the remaining
> ops.
> Please let me know if you still have concerns with these func ops.
>
Why not remove all of it and call this code from the encoder's enable function?
You shouldn't need a bridge at all.
/snip
> > > +static int dp_parser_init_clk_data(struct dp_parser *parser)
> > > +{
> > > + int num_clk = 0, i = 0, rc = 0;
> > > + int core_clk_count = 0, ctrl_clk_count = 0;
> > > + const char *core_clk = "core";
> > > + const char *ctrl_clk = "ctrl";
> > > + const char *clk_name;
> > > + struct device *dev = &parser->pdev->dev;
> > > + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
> > > + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
> > > +
> > > + num_clk = of_property_count_strings(dev->of_node, "clock-names");
> > > + if (num_clk <= 0) {
> > > + pr_err("no clocks are defined\n");
> > > + rc = -EINVAL;
> > > + goto exit;
> > > + }
> > > +
> > > + for (i = 0; i < num_clk; i++) {
> > > + of_property_read_string_index(dev->of_node,
> > > + "clock-names", i, &clk_name);
> >
> > Check return value
> >
> > > +
> > > + if (dp_parser_check_prefix(core_clk, clk_name))
> > > + core_clk_count++;
> > > +
> > > + if (dp_parser_check_prefix(ctrl_clk, clk_name))
> > > + ctrl_clk_count++;
> >
> > Just use "core"/"ctrl" directly here.
> >
> > Have you considered just having 2 clock lists? That would avoid having
> > to do
> > this.
> I am not clear on what you are suggesting? We currently have two clock-lists
> (core & ctrl).
>
>
I meant in the dt bindings. Instead of one clocks list, could you split into
core and ctrl? That way you wouldn't have to parse based on name.
/snip
> >
> > ---- START hmmmm
> I didn't understand, can you please provide more details regarding "START"
> and "END".
> >
> > > + struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
> > > + struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
> > > +
> > > + core_power = &parser->mp[DP_CORE_PM];
> > > + ctrl_power = &parser->mp[DP_CTRL_PM];
> >
> > ---- END hmmmm
> >
You assign core_power and ctrl_power in the declaration and then immediately
assign the value again.
/snip
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
