From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762753AbdAIQoK (ORCPT ); Mon, 9 Jan 2017 11:44:10 -0500 Received: from mail-yw0-f172.google.com ([209.85.161.172]:34631 "EHLO mail-yw0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760315AbdAIQoF (ORCPT ); Mon, 9 Jan 2017 11:44:05 -0500 MIME-Version: 1.0 In-Reply-To: References: <20170106143029.11553-1-tomeu.vizoso@collabora.com> <20170106143029.11553-2-tomeu.vizoso@collabora.com> From: Sean Paul Date: Mon, 9 Jan 2017 11:43:41 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux To: Tomeu Vizoso Cc: Linux Kernel Mailing List , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Thierry Reding , intel-gfx-trybot@lists.freedesktop.org, Emil Velikov , Daniel Vetter , Benjamin Gaignard , Jani Nikula , dri-devel , David Airlie Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 9, 2017 at 4:03 AM, Tomeu Vizoso wrote: > On 6 January 2017 at 16:56, Sean Paul wrote: >> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso wrote: >>> This backpointer allows DP helpers to access the connector it's being >>> used for. >>> >>> Signed-off-by: Tomeu Vizoso >>> --- >>> >>> include/drm/drm_dp_helper.h | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h >>> index 55bbeb0ff594..4fa77b434594 100644 >>> --- a/include/drm/drm_dp_helper.h >>> +++ b/include/drm/drm_dp_helper.h >>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg { >>> * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter >>> * @ddc: I2C adapter that can be used for I2C-over-AUX communication >>> * @dev: pointer to struct device that is the parent for this AUX channel >>> + * @connector: backpointer to connector that uses this AUX channel >>> * @hw_mutex: internal mutex used for locking transfers >>> * @transfer: transfers a message representing a single AUX transaction >>> * >>> @@ -757,6 +758,7 @@ struct drm_dp_aux { >>> const char *name; >>> struct i2c_adapter ddc; >>> struct device *dev; >>> + struct drm_connector *connector; >> >> Perhaps I'm misreading the series, but it seems like you could instead >> add int crc_pipe along with the other crc fields. Then when you start >> the crc, set the pipe number. If you have the pipe in the crc worker, >> you can wait vblank on that pipe (no pointers needed). >> >> Reasonable? > > I think that would work fine, but is it any better? I like that the > connector isn't going to change during the lifetime of the drm_dp_aux, Is this actually guaranteed, though? I am having a hard time thinking about a practical example where it's not implemented this way, but I don't see anything actually enforcing the lifetime relationship. > but crc_pipe could be changing between sampling sessions and any bugs > in keeping that field updated would be quite disconcerting and > cumbersome to debug. Couldn't the same could be said for connector->state->crtc? Sean > > crc_pipe's advantage is that we wouldn't need to update all callers of > drm_dp_aux_register, but I think it's better to have a connector field > that is mistakenly NULL, than a crc_pipe field with the wrong value. > > Regards, > > Tomeu > >> Sean >> >>> struct mutex hw_mutex; >>> ssize_t (*transfer)(struct drm_dp_aux *aux, >>> struct drm_dp_aux_msg *msg); >>> -- >>> 2.9.3 >>> >> >> >> >> -- >> Sean Paul, Software Engineer, Google / Chromium OS -- Sean Paul, Software Engineer, Google / Chromium OS