[PATCH v4 0/2] Add drm_dp_aux chardev support.

[PATCH v4 0/2] Add drm_dp_aux chardev support.

[Rafael Antognolli]

This series implement support to a drm_dp_aux chardev that allows reading and
writing an arbitrary amount of bytes to arbitrary dpcd register addresses using
regular read, write and lseek operations.

v2:
 - lseek is used to select the register to read/write
 - read/write are used instead of ioctl
 - no blocking_notifier is used, just a direct callback

v3:
 - use drm_dp_aux_dev prefix for public functions
 - chardev is named drm_dp_auxN
 - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
 time
 - remove notifier list from the implementation
 - option on menuconfig is now a boolean
 - add inline stub functions to avoid breakage when this option is disabled

v4:
 - fix build system changes - actually disable this module when not selected.

Rafael Antognolli (2):
  drm/dp: Store the drm_connector device pointer on the helper.
  drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

 drivers/gpu/drm/Kconfig          |   8 +
 drivers/gpu/drm/Makefile         |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 drivers/gpu/drm/i915/intel_dp.c  |   1 +
 include/drm/drm_dp_aux_dev.h     |  50 ++++++
 include/drm/drm_dp_helper.h      |   1 +
 7 files changed, 423 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

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(+)

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

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 17548 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6062 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Lukas Wunner]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Lukas Wunner]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Rafael Antognolli]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.

Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
---
 drivers/gpu/drm/Kconfig          |   8 +
 drivers/gpu/drm/Makefile         |   1 +
 drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_dp_helper.c  |   5 +
 include/drm/drm_dp_aux_dev.h     |  50 ++++++
 5 files changed, 421 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
 create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..64fa0f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
 	bool
 	depends on DRM
 
+config DRM_DP_AUX_CHARDEV
+	bool "DRM DP AUX Interface"
+	depends on DRM
+	help
+	  Choose this option to enable a /dev/drm_dp_auxN node that allows to
+	  read and write values to arbitrary DPCD registers on the DP aux
+	  channel.
+
 config DRM_KMS_HELPER
 	tristate
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..e5ae7f0 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 0000000..e337081
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Rafael Antognolli <rafael.antognolli@intel.com>
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <asm/uaccess.h>
+#include <drm/drm_dp_helper.h>
+#include <drm/drm_crtc.h>
+
+struct drm_dp_aux_dev {
+	struct list_head list;
+	unsigned index;
+	struct drm_dp_aux *aux;
+	struct device *dev;
+};
+
+#define DRM_AUX_MINORS	256
+static int aux_max_offset = 1 << 20;
+static int drm_dp_aux_dev_count = 0;
+static LIST_HEAD(drm_dp_aux_dev_list);
+static DEFINE_SPINLOCK(drm_dp_aux_dev_list_lock);
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+	struct drm_dp_aux_dev *aux_dev;
+
+	spin_lock(&drm_dp_aux_dev_list_lock);
+	list_for_each_entry(aux_dev, &drm_dp_aux_dev_list, list) {
+		if (aux_dev->index == index)
+			goto found;
+	}
+
+	aux_dev = NULL;
+found:
+	spin_unlock(&drm_dp_aux_dev_list_lock);
+	return aux_dev;
+}
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
+{
+	struct drm_dp_aux_dev *aux_dev;
+
+	spin_lock(&drm_dp_aux_dev_list_lock);
+	list_for_each_entry(aux_dev, &drm_dp_aux_dev_list, list) {
+		if (aux_dev->aux == aux)
+			goto found;
+	}
+
+	aux_dev = NULL;
+found:
+	spin_unlock(&drm_dp_aux_dev_list_lock);
+	return aux_dev;
+}
+
+static struct drm_dp_aux_dev *get_free_drm_dp_aux_dev(struct drm_dp_aux *aux)
+{
+	struct drm_dp_aux_dev *aux_dev;
+	int index;
+
+	spin_lock(&drm_dp_aux_dev_list_lock);
+	index = drm_dp_aux_dev_count;
+	spin_unlock(&drm_dp_aux_dev_list_lock);
+	if (index >= DRM_AUX_MINORS) {
+		printk(KERN_ERR "drm_dp_aux_dev: Out of device minors (%d)\n",
+		       index);
+		return ERR_PTR(-ENODEV);
+	}
+
+	aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+	if (!aux_dev)
+		return ERR_PTR(-ENOMEM);
+	aux_dev->aux = aux;
+	aux_dev->index = index;
+
+	spin_lock(&drm_dp_aux_dev_list_lock);
+	drm_dp_aux_dev_count++;
+	list_add_tail(&aux_dev->list, &drm_dp_aux_dev_list);
+	spin_unlock(&drm_dp_aux_dev_list_lock);
+	return aux_dev;
+}
+
+static void return_drm_dp_aux_dev(struct drm_dp_aux_dev *aux_dev)
+{
+	spin_lock(&drm_dp_aux_dev_list_lock);
+	list_del(&aux_dev->list);
+	spin_unlock(&drm_dp_aux_dev_list_lock);
+	kfree(aux_dev);
+}
+
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct drm_dp_aux_dev *aux_dev =
+		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
+
+	if (!aux_dev)
+		return -ENODEV;
+	return sprintf(buf, "%s\n", aux_dev->aux->name);
+}
+static DEVICE_ATTR_RO(name);
+
+static ssize_t connector_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct drm_dp_aux_dev *aux_dev =
+		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
+	struct drm_dp_aux *aux;
+	struct device *conn_dev;
+	struct drm_connector *connector = NULL;
+
+	if (!aux_dev)
+		return -ENODEV;
+	aux = aux_dev->aux;
+	conn_dev = aux->connector;
+	if (!conn_dev)
+		return sprintf(buf, "unknown\n");
+
+	connector = dev_get_drvdata(aux->connector);
+
+	return sprintf(buf, "%s\n", connector->name);
+}
+static DEVICE_ATTR_RO(connector);
+
+static struct attribute *drm_dp_aux_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_connector.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(drm_dp_aux);
+
+static int auxdev_open(struct inode *inode, struct file *file)
+{
+	unsigned int minor = iminor(inode);
+	struct drm_dp_aux_dev *aux_dev;
+
+	aux_dev = drm_dp_aux_dev_get_by_minor(minor);
+	if (!aux_dev)
+		return -ENODEV;
+
+	file->private_data = aux_dev;
+	return 0;
+}
+
+static loff_t auxdev_llseek(struct file *file, loff_t offset, int whence)
+{
+	return fixed_size_llseek(file, offset, whence, aux_max_offset);
+}
+
+static ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
+			   loff_t *offset)
+{
+	ssize_t bytes_pending, num_bytes_processed = 0;
+	struct drm_dp_aux_dev *aux_dev = file->private_data;
+
+	bytes_pending = aux_max_offset - *offset;
+	bytes_pending = count < bytes_pending ? count : bytes_pending;
+	if (bytes_pending < 0)
+		return -EFAULT;
+
+	if (!access_ok(VERIFY_WRITE, buf, bytes_pending))
+		return -EFAULT;
+
+	while (bytes_pending > 0) {
+		uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
+		ssize_t todo = sizeof(localbuf);
+		ssize_t res;
+		todo = bytes_pending < todo ? bytes_pending : todo;
+
+		res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, todo);
+		if (res <= 0) {
+			return num_bytes_processed ? num_bytes_processed : res;
+		}
+		if (__copy_to_user(buf + num_bytes_processed, localbuf, res)) {
+			return num_bytes_processed ?
+				num_bytes_processed : -EFAULT;
+		}
+		bytes_pending -= res;
+		*offset += res;
+		num_bytes_processed += res;
+	}
+
+	return num_bytes_processed;
+}
+
+static ssize_t auxdev_write(struct file *file, const char __user *buf,
+			    size_t count, loff_t *offset)
+{
+	ssize_t bytes_pending, num_bytes_processed = 0;
+	struct drm_dp_aux_dev *aux_dev = file->private_data;
+
+	bytes_pending = aux_max_offset - *offset;
+	bytes_pending = count < bytes_pending ? count : bytes_pending;
+	if (bytes_pending < 0)
+		return -EFAULT;
+
+	if (!access_ok(VERIFY_READ, buf, bytes_pending))
+		return -EFAULT;
+
+	while (bytes_pending > 0) {
+		uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
+		ssize_t todo = sizeof(localbuf);
+		ssize_t res;
+		todo = bytes_pending < todo ? bytes_pending : todo;
+		if (__copy_from_user(localbuf,
+				     buf + num_bytes_processed, todo)) {
+			return num_bytes_processed ?
+				num_bytes_processed : -EFAULT;
+		}
+
+		res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, todo);
+		if (res <= 0) {
+			return num_bytes_processed ? num_bytes_processed : res;
+		}
+		bytes_pending -= res;
+		*offset += res;
+		num_bytes_processed += res;
+	}
+
+	return num_bytes_processed;
+}
+
+static const struct file_operations auxdev_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= auxdev_llseek,
+	.read		= auxdev_read,
+	.write		= auxdev_write,
+	.open		= auxdev_open,
+};
+
+static struct class *drm_dp_aux_dev_class;
+static int drm_dev_major = -1;
+
+#define to_auxdev(d) container_of(d, struct drm_dp_aux_dev, aux)
+
+/**
+ * drm_dp_aux_register_devnode() - register a devnode for this aux channel
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
+{
+	struct drm_dp_aux_dev *aux_dev;
+	int res;
+
+	aux_dev = get_free_drm_dp_aux_dev(aux);
+	if (IS_ERR(aux_dev))
+		return PTR_ERR(aux_dev);
+
+	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
+				     MKDEV(drm_dev_major, aux_dev->index), NULL,
+				     "drm_dp_aux%d", aux_dev->index);
+	if (IS_ERR(aux_dev->dev)) {
+		res = PTR_ERR(aux_dev->dev);
+		goto error;
+	}
+
+	pr_debug("drm_dp_aux_dev: aux [%s] registered as minor %d\n",
+		 aux->name, aux_dev->index);
+	return 0;
+error:
+	return_drm_dp_aux_dev(aux_dev);
+	return res;
+
+}
+
+/**
+ * drm_dp_aux_unregister_devnode() - unregister a devnode for this aux channel
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
+{
+	int minor;
+	struct drm_dp_aux_dev *aux_dev;
+
+	aux_dev = drm_dp_aux_dev_get_by_aux(aux);
+	if (!aux_dev) /* attach must have failed */
+		return 0;
+
+	minor = aux_dev->index;
+	return_drm_dp_aux_dev(aux_dev);
+	device_destroy(drm_dp_aux_dev_class, MKDEV(drm_dev_major, minor));
+
+	pr_debug("drm_dp_aux_dev: aux [%s] unregistered\n", aux->name);
+	return 0;
+}
+
+static int __init drm_dp_aux_dev_init(void)
+{
+	int res;
+
+	printk(KERN_INFO "drm dp aux /dev entries driver\n");
+
+	res = register_chrdev(0, "aux", &auxdev_fops);
+	if (res < 0)
+		goto out;
+	drm_dev_major = res;
+
+	drm_dp_aux_dev_class = class_create(THIS_MODULE, "drm_dp_aux_dev");
+	if (IS_ERR(drm_dp_aux_dev_class)) {
+		res = PTR_ERR(drm_dp_aux_dev_class);
+		goto out_unreg_chrdev;
+	}
+	drm_dp_aux_dev_class->dev_groups = drm_dp_aux_groups;
+
+	return 0;
+
+out_unreg_chrdev:
+	unregister_chrdev(drm_dev_major, "aux");
+out:
+	printk(KERN_ERR "%s: Driver Initialisation failed\n", __FILE__);
+	return res;
+}
+
+static void __exit drm_dp_aux_dev_exit(void)
+{
+	printk(KERN_INFO "drm dp aux /dev entries driver - unloading\n");
+	class_destroy(drm_dp_aux_dev_class);
+	unregister_chrdev(drm_dev_major, "aux");
+}
+
+MODULE_AUTHOR("Rafael Antognolli <rafael.antognolli@intel.com>");
+MODULE_DESCRIPTION("DRM DP AUX /dev entries driver");
+MODULE_LICENSE("GPL");
+
+module_init(drm_dp_aux_dev_init);
+module_exit(drm_dp_aux_dev_exit);
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..f3a2eb0 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -28,6 +28,7 @@
 #include <linux/sched.h>
 #include <linux/i2c.h>
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_dp_aux_dev.h>
 #include <drm/drmP.h>
 
 /**
@@ -768,6 +769,9 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
 	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
 		sizeof(aux->ddc.name));
 
+
+	drm_dp_aux_register_devnode(aux);
+
 	return i2c_add_adapter(&aux->ddc);
 }
 EXPORT_SYMBOL(drm_dp_aux_register);
@@ -778,6 +782,7 @@ EXPORT_SYMBOL(drm_dp_aux_register);
  */
 void drm_dp_aux_unregister(struct drm_dp_aux *aux)
 {
+	drm_dp_aux_unregister_devnode(aux);
 	i2c_del_adapter(&aux->ddc);
 }
 EXPORT_SYMBOL(drm_dp_aux_unregister);
diff --git a/include/drm/drm_dp_aux_dev.h b/include/drm/drm_dp_aux_dev.h
new file mode 100644
index 0000000..77534e8
--- /dev/null
+++ b/include/drm/drm_dp_aux_dev.h
@@ -0,0 +1,50 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Rafael Antognolli <rafael.antognolli@intel.com>
+ *
+ */
+
+#ifndef _UAPI_LINUX_DRM_DP_AUX_DEV_
+#define _UAPI_LINUX_DRM_DP_AUX_DEV_
+
+#ifdef CONFIG_DRM_DP_AUX_CHARDEV
+
+int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
+int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
+
+#else
+
+static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
+{
+	return 0;
+}
+
+static inline int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
+{
+	return 0;
+}
+
+#endif
+
+#endif
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Ville Syrjälä]

On Mon, Sep 28, 2015 at 04:45:36PM -0700, Rafael Antognolli wrote:
> This module is heavily based on i2c-dev. Once loaded, it provides one
> dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
> 
> It's possible to know which connector owns this aux channel by looking
> at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
> the connector device pointer was correctly set in the aux helper struct.
> 
> Two main operations are provided on the registers read and write. The
> address of the register to be read or written is given using lseek. The
> seek position is updated upon read or write.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> ---
>  drivers/gpu/drm/Kconfig          |   8 +
>  drivers/gpu/drm/Makefile         |   1 +
>  drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_dp_helper.c  |   5 +
>  include/drm/drm_dp_aux_dev.h     |  50 ++++++
>  5 files changed, 421 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
>  create mode 100644 include/drm/drm_dp_aux_dev.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 1a0a8df..64fa0f4 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -25,6 +25,14 @@ config DRM_MIPI_DSI
>  	bool
>  	depends on DRM
>  
> +config DRM_DP_AUX_CHARDEV
> +	bool "DRM DP AUX Interface"
> +	depends on DRM
> +	help
> +	  Choose this option to enable a /dev/drm_dp_auxN node that allows to
> +	  read and write values to arbitrary DPCD registers on the DP aux
> +	  channel.
> +
>  config DRM_KMS_HELPER
>  	tristate
>  	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 45e7719..e5ae7f0 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>  
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> new file mode 100644
> index 0000000..e337081
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -0,0 +1,357 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Rafael Antognolli <rafael.antognolli@intel.com>
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <asm/uaccess.h>
> +#include <drm/drm_dp_helper.h>
> +#include <drm/drm_crtc.h>
> +
> +struct drm_dp_aux_dev {
> +	struct list_head list;
> +	unsigned index;
> +	struct drm_dp_aux *aux;
> +	struct device *dev;
> +};
> +
> +#define DRM_AUX_MINORS	256
> +static int aux_max_offset = 1 << 20;

People seem to prefer defines for such things.

> +static int drm_dp_aux_dev_count = 0;
> +static LIST_HEAD(drm_dp_aux_dev_list);
> +static DEFINE_SPINLOCK(drm_dp_aux_dev_list_lock);
> +
> +static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> +{
> +	struct drm_dp_aux_dev *aux_dev;
> +
> +	spin_lock(&drm_dp_aux_dev_list_lock);
> +	list_for_each_entry(aux_dev, &drm_dp_aux_dev_list, list) {
> +		if (aux_dev->index == index)
> +			goto found;
> +	}
> +
> +	aux_dev = NULL;
> +found:
> +	spin_unlock(&drm_dp_aux_dev_list_lock);
> +	return aux_dev;
> +}
> +
> +static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
> +{
> +	struct drm_dp_aux_dev *aux_dev;
> +
> +	spin_lock(&drm_dp_aux_dev_list_lock);
> +	list_for_each_entry(aux_dev, &drm_dp_aux_dev_list, list) {
> +		if (aux_dev->aux == aux)
> +			goto found;
> +	}
> +
> +	aux_dev = NULL;
> +found:
> +	spin_unlock(&drm_dp_aux_dev_list_lock);
> +	return aux_dev;
> +}
> +
> +static struct drm_dp_aux_dev *get_free_drm_dp_aux_dev(struct drm_dp_aux *aux)
> +{
> +	struct drm_dp_aux_dev *aux_dev;
> +	int index;
> +
> +	spin_lock(&drm_dp_aux_dev_list_lock);
> +	index = drm_dp_aux_dev_count;
> +	spin_unlock(&drm_dp_aux_dev_list_lock);
> +	if (index >= DRM_AUX_MINORS) {
> +		printk(KERN_ERR "drm_dp_aux_dev: Out of device minors (%d)\n",
> +		       index);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
> +	if (!aux_dev)
> +		return ERR_PTR(-ENOMEM);
> +	aux_dev->aux = aux;
> +	aux_dev->index = index;
> +
> +	spin_lock(&drm_dp_aux_dev_list_lock);
> +	drm_dp_aux_dev_count++;

That's racy. Someone else could have snuck in and pushed
drm_dp_aux_dev_count over the limit already.

Maybe we want to use an idr or something to allocate the
index?

> +	list_add_tail(&aux_dev->list, &drm_dp_aux_dev_list);
> +	spin_unlock(&drm_dp_aux_dev_list_lock);
> +	return aux_dev;
> +}
> +
> +static void return_drm_dp_aux_dev(struct drm_dp_aux_dev *aux_dev)
> +{
> +	spin_lock(&drm_dp_aux_dev_list_lock);
> +	list_del(&aux_dev->list);
> +	spin_unlock(&drm_dp_aux_dev_list_lock);
> +	kfree(aux_dev);
> +}

I'm not a fan of the get_free & return names. Maybe just alloc/free?

> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct drm_dp_aux_dev *aux_dev =
> +		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
> +
> +	if (!aux_dev)
> +		return -ENODEV;
> +	return sprintf(buf, "%s\n", aux_dev->aux->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t connector_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct drm_dp_aux_dev *aux_dev =
> +		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
> +	struct drm_dp_aux *aux;
> +	struct device *conn_dev;
> +	struct drm_connector *connector = NULL;
> +
> +	if (!aux_dev)
> +		return -ENODEV;
> +	aux = aux_dev->aux;
> +	conn_dev = aux->connector;
> +	if (!conn_dev)
> +		return sprintf(buf, "unknown\n");
> +
> +	connector = dev_get_drvdata(aux->connector);
> +
> +	return sprintf(buf, "%s\n", connector->name);
> +}
> +static DEVICE_ATTR_RO(connector);
> +
> +static struct attribute *drm_dp_aux_attrs[] = {
> +	&dev_attr_name.attr,
> +	&dev_attr_connector.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(drm_dp_aux);
> +
> +static int auxdev_open(struct inode *inode, struct file *file)
> +{
> +	unsigned int minor = iminor(inode);
> +	struct drm_dp_aux_dev *aux_dev;
> +
> +	aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> +	if (!aux_dev)
> +		return -ENODEV;
> +
> +	file->private_data = aux_dev;
> +	return 0;
> +}
> +
> +static loff_t auxdev_llseek(struct file *file, loff_t offset, int whence)
> +{
> +	return fixed_size_llseek(file, offset, whence, aux_max_offset);
> +}
> +
> +static ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> +			   loff_t *offset)
> +{
> +	ssize_t bytes_pending, num_bytes_processed = 0;
> +	struct drm_dp_aux_dev *aux_dev = file->private_data;
> +
> +	bytes_pending = aux_max_offset - *offset;
> +	bytes_pending = count < bytes_pending ? count : bytes_pending;

I would write it as
bytes_pending = min(count, aux_max_offset - *offset);

> +	if (bytes_pending < 0)
> +		return -EFAULT;

Can this actually happen somehow?

> +
> +	if (!access_ok(VERIFY_WRITE, buf, bytes_pending))
> +		return -EFAULT;
> +
> +	while (bytes_pending > 0) {
> +		uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
> +		ssize_t todo = sizeof(localbuf);
> +		ssize_t res;
> +		todo = bytes_pending < todo ? bytes_pending : todo;

Would write it as
todo = min(bytes_pending, sizeof(localbuf));

> +
> +		res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, todo);
> +		if (res <= 0) {
> +			return num_bytes_processed ? num_bytes_processed : res;
> +		}

Could drop the extra {}

> +		if (__copy_to_user(buf + num_bytes_processed, localbuf, res)) {
> +			return num_bytes_processed ?
> +				num_bytes_processed : -EFAULT;
> +		}

ditto

> +		bytes_pending -= res;
> +		*offset += res;
> +		num_bytes_processed += res;
> +	}
> +
> +	return num_bytes_processed;
> +}
> +
> +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> +			    size_t count, loff_t *offset)
> +{
> +	ssize_t bytes_pending, num_bytes_processed = 0;
> +	struct drm_dp_aux_dev *aux_dev = file->private_data;
> +
> +	bytes_pending = aux_max_offset - *offset;
> +	bytes_pending = count < bytes_pending ? count : bytes_pending;
> +	if (bytes_pending < 0)
> +		return -EFAULT;
> +
> +	if (!access_ok(VERIFY_READ, buf, bytes_pending))
> +		return -EFAULT;
> +
> +	while (bytes_pending > 0) {
> +		uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
> +		ssize_t todo = sizeof(localbuf);
> +		ssize_t res;
> +		todo = bytes_pending < todo ? bytes_pending : todo;
> +		if (__copy_from_user(localbuf,
> +				     buf + num_bytes_processed, todo)) {
> +			return num_bytes_processed ?
> +				num_bytes_processed : -EFAULT;
> +		}
> +
> +		res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, todo);
> +		if (res <= 0) {
> +			return num_bytes_processed ? num_bytes_processed : res;
> +		}

same comments for write as for read.

> +		bytes_pending -= res;
> +		*offset += res;
> +		num_bytes_processed += res;
> +	}
> +
> +	return num_bytes_processed;
> +}
> +
> +static const struct file_operations auxdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= auxdev_llseek,
> +	.read		= auxdev_read,
> +	.write		= auxdev_write,
> +	.open		= auxdev_open,
> +};
> +
> +static struct class *drm_dp_aux_dev_class;
> +static int drm_dev_major = -1;
> +
> +#define to_auxdev(d) container_of(d, struct drm_dp_aux_dev, aux)
> +
> +/**
> + * drm_dp_aux_register_devnode() - register a devnode for this aux channel
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +{
> +	struct drm_dp_aux_dev *aux_dev;
> +	int res;
> +
> +	aux_dev = get_free_drm_dp_aux_dev(aux);
> +	if (IS_ERR(aux_dev))
> +		return PTR_ERR(aux_dev);
> +
> +	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
> +				     MKDEV(drm_dev_major, aux_dev->index), NULL,
> +				     "drm_dp_aux%d", aux_dev->index);
> +	if (IS_ERR(aux_dev->dev)) {
> +		res = PTR_ERR(aux_dev->dev);
> +		goto error;
> +	}
> +
> +	pr_debug("drm_dp_aux_dev: aux [%s] registered as minor %d\n",
> +		 aux->name, aux_dev->index);
> +	return 0;
> +error:
> +	return_drm_dp_aux_dev(aux_dev);
> +	return res;
> +
> +}
> +
> +/**
> + * drm_dp_aux_unregister_devnode() - unregister a devnode for this aux channel
> + * @aux: DisplayPort AUX channel
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
> +{
> +	int minor;
> +	struct drm_dp_aux_dev *aux_dev;
> +
> +	aux_dev = drm_dp_aux_dev_get_by_aux(aux);
> +	if (!aux_dev) /* attach must have failed */
> +		return 0;
> +
> +	minor = aux_dev->index;
> +	return_drm_dp_aux_dev(aux_dev);
> +	device_destroy(drm_dp_aux_dev_class, MKDEV(drm_dev_major, minor));

device_destroy before you free the auxdev seem safer.

What happens if someone has the device still open? Do we need to kref the
aux_dev and add some kind of dead flag or something?

> +
> +	pr_debug("drm_dp_aux_dev: aux [%s] unregistered\n", aux->name);
> +	return 0;
> +}
> +
> +static int __init drm_dp_aux_dev_init(void)
> +{
> +	int res;
> +
> +	printk(KERN_INFO "drm dp aux /dev entries driver\n");

I don't see much point in this printk.

> +
> +	res = register_chrdev(0, "aux", &auxdev_fops);
> +	if (res < 0)
> +		goto out;
> +	drm_dev_major = res;
> +
> +	drm_dp_aux_dev_class = class_create(THIS_MODULE, "drm_dp_aux_dev");
> +	if (IS_ERR(drm_dp_aux_dev_class)) {
> +		res = PTR_ERR(drm_dp_aux_dev_class);
> +		goto out_unreg_chrdev;
> +	}
> +	drm_dp_aux_dev_class->dev_groups = drm_dp_aux_groups;
> +
> +	return 0;
> +
> +out_unreg_chrdev:
> +	unregister_chrdev(drm_dev_major, "aux");
> +out:
> +	printk(KERN_ERR "%s: Driver Initialisation failed\n", __FILE__);
> +	return res;
> +}
> +
> +static void __exit drm_dp_aux_dev_exit(void)
> +{
> +	printk(KERN_INFO "drm dp aux /dev entries driver - unloading\n");

ditto

> +	class_destroy(drm_dp_aux_dev_class);
> +	unregister_chrdev(drm_dev_major, "aux");
> +}
> +
> +MODULE_AUTHOR("Rafael Antognolli <rafael.antognolli@intel.com>");
> +MODULE_DESCRIPTION("DRM DP AUX /dev entries driver");
> +MODULE_LICENSE("GPL");

Doesn't match your license boilerplate.

> +
> +module_init(drm_dp_aux_dev_init);
> +module_exit(drm_dp_aux_dev_exit);
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9535c5b..f3a2eb0 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -28,6 +28,7 @@
>  #include <linux/sched.h>
>  #include <linux/i2c.h>
>  #include <drm/drm_dp_helper.h>
> +#include <drm/drm_dp_aux_dev.h>
>  #include <drm/drmP.h>
>  
>  /**
> @@ -768,6 +769,9 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
>  		sizeof(aux->ddc.name));
>  
> +
> +	drm_dp_aux_register_devnode(aux);
> +
>  	return i2c_add_adapter(&aux->ddc);
>  }
>  EXPORT_SYMBOL(drm_dp_aux_register);
> @@ -778,6 +782,7 @@ EXPORT_SYMBOL(drm_dp_aux_register);
>   */
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
>  {
> +	drm_dp_aux_unregister_devnode(aux);
>  	i2c_del_adapter(&aux->ddc);
>  }
>  EXPORT_SYMBOL(drm_dp_aux_unregister);
> diff --git a/include/drm/drm_dp_aux_dev.h b/include/drm/drm_dp_aux_dev.h
> new file mode 100644
> index 0000000..77534e8
> --- /dev/null
> +++ b/include/drm/drm_dp_aux_dev.h
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Rafael Antognolli <rafael.antognolli@intel.com>
> + *
> + */
> +
> +#ifndef _UAPI_LINUX_DRM_DP_AUX_DEV_
> +#define _UAPI_LINUX_DRM_DP_AUX_DEV_

Not uapi.

#ifndef DRM_DP_AUX_DEV_H
#define ...
would seem line enough.

> +
> +#ifdef CONFIG_DRM_DP_AUX_CHARDEV
> +
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> +int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
> +
> +#else
> +
> +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +{
> +	return 0;
> +}
> +
> +static inline int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
> +{
> +	return 0;
> +}
> +
> +#endif
> +
> +#endif
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

[Rafael Antognolli]

Thanks Ville for the review, I'm addressing all the issues but have some
questions on the ones mentioned below:

On Tue, Sep 29, 2015 at 05:09:23PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 28, 2015 at 04:45:36PM -0700, Rafael Antognolli wrote:
> > This module is heavily based on i2c-dev. Once loaded, it provides one
> > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
> > 
> > It's possible to know which connector owns this aux channel by looking
> > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
> > the connector device pointer was correctly set in the aux helper struct.
> > 
> > Two main operations are provided on the registers read and write. The
> > address of the register to be read or written is given using lseek. The
> > seek position is updated upon read or write.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com>
> > ---
> >  drivers/gpu/drm/Kconfig          |   8 +
> >  drivers/gpu/drm/Makefile         |   1 +
> >  drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_dp_helper.c  |   5 +
> >  include/drm/drm_dp_aux_dev.h     |  50 ++++++
> >  5 files changed, 421 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
> >  create mode 100644 include/drm/drm_dp_aux_dev.h
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 1a0a8df..64fa0f4 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI
> >  	bool
> >  	depends on DRM
> >  
> > +config DRM_DP_AUX_CHARDEV
> > +	bool "DRM DP AUX Interface"
> > +	depends on DRM
> > +	help
> > +	  Choose this option to enable a /dev/drm_dp_auxN node that allows to
> > +	  read and write values to arbitrary DPCD registers on the DP aux
> > +	  channel.
> > +
> >  config DRM_KMS_HELPER
> >  	tristate
> >  	depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 45e7719..e5ae7f0 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> >  drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >  
> >  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >  
> > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> > new file mode 100644
> > index 0000000..e337081
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > @@ -0,0 +1,357 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *    Rafael Antognolli <rafael.antognolli@intel.com>
> > + *
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <asm/uaccess.h>
> > +#include <drm/drm_dp_helper.h>
> > +#include <drm/drm_crtc.h>
> > +
> > +struct drm_dp_aux_dev {
> > +	struct list_head list;
> > +	unsigned index;
> > +	struct drm_dp_aux *aux;
> > +	struct device *dev;
> > +};
> > +
> > +#define DRM_AUX_MINORS	256
> > +static int aux_max_offset = 1 << 20;
> 
> People seem to prefer defines for such things.
> 
> > +static int drm_dp_aux_dev_count = 0;
> > +static LIST_HEAD(drm_dp_aux_dev_list);
> > +static DEFINE_SPINLOCK(drm_dp_aux_dev_list_lock);
> > +
> > +static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > +{
> > +	struct drm_dp_aux_dev *aux_dev;
> > +
> > +	spin_lock(&drm_dp_aux_dev_list_lock);
> > +	list_for_each_entry(aux_dev, &drm_dp_aux_dev_list, list) {
> > +		if (aux_dev->index == index)
> > +			goto found;
> > +	}
> > +
> > +	aux_dev = NULL;
> > +found:
> > +	spin_unlock(&drm_dp_aux_dev_list_lock);
> > +	return aux_dev;
> > +}
> > +
> > +static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
> > +{
> > +	struct drm_dp_aux_dev *aux_dev;
> > +
> > +	spin_lock(&drm_dp_aux_dev_list_lock);
> > +	list_for_each_entry(aux_dev, &drm_dp_aux_dev_list, list) {
> > +		if (aux_dev->aux == aux)
> > +			goto found;
> > +	}
> > +
> > +	aux_dev = NULL;
> > +found:
> > +	spin_unlock(&drm_dp_aux_dev_list_lock);
> > +	return aux_dev;
> > +}
> > +
> > +static struct drm_dp_aux_dev *get_free_drm_dp_aux_dev(struct drm_dp_aux *aux)
> > +{
> > +	struct drm_dp_aux_dev *aux_dev;
> > +	int index;
> > +
> > +	spin_lock(&drm_dp_aux_dev_list_lock);
> > +	index = drm_dp_aux_dev_count;
> > +	spin_unlock(&drm_dp_aux_dev_list_lock);
> > +	if (index >= DRM_AUX_MINORS) {
> > +		printk(KERN_ERR "drm_dp_aux_dev: Out of device minors (%d)\n",
> > +		       index);
> > +		return ERR_PTR(-ENODEV);
> > +	}
> > +
> > +	aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
> > +	if (!aux_dev)
> > +		return ERR_PTR(-ENOMEM);
> > +	aux_dev->aux = aux;
> > +	aux_dev->index = index;
> > +
> > +	spin_lock(&drm_dp_aux_dev_list_lock);
> > +	drm_dp_aux_dev_count++;
> 
> That's racy. Someone else could have snuck in and pushed
> drm_dp_aux_dev_count over the limit already.
> 
> Maybe we want to use an idr or something to allocate the
> index?
> 
> > +	list_add_tail(&aux_dev->list, &drm_dp_aux_dev_list);
> > +	spin_unlock(&drm_dp_aux_dev_list_lock);
> > +	return aux_dev;
> > +}
> > +
> > +static void return_drm_dp_aux_dev(struct drm_dp_aux_dev *aux_dev)
> > +{
> > +	spin_lock(&drm_dp_aux_dev_list_lock);
> > +	list_del(&aux_dev->list);
> > +	spin_unlock(&drm_dp_aux_dev_list_lock);
> > +	kfree(aux_dev);
> > +}
> 
> I'm not a fan of the get_free & return names. Maybe just alloc/free?
> 
> > +
> > +static ssize_t name_show(struct device *dev,
> > +			 struct device_attribute *attr, char *buf)
> > +{
> > +	struct drm_dp_aux_dev *aux_dev =
> > +		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
> > +
> > +	if (!aux_dev)
> > +		return -ENODEV;
> > +	return sprintf(buf, "%s\n", aux_dev->aux->name);
> > +}
> > +static DEVICE_ATTR_RO(name);
> > +
> > +static ssize_t connector_show(struct device *dev,
> > +			      struct device_attribute *attr, char *buf)
> > +{
> > +	struct drm_dp_aux_dev *aux_dev =
> > +		drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
> > +	struct drm_dp_aux *aux;
> > +	struct device *conn_dev;
> > +	struct drm_connector *connector = NULL;
> > +
> > +	if (!aux_dev)
> > +		return -ENODEV;
> > +	aux = aux_dev->aux;
> > +	conn_dev = aux->connector;
> > +	if (!conn_dev)
> > +		return sprintf(buf, "unknown\n");
> > +
> > +	connector = dev_get_drvdata(aux->connector);
> > +
> > +	return sprintf(buf, "%s\n", connector->name);
> > +}
> > +static DEVICE_ATTR_RO(connector);
> > +
> > +static struct attribute *drm_dp_aux_attrs[] = {
> > +	&dev_attr_name.attr,
> > +	&dev_attr_connector.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(drm_dp_aux);
> > +
> > +static int auxdev_open(struct inode *inode, struct file *file)
> > +{
> > +	unsigned int minor = iminor(inode);
> > +	struct drm_dp_aux_dev *aux_dev;
> > +
> > +	aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > +	if (!aux_dev)
> > +		return -ENODEV;
> > +
> > +	file->private_data = aux_dev;
> > +	return 0;
> > +}
> > +
> > +static loff_t auxdev_llseek(struct file *file, loff_t offset, int whence)
> > +{
> > +	return fixed_size_llseek(file, offset, whence, aux_max_offset);
> > +}
> > +
> > +static ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,
> > +			   loff_t *offset)
> > +{
> > +	ssize_t bytes_pending, num_bytes_processed = 0;
> > +	struct drm_dp_aux_dev *aux_dev = file->private_data;
> > +
> > +	bytes_pending = aux_max_offset - *offset;
> > +	bytes_pending = count < bytes_pending ? count : bytes_pending;
> 
> I would write it as
> bytes_pending = min(count, aux_max_offset - *offset);
> 
> > +	if (bytes_pending < 0)
> > +		return -EFAULT;
> 
> Can this actually happen somehow?

I believe the "count" argument could be negative, not sure if the kernel
does any checks on that. Still have to look that up. But if that's the
case, I could just check for count < 0 earlier on, right?

> > +
> > +	if (!access_ok(VERIFY_WRITE, buf, bytes_pending))
> > +		return -EFAULT;
> > +
> > +	while (bytes_pending > 0) {
> > +		uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
> > +		ssize_t todo = sizeof(localbuf);
> > +		ssize_t res;
> > +		todo = bytes_pending < todo ? bytes_pending : todo;
> 
> Would write it as
> todo = min(bytes_pending, sizeof(localbuf));
> 
> > +
> > +		res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, todo);
> > +		if (res <= 0) {
> > +			return num_bytes_processed ? num_bytes_processed : res;
> > +		}
> 
> Could drop the extra {}
> 
> > +		if (__copy_to_user(buf + num_bytes_processed, localbuf, res)) {
> > +			return num_bytes_processed ?
> > +				num_bytes_processed : -EFAULT;
> > +		}
> 
> ditto
> 
> > +		bytes_pending -= res;
> > +		*offset += res;
> > +		num_bytes_processed += res;
> > +	}
> > +
> > +	return num_bytes_processed;
> > +}
> > +
> > +static ssize_t auxdev_write(struct file *file, const char __user *buf,
> > +			    size_t count, loff_t *offset)
> > +{
> > +	ssize_t bytes_pending, num_bytes_processed = 0;
> > +	struct drm_dp_aux_dev *aux_dev = file->private_data;
> > +
> > +	bytes_pending = aux_max_offset - *offset;
> > +	bytes_pending = count < bytes_pending ? count : bytes_pending;
> > +	if (bytes_pending < 0)
> > +		return -EFAULT;
> > +
> > +	if (!access_ok(VERIFY_READ, buf, bytes_pending))
> > +		return -EFAULT;
> > +
> > +	while (bytes_pending > 0) {
> > +		uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
> > +		ssize_t todo = sizeof(localbuf);
> > +		ssize_t res;
> > +		todo = bytes_pending < todo ? bytes_pending : todo;
> > +		if (__copy_from_user(localbuf,
> > +				     buf + num_bytes_processed, todo)) {
> > +			return num_bytes_processed ?
> > +				num_bytes_processed : -EFAULT;
> > +		}
> > +
> > +		res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, todo);
> > +		if (res <= 0) {
> > +			return num_bytes_processed ? num_bytes_processed : res;
> > +		}
> 
> same comments for write as for read.
> 
> > +		bytes_pending -= res;
> > +		*offset += res;
> > +		num_bytes_processed += res;
> > +	}
> > +
> > +	return num_bytes_processed;
> > +}
> > +
> > +static const struct file_operations auxdev_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.llseek		= auxdev_llseek,
> > +	.read		= auxdev_read,
> > +	.write		= auxdev_write,
> > +	.open		= auxdev_open,
> > +};
> > +
> > +static struct class *drm_dp_aux_dev_class;
> > +static int drm_dev_major = -1;
> > +
> > +#define to_auxdev(d) container_of(d, struct drm_dp_aux_dev, aux)
> > +
> > +/**
> > + * drm_dp_aux_register_devnode() - register a devnode for this aux channel
> > + * @aux: DisplayPort AUX channel
> > + *
> > + * Returns 0 on success or a negative error code on failure.
> > + */
> > +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> > +{
> > +	struct drm_dp_aux_dev *aux_dev;
> > +	int res;
> > +
> > +	aux_dev = get_free_drm_dp_aux_dev(aux);
> > +	if (IS_ERR(aux_dev))
> > +		return PTR_ERR(aux_dev);
> > +
> > +	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
> > +				     MKDEV(drm_dev_major, aux_dev->index), NULL,
> > +				     "drm_dp_aux%d", aux_dev->index);
> > +	if (IS_ERR(aux_dev->dev)) {
> > +		res = PTR_ERR(aux_dev->dev);
> > +		goto error;
> > +	}
> > +
> > +	pr_debug("drm_dp_aux_dev: aux [%s] registered as minor %d\n",
> > +		 aux->name, aux_dev->index);
> > +	return 0;
> > +error:
> > +	return_drm_dp_aux_dev(aux_dev);
> > +	return res;
> > +
> > +}
> > +
> > +/**
> > + * drm_dp_aux_unregister_devnode() - unregister a devnode for this aux channel
> > + * @aux: DisplayPort AUX channel
> > + *
> > + * Returns 0 on success or a negative error code on failure.
> > + */
> > +int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
> > +{
> > +	int minor;
> > +	struct drm_dp_aux_dev *aux_dev;
> > +
> > +	aux_dev = drm_dp_aux_dev_get_by_aux(aux);
> > +	if (!aux_dev) /* attach must have failed */
> > +		return 0;
> > +
> > +	minor = aux_dev->index;
> > +	return_drm_dp_aux_dev(aux_dev);
> > +	device_destroy(drm_dp_aux_dev_class, MKDEV(drm_dev_major, minor));
> 
> device_destroy before you free the auxdev seem safer.
> 
> What happens if someone has the device still open? Do we need to kref the
> aux_dev and add some kind of dead flag or something?

Yeah, I couldn't find anything that says the device would stay around
while the file is still open, so I guess you are right. So, I would need
to put some locks around the aux_dev usage too, right?

> > +
> > +	pr_debug("drm_dp_aux_dev: aux [%s] unregistered\n", aux->name);
> > +	return 0;
> > +}
> > +
> > +static int __init drm_dp_aux_dev_init(void)
> > +{
> > +	int res;
> > +
> > +	printk(KERN_INFO "drm dp aux /dev entries driver\n");
> 
> I don't see much point in this printk.
> 
> > +
> > +	res = register_chrdev(0, "aux", &auxdev_fops);
> > +	if (res < 0)
> > +		goto out;
> > +	drm_dev_major = res;
> > +
> > +	drm_dp_aux_dev_class = class_create(THIS_MODULE, "drm_dp_aux_dev");
> > +	if (IS_ERR(drm_dp_aux_dev_class)) {
> > +		res = PTR_ERR(drm_dp_aux_dev_class);
> > +		goto out_unreg_chrdev;
> > +	}
> > +	drm_dp_aux_dev_class->dev_groups = drm_dp_aux_groups;
> > +
> > +	return 0;
> > +
> > +out_unreg_chrdev:
> > +	unregister_chrdev(drm_dev_major, "aux");
> > +out:
> > +	printk(KERN_ERR "%s: Driver Initialisation failed\n", __FILE__);
> > +	return res;
> > +}
> > +
> > +static void __exit drm_dp_aux_dev_exit(void)
> > +{
> > +	printk(KERN_INFO "drm dp aux /dev entries driver - unloading\n");
> 
> ditto
> 
> > +	class_destroy(drm_dp_aux_dev_class);
> > +	unregister_chrdev(drm_dev_major, "aux");
> > +}
> > +
> > +MODULE_AUTHOR("Rafael Antognolli <rafael.antognolli@intel.com>");
> > +MODULE_DESCRIPTION("DRM DP AUX /dev entries driver");
> > +MODULE_LICENSE("GPL");
> 
> Doesn't match your license boilerplate.

So, should it be "GPL and additional rights"?

> > +
> > +module_init(drm_dp_aux_dev_init);
> > +module_exit(drm_dp_aux_dev_exit);
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 9535c5b..f3a2eb0 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/i2c.h>
> >  #include <drm/drm_dp_helper.h>
> > +#include <drm/drm_dp_aux_dev.h>
> >  #include <drm/drmP.h>
> >  
> >  /**
> > @@ -768,6 +769,9 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> >  	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> >  		sizeof(aux->ddc.name));
> >  
> > +
> > +	drm_dp_aux_register_devnode(aux);
> > +
> >  	return i2c_add_adapter(&aux->ddc);
> >  }
> >  EXPORT_SYMBOL(drm_dp_aux_register);
> > @@ -778,6 +782,7 @@ EXPORT_SYMBOL(drm_dp_aux_register);
> >   */
> >  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> >  {
> > +	drm_dp_aux_unregister_devnode(aux);
> >  	i2c_del_adapter(&aux->ddc);
> >  }
> >  EXPORT_SYMBOL(drm_dp_aux_unregister);
> > diff --git a/include/drm/drm_dp_aux_dev.h b/include/drm/drm_dp_aux_dev.h
> > new file mode 100644
> > index 0000000..77534e8
> > --- /dev/null
> > +++ b/include/drm/drm_dp_aux_dev.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *    Rafael Antognolli <rafael.antognolli@intel.com>
> > + *
> > + */
> > +
> > +#ifndef _UAPI_LINUX_DRM_DP_AUX_DEV_
> > +#define _UAPI_LINUX_DRM_DP_AUX_DEV_
> 
> Not uapi.
> 
> #ifndef DRM_DP_AUX_DEV_H
> #define ...
> would seem line enough.
> 
> > +
> > +#ifdef CONFIG_DRM_DP_AUX_CHARDEV
> > +
> > +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> > +int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
> > +
> > +#else
> > +
> > +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
> > +{
> > +	return 0;
> > +}
> > +
> > +#endif
> > +
> > +#endif
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

Thanks,
Rafael
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx