[PATCH 0/2] Add HDMI audio support to the LT8912B bridge
From: Rafael Beims This patch series adds HDMI audio support to the Lontium LT8912B bridge driver using the I2S input. The audio output was tested using a Verdin iMX8MM + DSI to HDMI adapter connected to different monitors. Rafael Beims (2): dt-bindings: display: bridge: lt8912b: Add I2S audio input port drm/bridge: lt8912b: Add support for audio .../display/bridge/lontium,lt8912b.yaml | 8 ++ drivers/gpu/drm/bridge/Kconfig| 1 + drivers/gpu/drm/bridge/lontium-lt8912b.c | 107 +- 3 files changed, 115 insertions(+), 1 deletion(-) -- 2.47.2
[PATCH 2/2] drm/bridge: lt8912b: Add support for audio
From: Rafael Beims Add support for HDMI codec with audio coming from the I2S input. Support 48kHz and 96kHz sample rate, with 16 bits word size. Co-developed-by: João Paulo Gonçalves Signed-off-by: João Paulo Gonçalves Signed-off-by: Rafael Beims --- drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/lontium-lt8912b.c | 107 ++- 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 6b4664d91faa..489ce1041203 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -117,6 +117,7 @@ config DRM_ITE_IT6505 config DRM_LONTIUM_LT8912B tristate "Lontium LT8912B DSI/HDMI bridge" + select SND_SOC_HDMI_CODEC if SND_SOC depends on OF select DRM_PANEL_BRIDGE select DRM_KMS_HELPER diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c index 52da204f5740..2100b41e5f61 100644 --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include @@ -16,6 +18,8 @@ #include #include +#include + #include #define I2C_MAIN 0 @@ -24,7 +28,10 @@ #define I2C_CEC_DSI 1 #define I2C_ADDR_CEC_DSI 0x49 -#define I2C_MAX_IDX 2 +#define I2C_AUDIO 2 +#define I2C_ADDR_AUDIO 0x4a + +#define I2C_MAX_IDX 3 struct lt8912 { struct device *dev; @@ -38,6 +45,7 @@ struct lt8912 { struct drm_bridge *hdmi_port; struct mipi_dsi_device *dsi; + struct platform_device *audio_pdev; struct gpio_desc *gp_reset; @@ -226,6 +234,7 @@ static int lt8912_init_i2c(struct lt8912 *lt, struct i2c_client *client) struct i2c_board_info info[] = { { I2C_BOARD_INFO("lt8912p0", I2C_ADDR_MAIN), }, { I2C_BOARD_INFO("lt8912p1", I2C_ADDR_CEC_DSI), }, + { I2C_BOARD_INFO("lt8912p2", I2C_ADDR_AUDIO), }, }; if (!lt) @@ -754,6 +763,97 @@ static int lt8912_put_dt(struct lt8912 *lt) return 0; } +static int lt8912_hdmi_hw_params(struct device *dev, void *data, +struct hdmi_codec_daifmt *fmt, +struct hdmi_codec_params *hparms) +{ + struct lt8912 *lt = data; + unsigned int audio_params = 0x08; /* 16 bit word size */ + + if (hparms->sample_width != 16) + return -EINVAL; + + if (hparms->sample_rate == 48000) + audio_params |= 0x20; + else if (hparms->sample_rate == 96000) + audio_params |= 0xa0; + else + return -EINVAL; + + regmap_write(lt->regmap[I2C_AUDIO], 0x0f, audio_params); + regmap_write(lt->regmap[I2C_AUDIO], 0x35, 0x00); + regmap_write(lt->regmap[I2C_AUDIO], 0x36, 0x18); + regmap_write(lt->regmap[I2C_AUDIO], 0x37, 0x00); + + return 0; +} + +static int lt8912_audio_startup(struct device *dev, void *data) +{ + struct lt8912 *lt = data; + + regmap_write(lt->regmap[I2C_AUDIO], 0x34, 0xe2); + regmap_write(lt->regmap[I2C_AUDIO], 0x3c, 0x60); + regmap_write(lt->regmap[I2C_AUDIO], 0x07, 0xf0); + regmap_write(lt->regmap[I2C_AUDIO], 0x06, 0x08); + + return 0; +} + +static void lt8912_audio_shutdown(struct device *dev, void *data) +{ + struct lt8912 *lt = data; + + regmap_write(lt->regmap[I2C_AUDIO], 0x06, 0x00); + regmap_write(lt->regmap[I2C_AUDIO], 0x07, 0x00); + regmap_write(lt->regmap[I2C_AUDIO], 0x34, 0x52); + regmap_write(lt->regmap[I2C_AUDIO], 0x3c, 0x40); +} + +static int lt8912_hdmi_i2s_get_dai_id(struct snd_soc_component *component, + struct device_node *endpoint) +{ + struct of_endpoint of_ep; + int ret; + + ret = of_graph_parse_endpoint(endpoint, &of_ep); + if (ret < 0) + return ret; + + if (of_ep.port != 2) + return -EINVAL; + + return 0; +} + +static const struct hdmi_codec_ops lt8912_codec_ops = { + .hw_params = lt8912_hdmi_hw_params, + .audio_shutdown = lt8912_audio_shutdown, + .audio_startup = lt8912_audio_startup, + .get_dai_id = lt8912_hdmi_i2s_get_dai_id, +}; + +static int lt8912_audio_init(struct device *dev, struct lt8912 *lt) +{ + struct hdmi_codec_pdata codec_data = { + .ops = <8912_codec_ops, + .max_i2s_channels = 2, + .i2s = 1, + .data = lt, + }; + + lt->audio_pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME, + PLATFORM_DEVID_AUTO, + &codec_data, sizeof(codec_data)); + + return PTR_ERR_OR_ZERO(lt->
[PATCH 1/2] dt-bindings: display: bridge: lt8912b: Add I2S audio input port
From: Rafael Beims Add the I2S audio input port for audio over HDMI support. Signed-off-by: Rafael Beims --- .../bindings/display/bridge/lontium,lt8912b.yaml | 8 1 file changed, 8 insertions(+) diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml index 2cef25215798..ce555f52beb8 100644 --- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml @@ -51,6 +51,11 @@ properties: HDMI port, should be connected to a node compatible with the hdmi-connector binding. + port@2: +$ref: /schemas/graph.yaml#/properties/port +description: + I2S Audio input port + required: - port@0 - port@1 @@ -76,6 +81,9 @@ properties: vdd-supply: description: A 1.8V supply that powers the digital part. + "#sound-dai-cells": +const: 0 + required: - compatible - reg -- 2.47.2
[PATCH] drm/amdgpu: Fix memory leak in hpd_rx_irq_create_workqueue()
If construction of the array of work queues to handle hpd_rx_irq offload work fails, we need to unwind. Destroy all the created workqueues and the allocated memory for the hpd_rx_irq_offload_work_queue struct array. Fixes: 8e794421bc98 ("drm/amd/display: Fork thread to offload work of hpd_rx_irq") Signed-off-by: Rafael Mendonca --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 5140d9c2bf3b..6a2e455c5466 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1295,13 +1295,21 @@ static struct hpd_rx_irq_offload_work_queue *hpd_rx_irq_create_workqueue(struct if (hpd_rx_offload_wq[i].wq == NULL) { DRM_ERROR("create amdgpu_dm_hpd_rx_offload_wq fail!"); - return NULL; + goto out_err; } spin_lock_init(&hpd_rx_offload_wq[i].offload_lock); } return hpd_rx_offload_wq; + +out_err: + for (i = 0; i < max_caps; i++) { + if (hpd_rx_offload_wq[i].wq) + destroy_workqueue(hpd_rx_offload_wq[i].wq); + } + kfree(hpd_rx_offload_wq); + return NULL; } struct amdgpu_stutter_quirk { -- 2.34.1
[PATCH] drm/vmwgfx: Fix memory leak in vmw_mksstat_add_ioctl()
If the copy of the description string from userspace fails, then the page for the instance descriptor doesn't get freed before returning -EFAULT, which leads to a memleak. Fixes: 7a7a933edd6c ("drm/vmwgfx: Introduce VMware mks-guest-stats") Signed-off-by: Rafael Mendonca --- drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c index 2aceac7856e2..089046fa21be 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c @@ -1076,6 +1076,7 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data, if (desc_len < 0) { atomic_set(&dev_priv->mksstat_user_pids[slot], 0); + __free_page(page); return -EFAULT; } -- 2.34.1
[PATCH] drm/amdgpu/powerplay/psm: Fix memory leak in power state init
Commit 902bc65de0b3 ("drm/amdgpu/powerplay/psm: return an error in power state init") made the power state init function return early in case of failure to get an entry from the powerplay table, but it missed to clean up the allocated memory for the current power state before returning. Fixes: 902bc65de0b3 ("drm/amdgpu/powerplay/psm: return an error in power state init") Signed-off-by: Rafael Mendonca --- drivers/gpu/drm/amd/pm/powerplay/hwmgr/pp_psm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pp_psm.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pp_psm.c index 67d7da0b6fed..1d829402cd2e 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pp_psm.c +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/pp_psm.c @@ -75,8 +75,10 @@ int psm_init_power_state_table(struct pp_hwmgr *hwmgr) for (i = 0; i < table_entries; i++) { result = hwmgr->hwmgr_func->get_pp_table_entry(hwmgr, i, state); if (result) { + kfree(hwmgr->current_ps); kfree(hwmgr->request_ps); kfree(hwmgr->ps); + hwmgr->current_ps = NULL; hwmgr->request_ps = NULL; hwmgr->ps = NULL; return -EINVAL; -- 2.34.1
[PATCH] drm/amdkfd: Fix memory leak in kfd_mem_dmamap_userptr()
If the number of pages from the userptr BO differs from the SG BO then the allocated memory for the SG table doesn't get freed before returning -EINVAL, which may lead to a memory leak in some error paths. Fix this by checking the number of pages before allocating memory for the SG table. Fixes: 264fb4d332f5 ("drm/amdgpu: Add multi-GPU DMA mapping helpers") Signed-off-by: Rafael Mendonca --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 978d3970b5cc..84f44f7e4111 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -510,13 +510,13 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem, struct ttm_tt *ttm = bo->tbo.ttm; int ret; + if (WARN_ON(ttm->num_pages != src_ttm->num_pages)) + return -EINVAL; + ttm->sg = kmalloc(sizeof(*ttm->sg), GFP_KERNEL); if (unlikely(!ttm->sg)) return -ENOMEM; - if (WARN_ON(ttm->num_pages != src_ttm->num_pages)) - return -EINVAL; - /* Same sequence as in amdgpu_ttm_tt_pin_userptr */ ret = sg_alloc_table_from_pages(ttm->sg, src_ttm->pages, ttm->num_pages, 0, -- 2.34.1
[PATCH] drm/amd/display: Remove duplicate code for DCN314 DML calculation
This is an extension of commit fd3bc691fc7b ("drm/amd/display: Remove duplicate code across dcn30 and dcn31"), which removed duplicate code for the function CalculateBytePerPixelAnd256BBlockSizes() across dcn30 and dcn31. At the time the aforementioned commit was introduced, support for DCN 3.1.4 was still not merged. Thus, this deletes duplicate code for CalculateBytePerPixelAnd256BBlockSizes(), that was introduced later in DCN314, in favor of using the respective functionality from 'display_mode_vba_30.h'. Additionally, by doing that, we also fix a duplicate argument issue reported by coccinelle in 'display_rq_dlg_calc_314.c': static bool CalculateBytePerPixelAnd256BBlockSizes(...) { ... } else if (SourcePixelFormat == dm_444_16 || SourcePixelFormat == dm_444_16) { ... } Signed-off-by: Rafael Mendonca --- .../dc/dml/dcn314/display_mode_vba_314.c | 106 +- .../dc/dml/dcn314/display_rq_dlg_calc_314.c | 91 +-- 2 files changed, 6 insertions(+), 191 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c b/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c index 0d12fd079cd6..6e43cd21a7d3 100644 --- a/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c +++ b/drivers/gpu/drm/amd/display/dc/dml/dcn314/display_mode_vba_314.c @@ -32,6 +32,7 @@ #include "../display_mode_lib.h" #include "display_mode_vba_314.h" #include "../dml_inline_defs.h" +#include "dml/dcn30/display_mode_vba_30.h" /* * NOTE: @@ -90,17 +91,6 @@ typedef struct { #define BPP_INVALID 0 #define BPP_BLENDED_PIPE 0x -static bool CalculateBytePerPixelAnd256BBlockSizes( - enum source_format_class SourcePixelFormat, - enum dm_swizzle_mode SurfaceTiling, - unsigned int *BytePerPixelY, - unsigned int *BytePerPixelC, - double *BytePerPixelDETY, - double *BytePerPixelDETC, - unsigned int *BlockHeight256BytesY, - unsigned int *BlockHeight256BytesC, - unsigned int *BlockWidth256BytesY, - unsigned int *BlockWidth256BytesC); static void DisplayPipeConfiguration(struct display_mode_lib *mode_lib); static void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation(struct display_mode_lib *mode_lib); static unsigned int dscceComputeDelay( @@ -2178,7 +2168,7 @@ static void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerforman DTRACE(" return_bus_bw = %f", v->ReturnBW); for (k = 0; k < v->NumberOfActivePlanes; ++k) { - CalculateBytePerPixelAnd256BBlockSizes( + dml30_CalculateBytePerPixelAnd256BBlockSizes( v->SourcePixelFormat[k], v->SurfaceTiling[k], &v->BytePerPixelY[k], @@ -3317,7 +3307,7 @@ static void DisplayPipeConfiguration(struct display_mode_lib *mode_lib) for (k = 0; k < v->NumberOfActivePlanes; ++k) { - CalculateBytePerPixelAnd256BBlockSizes( + dml30_CalculateBytePerPixelAnd256BBlockSizes( v->SourcePixelFormat[k], v->SurfaceTiling[k], &BytePerPixY[k], @@ -3371,94 +3361,6 @@ static void DisplayPipeConfiguration(struct display_mode_lib *mode_lib) &dummysinglestring); } -static bool CalculateBytePerPixelAnd256BBlockSizes( - enum source_format_class SourcePixelFormat, - enum dm_swizzle_mode SurfaceTiling, - unsigned int *BytePerPixelY, - unsigned int *BytePerPixelC, - double *BytePerPixelDETY, - double *BytePerPixelDETC, - unsigned int *BlockHeight256BytesY, - unsigned int *BlockHeight256BytesC, - unsigned int *BlockWidth256BytesY, - unsigned int *BlockWidth256BytesC) -{ - if (SourcePixelFormat == dm_444_64) { - *BytePerPixelDETY = 8; - *BytePerPixelDETC = 0; - *BytePerPixelY = 8; - *BytePerPixelC = 0; - } else if (SourcePixelFormat == dm_444_32 || SourcePixelFormat == dm_rgbe) { - *BytePerPixelDETY = 4; - *BytePerPixelDETC = 0; - *BytePerPixelY = 4; - *BytePerPixelC = 0; - } else if (SourcePixelFormat == dm_444_16) { - *BytePerPixelDETY = 2; - *BytePerPixelDETC = 0; - *BytePerPixelY = 2; - *BytePerPixelC = 0; - } else if (SourcePixelFormat == dm_444_8) { - *BytePerPixelDETY = 1; - *BytePerPixelDETC = 0; - *BytePerPixelY = 1; -
[PATCH] dma-buf/sync_file: Always increment refcount when merging fences.
The refcount of a fence should be increased whenever it is added to a merged fence, since it will later be decreased when the merged fence is destroyed. Failing to do so will cause the original fence to be freed if the merged fence gets freed, but other places still referencing won't know about it. This patch fixes a kernel panic that can be triggered by creating a fence that is expired (or increasing the timeline until it expires), then creating a merged fence out of it, and deleting the merged fence. This will make the original expired fence's refcount go to zero. Signed-off-by: Rafael Antognolli --- Sample code to trigger the mentioned kernel panic (might need to be executed a couple times before it actually breaks everything): static void test_sync_expired_merge(void) { int iterations = 1 << 20; int timeline; int i; int fence_expired, fence_merged; timeline = sw_sync_timeline_create(); sw_sync_timeline_inc(timeline, 100); fence_expired = sw_sync_fence_create(timeline, 1); fence_merged = sw_sync_merge(fence_expired, fence_expired); sw_sync_fence_destroy(fence_merged); for (i = 0; i < iterations; i++) { int fence = sw_sync_merge(fence_expired, fence_expired); igt_assert_f(sw_sync_wait(fence, -1) > 0, "Failure waiting on fence\n"); sw_sync_fence_destroy(fence); } sw_sync_fence_destroy(fence_expired); } drivers/dma-buf/sync_file.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 486d29c..6ce6b8f 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -178,11 +178,8 @@ static struct fence **get_fences(struct sync_file *sync_file, int *num_fences) static void add_fence(struct fence **fences, int *i, struct fence *fence) { fences[*i] = fence; - - if (!fence_is_signaled(fence)) { - fence_get(fence); - (*i)++; - } + fence_get(fence); + (*i)++; } /** -- 2.7.4
[PATCH i-g-t] tests/sw_sync: Add subtest test_sync_expired_merge
This test creates an already expired fence, then creates a merged fence out of that expired one (passed twice to the merge operation), and finally closes the merged fence. It shows that if the refcounts are wrong on the original expired fence, it might get freed while still in use. Usually a kernel panick will follow. Signed-off-by: Rafael Antognolli --- tests/sw_sync.c | 28 1 file changed, 28 insertions(+) diff --git a/tests/sw_sync.c b/tests/sw_sync.c index 4336659..31cde50 100644 --- a/tests/sw_sync.c +++ b/tests/sw_sync.c @@ -582,6 +582,31 @@ static void test_sync_multi_producer_single_consumer(void) pthread_join(threads[i], NULL); } +static void test_sync_expired_merge(void) +{ + int iterations = 1 << 20; + int timeline; + int i; + int fence_expired, fence_merged; + + timeline = sw_sync_timeline_create(); + + sw_sync_timeline_inc(timeline, 100); + fence_expired = sw_sync_fence_create(timeline, 1); + fence_merged = sw_sync_merge(fence_expired, fence_expired); + sw_sync_fence_destroy(fence_merged); + + for (i = 0; i < iterations; i++) { + int fence = sw_sync_merge(fence_expired, fence_expired); + + igt_assert_f(sw_sync_wait(fence, -1) > 0, +"Failure waiting on fence\n"); + sw_sync_fence_destroy(fence); + } + + sw_sync_fence_destroy(fence_expired); +} + static void test_sync_random_merge(void) { int i, size, ret; @@ -687,6 +712,9 @@ igt_main igt_subtest("sync_multi_producer_single_consumer") test_sync_multi_producer_single_consumer(); + igt_subtest("sync_expired_merge") + test_sync_expired_merge(); + igt_subtest("sync_random_merge") test_sync_random_merge(); } -- 2.7.4
[PATCH] dma-buf/sync_file: Always increment refcount when merging fences.
On Wed, Sep 14, 2016 at 11:04:01AM -0300, Gustavo Padovan wrote: > 2016-09-14 Chris Wilson : > > > On Tue, Sep 13, 2016 at 04:24:27PM -0700, Rafael Antognolli wrote: > > > The refcount of a fence should be increased whenever it is added to a > > > merged > > > fence, since it will later be decreased when the merged fence is > > > destroyed. > > > Failing to do so will cause the original fence to be freed if the merged > > > fence > > > gets freed, but other places still referencing won't know about it. > > > > > > This patch fixes a kernel panic that can be triggered by creating a fence > > > that > > > is expired (or increasing the timeline until it expires), then creating a > > > merged fence out of it, and deleting the merged fence. This will make the > > > original expired fence's refcount go to zero. > > > > > > Signed-off-by: Rafael Antognolli > > > --- > > > > > > Sample code to trigger the mentioned kernel panic (might need to be > > > executed a > > > couple times before it actually breaks everything): > > > > > > static void test_sync_expired_merge(void) > > > { > > >int iterations = 1 << 20; > > >int timeline; > > >int i; > > >int fence_expired, fence_merged; > > > > > >timeline = sw_sync_timeline_create(); > > > > > >sw_sync_timeline_inc(timeline, 100); > > >fence_expired = sw_sync_fence_create(timeline, 1); > > >fence_merged = sw_sync_merge(fence_expired, fence_expired); > > >sw_sync_fence_destroy(fence_merged); > > > > > >for (i = 0; i < iterations; i++) { > > >int fence = sw_sync_merge(fence_expired, fence_expired); > > > > > >igt_assert_f(sw_sync_wait(fence, -1) > 0, > > > "Failure waiting on fence\n"); > > >sw_sync_fence_destroy(fence); > > >} > > > > > >sw_sync_fence_destroy(fence_expired); > > > } > > > > > > drivers/dma-buf/sync_file.c | 7 ++- > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > > > index 486d29c..6ce6b8f 100644 > > > --- a/drivers/dma-buf/sync_file.c > > > +++ b/drivers/dma-buf/sync_file.c > > > @@ -178,11 +178,8 @@ static struct fence **get_fences(struct sync_file > > > *sync_file, int *num_fences) > > > static void add_fence(struct fence **fences, int *i, struct fence *fence) > > > { > > > fences[*i] = fence; > > > - > > > - if (!fence_is_signaled(fence)) { > > > - fence_get(fence); > > > - (*i)++; > > > - } > > > + fence_get(fence); > > > + (*i)++; > > > } > > > > I think you'll find it's the caller: > > > > if (i == 0) { > > add_fence(fences, &i, a_fences[0]); > > i++; > > } > > > > that does the unexpected. > > > > This should be > > > > if (i == 0) > > fences[i++] = fence_get(a_fences[0]); > > You are right. Otherwise we would miss the extra reference. i == 0 here > means that all fences are signalled. > > > > > > > That ensures the sync_file inherits the signaled status without having > > to keep all fences. OK, so if you are building a merged fence out of several signaled fences, then you only keep one of them instead? I haven't noticed that was the intention. > > I think there still seems to be a memory leak when calling > > sync_file_set_fence() here with i == 1. > > I think that is something we discussed already, we don't hold an extra > ref there for i == 1 because it would leak the fence. > I agree, we already increase the reference count in add_fence anyway (or here, assuming we do what Chris suggested. But I also believe that's the cause of confusion. IMHO it would be better to call fence_get() inside fence_array_create(), so it would match the fence_put in fence_array_release(). And if we have only one fence, fence_get it inside sync_file_set_fence(). Rafael
[PATCH] dma-buf/sync_file: Increment refcount of fence when all are signaled.
When we merge several fences, if all of them are signaled already, we still keep one of them. So instead of using add_fence(), which will not increase the refcount of signaled fences, we should explicitly call fence_get() for the fence we are keeping. This patch fixes a kernel panic that can be triggered by creating a fence that is expired (or increasing the timeline until it expires), then creating a merged fence out of it, and deleting the merged fence. This will make the original expired fence's refcount go to zero. Signed-off-by: Rafael Antognolli --- drivers/dma-buf/sync_file.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index abb5fda..0fe7ec2 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -253,10 +253,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, for (; i_b < b_num_fences; i_b++) add_fence(fences, &i, b_fences[i_b]); - if (i == 0) { - add_fence(fences, &i, a_fences[0]); - i++; - } + if (i == 0) + fences[i++] = fence_get(a_fences[0]); if (num_fences > i) { nfences = krealloc(fences, i * sizeof(*fences), -- 2.7.4
[PATCH] dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0)
Hi Chris and Gustavo, On Mon, Aug 29, 2016 at 07:16:13PM +0100, Chris Wilson wrote: > If we being polled with a timeout of zero, a nonblocking busy query, > we don't need to install any fence callbacks as we will not be waiting. > As we only install the callback once, the overhead comes from the atomic > bit test that also causes serialisation between threads. > > Signed-off-by: Chris Wilson > Cc: Sumit Semwal > Cc: Gustavo Padovan > Cc: linux-media at vger.kernel.org > Cc: dri-devel at lists.freedesktop.org > Cc: linaro-mm-sig at lists.linaro.org > --- > drivers/dma-buf/sync_file.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 486d29c1a830..abb5fdab75fd 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -306,7 +306,8 @@ static unsigned int sync_file_poll(struct file *file, > poll_table *wait) > > poll_wait(file, &sync_file->wq, wait); > > - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { > + if (!poll_does_not_wait(wait) && > + !test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { > if (fence_add_callback(sync_file->fence, &sync_file->cb, > fence_check_cb_func) < 0) > wake_up_all(&sync_file->wq); This commit is causing an error on one of the tests that Robert Foss submitted for i-g-t. The one that does random merge of fences from different timelines. A simple version of the test that still triggers this is: static void test_sync_simple_merge(void) { int fence1, fence2, fence_merge, timeline1, timeline2; int ret; timeline1 = sw_sync_timeline_create(); timeline2 = sw_sync_timeline_create(); fence1 = sw_sync_fence_create(timeline1, 1); fence2 = sw_sync_fence_create(timeline2, 2); fence_merge = sw_sync_merge(fence1, fence2); sw_sync_timeline_inc(timeline1, 5); sw_sync_timeline_inc(timeline2, 5); ret = sw_sync_wait(fence_merge, 0); igt_assert_f(ret > 0, "Failure triggering fence\n"); sw_sync_fence_destroy(fence_merge); sw_sync_fence_destroy(fence1); sw_sync_fence_destroy(fence2); sw_sync_timeline_destroy(timeline1); sw_sync_timeline_destroy(timeline2); } It looks like you cannot trust fence_is_signaled() without a fence_add_callback(). I think the fence_array->num_pending won't get updated. Although I couldn't figure out why it only happens if you merge fences from different timelines. Regards, Rafael
[PATCH v2] dma-buf/sync_file: Increment refcount of fence when all are signaled.
When we merge several fences, if all of them are signaled already, we still keep one of them. So instead of using add_fence(), which will not increase the refcount of signaled fences, we should explicitly call fence_get() for the fence we are keeping. This patch fixes a kernel panic that can be triggered by creating a fence that is expired (or increasing the timeline until it expires), then creating a merged fence out of it, and deleting the merged fence. This will make the original expired fence's refcount go to zero. Testcase: igt/sw_sync/sync_expired_merge Signed-off-by: Rafael Antognolli --- drivers/dma-buf/sync_file.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index abb5fda..0fe7ec2 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -253,10 +253,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, for (; i_b < b_num_fences; i_b++) add_fence(fences, &i, b_fences[i_b]); - if (i == 0) { - add_fence(fences, &i, a_fences[0]); - i++; - } + if (i == 0) + fences[i++] = fence_get(a_fences[0]); if (num_fences > i) { nfences = krealloc(fences, i * sizeof(*fences), -- 2.7.4
[PATCH v3] dma-buf/sync_file: Increment refcount of fence when all are signaled.
When we merge several fences, if all of them are signaled already, we still keep one of them. So instead of using add_fence(), which will not increase the refcount of signaled fences, we should explicitly call fence_get() for the fence we are keeping. This patch fixes a kernel panic that can be triggered by creating a fence that is expired (or increasing the timeline until it expires), then creating a merged fence out of it, and deleting the merged fence. This will make the original expired fence's refcount go to zero. Testcase: igt/sw_sync/sync_expired_merge Signed-off-by: Rafael Antognolli Reviewed-by: Chris Wilson Reviewed-by: Gustavo Padovan --- drivers/dma-buf/sync_file.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index abb5fda..0fe7ec2 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -253,10 +253,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, for (; i_b < b_num_fences; i_b++) add_fence(fences, &i, b_fences[i_b]); - if (i == 0) { - add_fence(fences, &i, a_fences[0]); - i++; - } + if (i == 0) + fences[i++] = fence_get(a_fences[0]); if (num_fences > i) { nfences = krealloc(fences, i * sizeof(*fences), -- 2.7.4
[PATCH 2/7] drm/doc: Polish for drm_plane.[hc]
Hi Daniel, On Wed, Sep 21, 2016 at 10:59:25AM +0200, Daniel Vetter wrote: > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 1407715736a5..256219bfd07b 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -319,10 +319,48 @@ struct drm_plane_funcs { > void (*early_unregister)(struct drm_plane *plane); > }; > > +/** > + * enum drm_plane_type - uapi plane type enumeration > + * > + * For historical reasons not all planes are made the same. This enumeration > is > + * used to tell the different types of planes apart to implement the > different > + * uapi semantics for them. For userspace which is universal plane aware and > + * which is using that atomic IOCTL there's no difference between these > planes > + * (beyong what the driver and hardware can support of course). > + * > + * For compatibility with legacy userspace, only overlay planes are made > + * available to userspace by default. Userspace clients may set the > + * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that > they > + * wish to receive a universal plane list containing all plane types. See > also > + * drm_for_each_legacy_plane(). > + */ > enum drm_plane_type { > - DRM_PLANE_TYPE_OVERLAY, > + /** > + * @DRM_PLANE_TYPE_PRIMARY: > + * > + * Primary planes represent a "main" plane for a CRTC. Primary planes > + * are the planes operated upon by CRTC modesetting and flipping > + * operations described in the page_flip and set_config hooks in struct > + * &drm_crtc_funcs. > + */ > DRM_PLANE_TYPE_PRIMARY, > + > + /** > + * @DRM_PLANE_TYPE_CURSOR: > + * > + * Cursor planes represent a "cursor" plane for a CRTC. Cursor planes > + * are the planes operated upon by the DRM_IOCTL_MODE_CURSOR and > + * DRM_IOCTL_MODE_CURSOR2 IOCTLs. > + */ > DRM_PLANE_TYPE_CURSOR, > + > + /** > + * @DRM_PLANE_TYPE_OVERLAY: > + * > + * Overlay planes represent all non-primary, non-cursor planes. Some > + * drivers refer to these types of planes as "sprites" internally. > + */ > + DRM_PLANE_TYPE_OVERLAY, > }; This is changing the order (and consequently the values) of these enums. But it is not updated in libdrm. I noticed this is causing an issue when playing with robclark's version of kmscube (branch atomic): https://github.com/robclark/kmscube/tree/atomic It looks like IGT also uses this macro from libdrm: $ git grep -n DRM_PLANE_TYPE_PRIMARY lib/igt_kms.c:1398: case DRM_PLANE_TYPE_PRIMARY: tests/kms_frontbuffer_tracking.c:2302: drm.plane_types[i] == DRM_PLANE_TYPE_PRIMARY) tests/kms_frontbuffer_tracking.c:2602: drm.plane_types[i] == DRM_PLANE_TYPE_PRIMARY) { tests/kms_frontbuffer_tracking.c:2741: drm.plane_types[i] == DRM_PLANE_TYPE_PRIMARY) Anyway, should we update it on libdrm, bring the order back to its original values, or something else? Or am I missing something? Thanks, Rafael
[PATCH 2/7] drm/doc: Polish for drm_plane.[hc]
On Wed, Sep 28, 2016 at 11:11:52AM +0300, Jani Nikula wrote: > On Wed, 28 Sep 2016, Rafael Antognolli wrote: > > Hi Daniel, > > > > On Wed, Sep 21, 2016 at 10:59:25AM +0200, Daniel Vetter wrote: > >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > >> index 1407715736a5..256219bfd07b 100644 > >> --- a/include/drm/drm_plane.h > >> +++ b/include/drm/drm_plane.h > >> @@ -319,10 +319,48 @@ struct drm_plane_funcs { > >>void (*early_unregister)(struct drm_plane *plane); > >> }; > >> > >> +/** > >> + * enum drm_plane_type - uapi plane type enumeration > >> + * > >> + * For historical reasons not all planes are made the same. This > >> enumeration is > >> + * used to tell the different types of planes apart to implement the > >> different > >> + * uapi semantics for them. For userspace which is universal plane aware > >> and > >> + * which is using that atomic IOCTL there's no difference between these > >> planes > >> + * (beyong what the driver and hardware can support of course). > >> + * > >> + * For compatibility with legacy userspace, only overlay planes are made > >> + * available to userspace by default. Userspace clients may set the > >> + * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that > >> they > >> + * wish to receive a universal plane list containing all plane types. See > >> also > >> + * drm_for_each_legacy_plane(). > >> + */ > >> enum drm_plane_type { > >> - DRM_PLANE_TYPE_OVERLAY, > >> + /** > >> + * @DRM_PLANE_TYPE_PRIMARY: > >> + * > >> + * Primary planes represent a "main" plane for a CRTC. Primary planes > >> + * are the planes operated upon by CRTC modesetting and flipping > >> + * operations described in the page_flip and set_config hooks in struct > >> + * &drm_crtc_funcs. > >> + */ > >>DRM_PLANE_TYPE_PRIMARY, > >> + > >> + /** > >> + * @DRM_PLANE_TYPE_CURSOR: > >> + * > >> + * Cursor planes represent a "cursor" plane for a CRTC. Cursor planes > >> + * are the planes operated upon by the DRM_IOCTL_MODE_CURSOR and > >> + * DRM_IOCTL_MODE_CURSOR2 IOCTLs. > >> + */ > >>DRM_PLANE_TYPE_CURSOR, > >> + > >> + /** > >> + * @DRM_PLANE_TYPE_OVERLAY: > >> + * > >> + * Overlay planes represent all non-primary, non-cursor planes. Some > >> + * drivers refer to these types of planes as "sprites" internally. > >> + */ > >> + DRM_PLANE_TYPE_OVERLAY, > >> }; > > > > This is changing the order (and consequently the values) of these enums. > > But it is not updated in libdrm. I noticed this is causing an issue when > > playing with robclark's version of kmscube (branch atomic): > > > > https://github.com/robclark/kmscube/tree/atomic > > > > It looks like IGT also uses this macro from libdrm: > > > > $ git grep -n DRM_PLANE_TYPE_PRIMARY > > lib/igt_kms.c:1398: case DRM_PLANE_TYPE_PRIMARY: > > tests/kms_frontbuffer_tracking.c:2302: drm.plane_types[i] == > > DRM_PLANE_TYPE_PRIMARY) > > tests/kms_frontbuffer_tracking.c:2602: drm.plane_types[i] == > > DRM_PLANE_TYPE_PRIMARY) { > > tests/kms_frontbuffer_tracking.c:2741: drm.plane_types[i] == > > DRM_PLANE_TYPE_PRIMARY) > > > > Anyway, should we update it on libdrm, bring the order back to its > > original values, or something else? Or am I missing something? > > You're absolutely right. But you're missing the fix has already landed > in drm-misc tree: > > commit 226714dc7c6af6d0acee449eb2afce08d128edad > Author: Daniel Vetter > Date: Fri Sep 23 08:35:25 2016 +0200 > > drm: Fix plane type uabi breakage > > BR, > Jani. Oh, I was sure I had checked drm-intel-nightly too, but I clearly didn't. Sorry for the noise. Regards, Rafael
[PATCH v8 0/2] Add drm_dp_aux chardev support.
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. Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++ drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/i915/intel_dp.c | 19 +- include/drm/drm_dp_aux_dev.h | 50 ++ 6 files changed, 449 insertions(+), 18 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v8 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/i915/intel_dp.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8287df4..7aacc08 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; intel_dp->aux.name = name; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", name, connector->base.kdev->kobj.name); ret = drm_dp_aux_register(&intel_dp->aux); - if (ret < 0) { + if (ret < 0) DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n", name, ret); - return; - } - - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret); - drm_dp_aux_unregister(&intel_dp->aux); - } } static void intel_dp_connector_unregister(struct intel_connector *intel_connector) { - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); intel_connector_unregister(intel_connector); } -- 2.4.3
[PATCH v8 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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. 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. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device v6: - Use atomic_t for usage count - Use a mutex instead of spinlock for idr lock - Destroy chardev immediately on unregister - other minor suggestions from Ville v7: - style fixes - error handling fixes v8: - more error handling fixes Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++ drivers/gpu/drm/drm_dp_helper.c | 16 +- include/drm/drm_dp_aux_dev.h | 50 ++ 5 files changed, 447 insertions(+), 1 deletion(-) 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 c4bf9a1..daefcce 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 1e9ff4c..e48ec8f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -28,6 +28,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 000..0269556 --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,373 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_dp_aux_dev { + unsigned index; + struct drm_dp_aux *aux; + struct device *dev; + struct kref refcount; + atomic_t usecount; +}; + +#define DRM_AUX_MINORS 256 +#define AUX_MAX_OFFSET (1 &
[PATCH v10 0/4] Add drm_dp_aux chardev support.
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. Lukas Wunner (1): drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled Rafael Antognolli (3): drm/kms_helper: Add a common place to call init and exit functions. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/i915: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile| 5 +- drivers/gpu/drm/drm_crtc_helper.c | 3 - drivers/gpu/drm/drm_dp_aux_dev.c| 368 drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/drm_fb_helper.c | 9 +- drivers/gpu/drm/drm_kms_helper_common.c | 60 ++ drivers/gpu/drm/i915/intel_dp.c | 18 +- drivers/gpu/drm/radeon/radeon_display.c | 5 +- include/drm/drm_dp_aux_dev.h| 62 ++ include/drm/drm_fb_helper.h | 6 + 11 files changed, 532 insertions(+), 28 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v10 1/4] drm/kms_helper: Add a common place to call init and exit functions.
The module_init and module_exit functions will start here, and call the subsequent init's and exit's. v10: - Keep __init on drm_fb_helper init function. - Move MODULE_* macros to the common file. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Makefile| 4 ++- drivers/gpu/drm/drm_crtc_helper.c | 3 --- drivers/gpu/drm/drm_fb_helper.c | 9 +++ drivers/gpu/drm/drm_kms_helper_common.c | 47 + include/drm/drm_fb_helper.h | 6 + 5 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index f858aa2..dfe513f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o drm-y += $(drm-m) drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \ - drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o + drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \ + drm_kms_helper_common.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 diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 5d4bc64..baac181 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -73,9 +73,6 @@ * &drm_crtc_helper_funcs, struct &drm_encoder_helper_funcs and struct * &drm_connector_helper_funcs. */ -MODULE_AUTHOR("David Airlie, Jesse Barnes"); -MODULE_DESCRIPTION("DRM KMS helper"); -MODULE_LICENSE("GPL and additional rights"); /** * drm_helper_move_panel_connectors_to_head() - move panels to the front in the diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1e103c4..c27b964 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event); * but the module doesn't depend on any fb console symbols. At least * attempt to load fbcon to avoid leaving the system without a usable console. */ -#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT) -static int __init drm_fb_helper_modinit(void) +int __init drm_fb_helper_modinit(void) { +#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT) const char *name = "fbcon"; struct module *fbcon; @@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void) if (!fbcon) request_module_nowait(name); +#endif return 0; } - -module_init(drm_fb_helper_modinit); -#endif +EXPORT_SYMBOL(drm_fb_helper_modinit); diff --git a/drivers/gpu/drm/drm_kms_helper_common.c b/drivers/gpu/drm/drm_kms_helper_common.c new file mode 100644 index 000..d361005 --- /dev/null +++ b/drivers/gpu/drm/drm_kms_helper_common.c @@ -0,0 +1,47 @@ +/* + * 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 + * + */ + +#include +#include + +MODULE_AUTHOR("David Airlie, Jesse Barnes"); +MODULE_DESCRIPTION("DRM KMS helper"); +MODULE_LICENSE("GPL and additional rights"); + +static int __init drm_kms_helper_init(void) +{ + /* Call init functions from specific kms helpers here */ + return drm_fb_helper_modinit(); +} + +static void __exit drm_kms_helper_exit(void) +{ + /* Call exit functions from specific kms helpers here */ +} + +module_init(drm_kms_helper_init); +module_exit(drm_kms_helper_exit); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_
[PATCH v10 2/4] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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. 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. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device v6: - Use atomic_t for usage count - Use a mutex instead of spinlock for idr lock - Destroy chardev immediately on unregister - other minor suggestions from Ville v7: - style fixes - error handling fixes v8: - more error handling fixes v9: - remove module_init and module_exit, and add drm_dp_aux_dev_init/exit to drm_kms_helper_init/exit. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/drm_dp_aux_dev.c| 368 drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/drm_kms_helper_common.c | 15 +- include/drm/drm_dp_aux_dev.h| 62 ++ 6 files changed, 468 insertions(+), 2 deletions(-) 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 59babd5..dff87ca 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 dfe513f..424fcb7 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -30,6 +30,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 000..f73b38b --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,368 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_dp_aux_dev { + unsigned index; +
[PATCH v10 4/4] drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled
From: Lukas Wunner Rafael Antognolli's new DRM_DP_AUX_CHARDEV feature causes a WARN_ON if drm_dp_aux->dev == drm_connector->kdev and drm_dp_aux_unregister() is called after drm_connector_unregister(). radeon is the only driver affected by this besides i915. (amdgpu calls drm_dp_aux_unregister() before drm_connector_unregister().) Cc: Rafael Antognolli Cc: Ville Syrjälä Cc: Alex Deucher Signed-off-by: Lukas Wunner --- drivers/gpu/drm/radeon/radeon_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index b3bb923..a885dae 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1684,6 +1684,9 @@ void radeon_modeset_fini(struct radeon_device *rdev) radeon_fbdev_fini(rdev); kfree(rdev->mode_info.bios_hardcoded_edid); + /* free i2c buses */ + radeon_i2c_fini(rdev); + if (rdev->mode_info.mode_config_initialized) { radeon_afmt_fini(rdev); drm_kms_helper_poll_fini(rdev->ddev); @@ -1691,8 +1694,6 @@ void radeon_modeset_fini(struct radeon_device *rdev) drm_mode_config_cleanup(rdev->ddev); rdev->mode_info.mode_config_initialized = false; } - /* free i2c buses */ - radeon_i2c_fini(rdev); } static bool is_hdtv_mode(const struct drm_display_mode *mode) -- 2.4.3
[PATCH v10 3/4] drm/i915: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. v9: - As a side effect, drm_dp_aux_unregister() must be called before intel_connector_unregister(), as both the aux.dev and the i2c adapter dev are children of the drm_connector device now. Calling drm_dp_aux_unregister() before prevents them from being destroyed twice. v10: - move aux_fini() to connector_unregister(), instead of moving drm_dp_aux_unregister() outside of connector_register(). Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/i915/intel_dp.c | 18 ++ 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e2bea710..da704c6 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp) static int intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); enum port port = intel_dig_port->port; int ret; @@ -1199,7 +1198,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) if (!intel_dp->aux.name) return -ENOMEM; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", @@ -1214,16 +1213,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) return ret; } - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", - intel_dp->aux.name, ret); - intel_dp_aux_fini(intel_dp); - return ret; - } - return 0; } @@ -1232,9 +1221,7 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector) { struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); + intel_dp_aux_fini(intel_dp); intel_connector_unregister(intel_connector); } @@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); struct intel_dp *intel_dp = &intel_dig_port->dp; - intel_dp_aux_fini(intel_dp); intel_dp_mst_encoder_cleanup(intel_dig_port); if (is_edp(intel_dp)) { cancel_delayed_work_sync(&intel_dp->panel_vdd_work); -- 2.4.3
[PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
Hi Lukas, Sorry for the late answer, I went on vacation and then was busy with something else. Anyway, I tried to address all comments (yours and Daniel's) on a new version. See some comments below. On Sun, Dec 06, 2015 at 05:08:49PM +0100, Lukas Wunner wrote: > Hi Rafael, > > On Thu, Dec 03, 2015 at 02:54:02PM -0800, Rafael Antognolli wrote: > > So far, the i915 driver and some other drivers set it to the drm_device, > > which doesn't allow one to know which DP a given aux channel is related > > to. Changing this to be the drm_connector provides proper nesting, still > > allowing one to get the drm_device from it. Some drivers already set it > > to the drm_connector. > > > > This also removes the need to add a sysfs link for the i2c device under > > the connector, as it will already be there. > > > > v2: > > Two minor nits, I think this should be "v9" instead of "v2" because > this was newly added since v8, and the subject should be prefixed > "drm/i915" (or "drm/i915/dp" or whatever) instead of "drm/dp" because > this only touches i915 and not drm core. > > > > - As a side effect, drm_dp_aux_unregister() must be called before > > intel_connector_unregister(), as both the aux.dev and the i2c adapter > > dev are children of the drm_connector device now. Calling > > drm_dp_aux_unregister() before prevents them from being destroyed > > twice. > > I haven't verified what Ville wrote (that there are WARNs because of > this ordering issue), but if this is true then we need to make sure > all other drivers get the order right, otherwise they'd spew WARNs > as well once this gets merged. > > I've checked that for every driver and the only one affected is radeon. > A fix is now in my GitHub repo, feel free to include the commit in your > series if you want to. I haven't been able to actually test it though > as I don't have a radeon card: > https://github.com/l1k/linux/commit/485e197a7cac88e24348150526988149428feeaa > Nice, I added it to the series, though I also don't have a radeon to test it. > > > > Signed-off-by: Rafael Antognolli > > --- > > drivers/gpu/drm/i915/intel_dp.c | 22 -- > > 1 file changed, 4 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index f2bfca0..515893c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp > > *intel_dp) > > static void > > intel_dp_aux_fini(struct intel_dp *intel_dp) > > { > > - drm_dp_aux_unregister(&intel_dp->aux); > > kfree(intel_dp->aux.name); > > } > > Hm, instead of moving the single call of drm_dp_aux_unregister() > to intel_dp_connector_unregister() I think it would be more clear > to call intel_dp_aux_fini() from intel_dp_connector_unregister() > and remove its invocation from intel_dp_encoder_destroy(). > > Ville introduced intel_dp_aux_fini() very recently with a121f4e5fae5, > he should probably have a say on how to solve this. I makes sense to me, so I did what you suggest. Thanks for the review, Rafael > > > > static int > > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector > > *connector) > > { > > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > enum port port = intel_dig_port->port; > > int ret; > > @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct > > intel_connector *connector) > > if (!intel_dp->aux.name) > > return -ENOMEM; > > > > - intel_dp->aux.dev = dev->dev; > > + intel_dp->aux.dev = connector->base.kdev; > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", > > @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct > > intel_connector *connector) > > return ret; > > } > > > > - ret = sysfs_create_link(&connector->base.kdev->kobj, > > - &intel_dp->aux.ddc.dev.kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > - if (ret < 0) { > > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", > > - intel_dp->aux
[RFC 11/13] drm/dp: Add helper to dump DPCD
On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote: > On Wed, 12 Aug 2015, Thierry Reding wrote: > > From: Thierry Reding > > > > The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a > > seq_file and can be used to make the DPCD available via debugfs for > > example. > > See i915/i915_debugfs.c for one DPCD dump implementation. > > Around the time that was added, there was also some discussion (and > patches [1]) to expose a read/write debugfs interface to DPCD, letting > userspace access arbitrary DPCD registers. > > Just this week there was some discussion about revisiting that. It was > about accessing some proprietary panel features, but there's also the > ease of debugging without having to keep updating the kernel to dump > more. > > I think it would be great to agree on a common debugfs interface to > access DPCD arbitrarily. Last time I checked, the blocker to that was > access to the aux channel from generic code; it's always driver > specific. SMOP. ;) Do you mean it would require the generic code/interface to somehow route this to the driver specific code? I am not sure yet how this works (if there's something like it around), but I'll take a look. > I could put some effort into this (maybe Rafael too?), as long as we > could agree on the interface. As I wrote in the referenced thread, I > wasn't thrilled about what was proposed. > Yes, I'm willing to put effort into this, for sure. Any help pointing to which direction to follow is greatly appreciated. Thanks, Rafael > > > [1] http://mid.gmane.org/1428493301-20293-1-git-send-email-durgadoss.r at > intel.com > > > > > > > Signed-off-by: Thierry Reding > > --- > > drivers/gpu/drm/drm_dp_helper.c | 146 > > > > include/drm/drm_dp_helper.h | 2 + > > 2 files changed, 148 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c > > b/drivers/gpu/drm/drm_dp_helper.c > > index 8968201ea93c..ea74884c9cb3 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -27,6 +27,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -292,6 +293,151 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux > > *aux, > > } > > EXPORT_SYMBOL(drm_dp_dpcd_read_link_status); > > > > +/** > > + * drm_dp_dpcd_dump() - dump DPCD content > > + * @aux: DisplayPort AUX channel > > + * @s: destination for DPCD dump > > + * > > + * Reads registers from the DPCD via a DisplayPort AUX channel and dumps > > them > > + * to a seq_file. > > + */ > > +void drm_dp_dpcd_dump(struct drm_dp_aux *aux, struct seq_file *s) > > +{ > > +#define DUMP_REG(aux, offset) ({ \ > > + u8 value; \ > > + int err;\ > > + err = drm_dp_dpcd_readb(aux, offset, &value); \ > > + if (err < 0) { \ > > + dev_err((aux)->dev, "failed to read %s: %d\n", \ > > + #offset, err); \ > > + return; \ > > + } \ > > + seq_printf(s, "%-35s 0x%04x 0x%02x\n", #offset, offset, \ > > + value); \ > > + }) > > + > > + DUMP_REG(aux, DP_DPCD_REV); > > + DUMP_REG(aux, DP_MAX_LINK_RATE); > > + DUMP_REG(aux, DP_MAX_LANE_COUNT); > > + DUMP_REG(aux, DP_MAX_DOWNSPREAD); > > + DUMP_REG(aux, DP_NORP); > > + DUMP_REG(aux, DP_DOWNSTREAMPORT_PRESENT); > > + DUMP_REG(aux, DP_MAIN_LINK_CHANNEL_CODING); > > + DUMP_REG(aux, DP_DOWN_STREAM_PORT_COUNT); > > + DUMP_REG(aux, DP_RECEIVE_PORT_0_CAP_0); > > + DUMP_REG(aux, DP_RECEIVE_PORT_0_BUFFER_SIZE); > > + DUMP_REG(aux, DP_RECEIVE_PORT_1_CAP_0); > > + DUMP_REG(aux, DP_RECEIVE_PORT_1_BUFFER_SIZE); > > + DUMP_REG(aux, DP_I2C_SPEED_CAP); > > + DUMP_REG(aux, DP_EDP_CONFIGURATION_CAP); > > + DUMP_REG(aux, DP_TRAINING_AUX_RD_INTERVAL); > > + DUMP_REG(aux, DP_ADAPTER_CAP); > > + DUMP_REG(aux, DP_SUPPORTED_LINK_RATES); > > + DUMP_REG(aux, DP_FAUX_CAP); > > + DUMP_REG(aux, DP_MSTM_CAP); > > + DUMP_REG(aux, DP_NUMBER_
[RFC 11/13] drm/dp: Add helper to dump DPCD
On Mon, Aug 17, 2015 at 10:02:04AM +0300, Jani Nikula wrote: > On Fri, 14 Aug 2015, Rafael Antognolli wrote: > > On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote: > >> On Wed, 12 Aug 2015, Thierry Reding wrote: > >> > From: Thierry Reding > >> > > >> > The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a > >> > seq_file and can be used to make the DPCD available via debugfs for > >> > example. > >> > >> See i915/i915_debugfs.c for one DPCD dump implementation. > >> > >> Around the time that was added, there was also some discussion (and > >> patches [1]) to expose a read/write debugfs interface to DPCD, letting > >> userspace access arbitrary DPCD registers. > >> > >> Just this week there was some discussion about revisiting that. It was > >> about accessing some proprietary panel features, but there's also the > >> ease of debugging without having to keep updating the kernel to dump > >> more. > >> > >> I think it would be great to agree on a common debugfs interface to > >> access DPCD arbitrarily. Last time I checked, the blocker to that was > >> access to the aux channel from generic code; it's always driver > >> specific. SMOP. ;) > > > > Do you mean it would require the generic code/interface to somehow route > > this to the driver specific code? I am not sure yet how this works (if > > there's something like it around), but I'll take a look. > > Drivers can choose to support DP AUX any way they like, and they don't > have to use the common helpers to do so. It's pretty much an > implementation detail. I think we could require the use of the common > helpers to support generic DPCD access from debugfs, but we'd still have > to come up with a way to get hold of struct drm_dp_aux (again, driver > internals) given a drm_connector in generic debugfs code. I was under the assumption they always used the helpers, but I understand that's not always the case. Anyway, I was going to propose a new helper to "store" the drm_dp_aux inside the drm_connector. And just expose something on debugfs if it was used. Something like: int drm_dp_aux_store(struct drm_dp_aux *aux, struct drm_connector *connector); The helpers don't seem to know about drm_connector's though, so I'm not sure this is a good approach. > Thierry, do you see any problems with having a connector function to get > hold of its drm_dp_aux? Would this be a new function pointer inside struct drm_connector_funcs? If so, I'm fine with this approach too. Regarding the interface, you mentioned that you didn't like it because it had a state. What about just dumping the content of the register into dmesg when one tries to read it, and use a different sintaxe for writing, passing both the register addr and the value? Daniel had another suggestion, regarding an ioctl in debugfs, but I'm not sure I clearly understand that yet. Thanks, Rafael
[Intel-gfx] [PATCH v8 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
On Tue, Nov 24, 2015 at 10:31:41PM +0200, Ville Syrjälä wrote: > On Mon, Nov 02, 2015 at 12:33:48PM -0800, Rafael Antognolli wrote: > > So far, the i915 driver and some other drivers set it to the drm_device, > > which doesn't allow one to know which DP a given aux channel is related > > to. Changing this to be the drm_connector provides proper nesting, still > > allowing one to get the drm_device from it. Some drivers already set it > > to the drm_connector. > > > > This also removes the need to add a sysfs link for the i2c device under > > the connector, as it will already be there. > > > > Signed-off-by: Rafael Antognolli > > I gave aux_dev a bit of a testing here, and it appaers to work quite > splendidly. > > This patch however causes lots of WARN spew if I unload the driver > while in middle of dumping the DPCD via the aux_dev. It appears we > we clean up things the wrong order now. It looks like device_destroy is being called twice on the drm_aux_dev and i2c adapter devices. If I understood correctly, this is happening because I changed the parent device, from the drm_device to the drm_connector, but the drm_connector is being destroyed before the drm_aux_dev and i2c adapter devices, which already causes them to be destroyed too. Changing the drm_dp_aux_unregister() from inside intel_dp_aux_fini() to intel_dp_connector_unregister(), right before intel_connector_unregister(), seems to fix everything, and it still makes sense in my opinion, since the auxdev is going to be unregistered before we destroy the drm_connector. Do you think that would be ok? > > --- > > drivers/gpu/drm/i915/intel_dp.c | 19 ++- > > 1 file changed, 2 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 8287df4..7aacc08 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct > > intel_connector *connector) > > intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; > > > > intel_dp->aux.name = name; > > - intel_dp->aux.dev = dev->dev; > > + intel_dp->aux.dev = connector->base.kdev; > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > connector->base.kdev->kobj.name); > > > > ret = drm_dp_aux_register(&intel_dp->aux); > > - if (ret < 0) { > > + if (ret < 0) > > DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n", > > name, ret); > > - return; > > - } > > - > > - ret = sysfs_create_link(&connector->base.kdev->kobj, > > - &intel_dp->aux.ddc.dev.kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > - if (ret < 0) { > > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, > > ret); > > - drm_dp_aux_unregister(&intel_dp->aux); > > - } > > } > > > > static void > > intel_dp_connector_unregister(struct intel_connector *intel_connector) > > { > > - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); > > - > > - if (!intel_connector->mst_port) > > - sysfs_remove_link(&intel_connector->base.kdev->kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > intel_connector_unregister(intel_connector); > > } > > > > -- > > 2.4.3 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
[PATCH v9 0/3] Add drm_dp_aux chardev support.
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. Rafael Antognolli (3): drm/kms_helper: Add a common place to call init and exit functions. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile| 5 +- drivers/gpu/drm/drm_dp_aux_dev.c| 368 drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/drm_fb_helper.c | 9 +- drivers/gpu/drm/drm_kms_helper_common.c | 56 + drivers/gpu/drm/i915/intel_dp.c | 22 +- include/drm/drm_dp_aux_dev.h| 62 ++ include/drm/drm_fb_helper.h | 6 + 9 files changed, 527 insertions(+), 25 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v9 2/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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. 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. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device v6: - Use atomic_t for usage count - Use a mutex instead of spinlock for idr lock - Destroy chardev immediately on unregister - other minor suggestions from Ville v7: - style fixes - error handling fixes v8: - more error handling fixes v9: - remove module_init and module_exit, and add drm_dp_aux_dev_init/exit to drm_kms_helper_init/exit. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/drm_dp_aux_dev.c| 368 drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/drm_kms_helper_common.c | 15 +- include/drm/drm_dp_aux_dev.h| 62 ++ 6 files changed, 468 insertions(+), 2 deletions(-) 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 c4bf9a1..daefcce 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 76399da..57e153d 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -30,6 +30,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 000..f73b38b --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,368 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_dp_aux_dev { + unsigned index; +
[PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. v2: - As a side effect, drm_dp_aux_unregister() must be called before intel_connector_unregister(), as both the aux.dev and the i2c adapter dev are children of the drm_connector device now. Calling drm_dp_aux_unregister() before prevents them from being destroyed twice. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/i915/intel_dp.c | 22 -- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f2bfca0..515893c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) static void intel_dp_aux_fini(struct intel_dp *intel_dp) { - drm_dp_aux_unregister(&intel_dp->aux); kfree(intel_dp->aux.name); } static int intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); enum port port = intel_dig_port->port; int ret; @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) if (!intel_dp->aux.name) return -ENOMEM; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) return ret; } - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", - intel_dp->aux.name, ret); - intel_dp_aux_fini(intel_dp); - return ret; - } - return 0; } static void intel_dp_connector_unregister(struct intel_connector *intel_connector) { - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); + struct intel_dp *intel_dp = + enc_to_intel_dp(&intel_connector->encoder->base); + drm_dp_aux_unregister(&intel_dp->aux); intel_connector_unregister(intel_connector); } -- 2.4.3
[PATCH v9 1/3] drm/kms_helper: Add a common place to call init and exit functions.
The module_init and module_exit functions will start here, and call the subsequent init's and exit's. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Makefile| 4 ++- drivers/gpu/drm/drm_fb_helper.c | 9 +++ drivers/gpu/drm/drm_kms_helper_common.c | 43 + include/drm/drm_fb_helper.h | 6 + 4 files changed, 56 insertions(+), 6 deletions(-) create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1e9ff4c..76399da 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o drm-y += $(drm-m) drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \ - drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o + drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \ + drm_kms_helper_common.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 diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 69cbab5..576f769 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event); * but the module doesn't depend on any fb console symbols. At least * attempt to load fbcon to avoid leaving the system without a usable console. */ -#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT) -static int __init drm_fb_helper_modinit(void) +int drm_fb_helper_modinit(void) { +#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT) const char *name = "fbcon"; struct module *fbcon; @@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void) if (!fbcon) request_module_nowait(name); +#endif return 0; } - -module_init(drm_fb_helper_modinit); -#endif +EXPORT_SYMBOL(drm_fb_helper_modinit); diff --git a/drivers/gpu/drm/drm_kms_helper_common.c b/drivers/gpu/drm/drm_kms_helper_common.c new file mode 100644 index 000..920b2fb --- /dev/null +++ b/drivers/gpu/drm/drm_kms_helper_common.c @@ -0,0 +1,43 @@ +/* + * 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 + * + */ + +#include +#include + +static int __init drm_kms_helper_init(void) +{ + /* Call init functions from specific kms helpers here */ + return drm_fb_helper_modinit(); +} + +static void __exit drm_kms_helper_exit(void) +{ + /* Call exit functions from specific kms helpers here */ +} + +module_init(drm_kms_helper_init); +module_exit(drm_kms_helper_exit); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 87b090c..4fed7a2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -148,6 +148,7 @@ struct drm_fb_helper { }; #ifdef CONFIG_DRM_FBDEV_EMULATION +int drm_fb_helper_modinit(void); void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, const struct drm_fb_helper_funcs *funcs); int drm_fb_helper_init(struct drm_device *dev, @@ -212,6 +213,11 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct drm_ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper, struct drm_connector *connector); #else +static inline int drm_fb_helper_modinit(void) +{ + return 0; +} + static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Tue, Sep 22, 2015 at 10:59:51AM +0200, Daniel Vetter wrote: > On Tue, Sep 15, 2015 at 04:55:04PM -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_aux-N. > > > > It's possible to know which connector owns this aux channel by looking > > at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/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. > > Reading or writing does not update the offset of the file. > > I think not updating the read position is very surprising. Would it be > hard to fix that? No, not hard at all. But I assume then I should update the write position too, right? BTW, i2c-dev doesn't update either of them, but I'm not sure why. > > > > Signed-off-by: Rafael Antognolli > > --- > > drivers/gpu/drm/Kconfig | 4 + > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/drm_aux-dev.c | 326 > > ++ > > 3 files changed, 331 insertions(+) > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index 1a0a8df..eae847c 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -25,6 +25,10 @@ config DRM_MIPI_DSI > > bool > > depends on DRM > > > > +config DRM_AUX_CHARDEV > > + tristate "DRM DP AUX Interface" > > + depends on DRM > > + > > config DRM_KMS_HELPER > > tristate > > depends on DRM > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index 45e7719..a1a94306 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src) > > > > obj-$(CONFIG_DRM) += drm.o > > obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o > > +obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o > > obj-$(CONFIG_DRM_TTM) += ttm/ > > obj-$(CONFIG_DRM_TDFX) += tdfx/ > > obj-$(CONFIG_DRM_R128) += r128/ > > diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c > > new file mode 100644 > > index 000..fcc334a > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_aux-dev.c > > @@ -0,0 +1,326 @@ > > +/* > > + * 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 > > + * > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +struct drm_aux_dev { > > + struct list_head list; > > + unsigned index; > > + struct drm_dp_aux *aux; > > + struct device *dev; > > +}; > > + > > +#define DRM_AUX_MINORS 256 > > +static int drm_aux_dev_count = 0; > > +static LIST_HEAD(drm_aux_dev_list); > > +static DEFINE_SPINLOCK(drm_aux_dev_list_lock); > > + > > +static struct drm_aux_dev *d
[PATCH RFC v2 0/3] Add a drm_aux-dev module.
Second attempt at implementing a module that allows reading/writing arbitrary dpcd registers. Changes to this version: - 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. One thing to notice is that I am not updating the file offset during read or write, which is kind of breaking the filesystem abstraction. But i2c-dev doesn't do it either, so I assumed it's fine. Rafael Antognolli (3): drm/dp: Keep a list of drm_dp_aux helper. 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 | 4 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/drm_aux-dev.c | 326 drivers/gpu/drm/drm_dp_helper.c | 73 + drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 6 + 6 files changed, 411 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c -- 2.4.3
[PATCH v3 1/2] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel. Signed-off-by: Rafael Antognolli --- 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 06a2b10..1b1df1c 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
[PATCH v3 0/2] Add drm_dp_aux chardev support.
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 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 | 3 +- 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, 424 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v3 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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 --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 3 +- 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, 422 insertions(+), 1 deletion(-) 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..1afcbed 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -21,7 +21,8 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o drm-$(CONFIG_OF) += drm_of.o drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \ - drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o + drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \ + drm_dp_aux_dev.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 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 000..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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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; +fo
[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote: > On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 15, 2015 at 04:55:04PM -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_aux-N. > > > > > > It's possible to know which connector owns this aux channel by looking > > > at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/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. > > > Reading or writing does not update the offset of the file. > > > > > > Signed-off-by: Rafael Antognolli > > > --- > > > drivers/gpu/drm/Kconfig | 4 + > > > drivers/gpu/drm/Makefile | 1 + > > > drivers/gpu/drm/drm_aux-dev.c | 326 > > > ++ > > > 3 files changed, 331 insertions(+) > > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > > index 1a0a8df..eae847c 100644 > > > --- a/drivers/gpu/drm/Kconfig > > > +++ b/drivers/gpu/drm/Kconfig > > > @@ -25,6 +25,10 @@ config DRM_MIPI_DSI > > > bool > > > depends on DRM > > > > > > +config DRM_AUX_CHARDEV > > > + tristate "DRM DP AUX Interface" > > > + depends on DRM > > > + > > > config DRM_KMS_HELPER > > > tristate > > > depends on DRM > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > > index 45e7719..a1a94306 100644 > > > --- a/drivers/gpu/drm/Makefile > > > +++ b/drivers/gpu/drm/Makefile > > > @@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src) > > > > > > obj-$(CONFIG_DRM)+= drm.o > > > obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o > > > +obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o > > > obj-$(CONFIG_DRM_TTM)+= ttm/ > > > obj-$(CONFIG_DRM_TDFX) += tdfx/ > > > obj-$(CONFIG_DRM_R128) += r128/ > > > diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c > > > new file mode 100644 > > > index 000..fcc334a > > > --- /dev/null > > > +++ b/drivers/gpu/drm/drm_aux-dev.c > > > @@ -0,0 +1,326 @@ > > > +/* > > > + * 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 > > > + * > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +struct drm_aux_dev { > > > + struct list_head list; &
[PATCH RFC v2 0/3] Add a drm_aux-dev module.
Oops, sorry, this email shouldn't be in the patch set, please disconsider it. -- Rafael On Fri, Sep 25, 2015 at 04:54:53PM -0700, Rafael Antognolli wrote: > Second attempt at implementing a module that allows reading/writing arbitrary > dpcd registers. Changes to this version: > - 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. > > One thing to notice is that I am not updating the file offset during read or > write, which is kind of breaking the filesystem abstraction. But i2c-dev > doesn't do it either, so I assumed it's fine. > > Rafael Antognolli (3): > drm/dp: Keep a list of drm_dp_aux helper. > 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 | 4 + > drivers/gpu/drm/Makefile| 1 + > drivers/gpu/drm/drm_aux-dev.c | 326 > > drivers/gpu/drm/drm_dp_helper.c | 73 + > drivers/gpu/drm/i915/intel_dp.c | 1 + > include/drm/drm_dp_helper.h | 6 + > 6 files changed, 411 insertions(+) > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > -- > 2.4.3 >
[PATCH v4 0/2] Add drm_dp_aux chardev support.
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
[PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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 --- 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 000..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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +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
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel. Signed-off-by: Rafael Antognolli --- 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
[PATCH i-g-t] tests: Adding kms_dp_aux_dev test.
This new test makes some basic testing on the proposed drm_dp_aux_dev interface. If the feature is enabled and the drm_dp_aux_dev class is present, it will check for available DP aux channels and test them for: - basic seek to 0 and read 1 byte - seek to the last address and read, to confirm 0 is returned - seek one more byte and confirm that EINVAL is returned - try to read 64 bytes when at 8 bytes from the end of the address space - try to read 64 bytes at the address 0. So far, no write checks are done. Signed-off-by: Rafael Antognolli --- tests/.gitignore | 1 + tests/Makefile.sources | 1 + tests/kms_dp_aux_dev.c | 134 + 3 files changed, 136 insertions(+) create mode 100644 tests/kms_dp_aux_dev.c diff --git a/tests/.gitignore b/tests/.gitignore index dc8bb53..efdad75 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -127,6 +127,7 @@ kms_3d kms_addfb_basic kms_crtc_background_color kms_cursor_crc +kms_dp_aux_dev kms_draw_crc kms_fbc_crc kms_fbcon_fbt diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 2e2e088..dc07737 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -64,6 +64,7 @@ TESTS_progs_M = \ gem_write_read_ring_switch \ kms_addfb_basic \ kms_cursor_crc \ + kms_dp_aux_dev \ kms_draw_crc \ kms_fbc_crc \ kms_fbcon_fbt \ diff --git a/tests/kms_dp_aux_dev.c b/tests/kms_dp_aux_dev.c new file mode 100644 index 000..aab8c95 --- /dev/null +++ b/tests/kms_dp_aux_dev.c @@ -0,0 +1,134 @@ +/* + * 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. + */ + +/** @file kms_dp_aux_dev.c + * + * This is a test of functionality of drm_dp_aux_dev. + */ + +#include "igt.h" +#include +#include +#include +#include +#include +#include + +#include + +#define DRM_DP_AUX_SYSFS_PATH "/sys/class/drm_dp_aux_dev" + +IGT_TEST_DESCRIPTION("Test of drm_aux_dev nodes."); + +static void drm_dp_aux_test(const char *devname) +{ + char basedir[] = "/dev/"; + char name[256 + sizeof(basedir)]; + off_t address; + uint8_t buf; + uint8_t bigbuf[64]; + int fd; + + ssize_t res; + + snprintf(name, sizeof(name), "%s%s", basedir, devname); + igt_info("Running test on %s\n", name); + + /* Check if this DP aux channel is connected before continuing tests */ + fd = open(name, O_RDONLY); + igt_assert_lte(0, fd); + address = lseek(fd, 0x0, SEEK_SET); + igt_assert_eq(address, 0x0); + res = read(fd, &buf, sizeof(buf)); + igt_skip_on(res < 0 && errno == ETIMEDOUT); + + + /* Channel is connected, start tests */ + igt_assert_eq(res, sizeof(buf)); + + /* Test reads on the end of address space */ + address = lseek(fd, 0, SEEK_END); + igt_assert_eq(address, 1 << 20); + res = read(fd, &buf, sizeof(buf)); + igt_assert_eq(res, 0); + + address = lseek(fd, 1, SEEK_CUR); + igt_assert_eq(address, -1); + igt_assert_eq(errno, EINVAL); + + address = lseek(fd, -8, SEEK_END); + res = read(fd, bigbuf, sizeof(bigbuf)); + igt_assert_eq(res, 8); + + /* Test reading more than 16 bytes at once */ + address = lseek(fd, 0, SEEK_SET); + res = read(fd, bigbuf, sizeof(bigbuf)); + igt_assert_eq(res, sizeof(bigbuf)); +} + +static void for_each_dp_aux(DIR *dir) +{ + int count = 0; + struct dirent *dirent; + + if (!dir) + return; + + rewinddir(dir); + while ((dirent = readdir(dir))) { + if (dirent->d_name[0] == '.') + continue; + + count++; +
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
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 > > --- > > 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 at 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.
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 > > --- > > 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 000..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 > > + * > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +struct drm_dp_aux_dev { >
Re: [PATCH libdrm] intel/intel_chipset.h: Sync Cannonlake IDs.
This is matching both the kernel and the spec. Reviewed-by: Rafael Antognolli . On Wed, Feb 14, 2018 at 05:42:24PM -0800, Rodrigo Vivi wrote: > Let's sync CNL ids with Spec and kernel. > > Sync with kernel commit '3f43031b1693 ("drm/i915/cnl: > Add Cannonlake PCI IDs for another SKU.")' and > commit 'e3890d05b342 ("drm/i915/cnl: Sync PCI ID with Spec.")' > > Cc: James Ausmus > Cc: Lucas De Marchi > Cc: Paulo Zanoni > Signed-off-by: Rodrigo Vivi > --- > intel/intel_chipset.h | 52 > +++ > 1 file changed, 28 insertions(+), 24 deletions(-) > > diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h > index 3818e71e..01d250e8 100644 > --- a/intel/intel_chipset.h > +++ b/intel/intel_chipset.h > @@ -241,16 +241,20 @@ > #define PCI_CHIP_COFFEELAKE_U_GT3_4 0x3EA7 > #define PCI_CHIP_COFFEELAKE_U_GT3_5 0x3EA8 > > -#define PCI_CHIP_CANNONLAKE_U_GT2_0 0x5A52 > -#define PCI_CHIP_CANNONLAKE_U_GT2_1 0x5A5A > -#define PCI_CHIP_CANNONLAKE_U_GT2_2 0x5A42 > -#define PCI_CHIP_CANNONLAKE_U_GT2_3 0x5A4A > -#define PCI_CHIP_CANNONLAKE_Y_GT2_0 0x5A51 > -#define PCI_CHIP_CANNONLAKE_Y_GT2_1 0x5A59 > -#define PCI_CHIP_CANNONLAKE_Y_GT2_2 0x5A41 > -#define PCI_CHIP_CANNONLAKE_Y_GT2_3 0x5A49 > -#define PCI_CHIP_CANNONLAKE_Y_GT2_4 0x5A71 > -#define PCI_CHIP_CANNONLAKE_Y_GT2_5 0x5A79 > +#define PCI_CHIP_CANNONLAKE_00x5A51 > +#define PCI_CHIP_CANNONLAKE_10x5A59 > +#define PCI_CHIP_CANNONLAKE_20x5A41 > +#define PCI_CHIP_CANNONLAKE_30x5A49 > +#define PCI_CHIP_CANNONLAKE_40x5A52 > +#define PCI_CHIP_CANNONLAKE_50x5A5A > +#define PCI_CHIP_CANNONLAKE_60x5A42 > +#define PCI_CHIP_CANNONLAKE_70x5A4A > +#define PCI_CHIP_CANNONLAKE_80x5A50 > +#define PCI_CHIP_CANNONLAKE_90x5A40 > +#define PCI_CHIP_CANNONLAKE_10 0x5A54 > +#define PCI_CHIP_CANNONLAKE_11 0x5A5C > +#define PCI_CHIP_CANNONLAKE_12 0x5A44 > +#define PCI_CHIP_CANNONLAKE_13 0x5A4C > > #define IS_MOBILE(devid) ((devid) == PCI_CHIP_I855_GM || \ >(devid) == PCI_CHIP_I915_GM || \ > @@ -515,20 +519,20 @@ >IS_GEMINILAKE(devid) || \ >IS_COFFEELAKE(devid)) > > -#define IS_CNL_Y(devid) ((devid) == PCI_CHIP_CANNONLAKE_Y_GT2_0 > || \ > - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_1 || \ > - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_2 || \ > - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_3 || \ > - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_4 || \ > - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_5) > - > -#define IS_CNL_U(devid) ((devid) == PCI_CHIP_CANNONLAKE_U_GT2_0 > || \ > - (devid) == PCI_CHIP_CANNONLAKE_U_GT2_1 || \ > - (devid) == PCI_CHIP_CANNONLAKE_U_GT2_2 || \ > - (devid) == PCI_CHIP_CANNONLAKE_U_GT2_3) > - > -#define IS_CANNONLAKE(devid) (IS_CNL_U(devid) || \ > - IS_CNL_Y(devid)) > +#define IS_CANNONLAKE(devid) ((devid) == PCI_CHIP_CANNONLAKE_0 || \ > + (devid) == PCI_CHIP_CANNONLAKE_1 || \ > + (devid) == PCI_CHIP_CANNONLAKE_2 || \ > + (devid) == PCI_CHIP_CANNONLAKE_3 || \ > + (devid) == PCI_CHIP_CANNONLAKE_4 || \ > + (devid) == PCI_CHIP_CANNONLAKE_5 || \ > + (devid) == PCI_CHIP_CANNONLAKE_6 || \ > + (devid) == PCI_CHIP_CANNONLAKE_7 || \ > + (devid) == PCI_CHIP_CANNONLAKE_8 || \ > + (devid) == PCI_CHIP_CANNONLAKE_9 || \ > + (devid) == PCI_CHIP_CANNONLAKE_10 || \ > + (devid) == PCI_CHIP_CANNONLAKE_11 || \ > + (devid) == PCI_CHIP_CANNONLAKE_12 || \ > + (devid) == PCI_CHIP_CANNONLAKE_13) > > #define IS_GEN10(devid) (IS_CANNONLAKE(devid)) > > -- > 2.13.6 > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
re: [PATCH] r600g: remove useless call to u_upload_flush
I cannot test it now, but last time the flush call was removed it caused a major regression on Evergreen. See: https://bugs.freedesktop.org/show_bug.cgi?id=34008 So please do not remove unless there are also no regressions on Evergreen. Thanks. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] r600g: remove useless call to u_upload_flush
I cannot test it now, but last time the flush call was removed it caused a major regression on Evergreen. See: https://bugs.freedesktop.org/show_bug.cgi?id=34008 So please do not remove unless there are also no regressions on Evergreen. Thanks.
[PATCH v5 0/2] Add drm_dp_aux chardev support.
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. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++ drivers/gpu/drm/drm_dp_helper.c | 7 + drivers/gpu/drm/i915/intel_dp.c | 14 +- include/drm/drm_dp_aux_dev.h | 50 ++ 6 files changed, 441 insertions(+), 12 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v5 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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 --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++ drivers/gpu/drm/drm_dp_helper.c | 7 + include/drm/drm_dp_aux_dev.h | 50 ++ 5 files changed, 439 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 e814517..e5bc0ca 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -28,6 +28,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 000..a2861e2 --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,373 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_dp_aux_dev { + unsigned index; + struct drm_dp_aux *aux; + struct device *dev; + struct kref refcount; + bool removed; + spinlock_t removed_lock; +}; + +#define DRM_AUX_MINORS 256 +#define AUX_MAX_OFFSET (1 << 20) +static DEFINE_IDR(aux_idr); +static DEFINE_SPINLOCK(aux_idr_lock); +static struct class *drm_dp_aux_dev_class; +static int drm_dev_major = -1; + +static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index) +{ + struct drm_dp_aux_dev *aux_dev = NULL; + + spin_lock(&aux_idr_lock); + aux_dev = idr_find(&aux_idr, index); + if (!kref_get_unless_zero(&aux_dev->refcount)) + aux_dev = NULL; + spin_unlock(&aux_idr_lock); + + return aux_dev; +} + +static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux) +{ + struct drm_dp_aux_dev *aux_dev; + int index; + + + aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL); + if (!aux_dev) + return ERR_PTR(-ENOMEM); + aux_dev->aux = aux; + aux_dev->removed = false; + spin_lock_init(&aux_dev->removed_lock); + kref_init(&aux_dev->refcount); + + idr_preload(GFP_KERNEL); + spin_lock(&aux_idr_lock); + index = idr_alloc_cycl
[PATCH v5 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/i915/intel_dp.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 18bcfbe..0acdf0f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; intel_dp->aux.name = name; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", name, connector->base.kdev->kobj.name); ret = drm_dp_aux_register(&intel_dp->aux); - if (ret < 0) { + if (ret < 0) DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n", name, ret); - return; - } - - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret); - drm_dp_aux_unregister(&intel_dp->aux); - } } static void intel_dp_connector_unregister(struct intel_connector *intel_connector) { - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); intel_connector_unregister(intel_connector); } -- 2.4.3
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
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 > > > > > --- > > > > > 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) > > > > > > > > > &g
[PATCH 0/4] enable migration of driver pages
On Mon, Jul 13, 2015 at 05:35:15PM +0900, Gioh Kim wrote: > From: Gioh Kim > > Hello, > > This series try to enable migration of non-LRU pages, such as driver's page. > > My ARM-based platform occured severe fragmentation problem after long-term > (several days) test. Sometimes even order-3 page allocation failed. It has > memory size 512MB ~ 1024MB. 30% ~ 40% memory is consumed for graphic > processing > and 20~30 memory is reserved for zram. > > I found that many pages of GPU driver and zram are non-movable pages. So I > reported Minchan Kim, the maintainer of zram, and he made the internal > compaction logic of zram. And I made the internal compaction of GPU driver. > > They reduced some fragmentation but they are not enough effective. > They are activated by its own interface, /sys, so they are not cooperative > with kernel compaction. If there is too much fragmentation and kernel starts > to compaction, zram and GPU driver cannot work with the kernel compaction. > > So I thought there needs a interface to combine driver and kernel compaction. > This patch adds a generic isolate/migrate/putback callbacks for page > address-space and a new interface to create anon-inode to manage > address_space_operation. The zram and GPU, and any other modules can create > anon_inode and register its own migration method. The kernel compaction can > call the registered migration when it does compaction. > > My GPU driver source is not in-kernel driver so that I apply the interface > into balloon driver. The balloon driver is already merged > into the kernel compaction as a corner-case. This patch have the balloon > driver migration be called by the generic interface. > > > This patch set combines 4 patches. > > 1. patch 1/4: get inode from anon_inodes > This patch adds new interface to create inode from anon_inodes. > > 2. patch 2/4: framework to isolate/migrate/putback page > Add isolatepage, putbackpage into address_space_operations > and wrapper function to call them. > > 3. patch 3/4: apply the framework into balloon driver > The balloon driver is applied into the framework. It gets a inode > from anon_inodes and register operations in the inode. > The kernel compaction calls generic interfaces, not balloon > driver interfaces. > Any other drivers can register operations via inode like this > to migrate it's pages. > > 4. patch 4/4: remove direct calling of migration of driver pages > Non-lru pages are also migrated with lru pages by move_to_new_page(). > > This patch set is tested: > - turn on Ubuntu 14.04 with 1G memory on qemu. > - do kernel building > - after several seconds check more than 512MB is used with free command > - command "balloon 512" in qemu monitor > - check hundreds MB of pages are migrated > > My thanks to Konstantin Khlebnikov for his reviews of the RFC patch set. > Most of the changes were based on his feedback. > > This patch-set is based on v4.1 > > > Gioh Kim (4): > fs/anon_inodes: new interface to create new inode > mm/compaction: enable mobile-page migration > mm/balloon: apply mobile page migratable into balloon > mm: remove direct calling of migration > > drivers/virtio/virtio_balloon.c| 3 ++ > fs/anon_inodes.c | 6 +++ > fs/proc/page.c | 3 ++ > include/linux/anon_inodes.h| 1 + > include/linux/balloon_compaction.h | 15 +-- > include/linux/compaction.h | 80 > ++ > include/linux/fs.h | 2 + > include/linux/page-flags.h | 19 > include/uapi/linux/kernel-page-flags.h | 1 + > mm/balloon_compaction.c| 72 ++ > mm/compaction.c| 8 ++-- > mm/migrate.c | 24 +++--- > 12 files changed, 160 insertions(+), 74 deletions(-) > > -- > 2.1.4 > Acked-by: Rafael Aquini
[PATCH v6 0/2] Add drm_dp_aux chardev support.
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. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device v6: - Use atomic_t for usage count - Use a mutex instead of spinlock for idr lock - Destroy chardev immediately on unregister - other minor suggestions from Ville Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + drivers/gpu/drm/i915/intel_dp.c | 19 +- include/drm/drm_dp_aux_dev.h | 50 + 6 files changed, 447 insertions(+), 17 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v6 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/i915/intel_dp.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8287df4..7aacc08 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; intel_dp->aux.name = name; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", name, connector->base.kdev->kobj.name); ret = drm_dp_aux_register(&intel_dp->aux); - if (ret < 0) { + if (ret < 0) DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n", name, ret); - return; - } - - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret); - drm_dp_aux_unregister(&intel_dp->aux); - } } static void intel_dp_connector_unregister(struct intel_connector *intel_connector) { - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); intel_connector_unregister(intel_connector); } -- 2.4.3
[PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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 --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + include/drm/drm_dp_aux_dev.h | 50 + 5 files changed, 445 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 c4bf9a1..daefcce 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 1e9ff4c..e48ec8f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -28,6 +28,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 000..16dbc2e --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,381 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_dp_aux_dev { + unsigned index; + struct drm_dp_aux *aux; + struct device *dev; + struct kref refcount; + atomic_t usecount; +}; + +#define DRM_AUX_MINORS 256 +#define AUX_MAX_OFFSET (1 << 20) +static DEFINE_IDR(aux_idr); +static DEFINE_MUTEX(aux_idr_mutex); +static struct class *drm_dp_aux_dev_class; +static int drm_dev_major = -1; + +static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index) +{ + struct drm_dp_aux_dev *aux_dev = NULL; + + mutex_lock(&aux_idr_mutex); + aux_dev = idr_find(&aux_idr, index); + if (!kref_get_unless_zero(&aux_dev->refcount)) + aux_dev = NULL; + mutex_unlock(&aux_idr_mutex); + + return aux_dev; +} + +static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux) +{ + struct drm_dp_aux_dev *aux_dev; + int index; + + + aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL); + if (!aux_dev) + return ERR_PTR(-ENOMEM); + aux_dev->aux = aux; + atomic_set(&aux_dev->usecount, 1); + kref_init(&aux_dev->refcount); + + mutex_lock(&aux_idr_mutex); + index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS, +GFP_KERNEL);
[PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Fri, Oct 30, 2015 at 12:04:17PM +0200, Ville Syrjälä wrote: > On Thu, Oct 29, 2015 at 04:23:45PM -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. > > Note that we (i915 folks at least) generally like to have the changelog > in the commit message itself. So maybe move it here for the final > version. > > Anyways, this is looking rather nice now. I spotted a few remaining style > issues, and some error handling problems. Once those are dealt with > I think we should be done. > > > > > Signed-off-by: Rafael Antognolli > > --- > > drivers/gpu/drm/Kconfig | 8 + > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/drm_dp_aux_dev.c | 381 > > +++ > > drivers/gpu/drm/drm_dp_helper.c | 5 + > > include/drm/drm_dp_aux_dev.h | 50 + > > 5 files changed, 445 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 c4bf9a1..daefcce 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 1e9ff4c..e48ec8f 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -28,6 +28,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 000..16dbc2e > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c > > @@ -0,0 +1,381 @@ > > +/* > > + * 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 > > + * &
[PATCH v7 0/2] Add drm_dp_aux chardev support.
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. Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 374 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + drivers/gpu/drm/i915/intel_dp.c | 19 +- include/drm/drm_dp_aux_dev.h | 50 ++ 6 files changed, 440 insertions(+), 17 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v7 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
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. 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. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device v6: - Use atomic_t for usage count - Use a mutex instead of spinlock for idr lock - Destroy chardev immediately on unregister - other minor suggestions from Ville v7: - style fixes - error handling fixes Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 374 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + include/drm/drm_dp_aux_dev.h | 50 ++ 5 files changed, 438 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 c4bf9a1..daefcce 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 1e9ff4c..e48ec8f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -28,6 +28,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 000..1a3ca39 --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,374 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_dp_aux_dev { + unsigned index; + struct drm_dp_aux *aux; + struct device *dev; + struct kref refcount; + atomic_t usecount; +}; + +#define DRM_AUX_MINORS 256 +#define AUX_MAX_OFFSET (1 << 20) +static DEFINE_IDR(aux_idr); +static DEFINE_MUT
[PATCH v7 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/i915/intel_dp.c | 19 ++- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8287df4..7aacc08 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10; intel_dp->aux.name = name; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", name, connector->base.kdev->kobj.name); ret = drm_dp_aux_register(&intel_dp->aux); - if (ret < 0) { + if (ret < 0) DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n", name, ret); - return; - } - - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name, ret); - drm_dp_aux_unregister(&intel_dp->aux); - } } static void intel_dp_connector_unregister(struct intel_connector *intel_connector) { - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); intel_connector_unregister(intel_connector); } -- 2.4.3
[RFC 11/13] drm/dp: Add helper to dump DPCD
Hi, I'm back from vacation, so I'll be looking at this again. On Thu, Aug 20, 2015 at 04:26:42PM -0700, Rafael Antognolli wrote: > On Mon, Aug 17, 2015 at 10:02:04AM +0300, Jani Nikula wrote: > > On Fri, 14 Aug 2015, Rafael Antognolli > > wrote: > > > On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote: > > >> On Wed, 12 Aug 2015, Thierry Reding wrote: > > >> > From: Thierry Reding > > >> > > > >> > The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a > > >> > seq_file and can be used to make the DPCD available via debugfs for > > >> > example. > > >> > > >> See i915/i915_debugfs.c for one DPCD dump implementation. > > >> > > >> Around the time that was added, there was also some discussion (and > > >> patches [1]) to expose a read/write debugfs interface to DPCD, letting > > >> userspace access arbitrary DPCD registers. > > >> > > >> Just this week there was some discussion about revisiting that. It was > > >> about accessing some proprietary panel features, but there's also the > > >> ease of debugging without having to keep updating the kernel to dump > > >> more. > > >> > > >> I think it would be great to agree on a common debugfs interface to > > >> access DPCD arbitrarily. Last time I checked, the blocker to that was > > >> access to the aux channel from generic code; it's always driver > > >> specific. SMOP. ;) > > > > > > Do you mean it would require the generic code/interface to somehow route > > > this to the driver specific code? I am not sure yet how this works (if > > > there's something like it around), but I'll take a look. > > > > Drivers can choose to support DP AUX any way they like, and they don't > > have to use the common helpers to do so. It's pretty much an > > implementation detail. I think we could require the use of the common > > helpers to support generic DPCD access from debugfs, but we'd still have > > to come up with a way to get hold of struct drm_dp_aux (again, driver > > internals) given a drm_connector in generic debugfs code. > > I was under the assumption they always used the helpers, but I > understand that's not always the case. > > Anyway, I was going to propose a new helper to "store" the drm_dp_aux > inside the drm_connector. And just expose something on debugfs if it was > used. Something like: > > int drm_dp_aux_store(struct drm_dp_aux *aux, > struct drm_connector *connector); > > The helpers don't seem to know about drm_connector's though, so I'm not sure > this is a good approach. > > > Thierry, do you see any problems with having a connector function to get > > hold of its drm_dp_aux? > > Would this be a new function pointer inside struct drm_connector_funcs? > If so, I'm fine with this approach too. > > Regarding the interface, you mentioned that you didn't like it because > it had a state. What about just dumping the content of the register into > dmesg when one tries to read it, and use a different sintaxe for > writing, passing both the register addr and the value? > > Daniel had another suggestion, regarding an ioctl in debugfs, but I'm > not sure I clearly understand that yet. Regarding the interface, this is the comment Daniel did a while ago: "As mentioned in another thread I think the right approach here is to expose the dp aux interface through some ioctls in debugfs or dev somewhere, and then add simple tools on top of that. Similar to what we have with i2c. That would allow us to implement additional things on top like mst inspection or using the i2c-over-dp-aux sidechannel." If exposing through an ioctl, shouldn't it be yet another DRM ioctl? And if not, then I should create another /dev node/file specifically for this purpose, right? And if we are going with ioctls, I still don't understand exactly how it relates to debugfs. Thanks, Rafael
[PATCH 0/3] RFC: Add a drm_aux-dev module.
This is a tentative implementation of a module that allows reading/writing arbitrary dpcd registers, following the suggestion from Daniel Vetter. It assumes the drm aux helpers were used by the driver. I tried to follow the i2c-dev implementation as close as possible, but the only operations that are provided on the dev node are two different ioctl's, one for reading a register and another one for writing it. I have at least 2 open questions: * Right now the AUX channels are stored in a global list inside the drm_dp_helper implementation, and I assume that's not ideal. A different approach would be to iterate over the list of connectors, instead of the list of AUX channels, but that would require the struct drm_connector or similar to know about the respective aux helper. It could be added as a function to register that, or as a new method on the drm_connector_funcs to retrieve the aux channel helper. * From the created sysfs drm_aux-dev device it's possible to know what is the respective connector, but not the other way around. Is this good enough? Please provide any feedback you have and tell me if this is the idea you had initially for this kind of implementation. Or, if it's nothing like this, let me know what else you had in mind. If I'm going in the right direction, I'll refine the patch to provide full documentation and tests if needed. Rafael Antognolli (3): drm/dp: Keep a list of drm_dp_aux helper. drm/dp: Store the drm_connector device pointer on the helper. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. Documentation/ioctl/ioctl-number.txt | 1 + drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_aux-dev.c| 390 +++ drivers/gpu/drm/drm_dp_helper.c | 71 +++ drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 7 + include/uapi/linux/Kbuild| 1 + include/uapi/linux/drm_aux-dev.h | 45 9 files changed, 521 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c create mode 100644 include/uapi/linux/drm_aux-dev.h -- 2.4.0
[PATCH 2/3] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel. Signed-off-by: Rafael Antognolli --- 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 f8f4d99..6f481fc 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1078,6 +1078,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 023620c..e481fbd 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.0
[PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.
This list will be used to get the aux channels registered through the helpers. Two functions are provided to register/unregister notifier listeners on the list, and another functiont to iterate over the list of aux channels. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/drm_dp_helper.c | 71 + include/drm/drm_dp_helper.h | 6 2 files changed, 77 insertions(+) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 291734e..01a1489 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = { .master_xfer = drm_dp_i2c_xfer, }; +struct drm_dp_aux_node { + struct klist_node list; + struct drm_dp_aux *aux; +}; + +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL); + +static BLOCKING_NOTIFIER_HEAD(aux_notifier); + +int drm_dp_aux_register_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&aux_notifier, nb); +} +EXPORT_SYMBOL(drm_dp_aux_register_notifier); + +int drm_dp_aux_unregister_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&aux_notifier, nb); +} +EXPORT_SYMBOL(drm_dp_aux_unregister_notifier); + +static struct drm_dp_aux *next_aux(struct klist_iter *i) +{ + struct klist_node *n = klist_next(i); + struct drm_dp_aux *aux = NULL; + struct drm_dp_aux_node *aux_node; + + if (n) { + aux_node = container_of(n, struct drm_dp_aux_node, list); + aux = aux_node->aux; + } + return aux; +} + +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *)) +{ + struct klist_iter i; + struct drm_dp_aux *aux; + int error = 0; + + klist_iter_init(&drm_dp_aux_list, &i); + while ((aux = next_aux(&i)) && !error) + error = fn(aux, data); + klist_iter_exit(&i); + return error; +} +EXPORT_SYMBOL(drm_dp_aux_for_each); + /** * drm_dp_aux_register() - initialise and register aux channel * @aux: DisplayPort AUX channel @@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = { */ int drm_dp_aux_register(struct drm_dp_aux *aux) { + struct drm_dp_aux_node *aux_node; mutex_init(&aux->hw_mutex); aux->ddc.algo = &drm_dp_i2c_algo; @@ -732,6 +781,14 @@ 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)); + /* add aux to list and notify listeners */ + aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL); + if (!aux_node) + return -ENOMEM; + aux_node->aux = aux; + klist_add_tail(&aux_node->list, &drm_dp_aux_list); + blocking_notifier_call_chain(&aux_notifier, DRM_DP_ADD_AUX, aux); + return i2c_add_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_register); @@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register); */ void drm_dp_aux_unregister(struct drm_dp_aux *aux) { + struct klist_iter i; + struct klist_node *n; + + klist_iter_init(&drm_dp_aux_list, &i); + while ((n = klist_next(&i))) { + struct drm_dp_aux_node *aux_node = + container_of(n, struct drm_dp_aux_node, list); + if (aux_node->aux == aux) { + klist_del(n); + kfree(aux_node); + break; + } + } + blocking_notifier_call_chain(&aux_notifier, DRM_DP_DEL_AUX, aux); i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 8c52d0ef1..023620c 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -763,7 +763,13 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link); int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link); int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link); +#define DRM_DP_ADD_AUX 0x01 +#define DRM_DP_DEL_AUX 0x02 + int drm_dp_aux_register(struct drm_dp_aux *aux); void drm_dp_aux_unregister(struct drm_dp_aux *aux); +int drm_dp_aux_register_notifier(struct notifier_block *nb); +int drm_dp_aux_unregister_notifier(struct notifier_block *nb); +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *)); #endif /* _DRM_DP_HELPER_H_ */ -- 2.4.0
[PATCH 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one dev node per DP AUX channel, named drm_aux-N. It's possible to know which connector owns this aux channel by looking at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/connector, if the connector device pointer was correctly set in the aux helper struct. Two main operations are provided on the registers through ioctls: read and write; and a helper struct is provided for that, used as: - address is the address of the register to read/write; - len is the number of bytes to read/write; - buf is a pointer to a buffer where to store the read data, or where the data to be written is already stored; - the return value is the number of bytes read/written, on success, or the error code otherwise. The number of read/written bytes is also stored on the len field of the struct. Signed-off-by: Rafael Antognolli --- Documentation/ioctl/ioctl-number.txt | 1 + drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_aux-dev.c| 390 +++ include/uapi/linux/Kbuild| 1 + include/uapi/linux/drm_aux-dev.h | 45 6 files changed, 442 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c create mode 100644 include/uapi/linux/drm_aux-dev.h diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 611c522..ea76980 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -93,6 +93,7 @@ Code Seq#(hex) Include FileComments ';'64-7F linux/vfio.h '@'00-0F linux/radeonfb.hconflict! '@'00-0F drivers/video/aty/aty128fb.cconflict! +'@'10-2F drm_aux-dev 'A'00-1F linux/apm_bios.hconflict! 'A'00-0F linux/agpgart.h conflict! and drivers/char/agp/compat_ioctl.h diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 1a0a8df..eae847c 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -25,6 +25,10 @@ config DRM_MIPI_DSI bool depends on DRM +config DRM_AUX_CHARDEV + tristate "DRM DP AUX Interface" + depends on DRM + config DRM_KMS_HELPER tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 45e7719..a1a94306 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src) obj-$(CONFIG_DRM) += drm.o obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o +obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o obj-$(CONFIG_DRM_TTM) += ttm/ obj-$(CONFIG_DRM_TDFX) += tdfx/ obj-$(CONFIG_DRM_R128) += r128/ diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c new file mode 100644 index 000..3ae9064 --- /dev/null +++ b/drivers/gpu/drm/drm_aux-dev.c @@ -0,0 +1,390 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_aux_dev { + struct list_head list; + unsigned index; + struct drm_dp_aux *aux; + struct device *dev; +}; + +#define DRM_AUX_MINORS 256 +static int drm_aux_dev_count = 0; +static LIST_HEAD(drm_aux_dev_list); +static DEFINE_SPINLOCK(drm_aux_dev_list_lock); + +static struct drm_aux_dev *drm_aux_dev_get_by_minor(unsigned index) +{ + struct drm_aux_dev *aux_dev; + + spin_lock(&drm_aux_dev_list_lock); + list_for_each_entry(aux_dev, &drm_aux_dev_list, list) { + if (
[PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.
On Tue, Sep 15, 2015 at 10:46:43AM +0300, Ville Syrjälä wrote: > On Mon, Sep 14, 2015 at 04:12:30PM -0700, Rafael Antognolli wrote: > > This list will be used to get the aux channels registered through the > > helpers. Two functions are provided to register/unregister notifier > > listeners on the list, and another functiont to iterate over the list of > > aux channels. > > > > Signed-off-by: Rafael Antognolli > > --- > > drivers/gpu/drm/drm_dp_helper.c | 71 > > + > > include/drm/drm_dp_helper.h | 6 > > 2 files changed, 77 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c > > b/drivers/gpu/drm/drm_dp_helper.c > > index 291734e..01a1489 100644 > > --- a/drivers/gpu/drm/drm_dp_helper.c > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = { > > .master_xfer = drm_dp_i2c_xfer, > > }; > > > > +struct drm_dp_aux_node { > > + struct klist_node list; > > + struct drm_dp_aux *aux; > > +}; > > + > > +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL); > > + > > +static BLOCKING_NOTIFIER_HEAD(aux_notifier); > > + > > +int drm_dp_aux_register_notifier(struct notifier_block *nb) > > +{ > > + return blocking_notifier_chain_register(&aux_notifier, nb); > > +} > > +EXPORT_SYMBOL(drm_dp_aux_register_notifier); > > Why is this notifier stuff needed? Why not just register/unregister the > aux-dev directly? > I am not sure it's needed, I was just looking for the best way of informing aux-dev that a new aux channel was added. By register/unregister the aux-dev directly, do you mean making this drm_dp_helper aware of the aux dev, when it's registered, and directly calling some callback to inform that new aux channels were added? > >+ > > +int drm_dp_aux_unregister_notifier(struct notifier_block *nb) > > +{ > > + return blocking_notifier_chain_unregister(&aux_notifier, nb); > > +} > > +EXPORT_SYMBOL(drm_dp_aux_unregister_notifier); > > + > > +static struct drm_dp_aux *next_aux(struct klist_iter *i) > > +{ > > + struct klist_node *n = klist_next(i); > > + struct drm_dp_aux *aux = NULL; > > + struct drm_dp_aux_node *aux_node; > > + > > + if (n) { > > + aux_node = container_of(n, struct drm_dp_aux_node, list); > > + aux = aux_node->aux; > > + } > > + return aux; > > +} > > + > > +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *)) > > +{ > > + struct klist_iter i; > > + struct drm_dp_aux *aux; > > + int error = 0; > > + > > + klist_iter_init(&drm_dp_aux_list, &i); > > + while ((aux = next_aux(&i)) && !error) > > + error = fn(aux, data); > > + klist_iter_exit(&i); > > + return error; > > +} > > +EXPORT_SYMBOL(drm_dp_aux_for_each); > > + > > /** > > * drm_dp_aux_register() - initialise and register aux channel > > * @aux: DisplayPort AUX channel > > @@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = { > > */ > > int drm_dp_aux_register(struct drm_dp_aux *aux) > > { > > + struct drm_dp_aux_node *aux_node; > > mutex_init(&aux->hw_mutex); > > > > aux->ddc.algo = &drm_dp_i2c_algo; > > @@ -732,6 +781,14 @@ 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)); > > > > + /* add aux to list and notify listeners */ > > + aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL); > > + if (!aux_node) > > + return -ENOMEM; > > + aux_node->aux = aux; > > + klist_add_tail(&aux_node->list, &drm_dp_aux_list); > > + blocking_notifier_call_chain(&aux_notifier, DRM_DP_ADD_AUX, aux); > > + > > return i2c_add_adapter(&aux->ddc); > > } > > EXPORT_SYMBOL(drm_dp_aux_register); > > @@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register); > > */ > > void drm_dp_aux_unregister(struct drm_dp_aux *aux) > > { > > + struct klist_iter i; > > + struct klist_node *n; > > + > > + klist_iter_init(&drm_dp_aux_list, &i); > > + while ((n = klist_next(&i))) { > > + struct drm_dp_aux_node *aux_node = > > + container_of(n, struct drm_dp_aux_node, list); > > +
[PATCH 0/3] RFC: Add a drm_aux-dev module.
On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote: > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote: > > This is a tentative implementation of a module that allows reading/writing > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It > > assumes the drm aux helpers were used by the driver. > > > > I tried to follow the i2c-dev implementation as close as possible, but the > > only > > operations that are provided on the dev node are two different ioctl's, one > > for > > reading a register and another one for writing it. > > Why would you use ioctls instead of normal read/write syscalls? > For writing, that should work fine, I can easily change that if you prefer. For reading, one has to first tell which register address is going to be read. So it would require to first write the address on the file, to then read. It was suggested that an ioctl to be used, and I understood it was to solve this, but maybe I'm wrong. I don't have any particular preference for the API, could easily change this code to use just read/writes. Thanks, Rafael > > > > I have at least 2 open questions: > > > > * Right now the AUX channels are stored in a global list inside the > >drm_dp_helper implementation, and I assume that's not ideal. A different > >approach would be to iterate over the list of connectors, instead of the > >list of AUX channels, but that would require the struct drm_connector or > >similar to know about the respective aux helper. It could be added as a > >function to register that, or as a new method on the drm_connector_funcs > > to > >retrieve the aux channel helper. > > > > * From the created sysfs drm_aux-dev device it's possible to know what is > > the > >respective connector, but not the other way around. Is this good enough? > > > > Please provide any feedback you have and tell me if this is the idea you had > > initially for this kind of implementation. Or, if it's nothing like this, > > let > > me know what else you had in mind. > > > > If I'm going in the right direction, I'll refine the patch to provide full > > documentation and tests if needed. > > > > Rafael Antognolli (3): > > drm/dp: Keep a list of drm_dp_aux helper. > > drm/dp: Store the drm_connector device pointer on the helper. > > drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. > > > > Documentation/ioctl/ioctl-number.txt | 1 + > > drivers/gpu/drm/Kconfig | 4 + > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/drm_aux-dev.c| 390 > > +++ > > drivers/gpu/drm/drm_dp_helper.c | 71 +++ > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > include/drm/drm_dp_helper.h | 7 + > > include/uapi/linux/Kbuild| 1 + > > include/uapi/linux/drm_aux-dev.h | 45 > > 9 files changed, 521 insertions(+) > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > create mode 100644 include/uapi/linux/drm_aux-dev.h > > > > -- > > 2.4.0 > > > > ___ > > dri-devel mailing list > > dri-devel at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC
[PATCH 0/3] RFC: Add a drm_aux-dev module.
On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote: > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote: > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote: > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote: > > > > > This is a tentative implementation of a module that allows > > > > > reading/writing > > > > > arbitrary dpcd registers, following the suggestion from Daniel > > > > > Vetter. It > > > > > assumes the drm aux helpers were used by the driver. > > > > > > > > > > I tried to follow the i2c-dev implementation as close as possible, > > > > > but the only > > > > > operations that are provided on the dev node are two different > > > > > ioctl's, one for > > > > > reading a register and another one for writing it. > > > > > > > > Why would you use ioctls instead of normal read/write syscalls? > > > > > > > > > > For writing, that should work fine, I can easily change that if you > > > prefer. > > > > > > For reading, one has to first tell which register address is going to be > > > read. > > > > seek() > > Sorry typo. Make that lseek(). > Oh, nice, I'll update the patch with this and remove the notifier stuff, thanks! > > > > > So it would require to first write the address on the file, to > > > then read. It was suggested that an ioctl to be used, and I understood > > > it was to solve this, but maybe I'm wrong. > > > > > > I don't have any particular preference for the API, could easily change > > > this code to use just read/writes. > > > > > > Thanks, > > > Rafael > > > > > > > > > > > > > I have at least 2 open questions: > > > > > > > > > > * Right now the AUX channels are stored in a global list inside the > > > > >drm_dp_helper implementation, and I assume that's not ideal. A > > > > > different > > > > >approach would be to iterate over the list of connectors, instead > > > > > of the > > > > >list of AUX channels, but that would require the struct > > > > > drm_connector or > > > > >similar to know about the respective aux helper. It could be added > > > > > as a > > > > >function to register that, or as a new method on the > > > > > drm_connector_funcs to > > > > >retrieve the aux channel helper. > > > > > > > > > > * From the created sysfs drm_aux-dev device it's possible to know > > > > > what is the > > > > >respective connector, but not the other way around. Is this good > > > > > enough? > > > > > > > > > > Please provide any feedback you have and tell me if this is the idea > > > > > you had > > > > > initially for this kind of implementation. Or, if it's nothing like > > > > > this, let > > > > > me know what else you had in mind. > > > > > > > > > > If I'm going in the right direction, I'll refine the patch to provide > > > > > full > > > > > documentation and tests if needed. > > > > > > > > > > Rafael Antognolli (3): > > > > > drm/dp: Keep a list of drm_dp_aux helper. > > > > > drm/dp: Store the drm_connector device pointer on the helper. > > > > > drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. > > > > > > > > > > Documentation/ioctl/ioctl-number.txt | 1 + > > > > > drivers/gpu/drm/Kconfig | 4 + > > > > > drivers/gpu/drm/Makefile | 1 + > > > > > drivers/gpu/drm/drm_aux-dev.c| 390 > > > > > +++ > > > > > drivers/gpu/drm/drm_dp_helper.c | 71 +++ > > > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > > > include/drm/drm_dp_helper.h | 7 + > > > > > include/uapi/linux/Kbuild| 1 + > > > > > include/uapi/linux/drm_aux-dev.h | 45 > > > > > 9 files changed, 521 insertions(+) > > > > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > > > > create mode 100644 include/uapi/linux/drm_aux-dev.h > > > > > > > > > > -- > > > > > 2.4.0 > > > > > > > > > > ___ > > > > > dri-devel mailing list > > > > > dri-devel at lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel OTC > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC
[PATCH RFC v2 0/3] Add a drm_aux-dev module.
Second attempt at implementing a module that allows reading/writing arbitrary dpcd registers. Changes to this version: - 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. One thing to notice is that I am not updating the file offset during read or write, which is kind of breaking the filesystem abstraction. But i2c-dev doesn't do it either, so I assumed it's fine. Rafael Antognolli (3): drm/dp: Keep a list of drm_dp_aux helper. 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 | 4 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/drm_aux-dev.c | 326 drivers/gpu/drm/drm_dp_helper.c | 73 + drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 6 + 6 files changed, 411 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c -- 2.4.3
[PATCH RFC v2 1/3] drm/dp: Keep a list of drm_dp_aux helper.
This list will be used to get the aux channels registered through the helpers. One function is provided to set a callback for added/removed aux channels, and another function to iterate over the list of aux channels. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/drm_dp_helper.c | 73 + include/drm/drm_dp_helper.h | 5 +++ 2 files changed, 78 insertions(+) diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 9535c5b..41bdd93 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -746,6 +746,56 @@ static const struct i2c_algorithm drm_dp_i2c_algo = { .master_xfer = drm_dp_i2c_xfer, }; +struct drm_dp_aux_node { + struct klist_node list; + struct drm_dp_aux *aux; +}; + +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL); + +static int (*aux_dev_cb)(int action, struct drm_dp_aux *aux) = NULL; + +void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *)) +{ + aux_dev_cb = fn; +} +EXPORT_SYMBOL(drm_dp_aux_set_aux_dev); + +static struct drm_dp_aux *next_aux(struct klist_iter *i) +{ + struct klist_node *n = klist_next(i); + struct drm_dp_aux *aux = NULL; + struct drm_dp_aux_node *aux_node; + + if (n) { + aux_node = container_of(n, struct drm_dp_aux_node, list); + aux = aux_node->aux; + } + return aux; +} + +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *)) +{ + struct klist_iter i; + struct drm_dp_aux *aux; + int error = 0; + + klist_iter_init(&drm_dp_aux_list, &i); + while ((aux = next_aux(&i)) && !error) + error = fn(aux, data); + klist_iter_exit(&i); + return error; +} +EXPORT_SYMBOL(drm_dp_aux_for_each); + +static int drm_dp_aux_dev_inform(int action, struct drm_dp_aux *aux) +{ + if (aux_dev_cb) { + return aux_dev_cb(action, aux); + } + return 0; +} + /** * drm_dp_aux_register() - initialise and register aux channel * @aux: DisplayPort AUX channel @@ -754,6 +804,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = { */ int drm_dp_aux_register(struct drm_dp_aux *aux) { + struct drm_dp_aux_node *aux_node; mutex_init(&aux->hw_mutex); aux->ddc.algo = &drm_dp_i2c_algo; @@ -768,6 +819,14 @@ 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)); + /* add aux to list and notify listeners */ + aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL); + if (!aux_node) + return -ENOMEM; + aux_node->aux = aux; + klist_add_tail(&aux_node->list, &drm_dp_aux_list); + drm_dp_aux_dev_inform(DRM_DP_ADD_AUX, aux); + return i2c_add_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_register); @@ -778,6 +837,20 @@ EXPORT_SYMBOL(drm_dp_aux_register); */ void drm_dp_aux_unregister(struct drm_dp_aux *aux) { + struct klist_iter i; + struct klist_node *n; + + klist_iter_init(&drm_dp_aux_list, &i); + while ((n = klist_next(&i))) { + struct drm_dp_aux_node *aux_node = + container_of(n, struct drm_dp_aux_node, list); + if (aux_node->aux == aux) { + klist_del(n); + kfree(aux_node); + break; + } + } + drm_dp_aux_dev_inform(DRM_DP_DEL_AUX, aux); i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister); diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 9ec4716..f92de26 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -763,7 +763,12 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link); int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link); int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link); +#define DRM_DP_ADD_AUX 0x01 +#define DRM_DP_DEL_AUX 0x02 + int drm_dp_aux_register(struct drm_dp_aux *aux); void drm_dp_aux_unregister(struct drm_dp_aux *aux); +void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *)); +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *)); #endif /* _DRM_DP_HELPER_H_ */ -- 2.4.3
[PATCH RFC v2 2/3] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel. Signed-off-by: Rafael Antognolli --- 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 a687250..23addc3 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 f92de26..d762535 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
[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one dev node per DP AUX channel, named drm_aux-N. It's possible to know which connector owns this aux channel by looking at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/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. Reading or writing does not update the offset of the file. Signed-off-by: Rafael Antognolli --- drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_aux-dev.c | 326 ++ 3 files changed, 331 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 1a0a8df..eae847c 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -25,6 +25,10 @@ config DRM_MIPI_DSI bool depends on DRM +config DRM_AUX_CHARDEV + tristate "DRM DP AUX Interface" + depends on DRM + config DRM_KMS_HELPER tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 45e7719..a1a94306 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src) obj-$(CONFIG_DRM) += drm.o obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o +obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o obj-$(CONFIG_DRM_TTM) += ttm/ obj-$(CONFIG_DRM_TDFX) += tdfx/ obj-$(CONFIG_DRM_R128) += r128/ diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c new file mode 100644 index 000..fcc334a --- /dev/null +++ b/drivers/gpu/drm/drm_aux-dev.c @@ -0,0 +1,326 @@ +/* + * 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 + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct drm_aux_dev { + struct list_head list; + unsigned index; + struct drm_dp_aux *aux; + struct device *dev; +}; + +#define DRM_AUX_MINORS 256 +static int drm_aux_dev_count = 0; +static LIST_HEAD(drm_aux_dev_list); +static DEFINE_SPINLOCK(drm_aux_dev_list_lock); + +static struct drm_aux_dev *drm_aux_dev_get_by_minor(unsigned index) +{ + struct drm_aux_dev *aux_dev; + + spin_lock(&drm_aux_dev_list_lock); + list_for_each_entry(aux_dev, &drm_aux_dev_list, list) { + if (aux_dev->index == index) + goto found; + } + + aux_dev = NULL; +found: + spin_unlock(&drm_aux_dev_list_lock); + return aux_dev; +} + +static struct drm_aux_dev *drm_aux_dev_get_by_aux(struct drm_dp_aux *aux) +{ + struct drm_aux_dev *aux_dev; + + spin_lock(&drm_aux_dev_list_lock); + list_for_each_entry(aux_dev, &drm_aux_dev_list, list) { + if (aux_dev->aux == aux) + goto found; + } + + aux_dev = NULL; +found: + spin_unlock(&drm_aux_dev_list_lock); + return aux_dev; +} + +static struct drm_aux_dev *get_free_drm_aux_dev(struct drm_dp_aux *aux) +{ + struct drm_aux_dev *aux_dev; + int index; + + spin_lock(&drm_aux_dev_list_lock); + index = drm_aux_dev_count; + spin_unlock(&drm_aux_dev_list_lock); + if (index >= DRM_AUX_MINORS) { + printk(KERN_ERR "i2c-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_d
[PATCH 0/3] RFC: Add a drm_aux-dev module.
On Wed, Sep 16, 2015 at 11:47:21PM +0300, Ville Syrjälä wrote: > On Wed, Sep 16, 2015 at 01:09:54PM -0700, Daniel Vetter wrote: > > On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote: > > > On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote: > > > > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote: > > > > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote: > > > > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote: > > > > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote: > > > > > > > > This is a tentative implementation of a module that allows > > > > > > > > reading/writing > > > > > > > > arbitrary dpcd registers, following the suggestion from Daniel > > > > > > > > Vetter. It > > > > > > > > assumes the drm aux helpers were used by the driver. > > > > > > > > > > > > > > > > I tried to follow the i2c-dev implementation as close as > > > > > > > > possible, but the only > > > > > > > > operations that are provided on the dev node are two different > > > > > > > > ioctl's, one for > > > > > > > > reading a register and another one for writing it. > > > > > > > > > > > > > > Why would you use ioctls instead of normal read/write syscalls? > > > > > > > > > > > > > > > > > > > For writing, that should work fine, I can easily change that if you > > > > > > prefer. > > > > > > > > > > > > For reading, one has to first tell which register address is going > > > > > > to be > > > > > > read. > > > > > > > > > > seek() > > > > > > > > Sorry typo. Make that lseek(). > > > > > > > Oh, nice, I'll update the patch with this and remove the notifier stuff, > > > thanks! > > > > Well there's also other modes like i2c over aux, and that would be easier > > to support with an ioctl in a uniform way. So not sure how much value > > there is in reusing read/write for i2c ... > > We already have i2c-dev for i2c. So not sure what you're after here. > > i2c is a bit of a mess on the protocol level. Lots of devices do > interesting things with it, so it can't really make do with just > read/write/lseek. Apart from i2c-over-aux, the rest is all about > DPCD access, and for that read/write/lseek is all you ever need. I just noticed that I forgot to cc you guys, but yesterday I sent an updated version of the patch set to this list: http://lists.freedesktop.org/archives/dri-devel/2015-September/090366.html I also don't have a strong opinion about ioctl vs read/write/lseek, but at least my second implementation did get a little cleaner. Thanks, Rafael
Re: [PATCH 2/2] drm/bridge: lt8912b: Add support for audio
On 03/02/2025 16:23, raf...@beims.me wrote: From: Rafael Beims Add support for HDMI codec with audio coming from the I2S input. Support 48kHz and 96kHz sample rate, with 16 bits word size. Co-developed-by: João Paulo Gonçalves Signed-off-by: João Paulo Gonçalves Signed-off-by: Rafael Beims --- drivers/gpu/drm/bridge/Kconfig | 1 + drivers/gpu/drm/bridge/lontium-lt8912b.c | 107 ++- 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 6b4664d91faa..489ce1041203 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -117,6 +117,7 @@ config DRM_ITE_IT6505 config DRM_LONTIUM_LT8912B tristate "Lontium LT8912B DSI/HDMI bridge" + select SND_SOC_HDMI_CODEC if SND_SOC depends on OF select DRM_PANEL_BRIDGE select DRM_KMS_HELPER diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c index 52da204f5740..2100b41e5f61 100644 --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include @@ -16,6 +18,8 @@ #include #include +#include + #include #define I2C_MAIN 0 @@ -24,7 +28,10 @@ #define I2C_CEC_DSI 1 #define I2C_ADDR_CEC_DSI 0x49 -#define I2C_MAX_IDX 2 +#define I2C_AUDIO 2 +#define I2C_ADDR_AUDIO 0x4a + +#define I2C_MAX_IDX 3 struct lt8912 { struct device *dev; @@ -38,6 +45,7 @@ struct lt8912 { struct drm_bridge *hdmi_port; struct mipi_dsi_device *dsi; + struct platform_device *audio_pdev; struct gpio_desc *gp_reset; @@ -226,6 +234,7 @@ static int lt8912_init_i2c(struct lt8912 *lt, struct i2c_client *client) struct i2c_board_info info[] = { { I2C_BOARD_INFO("lt8912p0", I2C_ADDR_MAIN), }, { I2C_BOARD_INFO("lt8912p1", I2C_ADDR_CEC_DSI), }, + { I2C_BOARD_INFO("lt8912p2", I2C_ADDR_AUDIO), }, }; if (!lt) @@ -754,6 +763,97 @@ static int lt8912_put_dt(struct lt8912 *lt) return 0; } +static int lt8912_hdmi_hw_params(struct device *dev, void *data, +struct hdmi_codec_daifmt *fmt, +struct hdmi_codec_params *hparms) +{ + struct lt8912 *lt = data; + unsigned int audio_params = 0x08; /* 16 bit word size */ + + if (hparms->sample_width != 16) + return -EINVAL; + + if (hparms->sample_rate == 48000) + audio_params |= 0x20; + else if (hparms->sample_rate == 96000) + audio_params |= 0xa0; + else + return -EINVAL; + + regmap_write(lt->regmap[I2C_AUDIO], 0x0f, audio_params); + regmap_write(lt->regmap[I2C_AUDIO], 0x35, 0x00); + regmap_write(lt->regmap[I2C_AUDIO], 0x36, 0x18); + regmap_write(lt->regmap[I2C_AUDIO], 0x37, 0x00); + + return 0; +} + +static int lt8912_audio_startup(struct device *dev, void *data) +{ + struct lt8912 *lt = data; + + regmap_write(lt->regmap[I2C_AUDIO], 0x34, 0xe2); + regmap_write(lt->regmap[I2C_AUDIO], 0x3c, 0x60); + regmap_write(lt->regmap[I2C_AUDIO], 0x07, 0xf0); + regmap_write(lt->regmap[I2C_AUDIO], 0x06, 0x08); + + return 0; +} + +static void lt8912_audio_shutdown(struct device *dev, void *data) +{ + struct lt8912 *lt = data; + + regmap_write(lt->regmap[I2C_AUDIO], 0x06, 0x00); + regmap_write(lt->regmap[I2C_AUDIO], 0x07, 0x00); + regmap_write(lt->regmap[I2C_AUDIO], 0x34, 0x52); + regmap_write(lt->regmap[I2C_AUDIO], 0x3c, 0x40); +} + +static int lt8912_hdmi_i2s_get_dai_id(struct snd_soc_component *component, + struct device_node *endpoint) +{ + struct of_endpoint of_ep; + int ret; + + ret = of_graph_parse_endpoint(endpoint, &of_ep); + if (ret < 0) + return ret; + + if (of_ep.port != 2) + return -EINVAL; + + return 0; +} + +static const struct hdmi_codec_ops lt8912_codec_ops = { + .hw_params = lt8912_hdmi_hw_params, + .audio_shutdown = lt8912_audio_shutdown, + .audio_startup = lt8912_audio_startup, + .get_dai_id = lt8912_hdmi_i2s_get_dai_id, +}; + +static int lt8912_audio_init(struct device *dev, struct lt8912 *lt) +{ + struct hdmi_codec_pdata codec_data = { + .ops = <8912_codec_ops, + .max_i2s_channels = 2, + .i2s = 1, + .data = lt, + }; + + lt->audio_pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME, + PLATFORM_DEVID_AUTO, + &
Re: NVIDIA GPU fallen off the bus after exiting s2idle
On Tue, May 4, 2021 at 10:08 AM Chris Chiu wrote: > > Hi, > We have some Intel laptops (11th generation CPU) with NVIDIA GPU > suffering the same GPU falling off the bus problem while exiting > s2idle with external display connected. These laptops connect the > external display via the HDMI/DisplayPort on a USB Type-C interfaced > dock. If we enter and exit s2idle with the dock connected, the NVIDIA > GPU (confirmed on 10de:24b6 and 10de:25b8) and the PCIe port can come > back to D0 w/o problem. If we enter the s2idle, disconnect the dock, > then exit the s2idle, both external display and the panel will remain > with no output. The dmesg as follows shows the "nvidia :01:00.0: > can't change power state from D3cold to D0 (config space > inaccessible)" due to the following ACPI error > [ 154.446781] > [ 154.446783] > [ 154.446783] Initialized Local Variables for Method [IPCS]: > [ 154.446784] Local0: 9863e365 Integer 09C5 > [ 154.446790] > [ 154.446791] Initialized Arguments for Method [IPCS]: (7 arguments > defined for method invocation) > [ 154.446792] Arg0: 25568fbd Integer 00AC > [ 154.446795] Arg1: 9ef30e76 Integer > [ 154.446798] Arg2: fdf820f0 Integer 0010 > [ 154.446801] Arg3: 9fc2a088 Integer 0001 > [ 154.446804] Arg4: 3a3418f7 Integer 0001 > [ 154.446807] Arg5: 20c4b87c Integer > [ 154.446810] Arg6: 8b965a8a Integer > [ 154.446813] > [ 154.446815] ACPI Error: Aborting method \IPCS due to previous error > (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446824] ACPI Error: Aborting method \MCUI due to previous error > (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446829] ACPI Error: Aborting method \SPCX due to previous error > (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446835] ACPI Error: Aborting method \_SB.PC00.PGSC due to > previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446841] ACPI Error: Aborting method \_SB.PC00.PGON due to > previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446846] ACPI Error: Aborting method \_SB.PC00.PEG1.NPON due to > previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446852] ACPI Error: Aborting method \_SB.PC00.PEG1.PG01._ON due > to previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529) > [ 154.446860] acpi device:02: Failed to change power state to D0 > [ 154.690760] video LNXVIDEO:00: Cannot transition to power state D0 > for parent in (unknown) If I were to guess, I would say that AML tries to access memory that is not accessible while suspended, probably PCI config space. > The IPCS is the last function called from \_SB.PC00.PEG1.PG01._ON > which we expect it to prepare everything before bringing back the > NVIDIA GPU but it's stuck in the infinite loop as described below. > Please refer to > https://gist.github.com/mschiu77/fa4f5a97297749d0d66fe60c1d421c44 for > the full DSDT.dsl. The DSDT alone may not be sufficient. Can you please create a bug entry at bugzilla.kernel.org for this issue and attach the full output of acpidump from one of the affected machines to it? And please let me know the number of the bug. Also please attach the output of dmesg including a suspend-resume cycle including dock disconnection while suspended and the ACPI messages quoted below. >While (One) > { > If ((!IBSY || (IERR == One))) > { > Break > } > > If ((Local0 > TMOV)) > { > RPKG [Zero] = 0x03 > Return (RPKG) /* \IPCS.RPKG */ > } > > Sleep (One) > Local0++ > } > > And the upstream PCIe port of NVIDIA seems to become inaccessible due > to the messages as follows. > [ 292.746508] pcieport :00:01.0: waiting 100 ms for downstream > link, after activation > [ 292.882296] pci :01:00.0: waiting additional 100 ms to become accessible > [ 316.876997] pci :01:00.0: can't change power state from D3cold > to D0 (config space inaccessible) > > Since the IPCS is the Intel Reference Code and we don't really know > why the never-end loop happens just because we unplug the dock while > the system still stays in s2idle. Can anyone from Intel suggest what > happens here? This list is not the right channel for inquiries related to Intel support, we can only help you as Linux kernel developers in this venue. > And one thing also worth mentioning, if we unplug the display cable > from the dock before entering the s2idle, NVIDIA GPU can come back w/o > problem even if we disconnect the dock before exiting s2idle. Here's > the lspci information > https://gist.github.com/mschiu77/0bfc439d15d52d20de0129b1b2a86dc4 and > the dmesg log with ACPI trace_state enabled and dynamic debug on for > drivers/p
Re: [PATCH] component: Move host device to end of device lists on binding
On Sat, May 8, 2021 at 9:41 AM Stephen Boyd wrote: > > The device lists are poorly ordered when the component device code is > used. This is because component_master_add_with_match() returns 0 > regardless of component devices calling component_add() first. It can > really only fail if an allocation fails, in which case everything is > going bad and we're out of memory. The host device (called master_dev in > the code), can succeed at probe and be put on the device lists before > any of the component devices are probed and put on the lists. > > Within the component device framework this usually isn't that bad > because the real driver work is done at bind time via > component{,master}_ops::bind(). It becomes a problem when the driver > core, or host driver, wants to operate on the component device outside > of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The > driver core doesn't understand the relationship between the host device > and the component devices and could possibly try to operate on component > devices when they're already removed from the system or shut down. > > Normally, device links or probe defer would reorder the lists and put > devices that depend on other devices in the lists at the correct > location, but with component devices this doesn't happen because this > information isn't expressed anywhere. Drivers simply succeed at > registering their component or host with the component framework and > wait for their bind() callback to be called once the other components > are ready. We could make various device links between 'master_dev' and > 'component->dev' but it's not necessary. Let's simply move the hosting > device to the end of the device lists when the component device fully > binds. This way we know that all components are present and have probed > properly and now the host device has really probed so it's safe to > assume the host driver ops can operate on any component device. Moving a device to the end of dpm_list is generally risky in cases when some dependency information may be missing. For example, if there is a device depending on the hosting one, but that dependency is not represented by a device link or a direct ancestor-descendant relationship (or generally a path in the device dependency graph leading from one of them to the other), then moving it to the end of dpm_list would cause system-wide suspend to fail (the hosting device would be suspended before the one depending on it). That may not be a concern here, but at least it would be good to document why it is not a concern.
Re: [PATCH] component: Move host device to end of device lists on binding
On Mon, May 10, 2021 at 9:08 PM Stephen Boyd wrote: [cut] > > > > > > I will try it, but then I wonder about things like system wide > > > suspend/resume too. The drm encoder chain would need to reimplement the > > > logic for system wide suspend/resume so that any PM ops attached to the > > > msm device run in the correct order. Right now the bridge PM ops will > > > run, the i2c bus PM ops will run, and then the msm PM ops will run. > > > After this change, the msm PM ops will run, the bridge PM ops will run, > > > and then the i2c bus PM ops will run. It feels like that could be a > > > problem if we're suspending the DSI encoder while the bridge is still > > > active. > > > > Yup suspend/resume has the exact same problem as shutdown. > > I think suspend/resume has the exact opposite problem. At least I think > the correct order is to suspend the bridge, then the encoder, i.e. DSI, > like is happening today. It looks like drm_atomic_helper_shutdown() > operates from the top down when we want bottom up? I admit I have no > idea what is supposed to happen here. Why would the system-wide suspend ordering be different from the shutdown ordering?
Re: [PATCH] component: Move host device to end of device lists on binding
On Tue, May 11, 2021 at 7:00 PM Stephen Boyd wrote: > > Quoting Rafael J. Wysocki (2021-05-11 03:52:06) > > On Mon, May 10, 2021 at 9:08 PM Stephen Boyd wrote: > > > > [cut] > > > > > > > > > > > > > > I will try it, but then I wonder about things like system wide > > > > > suspend/resume too. The drm encoder chain would need to reimplement > > > > > the > > > > > logic for system wide suspend/resume so that any PM ops attached to > > > > > the > > > > > msm device run in the correct order. Right now the bridge PM ops will > > > > > run, the i2c bus PM ops will run, and then the msm PM ops will run. > > > > > After this change, the msm PM ops will run, the bridge PM ops will > > > > > run, > > > > > and then the i2c bus PM ops will run. It feels like that could be a > > > > > problem if we're suspending the DSI encoder while the bridge is still > > > > > active. > > > > > > > > Yup suspend/resume has the exact same problem as shutdown. > > > > > > I think suspend/resume has the exact opposite problem. At least I think > > > the correct order is to suspend the bridge, then the encoder, i.e. DSI, > > > like is happening today. It looks like drm_atomic_helper_shutdown() > > > operates from the top down when we want bottom up? I admit I have no > > > idea what is supposed to happen here. > > > > Why would the system-wide suspend ordering be different from the > > shutdown ordering? > > I don't really know. I'm mostly noting that today the order of suspend > is to suspend the bridge device first and then the aggregate device. If > the suspend of the aggregate device is traversing the devices like > drm_atomic_helper_shutdown() then it would operate on the bridge device > after it has been suspended, like is happening during shutdown. But it > looks like that isn't happening. At least for the msm driver we're > suspending the aggregate device after the bridge, and there are some > weird usages of prepare and complete in there (see msm_pm_prepare() and > msm_pm_complete) which makes me think that it's all working around this > component code. Well, it looks like the "prepare" phase is used sort-of against the rules (because "prepare" is not supposed to make changes to the hardware configuration or at least that is not its role) in order to work around an ordering issue that is present in shutdown which doesn't have a "prepare" phase. > The prepare phase is going to suspend the display pipeline, and then the > bridge device will run its suspend hooks, and then the aggregate driver > will run its suspend hooks. If we had a proper device for the aggregate > device instead of the bind/unbind component hooks we could clean this > up. I'm not sufficiently familiar with the component code to add anything constructive here, but generally speaking it looks like the "natural" dpm_list ordering does not match the order in which the devices in question should be suspended (or shut down for that matter), so indeed it is necessary to reorder dpm_list this way or another. Please also note that it generally may not be sufficient to reorder dpm_list if the devices are suspended and resumed asynchronously during system-wide transitions, because in that case the callbacks of different devices are only started in the dpm_list order, but they may be completed in a different order (and of course they may run in parallel with each other). Shutdown is simpler, because it runs the callback synchronously for all devices IIRC.
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley wrote: > > On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote: > > On Sun, Nov 22, 2020 at 11:36 PM James Bottomley > > wrote: [cut] > > > > Maintainers routinely review 1-line trivial patches, not to mention > > internal API changes, etc. > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 Right. > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. > > > If some company does not want to pay for that, that's fine, but they > > don't get to be maintainers and claim `Supported`. > > What I'm actually trying to articulate is a way of measuring value of > the patch vs cost ... it has nothing really to do with who foots the > actual bill. > > One thesis I'm actually starting to formulate is that this continual > devaluing of maintainers is why we have so much difficulty keeping and > recruiting them. Absolutely. This is just one of the factors involved, but a significant one IMV. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4] Add power/gpu_frequency tracepoint.
On 11/16/2020 10:05 PM, Steven Rostedt wrote: On Mon, 16 Nov 2020 12:55:29 -0800 Peiyong Lin wrote: Hi there, May I ask whether the merge window has passed? If so is it possible to ask for a review? This is up to the maintainers of power management to accept this. Rafael? I'd say up to the GPU people rather (dri-devel CCed) since that's where it is going to be used. Also it would be good to see at least one in-the-tree user of this (or a usage example at least). Cheers! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 2/7] nouveau: ACPI: Use the ACPI_COMPANION() macro directly
From: Rafael J. Wysocki The ACPI_HANDLE() macro is a wrapper arond the ACPI_COMPANION() macro and the ACPI handle produced by the former comes from the ACPI device object produced by the latter, so it is way more straightforward to evaluate the latter directly instead of passing the handle produced by the former to acpi_bus_get_device(). Modify nouveau_acpi_edid() accordingly (no intentional functional impact). Signed-off-by: Rafael J. Wysocki --- drivers/gpu/drm/nouveau/nouveau_acpi.c |9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) Index: linux-pm/drivers/gpu/drm/nouveau/nouveau_acpi.c === --- linux-pm.orig/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ linux-pm/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -364,7 +364,6 @@ void * nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) { struct acpi_device *acpidev; - acpi_handle handle; int type, ret; void *edid; @@ -377,12 +376,8 @@ nouveau_acpi_edid(struct drm_device *dev return NULL; } - handle = ACPI_HANDLE(dev->dev); - if (!handle) - return NULL; - - ret = acpi_bus_get_device(handle, &acpidev); - if (ret) + acpidev = ACPI_COMPANION(dev->dev); + if (!acpidev) return NULL; ret = acpi_video_get_edid(acpidev, type, -1, &edid);
[PATCH v2 2/7] nouveau: ACPI: Use the ACPI_COMPANION() macro directly
From: Rafael J. Wysocki The ACPI_HANDLE() macro is a wrapper arond the ACPI_COMPANION() macro and the ACPI handle produced by the former comes from the ACPI device object produced by the latter, so it is way more straightforward to evaluate the latter directly instead of passing the handle produced by the former to acpi_bus_get_device(). Modify nouveau_acpi_edid() accordingly (no intentional functional impact). Signed-off-by: Rafael J. Wysocki Reviewed-by: Ben Skeggs --- v1 -> v2: * Resend with a different From and S-o-b address and with R-by from Ben. No other changes. --- drivers/gpu/drm/nouveau/nouveau_acpi.c |9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) Index: linux-pm/drivers/gpu/drm/nouveau/nouveau_acpi.c === --- linux-pm.orig/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ linux-pm/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -364,7 +364,6 @@ void * nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) { struct acpi_device *acpidev; - acpi_handle handle; int type, ret; void *edid; @@ -377,12 +376,8 @@ nouveau_acpi_edid(struct drm_device *dev return NULL; } - handle = ACPI_HANDLE(dev->dev); - if (!handle) - return NULL; - - ret = acpi_bus_get_device(handle, &acpidev); - if (ret) + acpidev = ACPI_COMPANION(dev->dev); + if (!acpidev) return NULL; ret = acpi_video_get_edid(acpidev, type, -1, &edid);
Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)
On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede wrote: > > Hi All, > > On 1/23/22 10:10, Tong Zhang wrote: > > when acpi=off is provided in bootarg, kernel crash with > > > > [1.252739] BUG: kernel NULL pointer dereference, address: > > 0018 > > [1.258308] Call Trace: > > [1.258490] ? acpi_walk_namespace+0x147/0x147 > > [1.258770] acpi_get_devices+0xe4/0x137 > > [1.258921] ? drm_core_init+0xc0/0xc0 [drm] > > [1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm] > > [1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm] > > > > The reason is that acpi_walk_namespace expects acpi related stuff > > initialized but in fact it wouldn't when acpi is set to off. In this case > > we should honor acpi=off in detect_thinkpad_privacy_screen(). > > > > Signed-off-by: Tong Zhang > > Thank you for catching this and thank you for your patch. I was about to merge > this, but then I realized that this might not be the best way to fix this. > > A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi, > and at a first glance about half of those are missing an acpi_disabled > check. IMHO it would be better to simply add an acpi_disabled check to > acpi_get_devices() itself. > > Rafael, do you agree ? Yes, I do. > Note the just added chrome privacy-screen check uses > acpi_dev_present(), this is also used in about 10 places outside > of drivers/acpi and AFAIK none of those do an acpi_disabled check. > > acpi_dev_present() uses bus_find_device(&acpi_bus_type, ...) > but the acpi_bus_type does not get registered when acpi_disabled > is set. In the end this is fine though since bus_find_device > checks for the bus not being registered and then just returns > NULL. Right. > > --- > > v2: fix typo in previous commit -- my keyboard is eating letters > > > > drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c > > b/drivers/gpu/drm/drm_privacy_screen_x86.c > > index a2cafb294ca6..e7aa74ad0b24 100644 > > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c > > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c > > @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void) > > unsigned long long output; > > acpi_status status; > > > > + if (acpi_disabled) > > + return false; > > + > > /* Get embedded-controller handle */ > > status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, > > &ec_handle); > > if (ACPI_FAILURE(status) || !ec_handle) >
Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)
On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede wrote: > > Hi, > > On 1/26/22 16:54, Rafael J. Wysocki wrote: > > On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede wrote: > >> > >> Hi All, > >> > >> On 1/23/22 10:10, Tong Zhang wrote: > >>> when acpi=off is provided in bootarg, kernel crash with > >>> > >>> [1.252739] BUG: kernel NULL pointer dereference, address: > >>> 0018 > >>> [1.258308] Call Trace: > >>> [1.258490] ? acpi_walk_namespace+0x147/0x147 > >>> [1.258770] acpi_get_devices+0xe4/0x137 > >>> [1.258921] ? drm_core_init+0xc0/0xc0 [drm] > >>> [1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm] > >>> [1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm] > >>> > >>> The reason is that acpi_walk_namespace expects acpi related stuff > >>> initialized but in fact it wouldn't when acpi is set to off. In this case > >>> we should honor acpi=off in detect_thinkpad_privacy_screen(). > >>> > >>> Signed-off-by: Tong Zhang > >> > >> Thank you for catching this and thank you for your patch. I was about to > >> merge > >> this, but then I realized that this might not be the best way to fix this. > >> > >> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi, > >> and at a first glance about half of those are missing an acpi_disabled > >> check. IMHO it would be better to simply add an acpi_disabled check to > >> acpi_get_devices() itself. > >> > >> Rafael, do you agree ? > > > > Yes, I do. > > Did you see my follow-up that that is not going to work because > acpi_get_devices() is an acpica function ? No, I didn't, but it is possible to add a wrapper doing the check around it and convert all of the users. Alternatively, the ACPICA function can check acpi_gbl_root_node against NULL, like in the attached (untested) patch. --- drivers/acpi/acpica/nswalk.c |3 +++ 1 file changed, 3 insertions(+) Index: linux-pm/drivers/acpi/acpica/nswalk.c === --- linux-pm.orig/drivers/acpi/acpica/nswalk.c +++ linux-pm/drivers/acpi/acpica/nswalk.c @@ -169,6 +169,9 @@ acpi_ns_walk_namespace(acpi_object_type if (start_node == ACPI_ROOT_OBJECT) { start_node = acpi_gbl_root_node; + if (!start_node) { + return_ACPI_STATUS(AE_NO_NAMESPACE); + } } /* Null child means "get first node" */
Re: acpi_get_devices() crash when acpi_disabled==true (was [PATCH v2] drm/privacy-screen: honor acpi=off in detect_thinkpad_privacy_screen)
On Thu, Jan 27, 2022 at 2:05 PM Hans de Goede wrote: > > Hi, > > On 1/26/22 18:11, Rafael J. Wysocki wrote: > > On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede wrote: > >> > >> Hi, > >> > >> On 1/26/22 16:54, Rafael J. Wysocki wrote: > >>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede wrote: > >>>> > >>>> Hi All, > >>>> > >>>> On 1/23/22 10:10, Tong Zhang wrote: > >>>>> when acpi=off is provided in bootarg, kernel crash with > >>>>> > >>>>> [1.252739] BUG: kernel NULL pointer dereference, address: > >>>>> 0018 > >>>>> [1.258308] Call Trace: > >>>>> [1.258490] ? acpi_walk_namespace+0x147/0x147 > >>>>> [1.258770] acpi_get_devices+0xe4/0x137 > >>>>> [1.258921] ? drm_core_init+0xc0/0xc0 [drm] > >>>>> [1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm] > >>>>> [1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm] > >>>>> > >>>>> The reason is that acpi_walk_namespace expects acpi related stuff > >>>>> initialized but in fact it wouldn't when acpi is set to off. In this > >>>>> case > >>>>> we should honor acpi=off in detect_thinkpad_privacy_screen(). > >>>>> > >>>>> Signed-off-by: Tong Zhang > >>>> > >>>> Thank you for catching this and thank you for your patch. I was about to > >>>> merge > >>>> this, but then I realized that this might not be the best way to fix > >>>> this. > >>>> > >>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi, > >>>> and at a first glance about half of those are missing an acpi_disabled > >>>> check. IMHO it would be better to simply add an acpi_disabled check to > >>>> acpi_get_devices() itself. > >>>> > >>>> Rafael, do you agree ? > >>> > >>> Yes, I do. > >> > >> Did you see my follow-up that that is not going to work because > >> acpi_get_devices() is an acpica function ? > > > > No, I didn't, but it is possible to add a wrapper doing the check > > around it and convert all of the users. > > Yes I did think about that. Note that I've gone ahead and pushed > the fix which started this to drm-misc-fixes, to resolve the crash > for now. OK > If we add such a wrapper we can remove a bunch of acpi_disabled checks > from various callers. > > > Alternatively, the ACPICA function can check acpi_gbl_root_node > > against NULL, like in the attached (untested) patch. > > That is probably an even better idea, as that avoids the need > for a wrapper altogether. So I believe that that is the best > solution. Allright, let me cut an analogous patch for the upstream ACPICA, then.
Re: [PATCH v14 20/39] pwm: tegra: Add runtime PM and OPP support
On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko wrote: > > 26.10.2021 01:40, Dmitry Osipenko пишет: > > + ret = devm_pm_runtime_enable(&pdev->dev); > > + if (ret) > > + return ret; > > + > > + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > > + if (ret) > > + return ret; > > + > > + ret = pm_runtime_resume_and_get(&pdev->dev); > > + if (ret) > > + return ret; > > + > > /* Set maximum frequency of the IP */ > > - ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency); > > + ret = dev_pm_opp_set_rate(pwm->dev, pwm->soc->max_frequency); > > if (ret < 0) { > > dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret); > > - return ret; > > + goto put_pm; > > } > > > > /* > > @@ -278,7 +294,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) > > if (IS_ERR(pwm->rst)) { > > ret = PTR_ERR(pwm->rst); > > dev_err(&pdev->dev, "Reset control is not found: %d\n", ret); > > - return ret; > > + goto put_pm; > > } > > > > reset_control_deassert(pwm->rst); > > @@ -291,10 +307,15 @@ static int tegra_pwm_probe(struct platform_device > > *pdev) > > if (ret < 0) { > > dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); > > reset_control_assert(pwm->rst); > > - return ret; > > + goto put_pm; > > } > > > > + pm_runtime_put(&pdev->dev); > > + > > return 0; > > +put_pm: > > + pm_runtime_put_sync_suspend(&pdev->dev); > > + return ret; > > } > > > > static int tegra_pwm_remove(struct platform_device *pdev) > > @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device > > *pdev) > > > > reset_control_assert(pc->rst); > > > > + pm_runtime_force_suspend(&pdev->dev); > > I just noticed that RPM core doesn't reset RPM-enable count of a device > on driver's unbind (pm_runtime_reinit). It was a bad idea to use > devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is > disabled twice on driver's removal, and thus, RPM will never be enabled > again. > > I'll fix it for PWM and other drivers in this series, in v15. Well, for the record, IMV using pm_runtime_force_suspend() is generally a questionable idea.
Re: [PATCH v14 20/39] pwm: tegra: Add runtime PM and OPP support
On Fri, Oct 29, 2021 at 6:29 PM Dmitry Osipenko wrote: > > 29.10.2021 18:56, Rafael J. Wysocki пишет: > > On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko wrote: > >> > >> 26.10.2021 01:40, Dmitry Osipenko пишет: > >>> + ret = devm_pm_runtime_enable(&pdev->dev); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = pm_runtime_resume_and_get(&pdev->dev); > >>> + if (ret) > >>> + return ret; > >>> + > >>> /* Set maximum frequency of the IP */ > >>> - ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency); > >>> + ret = dev_pm_opp_set_rate(pwm->dev, pwm->soc->max_frequency); > >>> if (ret < 0) { > >>> dev_err(&pdev->dev, "Failed to set max frequency: %d\n", > >>> ret); > >>> - return ret; > >>> + goto put_pm; > >>> } > >>> > >>> /* > >>> @@ -278,7 +294,7 @@ static int tegra_pwm_probe(struct platform_device > >>> *pdev) > >>> if (IS_ERR(pwm->rst)) { > >>> ret = PTR_ERR(pwm->rst); > >>> dev_err(&pdev->dev, "Reset control is not found: %d\n", > >>> ret); > >>> - return ret; > >>> + goto put_pm; > >>> } > >>> > >>> reset_control_deassert(pwm->rst); > >>> @@ -291,10 +307,15 @@ static int tegra_pwm_probe(struct platform_device > >>> *pdev) > >>> if (ret < 0) { > >>> dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); > >>> reset_control_assert(pwm->rst); > >>> - return ret; > >>> + goto put_pm; > >>> } > >>> > >>> + pm_runtime_put(&pdev->dev); > >>> + > >>> return 0; > >>> +put_pm: > >>> + pm_runtime_put_sync_suspend(&pdev->dev); > >>> + return ret; > >>> } > >>> > >>> static int tegra_pwm_remove(struct platform_device *pdev) > >>> @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device > >>> *pdev) > >>> > >>> reset_control_assert(pc->rst); > >>> > >>> + pm_runtime_force_suspend(&pdev->dev); > >> > >> I just noticed that RPM core doesn't reset RPM-enable count of a device > >> on driver's unbind (pm_runtime_reinit). It was a bad idea to use > >> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is > >> disabled twice on driver's removal, and thus, RPM will never be enabled > >> again. > >> > >> I'll fix it for PWM and other drivers in this series, in v15. > > > > Well, for the record, IMV using pm_runtime_force_suspend() is > > generally a questionable idea. > > > > Please clarify why it's a questionable idea. There are a few reasons. Generally speaking, it makes assumptions that may not be satisfied. For instance, it assumes that the driver will never have to work with the ACPI PM domain, because the ACPI PM domain has a separate set of callbacks for system-wide suspend and resume and they are not the same as its PM-runtime callbacks, so if the driver is combined with the ACPI PM domain, running pm_runtime_force_suspend() may not work as expected. Next, it assumes that PM-runtime is actually enabled for the device and the RPM_STATUS of it is valid when it is running. Further, it assumes that the PM-runtime suspend callback of the driver will always be suitable for system-wide suspend which may not be the case if the device can generate wakeup signals and it is not allowed to wake up the system from sleep by user space. Next, if the driver has to work with a PM domain (other than the ACPI one) or bus type that doesn't take the pm_runtime_force_suspend() explicitly into account, it may end up running the runtime-suspend callback provided by that entity from within its system-wide suspend callback which may not work as expected. I guess I could add a few if I had to.
Re: [PATCH v2 12/63] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region
On Wed, Aug 18, 2021 at 8:08 AM Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), avoid intentionally writing across > neighboring fields. > > Use struct_group() in struct art around members weight, and ac[0-9]_max, > so they can be referenced together. This will allow memcpy() and sizeof() > to more easily reason about sizes, improve readability, and avoid future > warnings about writing beyond the end of weight. > > "pahole" shows no size nor member offset changes to struct art. > "objdump -d" shows no meaningful object code changes (i.e. only source > line number induced differences). > > Cc: Zhang Rui > Cc: Daniel Lezcano > Cc: Amit Kucheria > Cc: linux...@vger.kernel.org > Signed-off-by: Kees Cook Rui, Srinivas, any comments here? > --- > .../intel/int340x_thermal/acpi_thermal_rel.c | 5 +- > .../intel/int340x_thermal/acpi_thermal_rel.h | 48 ++- > 2 files changed, 29 insertions(+), 24 deletions(-) > > diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c > b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c > index a478cff8162a..e90690a234c4 100644 > --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c > +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c > @@ -250,8 +250,9 @@ static int fill_art(char __user *ubuf) > get_single_name(arts[i].source, art_user[i].source_device); > get_single_name(arts[i].target, art_user[i].target_device); > /* copy the rest int data in addition to source and target */ > - memcpy(&art_user[i].weight, &arts[i].weight, > - sizeof(u64) * (ACPI_NR_ART_ELEMENTS - 2)); > + BUILD_BUG_ON(sizeof(art_user[i].data) != > +sizeof(u64) * (ACPI_NR_ART_ELEMENTS - 2)); > + memcpy(&art_user[i].data, &arts[i].data, > sizeof(art_user[i].data)); > } > > if (copy_to_user(ubuf, art_user, art_len)) > diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h > b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h > index 58822575fd54..78d942477035 100644 > --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h > +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h > @@ -17,17 +17,19 @@ > struct art { > acpi_handle source; > acpi_handle target; > - u64 weight; > - u64 ac0_max; > - u64 ac1_max; > - u64 ac2_max; > - u64 ac3_max; > - u64 ac4_max; > - u64 ac5_max; > - u64 ac6_max; > - u64 ac7_max; > - u64 ac8_max; > - u64 ac9_max; > + struct_group(data, > + u64 weight; > + u64 ac0_max; > + u64 ac1_max; > + u64 ac2_max; > + u64 ac3_max; > + u64 ac4_max; > + u64 ac5_max; > + u64 ac6_max; > + u64 ac7_max; > + u64 ac8_max; > + u64 ac9_max; > + ); > } __packed; > > struct trt { > @@ -47,17 +49,19 @@ union art_object { > struct { > char source_device[8]; /* ACPI single name */ > char target_device[8]; /* ACPI single name */ > - u64 weight; > - u64 ac0_max_level; > - u64 ac1_max_level; > - u64 ac2_max_level; > - u64 ac3_max_level; > - u64 ac4_max_level; > - u64 ac5_max_level; > - u64 ac6_max_level; > - u64 ac7_max_level; > - u64 ac8_max_level; > - u64 ac9_max_level; > + struct_group(data, > + u64 weight; > + u64 ac0_max_level; > + u64 ac1_max_level; > + u64 ac2_max_level; > + u64 ac3_max_level; > + u64 ac4_max_level; > + u64 ac5_max_level; > + u64 ac6_max_level; > + u64 ac7_max_level; > + u64 ac8_max_level; > + u64 ac9_max_level; > + ); > }; > u64 __data[ACPI_NR_ART_ELEMENTS]; > }; > -- > 2.30.2 >
Re: [PATCH v2 12/63] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region
On Wed, Nov 24, 2021 at 12:53 AM Srinivas Pandruvada wrote: > > On Tue, 2021-11-23 at 14:19 +0100, Rafael J. Wysocki wrote: > > On Wed, Aug 18, 2021 at 8:08 AM Kees Cook > > wrote: > > > > > > In preparation for FORTIFY_SOURCE performing compile-time and run- > > > time > > > field bounds checking for memcpy(), avoid intentionally writing > > > across > > > neighboring fields. > > > > > > Use struct_group() in struct art around members weight, and ac[0- > > > 9]_max, > > > so they can be referenced together. This will allow memcpy() and > > > sizeof() > > > to more easily reason about sizes, improve readability, and avoid > > > future > > > warnings about writing beyond the end of weight. > > > > > > "pahole" shows no size nor member offset changes to struct art. > > > "objdump -d" shows no meaningful object code changes (i.e. only > > > source > > > line number induced differences). > > > > > > Cc: Zhang Rui > > > Cc: Daniel Lezcano > > > Cc: Amit Kucheria > > > Cc: linux...@vger.kernel.org > > > Signed-off-by: Kees Cook > > > > Rui, Srinivas, any comments here? > Looks good. > Reviewed-by: Srinivas Pandruvada Applied as 5.17 material, thank you!
Re: Regression report on laptop suspend
CC Daniel, Thomas and dri-devel. On Mon, Dec 27, 2021 at 5:32 PM wrote: > > Hello > > I've noticed my laptop totally freeze when going to hibernation. > The git bisect log is appended below. > Please note however that even the previous good commit was "good" (ie : > laptop managed to suspend and resume), the system was unstable and froze few > minutes later. So the breakage need not be related to the first bad commit. Have you tried to revert that commit? If so, has it helped? > Hardware specs: AMD Ryzen 5 4600H with Vega graphics + Nvidia 1650Ti (unused) > Software: Slackware 14.2 / X.org. > > Seems to be related to drm stuff. > I've issued bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=215427 > > Thanks > > git bisect start > # good: [8bb7eca972ad531c9b149c0a51ab43a417385813] Linux 5.15 > git bisect good 8bb7eca972ad531c9b149c0a51ab43a417385813 > # bad: [a7904a538933c525096ca2ccde1e60d0ee62c08e] Linux 5.16-rc6 > git bisect bad a7904a538933c525096ca2ccde1e60d0ee62c08e > # bad: [43e1b12927276cde8052122a24ff796649f09d60] Merge tag 'for_linus' of > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost > git bisect bad 43e1b12927276cde8052122a24ff796649f09d60 > # good: [fc02cb2b37fe2cbf1d3334b9f0f0eab9431766c4] Merge tag > 'net-next-for-5.16' of > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next > git bisect good fc02cb2b37fe2cbf1d3334b9f0f0eab9431766c4 > # bad: [d9bd054177fbd2c4762546aec40fc3071bfe4cc0] Merge tag > 'amd-drm-next-5.16-2021-10-29' of https://gitlab.freedesktop.org/agd5f/linux > into drm-next > git bisect bad d9bd054177fbd2c4762546aec40fc3071bfe4cc0 > # skip: [797d72ce8e0f8fa8a808cb189b5411046432cfd3] Merge tag > 'drm-misc-next-2021-10-06' of git://anongit.freedesktop.org/drm/drm-misc into > drm-next > git bisect skip 797d72ce8e0f8fa8a808cb189b5411046432cfd3 > # skip: [bf72ca73aaa6629568cb9b0761be6efdd02a2591] drm/amd/display: [FW > Promotion] Release 0.0.85 > git bisect skip bf72ca73aaa6629568cb9b0761be6efdd02a2591 > # good: [bc41f059a080e487c235b539f1e5cdbf605aba9f] drm/i915/dp: fix DG1 and > RKL max source rates > git bisect good bc41f059a080e487c235b539f1e5cdbf605aba9f > # skip: [58144d283712c9e80e528e001af6ac5aeee71af2] drm/amdgpu: unify BO > evicting method in amdgpu_ttm > git bisect skip 58144d283712c9e80e528e001af6ac5aeee71af2 > # skip: [a5b51a9f8523a0b88ce7e8e8059f75a43c34c57f] drm/i915/gt: add > asm/cacheflush.h for use of clflush() > git bisect skip a5b51a9f8523a0b88ce7e8e8059f75a43c34c57f > # skip: [40348baedfbc6500e7a090c7da1d55b6c94c334f] drm/amd/display: fix > duplicated inclusion > git bisect skip 40348baedfbc6500e7a090c7da1d55b6c94c334f > # skip: [7547675b84bf452542463db29adb113cadb7dd6d] drm/virtio: implement > context init: track {ring_idx, emit_fence_info} in virtio_gpu_fence > git bisect skip 7547675b84bf452542463db29adb113cadb7dd6d > # good: [f01ee019586220c86f238263a4fbde6e72085e11] drm/amd/display: Add DP > 2.0 SST DC Support > git bisect good f01ee019586220c86f238263a4fbde6e72085e11 > # good: [f3ede209d44d71636890a78fa89c5b1c83340320] drm/i915/pci: rename > functions to have i915_pci prefix > git bisect good f3ede209d44d71636890a78fa89c5b1c83340320 > # skip: [4fb530e5caf7cb666948db65f245b350ce520436] drm/virtio: implement > context init: support init ioctl > git bisect skip 4fb530e5caf7cb666948db65f245b350ce520436 > # good: [c7c4dfb6fe704ae3cce1a8f438db75b1a0a9061f] drm/i915/display: Some > code improvements and code style fixes for DRRS > git bisect good c7c4dfb6fe704ae3cce1a8f438db75b1a0a9061f > # skip: [7a28bee067d524c1b8770aa72a82263eb9fc53f0] drm/amd/display: Disable > dpp root clock when not being used > git bisect skip 7a28bee067d524c1b8770aa72a82263eb9fc53f0 > # good: [5b116c17e6babc6de2e26714bc66228c74038b71] drm/i915/guc: Drop pin > count check trick between sched_disable and re-pin > git bisect good 5b116c17e6babc6de2e26714bc66228c74038b71 > # skip: [9878844094703fbae1c3b301c9bb71253a30efe7] drm/amdgpu: drive all vega > asics from the IP discovery table > git bisect skip 9878844094703fbae1c3b301c9bb71253a30efe7 > # skip: [7194dc998dfffca096c30b3cd39625158608992d] drm/i915/tc: Fix TypeC > port init/resume time sanitization > git bisect skip 7194dc998dfffca096c30b3cd39625158608992d > # skip: [5c3720be7d46581181782f5cf9585b532feed947] drm/amdgpu: get VCN and > SDMA instances from IP discovery table > git bisect skip 5c3720be7d46581181782f5cf9585b532feed947 > # skip: [a53f2c035e9832d20775d2c66c71495f2dc27699] drm/panfrost: Calculate > lock region size correctly > git bisect skip a53f2c035e9832d20775d2c66c71495f2dc27699 > # skip: [d04287d062a4198ec0bf0112db03618f65d7428a] drm/amdgpu: During s0ix > don't wait to signal GFXOFF > git bisect skip d04287d062a4198ec0bf0112db03618f65d7428a > # skip: [9ced12182d0d8401d821e9602e56e276459900fc] drm/i915: Catch yet > another unconditioal clflush > git bisect skip 9ced12182d0d8401d821e9602e56e276459900fc > # skip: [dac3c405b9aedee301d0634b4e275b81f0d74363] drm/amd/display:
Re: [PATCH v5 3/3] PM/runtime:Replace jiffies based accounting with ktime based accounting
On Mon, Jan 21, 2019 at 4:17 PM Vincent Guittot wrote: > > On Fri, 18 Jan 2019 at 13:08, Guenter Roeck wrote: > > > > On 1/18/19 3:05 AM, Rafael J. Wysocki wrote: > > > On Fri, Jan 18, 2019 at 11:53 AM Vincent Guittot > > > wrote: > > >> > > >> On Fri, 18 Jan 2019 at 11:42, Vincent Guittot > > >> wrote: > > >>> > > >>> Hi Guenter, > > >>> > > >>> Le Thursday 17 Jan 2019 à 14:16:28 (-0800), Guenter Roeck a écrit : > > >>>> On Fri, Dec 21, 2018 at 11:33:56AM +0100, Vincent Guittot wrote: > > >>>>> From: Thara Gopinath > > >>>>> > > >>>>> This patch replaces jiffies based accounting for runtime_active_time > > >>>>> and runtime_suspended_time with ktime base accounting. This makes the > > >>>>> runtime debug counters inline with genpd and other pm subsytems which > > >>>>> uses ktime based accounting. > > >>>>> > > >>>>> timekeeping is initialized before pm_runtime_init() so ktime_get() > > >>>>> will > > >>>>> be ready before first call. In fact, timekeeping_init() is called > > >>>>> early > > >>>>> in start_kernel() which is way before driver_init() (and that's when > > >>>>> devices can start to be initialized) called from rest_init() via > > >>>>> kernel_init_freeable() and do_basic_setup(). > > >>>>> > > >>>> This is not (always) correct. My qemu "collie" boot test fails with > > >>>> this > > >>>> patch applied. Reverting the patch fixes the problem. Bisect log > > >>>> attached. > > >>>> > > >>> > > >>> Can you try the patch below ? > > >>> ktime_get_mono_fast_ns() has the advantage of being init with dummy > > >>> clock so > > >>> it can be used at early_init. > > >> > > >> Another possibility would be delay the init of the gpiochip > > > > > > Well, right. > > > > > > Initializing devices before timekeeping doesn't feel particularly > > > robust from the design perspective. > > > > > > How exactly does that happen? > > > > > > > With an added 'initialized' flag and backtrace into the timekeeping code, > > with the change suggested earlier applied: > > > > [ cut here ] > > WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping.c:453 > > ktime_get_mono_fast_ns+0x114/0x12c > > Timekeeping not initialized > > CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc2-next-20190117-dirty #2 > > Hardware name: Sharp-Collie > > Backtrace: > > [] (dump_backtrace) from [] (show_stack+0x18/0x1c) > > r7:0009 r6: r5:c065ba90 r4:c06d3e54 > > [] (show_stack) from [] (dump_stack+0x20/0x28) > > [] (dump_stack) from [] (__warn+0xcc/0xf4) > > [] (__warn) from [] (warn_slowpath_fmt+0x4c/0x6c) > > r8:df407b08 r7: r6:c0c01550 r5:c065bad8 r4:c06dd028 > > [] (warn_slowpath_fmt) from [] > > (ktime_get_mono_fast_ns+0x114/0x12c) > > r3: r2:c065bad8 > > r5: r4:df407b08 > > [] (ktime_get_mono_fast_ns) from [] > > (pm_runtime_init+0x38/0xb8) > > r9:c06c9a5c r8:df407b08 r7: r6:c0c01550 r5: r4:df407b08 > > [] (pm_runtime_init) from [] > > (device_initialize+0xb0/0xec) > > r7: r6:c0c01550 r5: r4:df407b08 > > [] (device_initialize) from [] > > (gpiochip_add_data_with_key+0x9c/0x884) > > r7: r6:c06fca34 r5: r4: > > [] (gpiochip_add_data_with_key) from [] > > (sa1100_init_gpio+0x40/0x98) > > r10:dfffcd60 r9:c06c9a5c r8:c06dd020 r7:c06dd028 r6: r5: > > r4:c06fca34 > > [] (sa1100_init_gpio) from [] > > (sa1100_init_irq+0x2c/0x3c) > > r7:c06dd028 r6: r5:c0713300 r4:c06e1070 > > [] (sa1100_init_irq) from [] (init_IRQ+0x20/0x28) > > r5:c0713300 r4: > > [] (init_IRQ) from [] (start_kernel+0x254/0x4cc) > > [] (start_kernel) from [<>] ( (null)) > > r10:717f r9:6901b119 r8:c100 r7:0092 r6:313d r5:0053 > > r4:c06a7330 > > ---[ end trace 91e1bd00dd7cce32 ]--- > > Does it means that only the pm_runtime_init is done before > timekeeping_init() but no update_pm_runtime_accounting() ? This platform calls device_initialize(), via sa1100_init_irq(), from init_IRQ() which is in the start_kernel() code path before timekeeping_init(). That's the initialization of structure fields alone. Runtime PM really cannot be used legitimately before driver_init(), because it needs bus types to be there at least. > In this case, we can keep using ktimeçget in > update_pm_runtime_accounting() and find a solution to deal with > early_call of pm_runtime_init() Given the above, I think that initializing accounting_timestamp in pm_runtime_init() to anything different from 0 is a mistake. Note that update_pm_runtime_accounting() ignores the delta value if power.disable_depth is not zero anyway, so it really should be sufficient to update accounting_timestamp when enabling runtime PM - and I'm not sure why it is not updated in pm_runtime_enable() for that matter (that looks like a bug to me). ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Armada DRM: bridge with componentized devices
On Friday, January 18, 2019 1:57:47 PM CET Daniel Vetter wrote: > On Fri, Jan 18, 2019 at 12:38 PM Rafael J. Wysocki wrote: > > > > On Fri, Jan 18, 2019 at 12:17 PM Rafael J. Wysocki > > wrote: > > > > > > On Fri, Jan 18, 2019 at 12:06 PM Daniel Vetter wrote: > > > > > > > > On Fri, Jan 18, 2019 at 11:03 AM Rafael J. Wysocki > > > > wrote: > > > > > > > > > > On Fri, Jan 18, 2019 at 10:37 AM Lucas Stach > > > > > wrote: > > > > > > > > > > > > Am Donnerstag, den 17.01.2019, 13:20 +0100 schrieb Daniel Vetter: > > > > > > [...] > > > > > > > > > > > > > > > I don't think it is addressed here. > > > > > > > > > > > > > > > > Can anyone please explain to me what happens in more detail? > > > > > > > > > > > > > > My understanding (and this might be wrong, Russell, Andrzej, ... > > > > > > > pls > > > > > > > correct) is that the following sequence goes wrong: > > > > > > > > > > > > > > - componentized driver (both producer and consumer) fully > > > > > > > loaded&working > > > > > > > - you unbind the producer/one of the components > > > > > > > - component framework or driver core through device_link also > > > > > > > unbinds > > > > > > > the master/consumer > > > > > > > - you reload/rebind the component > > > > > > > - with the component framework this results in the master being > > > > > > > rebound, and the overall driver working again. With device_links > > > > > > > nothing happens. > > > > > > > > > > > > > > I think there was a patch floating around once to put drivers > > > > > > > unbound > > > > > > > due to device_links onto the deferred probe list, so that the next > > > > > > > time something is bound they have a chance to rebind. But I can't > > > > > > > find > > > > > > > that patch anymore, maybe someone else has the link still? > > > > > > > > > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1348023.html > > > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1355243.html > > > > > > > > Thanks for digging it out, those are the patches I had in mind. > > > > > > > > > > I can repost if this is finally deemed a good idea. > > > > > > > > > > I don't think this will help. > > > > > > > > > > Two things appear to be needed here: missing suppliers need to be > > > > > probed automatically at a consumer probe time and "persistent" links > > > > > created by ->probe() callbacks should not be reference-counted when > > > > > those callbacks run again. I'm going to cut a patch to do that (early > > > > > next week if all goes well). > > > > > > > > I thought this should work, since it makes an unbind of a consumer act > > > > the same way as if that consumer's bind function has hit an > > > > EPROBE_DEFER. Example: > > > > > > > > 1. consumer's bind is called, realizes (through a subsystem function > > > > like of_drm_find_panel) that the producer isn't there yet, fails with > > > > EPROBE DEFER. > > > > > > > > 2. driver core puts the consumer onto the deferred probe list. > > > > > > > > 3. producer gets loaded, registers the panel > > > > > > > > 4. driver core re-runs the consumer's bind function > > > > > > > > 5. consumer's bind function finds the produced and sets up the device > > > > link. > > > > > > > > Now for the unbind case: > > > > > > > > 1. producer is unbound, driver core unbinds consumer due to the > > > > device_link > > > > > > > > 2. (Only with Lucas' patch) driver core puts the consumer onto the > > > > deferred probe list. > > > > > > > > 3. Developer (this really is useful for development) rebinds/reloads > > > > producer driver, which re-registers the panel > > > >
Re: [PATCH 1/2] component: Add documentation
w/compOn Thu, Jan 31, 2019 at 3:46 PM Daniel Vetter wrote: > > Someone owes me a beer ... > > While typing these I think doing an s/component_master/aggregate/ > would be useful: > - it's shorter :-) > - I think component/aggregate is much more meaningful naming than > component/puppetmaster or something like that. At least to my > English ear "aggregate" emphasizes much more the "assemble a pile of > things into something bigger" aspect, and there's not really much > of a control hierarchy between aggregate and constituing components. > > But that's way more than a quick doc typing exercise ... > > Thanks to Ram for commenting on an initial draft of these docs. Look goods to me overall (even though I'm not super-familiar with the component framework), but see below. > Cc: "C, Ramalingam" > Cc: Greg Kroah-Hartman > Cc: Russell King > Cc: Rafael J. Wysocki > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: Daniel Vetter > --- > Documentation/driver-api/device_link.rst | 3 + > Documentation/driver-api/index.rst | 1 + > drivers/base/component.c | 107 ++- > include/linux/component.h| 70 +++ > 4 files changed, 178 insertions(+), 3 deletions(-) > > diff --git a/Documentation/driver-api/device_link.rst > b/Documentation/driver-api/device_link.rst > index d6763272e747..2d5919b2b337 100644 > --- a/Documentation/driver-api/device_link.rst > +++ b/Documentation/driver-api/device_link.rst > @@ -1,6 +1,9 @@ > .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain > ` > .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain > ` > > + > +.. _device_link: > + > > Device links > > diff --git a/Documentation/driver-api/index.rst > b/Documentation/driver-api/index.rst > index ab38ced66a44..c0b600ed9961 100644 > --- a/Documentation/driver-api/index.rst > +++ b/Documentation/driver-api/index.rst > @@ -22,6 +22,7 @@ available subsections can be seen below. > device_connection > dma-buf > device_link > + component Do I think correctly that this doc is going to be generated automatically from the kerneldoc comments in component.c? > message-based > sound > frame-buffer > diff --git a/drivers/base/component.c b/drivers/base/component.c > index ddcea8739c12..e5b04bce8544 100644 > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -16,6 +16,33 @@ > #include > #include > > +/** > + * DOC: overview > + * > + * The component frameworks allows drivers to collect a pile of sub-devices, s/frameworks/framework/ > + * including their bound drivers, into an aggregate driver. Various subsystem s/subsystem/subsystems/ > + * already provide functions to get hold of such components, e.g. > + * of_clk_get_by_name(). Anytime there's such a subsystem specific way to > find a > + * a device the component framework should not be used. I would use a positive statement here, like "The component framework can be used when such a subsystem-specific way to find a device is not available". > The component framework > + * fills the niche of aggregate drivers for specific hardware, where further > + * standardization into a subsystem doesn't make sense. I would say "would not be practical" instead of "doesn't make sense". > The common example is > + * when a logical device (e.g. a DRM display driver) is spread around the > SoC on > + * various component (scanout engines, blending blocks, transcoders for > various > + * outputs and so on). > + * > + * The component framework also doesn't solve runtime dependencies, e.g. for > + * system suspend and resume operations. See also :ref:`device > + * links`. > + * > + * Components are registered using component_add() and unregistered with > + * component_del(), usually from the driver's probe and disconnect functions. > + * > + * Aggregate drivers first assemble a component match list of what they need > + * using component_match_add(). This is then registered as an aggregate > driver > + * using component_master_add_with_match(), and unregistered using > + * component_master_del(). > + */ > + > struct component; > > struct component_match_array { > @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev, > return 0; > } > > -/* > - * Add a component to be matched, with a release function. > +/** > + * component_match_add_release - add a compent match with release callback s/com
Re: [PATCH] component: Add documentation
On Wed, Feb 6, 2019 at 5:47 PM Daniel Vetter wrote: > > While typing these I think doing an s/component_master/aggregate/ > would be useful: > - it's shorter :-) > - I think component/aggregate is much more meaningful naming than > component/puppetmaster or something like that. At least to my > English ear "aggregate" emphasizes much more the "assemble a pile of > things into something bigger" aspect, and there's not really much > of a control hierarchy between aggregate and constituing components. > > But that's way more than a quick doc typing exercise ... > > Thanks to Ram for commenting on an initial draft of these docs. > > v2: Review from Rafael: > - git add Documenation/driver-api/component.rst > - lots of polish to the wording + spelling fixes. > > v3: Review from Russell: > - s/framework/helper > - clarify the documentation for component_match_add functions. > > Cc: "C, Ramalingam" > Cc: Greg Kroah-Hartman > Cc: Russell King > Cc: Rafael J. Wysocki > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: Daniel Vetter Reviewed-by: Rafael J. Wysocki > --- > Documentation/driver-api/component.rst | 17 > Documentation/driver-api/device_link.rst | 3 + > Documentation/driver-api/index.rst | 1 + > drivers/base/component.c | 107 ++- > include/linux/component.h| 71 +++ > 5 files changed, 196 insertions(+), 3 deletions(-) > create mode 100644 Documentation/driver-api/component.rst > > diff --git a/Documentation/driver-api/component.rst > b/Documentation/driver-api/component.rst > new file mode 100644 > index ..2da4a8f20607 > --- /dev/null > +++ b/Documentation/driver-api/component.rst > @@ -0,0 +1,17 @@ > +== > +Component Helper for Aggregate Drivers > +== > + > +.. kernel-doc:: drivers/base/component.c > + :doc: overview > + > + > +API > +=== > + > +.. kernel-doc:: include/linux/component.h > + :internal: > + > +.. kernel-doc:: drivers/base/component.c > + :export: > + > diff --git a/Documentation/driver-api/device_link.rst > b/Documentation/driver-api/device_link.rst > index d6763272e747..2d5919b2b337 100644 > --- a/Documentation/driver-api/device_link.rst > +++ b/Documentation/driver-api/device_link.rst > @@ -1,6 +1,9 @@ > .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain > ` > .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain > ` > > + > +.. _device_link: > + > > Device links > > diff --git a/Documentation/driver-api/index.rst > b/Documentation/driver-api/index.rst > index ab38ced66a44..c0b600ed9961 100644 > --- a/Documentation/driver-api/index.rst > +++ b/Documentation/driver-api/index.rst > @@ -22,6 +22,7 @@ available subsections can be seen below. > device_connection > dma-buf > device_link > + component > message-based > sound > frame-buffer > diff --git a/drivers/base/component.c b/drivers/base/component.c > index ddcea8739c12..f34d4b784709 100644 > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -16,6 +16,32 @@ > #include > #include > > +/** > + * DOC: overview > + * > + * The component helper allows drivers to collect a pile of sub-devices, > + * including their bound drivers, into an aggregate driver. Various > subsystems > + * already provide functions to get hold of such components, e.g. > + * of_clk_get_by_name(). The component helper can be used when such a > + * subsystem-specific way to find a device is not available: The component > + * helper fills the niche of aggregate drivers for specific hardware, where > + * further standardization into a subsystem would not be practical. The > common > + * example is when a logical device (e.g. a DRM display driver) is spread > around > + * the SoC on various component (scanout engines, blending blocks, > transcoders > + * for various outputs and so on). > + * > + * The component helper also doesn't solve runtime dependencies, e.g. for > system > + * suspend and resume operations. See also :ref:`device links`. > + * > + * Components are registered using component_add() and unregistered with > + * component_del(), usually from the driver's probe and disconnect functions. > + * > + * Aggregate drivers first assemble a component match list of what they need > + * using component_match_add(). This is then registered as an aggregate > driver >
Re: [PATCH 2/3] components: multiple components for a device
) On Wed, Feb 6, 2019 at 5:46 PM Daniel Vetter wrote: > > Component framework is extended to support multiple components for > a struct device. These will be matched with different masters based on > its sub component value. > > We are introducing this, as I915 needs two different components > with different subcomponent value, which will be matched to two > different component masters(Audio and HDCP) based on the subcomponent > values. > > v2: Add documenation. > > v3: Rebase on top of updated documenation. > > Signed-off-by: Daniel Vetter (v1 code) > Signed-off-by: Ramalingam C (v1 commit message) > Cc: Ramalingam C > Cc: Greg Kroah-Hartman > Cc: Russell King > Cc: Rafael J. Wysocki > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: Daniel Vetter > --- > drivers/base/component.c | 160 +- > include/linux/component.h | 9 ++- > 2 files changed, 129 insertions(+), 40 deletions(-) > > diff --git a/drivers/base/component.c b/drivers/base/component.c > index f34d4b784709..68ccd5a0d5d6 100644 > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -47,6 +47,7 @@ struct component; > struct component_match_array { > void *data; > int (*compare)(struct device *, void *); > + int (*compare_typed)(struct device *, int, void *); > void (*release)(struct device *, void *); > struct component *component; > bool duplicate; > @@ -74,6 +75,7 @@ struct component { > bool bound; > > const struct component_ops *ops; > + int subcomponent; > struct device *dev; > }; > > @@ -158,7 +160,7 @@ static struct master *__master_find(struct device *dev, > } > > static struct component *find_component(struct master *master, > - int (*compare)(struct device *, void *), void *compare_data) > + struct component_match_array *mc) > { > struct component *c; > > @@ -166,8 +168,13 @@ static struct component *find_component(struct master > *master, > if (c->master && c->master != master) > continue; > > - if (compare(c->dev, compare_data)) > + if (mc->compare_typed) { > + if (mc->compare_typed(c->dev, c->subcomponent, > + mc->data)) This line break looks kind of weird to me, > + return c; > + } else if (mc->compare(c->dev, mc->data)) { > return c; > + } Also, why don't you do if (mc->compare(c->dev, mc->data) || (mc->compare_typed && mc->compare_typed(c->dev, c->subcomponent, mc->data))) return c; The only difference is that ->compare() will run first and if it finds a match, c will be returned right away. Does it matter? > } > > return NULL; > @@ -192,7 +199,7 @@ static int find_components(struct master *master) > if (match->compare[i].component) > continue; > > - c = find_component(master, mc->compare, mc->data); > + c = find_component(master, mc); > if (!c) { > ret = -ENXIO; > break; > @@ -327,30 +334,12 @@ static int component_match_realloc(struct device *dev, > return 0; > } > > -/** > - * component_match_add_release - add a component match with release callback > - * @master: device with the aggregate driver > - * @matchptr: pointer to the list of component matches > - * @release: release function for @compare_data > - * @compare: compare function to match against all components > - * @compare_data: opaque pointer passed to the @compare function > - * > - * This adds a new component match to the list stored in @matchptr, which the "This" appears to be redundant here (and in some places below too). ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] components: multiple components for a device
On Thu, Feb 7, 2019 at 11:40 PM Daniel Vetter wrote: > > On Wed, Feb 06, 2019 at 11:57:04PM +0100, Rafael J. Wysocki wrote: > > ) On Wed, Feb 6, 2019 at 5:46 PM Daniel Vetter > > wrote: > > > > > > Component framework is extended to support multiple components for > > > a struct device. These will be matched with different masters based on > > > its sub component value. > > > > > > We are introducing this, as I915 needs two different components > > > with different subcomponent value, which will be matched to two > > > different component masters(Audio and HDCP) based on the subcomponent > > > values. > > > > > > v2: Add documenation. > > > > > > v3: Rebase on top of updated documenation. > > > > > > Signed-off-by: Daniel Vetter (v1 code) > > > Signed-off-by: Ramalingam C (v1 commit message) > > > Cc: Ramalingam C > > > Cc: Greg Kroah-Hartman > > > Cc: Russell King > > > Cc: Rafael J. Wysocki > > > Cc: Jaroslav Kysela > > > Cc: Takashi Iwai > > > Cc: Rodrigo Vivi > > > Cc: Jani Nikula > > > Signed-off-by: Daniel Vetter > > > --- > > > drivers/base/component.c | 160 +- > > > include/linux/component.h | 9 ++- > > > 2 files changed, 129 insertions(+), 40 deletions(-) > > > > > > diff --git a/drivers/base/component.c b/drivers/base/component.c > > > index f34d4b784709..68ccd5a0d5d6 100644 > > > --- a/drivers/base/component.c > > > +++ b/drivers/base/component.c > > > @@ -47,6 +47,7 @@ struct component; > > > struct component_match_array { > > > void *data; > > > int (*compare)(struct device *, void *); > > > + int (*compare_typed)(struct device *, int, void *); > > > void (*release)(struct device *, void *); > > > struct component *component; > > > bool duplicate; > > > @@ -74,6 +75,7 @@ struct component { > > > bool bound; > > > > > > const struct component_ops *ops; > > > + int subcomponent; > > > struct device *dev; > > > }; > > > > > > @@ -158,7 +160,7 @@ static struct master *__master_find(struct device > > > *dev, > > > } > > > > > > static struct component *find_component(struct master *master, > > > - int (*compare)(struct device *, void *), void *compare_data) > > > + struct component_match_array *mc) > > > { > > > struct component *c; > > > > > > @@ -166,8 +168,13 @@ static struct component *find_component(struct > > > master *master, > > > if (c->master && c->master != master) > > > continue; > > > > > > - if (compare(c->dev, compare_data)) > > > + if (mc->compare_typed) { > > > + if (mc->compare_typed(c->dev, c->subcomponent, > > > + mc->data)) > > > > This line break looks kind of weird to me, > > > > > + return c; > > > + } else if (mc->compare(c->dev, mc->data)) { > > > return c; > > > + } > > > > Also, why don't you do > > > > if (mc->compare(c->dev, mc->data) || (mc->compare_typed && > > mc->compare_typed(c->dev, c->subcomponent, mc->data))) > > return c; > > > > The only difference is that ->compare() will run first and if it finds > > a match, c will be returned right away. Does it matter? > > Tried it, yes it does matter: We have either ->compare or ->compare_typed, Well, that's not obvious. :-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/4] components: multiple components for a device
On Fri, Feb 8, 2019 at 12:28 AM Daniel Vetter wrote: > > Component framework is extended to support multiple components for > a struct device. These will be matched with different masters based on > its sub component value. > > We are introducing this, as I915 needs two different components > with different subcomponent value, which will be matched to two > different component masters(Audio and HDCP) based on the subcomponent > values. > > v2: Add documenation. > > v3: Rebase on top of updated documenation. > > v4: Review from Rafael: > - Remove redundant "This" from kerneldoc (also in the previous patch) > - Streamline the logic in find_component() a bit. > > Signed-off-by: Daniel Vetter (v1 code) > Signed-off-by: Ramalingam C (v1 commit message) > Cc: Ramalingam C > Cc: Greg Kroah-Hartman > Cc: Russell King > Cc: Rafael J. Wysocki > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: Daniel Vetter Reviewed-by: Rafael J. Wysocki > --- > drivers/base/component.c | 158 +- > include/linux/component.h | 10 ++- > 2 files changed, 129 insertions(+), 39 deletions(-) > > diff --git a/drivers/base/component.c b/drivers/base/component.c > index 1624c2a892a5..7dbc41cccd58 100644 > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -47,6 +47,7 @@ struct component; > struct component_match_array { > void *data; > int (*compare)(struct device *, void *); > + int (*compare_typed)(struct device *, int, void *); > void (*release)(struct device *, void *); > struct component *component; > bool duplicate; > @@ -74,6 +75,7 @@ struct component { > bool bound; > > const struct component_ops *ops; > + int subcomponent; > struct device *dev; > }; > > @@ -158,7 +160,7 @@ static struct master *__master_find(struct device *dev, > } > > static struct component *find_component(struct master *master, > - int (*compare)(struct device *, void *), void *compare_data) > + struct component_match_array *mc) > { > struct component *c; > > @@ -166,7 +168,11 @@ static struct component *find_component(struct master > *master, > if (c->master && c->master != master) > continue; > > - if (compare(c->dev, compare_data)) > + if (mc->compare && mc->compare(c->dev, mc->data)) > + return c; > + > + if (mc->compare_typed && > + mc->compare_typed(c->dev, c->subcomponent, mc->data)) > return c; > } > > @@ -192,7 +198,7 @@ static int find_components(struct master *master) > if (match->compare[i].component) > continue; > > - c = find_component(master, mc->compare, mc->data); > + c = find_component(master, mc); > if (!c) { > ret = -ENXIO; > break; > @@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev, > return 0; > } > > -/** > - * component_match_add_release - add a component match with release callback > - * @master: device with the aggregate driver > - * @matchptr: pointer to the list of component matches > - * @release: release function for @compare_data > - * @compare: compare function to match against all components > - * @compare_data: opaque pointer passed to the @compare function > - * > - * Adds a new component match to the list stored in @matchptr, which the > @master > - * aggregate driver needs to function. The list of component matches pointed > to > - * by @matchptr must be initialized to NULL before adding the first match. > - * > - * The allocated match list in @matchptr is automatically released using devm > - * actions, where upon @release will be called to free any references held by > - * @compare_data, e.g. when @compare_data is a &device_node that must be > - * released with of_node_put(). > - * > - * See also component_match_add(). > - */ > -void component_match_add_release(struct device *master, > +static void __component_match_add(struct device *master, > struct component_match **matchptr, > void (*release)(struct device *, void *), > - int (*compare)(struct device *, void *), void *compare_data) > + int (*compare)(struct device *, void *), > + int (*compare_typed)(struct device *, int, void *), > + void *compare_data) >
Re: [PATCH v2 3/3] drm/i915: Move to new PM core fields
On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot wrote: > > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson wrote: > > > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot > > wrote: > > > > > > With jiffies been replaced by raw ns in PM core accounting, 915 driver is > > > updated to use this new time infrastructure. > > > > > > Signed-off-by: Vincent Guittot > > > --- > > > drivers/gpu/drm/i915/i915_pmu.c | 12 ++-- > > > drivers/gpu/drm/i915/i915_pmu.h | 4 ++-- > > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c > > > b/drivers/gpu/drm/i915/i915_pmu.c > > > index d6c8f8f..cf6437d 100644 > > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private *i915) > > > */ > > > if (kdev->power.runtime_status == RPM_SUSPENDED) { > > > if > > > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) > > > - i915->pmu.suspended_jiffies_last = > > > - > > > kdev->power.suspended_jiffies; > > > + i915->pmu.suspended_time_last = > > > + kdev->power.suspended_time; > > > > > > > Huh, so patch 2 introduces a complier error because of removing the > > old fields. We can't have that. > > I agree > The patch was mainly to raise discussion OK, so patch [1/3] from this series should be applicable regardless, right? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm/i915: Move to new PM core fields
On Tue, Dec 18, 2018 at 10:58 AM Vincent Guittot wrote: > > On Tue, 18 Dec 2018 at 10:57, Rafael J. Wysocki wrote: > > > > On Mon, Dec 17, 2018 at 3:22 PM Vincent Guittot > > wrote: > > > > > > On Fri, 14 Dec 2018 at 15:36, Ulf Hansson wrote: > > > > > > > > On Fri, 14 Dec 2018 at 15:22, Vincent Guittot > > > > wrote: > > > > > > > > > > With jiffies been replaced by raw ns in PM core accounting, 915 > > > > > driver is > > > > > updated to use this new time infrastructure. > > > > > > > > > > Signed-off-by: Vincent Guittot > > > > > --- > > > > > drivers/gpu/drm/i915/i915_pmu.c | 12 ++-- > > > > > drivers/gpu/drm/i915/i915_pmu.h | 4 ++-- > > > > > 2 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c > > > > > b/drivers/gpu/drm/i915/i915_pmu.c > > > > > index d6c8f8f..cf6437d 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > > > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > > > > @@ -493,14 +493,14 @@ static u64 get_rc6(struct drm_i915_private > > > > > *i915) > > > > > */ > > > > > if (kdev->power.runtime_status == RPM_SUSPENDED) { > > > > > if > > > > > (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) > > > > > - i915->pmu.suspended_jiffies_last = > > > > > - > > > > > kdev->power.suspended_jiffies; > > > > > + i915->pmu.suspended_time_last = > > > > > + kdev->power.suspended_time; > > > > > > > > > > > > > Huh, so patch 2 introduces a complier error because of removing the > > > > old fields. We can't have that. > > > > > > I agree > > > The patch was mainly to raise discussion > > > > OK, so patch [1/3] from this series should be applicable regardless, right? > > Yes OK, I'll queue it up, then. Next time you do something like that please mark patches for discussion in a series as [RFC] so it is all clear. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel