| Message ID | 1443483936-2348-2-git-send-email-rafael.antognolli@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rafael,
[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt'
include/drm/drm_crtc.h:310: warning: No description found for parameter 'mode_blob'
include/drm/drm_crtc.h:748: warning: No description found for parameter 'tile_blob_ptr'
include/drm/drm_crtc.h:787: warning: No description found for parameter 'rotation'
include/drm/drm_crtc.h:883: warning: No description found for parameter 'mutex'
include/drm/drm_crtc.h:883: warning: No description found for parameter 'helper_private'
include/drm/drm_crtc.h:931: warning: Excess struct/union/enum/typedef member 'base' description in 'drm_bridge'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tile_idr'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'delayed_event'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'edid_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dpms_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'path_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tile_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'plane_type_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'rotation_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_x'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_y'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_w'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_h'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_x'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_y'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_w'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_h'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_fb_id'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_id'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_active'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_mode_id'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dvi_i_subconnector_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dvi_i_select_subconnector_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_subconnector_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_select_subconnector_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_mode_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_left_margin_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_right_margin_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_top_margin_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_bottom_margin_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_brightness_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_contrast_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_flicker_reduction_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_overscan_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_saturation_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_hue_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'scaling_mode_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'aspect_ratio_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dirty_info_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'suggested_x_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'suggested_y_property'
include/drm/drm_crtc.h:1169: warning: No description found for parameter 'allow_fb_modifiers'
Warning(include/drm/drm_modeset_lock.h:47): Incorrect use of kernel-doc format: * Contended lock: if a lock is contended you should only call
Warning(include/drm/drm_modeset_lock.h:54): Incorrect use of kernel-doc format: * list of held locks (drm_modeset_lock)
Warning(include/drm/drm_modeset_lock.h:59): Incorrect use of kernel-doc format: * Trylock mode, use only for panic handlers!
Warning(include/drm/drm_modeset_lock.h:74): Incorrect use of kernel-doc format: * modeset lock
Warning(include/drm/drm_modeset_lock.h:79): Incorrect use of kernel-doc format: * Resources that are locked as part of an atomic update are added
>> include/drm/drm_dp_helper.h:703: warning: No description found for parameter 'connector'
include/drm/drm_dp_helper.h:703: warning: No description found for parameter 'i2c_nack_count'
include/drm/drm_dp_helper.h:703: warning: No description found for parameter 'i2c_defer_count'
drivers/gpu/drm/drm_dp_mst_topology.c:2211: warning: No description found for parameter 'connector'
include/drm/drm_dp_mst_helper.h:97: warning: No description found for parameter 'cached_edid'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'max_dpcd_transaction_bytes'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'sink_count'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'total_slots'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'avail_slots'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'total_pbn'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'qlock'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'tx_msg_downq'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'tx_msg_upq'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'tx_down_in_progress'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'tx_up_in_progress'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'payload_lock'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'proposed_vcpis'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'payloads'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'payload_mask'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'vcpi_mask'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'tx_waitq'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'work'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'tx_work'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'destroy_connector_list'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'destroy_connector_lock'
include/drm/drm_dp_mst_helper.h:470: warning: No description found for parameter 'destroy_connector_work'
drivers/gpu/drm/drm_dp_mst_topology.c:2211: warning: No description found for parameter 'connector'
include/drm/drmP.h:164: warning: No description found for parameter 'fmt'
include/drm/drmP.h:180: warning: No description found for parameter 'fmt'
include/drm/drmP.h:198: warning: No description found for parameter 'fmt'
include/drm/drmP.h:238: warning: No description found for parameter 'dev'
include/drm/drmP.h:238: warning: No description found for parameter 'data'
include/drm/drmP.h:238: warning: No description found for parameter 'file_priv'
include/drm/drmP.h:271: warning: No description found for parameter 'ioctl'
include/drm/drmP.h:271: warning: No description found for parameter '_func'
include/drm/drmP.h:271: warning: No description found for parameter '_flags'
include/drm/drmP.h:344: warning: cannot understand function prototype: 'struct drm_lock_data '
include/drm/drmP.h:397: warning: cannot understand function prototype: 'struct drm_driver '
include/drm/drmP.h:646: warning: cannot understand function prototype: 'struct drm_info_list '
include/drm/drmP.h:656: warning: cannot understand function prototype: 'struct drm_info_node '
include/drm/drmP.h:666: warning: cannot understand function prototype: 'struct drm_minor '
include/drm/drmP.h:712: warning: cannot understand function prototype: 'struct drm_device '
drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged'
drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt'
drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args'
drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1027: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1027: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1027: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1192: warning: No description found for parameter 'rps'
drivers/gpu/drm/i915/i915_gem.c:1398: warning: No description found for parameter 'req'
drivers/gpu/drm/i915/i915_gem.c:1433: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:1433: warning: No description found for parameter 'readonly'
drivers/gpu/drm/i915/i915_gem.c:1556: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1556: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1556: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1664: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1664: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:1664: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'vma'
drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'vmf'
drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'size'
drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'tiling_mode'
drivers/gpu/drm/i915/i915_gem.c:1952: warning: No description found for parameter 'fenced'
drivers/gpu/drm/i915/i915_gem.c:1952: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment'
drivers/gpu/drm/i915/i915_gem.c:2815: warning: No description found for parameter 'ring'
drivers/gpu/drm/i915/i915_gem.c:2944: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:2994: warning: No description found for parameter 'dev'
drivers/gpu/drm/i915/i915_gem.c:2994: warning: No description found for parameter 'data'
drivers/gpu/drm/i915/i915_gem.c:2994: warning: No description found for parameter 'file'
drivers/gpu/drm/i915/i915_gem.c:2994: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl'
drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'obj'
drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'vm'
drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'ggtt_view'
drivers/gpu/drm/i915/i915_gem.c:3355: warning: No description found for parameter 'alignment'
vim +/connector +703 include/drm/drm_dp_helper.h
1d002fa7 Simon Farnsworth 2015-02-10 687 * received, the adapter will drop down to the size given by the partial
1d002fa7 Simon Farnsworth 2015-02-10 688 * response for this transaction only.
732d50b4 Alex Deucher 2014-04-07 689 *
732d50b4 Alex Deucher 2014-04-07 690 * Note that the aux helper code assumes that the .transfer() function
732d50b4 Alex Deucher 2014-04-07 691 * only modifies the reply field of the drm_dp_aux_msg structure. The
732d50b4 Alex Deucher 2014-04-07 692 * retry logic and i2c helpers assume this is the case.
c197db75 Thierry Reding 2013-11-28 693 */
c197db75 Thierry Reding 2013-11-28 694 struct drm_dp_aux {
9dc40560 Jani Nikula 2014-03-14 695 const char *name;
88759686 Thierry Reding 2013-12-12 696 struct i2c_adapter ddc;
c197db75 Thierry Reding 2013-11-28 697 struct device *dev;
2bb29be3 Rafael Antognolli 2015-09-28 698 struct device *connector;
4f71d0cb Dave Airlie 2014-06-04 699 struct mutex hw_mutex;
c197db75 Thierry Reding 2013-11-28 700 ssize_t (*transfer)(struct drm_dp_aux *aux,
c197db75 Thierry Reding 2013-11-28 701 struct drm_dp_aux_msg *msg);
e9cf6194 Todd Previte 2014-11-04 702 unsigned i2c_nack_count, i2c_defer_count;
c197db75 Thierry Reding 2013-11-28 @703 };
c197db75 Thierry Reding 2013-11-28 704
c197db75 Thierry Reding 2013-11-28 705 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
c197db75 Thierry Reding 2013-11-28 706 void *buffer, size_t size);
c197db75 Thierry Reding 2013-11-28 707 ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
c197db75 Thierry Reding 2013-11-28 708 void *buffer, size_t size);
c197db75 Thierry Reding 2013-11-28 709
c197db75 Thierry Reding 2013-11-28 710 /**
c197db75 Thierry Reding 2013-11-28 711 * drm_dp_dpcd_readb() - read a single byte from the DPCD
:::::: The code at line 703 was first introduced by commit
:::::: c197db75ff5c1d4f015c7668a3715e230a5d7e27 drm/dp: Add AUX channel infrastructure
:::::: TO: Thierry Reding <treding@nvidia.com>
:::::: CC: Thierry Reding <treding@nvidia.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Rafael, On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > This is useful to determine which connector owns this AUX channel. WTF? I posted a patch in August which does exactly that: http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html Can also be pulled in from this git repo: https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 My patch has the advantage that it updates all the drivers which use drm_dp_aux to fill that attribute. Yours only updates i915. Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, quote: "That will also clear up the confusion with drm_dp_aux, adding a drm_connector there feels wrong since not every dp_aux line has a connector (e.g. for dp mst). If we can lift this relation out into drivers (where this is known) that seems cleaner." So now Intel itself does precisely what Daniel criticized? Confusing! Source: http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html Best regards, Lukas > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 1 + > include/drm/drm_dp_helper.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 77f7330..f90439d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > intel_dp->aux.name = name; > intel_dp->aux.dev = dev->dev; > + intel_dp->aux.connector = connector->base.kdev; > intel_dp->aux.transfer = intel_dp_aux_transfer; > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 9ec4716..e009b5d 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -702,6 +702,7 @@ struct drm_dp_aux { > const char *name; > struct i2c_adapter ddc; > struct device *dev; > + struct device *connector; > struct mutex hw_mutex; > ssize_t (*transfer)(struct drm_dp_aux *aux, > struct drm_dp_aux_msg *msg); > -- > 2.4.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote: > Hi Rafael, > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > > This is useful to determine which connector owns this AUX channel. > > WTF? I posted a patch in August which does exactly that: > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html > > Can also be pulled in from this git repo: > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 > > My patch has the advantage that it updates all the drivers which use > drm_dp_aux to fill that attribute. Yours only updates i915. > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, > quote: > > "That will also clear up the confusion with drm_dp_aux, adding a > drm_connector there feels wrong since not every dp_aux line has a > connector (e.g. for dp mst). If we can lift this relation out into drivers > (where this is known) that seems cleaner." > > So now Intel itself does precisely what Daniel criticized? Confusing! > > Source: > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html Critism is still valid, and thinking about this again a cleaner solution would be to just have a correct parent/child relationship in the device hirarchy. I.e. add a struct device *parent to the aux channel structure which should point to the right connector. Thanks for pointing out that I missed properly delayering this. -Daniel > > > Best regards, > > Lukas > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > include/drm/drm_dp_helper.h | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 77f7330..f90439d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > > > intel_dp->aux.name = name; > > intel_dp->aux.dev = dev->dev; > > + intel_dp->aux.connector = connector->base.kdev; > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > index 9ec4716..e009b5d 100644 > > --- a/include/drm/drm_dp_helper.h > > +++ b/include/drm/drm_dp_helper.h > > @@ -702,6 +702,7 @@ struct drm_dp_aux { > > const char *name; > > struct i2c_adapter ddc; > > struct device *dev; > > + struct device *connector; > > struct mutex hw_mutex; > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > struct drm_dp_aux_msg *msg); > > -- > > 2.4.3 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote: > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote: > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > > > This is useful to determine which connector owns this AUX channel. > > > > WTF? I posted a patch in August which does exactly that: > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html > > > > Can also be pulled in from this git repo: > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 > > > > My patch has the advantage that it updates all the drivers which use > > drm_dp_aux to fill that attribute. Yours only updates i915. > > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, > > quote: > > > > "That will also clear up the confusion with drm_dp_aux, adding a > > drm_connector there feels wrong since not every dp_aux line has a > > connector (e.g. for dp mst). If we can lift this relation out into drivers > > (where this is known) that seems cleaner." > > > > So now Intel itself does precisely what Daniel criticized? Confusing! > > > > Source: > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html > > Critism is still valid, and thinking about this again a cleaner solution > would be to just have a correct parent/child relationship in the device > hirarchy. I.e. add a struct device *parent to the aux channel structure > which should point to the right connector. We already have that: struct drm_dp_aux { const char *name; struct i2c_adapter ddc; struct device *dev; <----------- struct mutex hw_mutex; ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg); unsigned i2c_nack_count, i2c_defer_count; }; What Rafael is struggling with is that you cannot unambiguously get from drm_dp_aux->dev to the drm_connector. (The drm_device may have multiple drm_connectors with type DRM_MODE_CONNECTOR_DisplayPort.) > > Thanks for pointing out that I missed properly delayering this. > -Daniel > > > > > > > Best regards, > > > > Lukas > > > > > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > include/drm/drm_dp_helper.h | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 77f7330..f90439d 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > > > > > intel_dp->aux.name = name; > > > intel_dp->aux.dev = dev->dev; > > > + intel_dp->aux.connector = connector->base.kdev; > > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > index 9ec4716..e009b5d 100644 > > > --- a/include/drm/drm_dp_helper.h > > > +++ b/include/drm/drm_dp_helper.h > > > @@ -702,6 +702,7 @@ struct drm_dp_aux { > > > const char *name; > > > struct i2c_adapter ddc; > > > struct device *dev; > > > + struct device *connector; > > > struct mutex hw_mutex; > > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > > struct drm_dp_aux_msg *msg); > > > -- > > > 2.4.3 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Sep 29, 2015 at 05:27:33PM +0200, Lukas Wunner wrote: > Hi Daniel, > > On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote: > > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote: > > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > > > > This is useful to determine which connector owns this AUX channel. > > > > > > WTF? I posted a patch in August which does exactly that: > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html > > > > > > Can also be pulled in from this git repo: > > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 > > > > > > My patch has the advantage that it updates all the drivers which use > > > drm_dp_aux to fill that attribute. Yours only updates i915. > > > > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, > > > quote: > > > > > > "That will also clear up the confusion with drm_dp_aux, adding a > > > drm_connector there feels wrong since not every dp_aux line has a > > > connector (e.g. for dp mst). If we can lift this relation out into drivers > > > (where this is known) that seems cleaner." > > > > > > So now Intel itself does precisely what Daniel criticized? Confusing! > > > > > > Source: > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html > > > > Critism is still valid, and thinking about this again a cleaner solution > > would be to just have a correct parent/child relationship in the device > > hirarchy. I.e. add a struct device *parent to the aux channel structure > > which should point to the right connector. > > We already have that: > > struct drm_dp_aux { > const char *name; > struct i2c_adapter ddc; > struct device *dev; <----------- > struct mutex hw_mutex; > ssize_t (*transfer)(struct drm_dp_aux *aux, > struct drm_dp_aux_msg *msg); > unsigned i2c_nack_count, i2c_defer_count; > }; > > What Rafael is struggling with is that you cannot unambiguously > get from drm_dp_aux->dev to the drm_connector. (The drm_device > may have multiple drm_connectors with type > DRM_MODE_CONNECTOR_DisplayPort.) What I meant to say is that we don't need that, if instead of filling in the overall dev in dp_aux->dev we fill in the connector sysfs device thing. The we have proper nesting, like with i2c buses. And then there's no need for a connector property in sysfs to show that link (which should be done with a proper sysfs link anyway). -Daniel > > > > > Thanks for pointing out that I missed properly delayering this. > > -Daniel > > > > > > > > > > > Best regards, > > > > > > Lukas > > > > > > > > > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > > include/drm/drm_dp_helper.h | 1 + > > > > 2 files changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index 77f7330..f90439d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > > > > > > > intel_dp->aux.name = name; > > > > intel_dp->aux.dev = dev->dev; > > > > + intel_dp->aux.connector = connector->base.kdev; > > > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > > index 9ec4716..e009b5d 100644 > > > > --- a/include/drm/drm_dp_helper.h > > > > +++ b/include/drm/drm_dp_helper.h > > > > @@ -702,6 +702,7 @@ struct drm_dp_aux { > > > > const char *name; > > > > struct i2c_adapter ddc; > > > > struct device *dev; > > > > + struct device *connector; > > > > struct mutex hw_mutex; > > > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > > > struct drm_dp_aux_msg *msg); > > > > -- > > > > 2.4.3 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote: > Hi Rafael, > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > > This is useful to determine which connector owns this AUX channel. > > WTF? I posted a patch in August which does exactly that: > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html > > Can also be pulled in from this git repo: > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 > > My patch has the advantage that it updates all the drivers which use > drm_dp_aux to fill that attribute. Yours only updates i915. > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, > quote: > > "That will also clear up the confusion with drm_dp_aux, adding a > drm_connector there feels wrong since not every dp_aux line has a > connector (e.g. for dp mst). If we can lift this relation out into drivers > (where this is known) that seems cleaner." > > So now Intel itself does precisely what Daniel criticized? Confusing! I'm sorry, I don't follow this list as closely yet, so I didn't see that patch there, otherwise I would have talked to Daniel about it in the first place. Thanks, Rafael > Source: > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html > > > Best regards, > > Lukas > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > include/drm/drm_dp_helper.h | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 77f7330..f90439d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > > > intel_dp->aux.name = name; > > intel_dp->aux.dev = dev->dev; > > + intel_dp->aux.connector = connector->base.kdev; > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > index 9ec4716..e009b5d 100644 > > --- a/include/drm/drm_dp_helper.h > > +++ b/include/drm/drm_dp_helper.h > > @@ -702,6 +702,7 @@ struct drm_dp_aux { > > const char *name; > > struct i2c_adapter ddc; > > struct device *dev; > > + struct device *connector; > > struct mutex hw_mutex; > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > struct drm_dp_aux_msg *msg); > > -- > > 2.4.3 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 29, 2015 at 06:25:44PM +0200, Daniel Vetter wrote: > On Tue, Sep 29, 2015 at 05:27:33PM +0200, Lukas Wunner wrote: > > Hi Daniel, > > > > On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote: > > > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote: > > > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > > > > > This is useful to determine which connector owns this AUX channel. > > > > > > > > WTF? I posted a patch in August which does exactly that: > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html > > > > > > > > Can also be pulled in from this git repo: > > > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 > > > > > > > > My patch has the advantage that it updates all the drivers which use > > > > drm_dp_aux to fill that attribute. Yours only updates i915. > > > > > > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, > > > > quote: > > > > > > > > "That will also clear up the confusion with drm_dp_aux, adding a > > > > drm_connector there feels wrong since not every dp_aux line has a > > > > connector (e.g. for dp mst). If we can lift this relation out into drivers > > > > (where this is known) that seems cleaner." > > > > > > > > So now Intel itself does precisely what Daniel criticized? Confusing! > > > > > > > > Source: > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html > > > > > > Critism is still valid, and thinking about this again a cleaner solution > > > would be to just have a correct parent/child relationship in the device > > > hirarchy. I.e. add a struct device *parent to the aux channel structure > > > which should point to the right connector. > > > > We already have that: > > > > struct drm_dp_aux { > > const char *name; > > struct i2c_adapter ddc; > > struct device *dev; <----------- > > struct mutex hw_mutex; > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > struct drm_dp_aux_msg *msg); > > unsigned i2c_nack_count, i2c_defer_count; > > }; > > > > What Rafael is struggling with is that you cannot unambiguously > > get from drm_dp_aux->dev to the drm_connector. (The drm_device > > may have multiple drm_connectors with type > > DRM_MODE_CONNECTOR_DisplayPort.) > > What I meant to say is that we don't need that, if instead of filling in > the overall dev in dp_aux->dev we fill in the connector sysfs device > thing. The we have proper nesting, like with i2c buses. And then there's > no need for a connector property in sysfs to show that link (which should > be done with a proper sysfs link anyway). OK, I sent a new version, which does not add a new *connector pointer, and uses the dev pointer on the struct to store the drm_connector device, instead of the drm_device device. Is that what you meant? In any case, as I mention on the patch, it is already how some drivers do, while others store the drm_device. This leaves the aux device, for instance in my case, at: /sys/class/drm/card0/card0-eDP-1/drm_dp_aux0 If this is what you wanted, I can send other patches to the proper mailing lists, trying to update other drivers. -- Rafael > > > > > > > > Thanks for pointing out that I missed properly delayering this. > > > -Daniel > > > > > > > > > > > > > > > Best regards, > > > > > > > > Lukas > > > > > > > > > > > > > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > > > include/drm/drm_dp_helper.h | 1 + > > > > > 2 files changed, 2 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > > index 77f7330..f90439d 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > > > > > > > > > intel_dp->aux.name = name; > > > > > intel_dp->aux.dev = dev->dev; > > > > > + intel_dp->aux.connector = connector->base.kdev; > > > > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > > > > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > > > index 9ec4716..e009b5d 100644 > > > > > --- a/include/drm/drm_dp_helper.h > > > > > +++ b/include/drm/drm_dp_helper.h > > > > > @@ -702,6 +702,7 @@ struct drm_dp_aux { > > > > > const char *name; > > > > > struct i2c_adapter ddc; > > > > > struct device *dev; > > > > > + struct device *connector; > > > > > struct mutex hw_mutex; > > > > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > > > > struct drm_dp_aux_msg *msg); > > > > > -- > > > > > 2.4.3 > > > > > > > > > > _______________________________________________ > > > > > dri-devel mailing list > > > > > dri-devel@lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Oct 09, 2015 at 02:50:10PM -0700, Rafael Antognolli wrote: > On Tue, Sep 29, 2015 at 06:25:44PM +0200, Daniel Vetter wrote: > > On Tue, Sep 29, 2015 at 05:27:33PM +0200, Lukas Wunner wrote: > > > Hi Daniel, > > > > > > On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote: > > > > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote: > > > > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote: > > > > > > This is useful to determine which connector owns this AUX channel. > > > > > > > > > > WTF? I posted a patch in August which does exactly that: > > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html > > > > > > > > > > Can also be pulled in from this git repo: > > > > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6 > > > > > > > > > > My patch has the advantage that it updates all the drivers which use > > > > > drm_dp_aux to fill that attribute. Yours only updates i915. > > > > > > > > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux, > > > > > quote: > > > > > > > > > > "That will also clear up the confusion with drm_dp_aux, adding a > > > > > drm_connector there feels wrong since not every dp_aux line has a > > > > > connector (e.g. for dp mst). If we can lift this relation out into drivers > > > > > (where this is known) that seems cleaner." > > > > > > > > > > So now Intel itself does precisely what Daniel criticized? Confusing! > > > > > > > > > > Source: > > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html > > > > > > > > Critism is still valid, and thinking about this again a cleaner solution > > > > would be to just have a correct parent/child relationship in the device > > > > hirarchy. I.e. add a struct device *parent to the aux channel structure > > > > which should point to the right connector. > > > > > > We already have that: > > > > > > struct drm_dp_aux { > > > const char *name; > > > struct i2c_adapter ddc; > > > struct device *dev; <----------- > > > struct mutex hw_mutex; > > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > > struct drm_dp_aux_msg *msg); > > > unsigned i2c_nack_count, i2c_defer_count; > > > }; > > > > > > What Rafael is struggling with is that you cannot unambiguously > > > get from drm_dp_aux->dev to the drm_connector. (The drm_device > > > may have multiple drm_connectors with type > > > DRM_MODE_CONNECTOR_DisplayPort.) > > > > What I meant to say is that we don't need that, if instead of filling in > > the overall dev in dp_aux->dev we fill in the connector sysfs device > > thing. The we have proper nesting, like with i2c buses. And then there's > > no need for a connector property in sysfs to show that link (which should > > be done with a proper sysfs link anyway). > > OK, I sent a new version, which does not add a new *connector pointer, > and uses the dev pointer on the struct to store the drm_connector > device, instead of the drm_device device. Is that what you meant? In > any case, as I mention on the patch, it is already how some drivers do, > while others store the drm_device. > > This leaves the aux device, for instance in my case, at: > > /sys/class/drm/card0/card0-eDP-1/drm_dp_aux0 > > If this is what you wanted, I can send other patches to the proper > mailing lists, trying to update other drivers. Yeah that's kinda what I had in mind, makes the nesting more obvious. Especially for mst hub dp aux channels. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 77f7330..f90439d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) intel_dp->aux.name = name; intel_dp->aux.dev = dev->dev; + intel_dp->aux.connector = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", name, diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 9ec4716..e009b5d 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -702,6 +702,7 @@ struct drm_dp_aux { const char *name; struct i2c_adapter ddc; struct device *dev; + struct device *connector; struct mutex hw_mutex; ssize_t (*transfer)(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg);
This is useful to determine which connector owns this AUX channel. Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 1 + 2 files changed, 2 insertions(+)