Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753542AbcJOJK7 (ORCPT ); Sat, 15 Oct 2016 05:10:59 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:44068 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751148AbcJOJKv (ORCPT ); Sat, 15 Oct 2016 05:10:51 -0400 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 9C98E616FD Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=architt@codeaurora.org Subject: Re: [PATCH] drm/arcpgu: Accommodate adv7511 switch to DRM bridge To: Eugeniy Paltsev , dri-devel@lists.freedesktop.org References: <1476451653-8179-1-git-send-email-Eugeniy.Paltsev@synopsys.com> Cc: linux-kernel@vger.kernel.org, airlied@linux.ie, linux-snps-arc@lists.infradead.org, Alexey.Brodkin@synopsys.com From: Archit Taneja Message-ID: <009fbf0a-f79e-1708-f2ed-66a55cb1e95e@codeaurora.org> Date: Sat, 15 Oct 2016 14:40:04 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1476451653-8179-1-git-send-email-Eugeniy.Paltsev@synopsys.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9729 Lines: 296 Hi, On 10/14/2016 6:57 PM, Eugeniy Paltsev wrote: > ARC PGU driver starts crashing on initialization after > 'commit e12c2f645557 ("drm/i2c: adv7511: Convert to drm_bridge")' > This happenes because in "arcpgu_drm_hdmi_init" function we get pointer > of "drm_i2c_encoder_driver" structure, which doesn't exist after > adv7511 hdmi encoder interface changed from slave encoder to drm bridge. > So, when we call "encoder_init" function from this structure driver > crashes. > The conversion doesn't seem entirely correct. Some comments below. > Bootlog: > ------------------------------------->8-------------------------------- > [drm] Initialized drm 1.1.0 20060810 > arcpgu e0017000.pgu: arc_pgu ID: 0xabbabaab > arcpgu e0017000.pgu: assigned reserved memory node frame_buffer@9e000000 > Path: (null) > CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-00001-gb5642252fa01-dirty #8 > task: 9a058000 task.stack: 9a032000 > > [ECR ]: 0x00220100 => Invalid Read @ 0x00000004 by insn @ 0x803934e8 > [EFA ]: 0x00000004 > [BLINK ]: drm_atomic_helper_connector_dpms+0xa6/0x230 > [ERET ]: drm_atomic_helper_connector_dpms+0xa4/0x230 > [STAT32]: 0x00000846 : K DE E2 E1 > BTA: 0x8016d949 SP: 0x9a033e34 FP: 0x00000000 > LPS: 0x8036f6fc LPE: 0x8036f700 LPC: 0x00000000 > r00: 0x8063c118 r01: 0x805b98ac r02: 0x00000b11 > r03: 0x00000000 r04: 0x9a010f54 r05: 0x00000000 > r06: 0x00000001 r07: 0x00000000 r08: 0x00000028 > r09: 0x00000001 r10: 0x00000007 r11: 0x00000054 > r12: 0x720a3033 > > Stack Trace: > drm_atomic_helper_connector_dpms+0xa4/0x230 > arcpgu_drm_hdmi_init+0xbc/0x228 > arcpgu_probe+0x168/0x244 > platform_drv_probe+0x26/0x64 > really_probe+0x1f0/0x32c > __driver_attach+0xa8/0xd0 > bus_for_each_dev+0x3c/0x74 > bus_add_driver+0xc2/0x184 > driver_register+0x50/0xec > do_one_initcall+0x3a/0x120 > kernel_init_freeable+0x108/0x1a0 > ------------------------------------->8-------------------------------- > > Fix ARC PGU driver to be able work with drm bridge hdmi encoder > interface. The hdmi connector code isn't needed anymore as we expect > the adv7511 bridge driver to create/manage the connector. > > Signed-off-by: Eugeniy Paltsev > --- > drivers/gpu/drm/arc/arcpgu_hdmi.c | 144 +++++++++----------------------------- > 1 file changed, 35 insertions(+), 109 deletions(-) > > diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c > index b7a8b2a..48a6c63 100644 > --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c > +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c > @@ -14,104 +14,54 @@ > * > */ > > +#include > #include > #include > #include > > #include "arcpgu.h" > > -struct arcpgu_drm_connector { > - struct drm_connector connector; > - struct drm_encoder_slave *encoder_slave; > -}; > - > -static int arcpgu_drm_connector_get_modes(struct drm_connector *connector) > +static void arcpgu_drm_i2c_encoder_mode_set(struct drm_encoder *encoder, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > { > - const struct drm_encoder_slave_funcs *sfuncs; > - struct drm_encoder_slave *slave; > - struct arcpgu_drm_connector *con = > - container_of(connector, struct arcpgu_drm_connector, connector); > - > - slave = con->encoder_slave; > - if (slave == NULL) { > - dev_err(connector->dev->dev, > - "connector_get_modes: cannot find slave encoder for connector\n"); > - return 0; > - } > - > - sfuncs = slave->slave_funcs; > - if (sfuncs->get_modes == NULL) > - return 0; > - > - return sfuncs->get_modes(&slave->base, connector); > -} > - > -static enum drm_connector_status > -arcpgu_drm_connector_detect(struct drm_connector *connector, bool force) > -{ > - enum drm_connector_status status = connector_status_unknown; > - const struct drm_encoder_slave_funcs *sfuncs; > - struct drm_encoder_slave *slave; > - > - struct arcpgu_drm_connector *con = > - container_of(connector, struct arcpgu_drm_connector, connector); > - > - slave = con->encoder_slave; > - if (slave == NULL) { > - dev_err(connector->dev->dev, > - "connector_detect: cannot find slave encoder for connector\n"); > - return status; > - } > + struct drm_bridge *bridge = encoder->bridge; > > - sfuncs = slave->slave_funcs; > - if (sfuncs && sfuncs->detect) > - return sfuncs->detect(&slave->base, connector); > - > - dev_err(connector->dev->dev, "connector_detect: could not detect slave funcs\n"); > - return status; > + bridge->funcs->mode_set(bridge, mode, adjusted_mode); The bridge functions shouldn't really be called by the kms driver. They're called automatically by the drm core for the bridge attached to the encoder. See mode_fixup in drivers/gpu/drm/drm_atomic_helper.c > } > > -static void arcpgu_drm_connector_destroy(struct drm_connector *connector) > -{ > - drm_connector_unregister(connector); > - drm_connector_cleanup(connector); > -} > - > -static const struct drm_connector_helper_funcs > -arcpgu_drm_connector_helper_funcs = { > - .get_modes = arcpgu_drm_connector_get_modes, > -}; > - > -static const struct drm_connector_funcs arcpgu_drm_connector_funcs = { > - .dpms = drm_helper_connector_dpms, > - .reset = drm_atomic_helper_connector_reset, > - .detect = arcpgu_drm_connector_detect, > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = arcpgu_drm_connector_destroy, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > static struct drm_encoder_helper_funcs arcpgu_drm_encoder_helper_funcs = { > .dpms = drm_i2c_encoder_dpms, > .mode_fixup = drm_i2c_encoder_mode_fixup, > - .mode_set = drm_i2c_encoder_mode_set, > + .mode_set = arcpgu_drm_i2c_encoder_mode_set, > .prepare = drm_i2c_encoder_prepare, > .commit = drm_i2c_encoder_commit, > - .detect = drm_i2c_encoder_detect, > }; > > static struct drm_encoder_funcs arcpgu_drm_encoder_funcs = { > .destroy = drm_encoder_cleanup, > }; > > +static void arcpgu_drm_i2c_encoder_dpms(struct drm_encoder *encoder, int mode) > +{ > + struct drm_bridge *bridge = encoder->bridge; > + > + if (mode == DRM_MODE_DPMS_ON) > + bridge->funcs->enable(bridge); > + else > + bridge->funcs->disable(bridge); > +} > + > +static struct drm_encoder_slave_funcs arcpgu_drm_encoder_slave_funcs = { > + .dpms = arcpgu_drm_i2c_encoder_dpms, > +}; > + > int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) > { > - struct arcpgu_drm_connector *arcpgu_connector; > - struct drm_i2c_encoder_driver *driver; > struct drm_encoder_slave *encoder; We should get rid of the drm_encoder_slave interface here entirely. A regular drm_encoder should be created here, with mostly noop funcs so that it doesn't do anything and lets the bridge manage things. > - struct drm_connector *connector; > struct i2c_client *i2c_slave; > + struct drm_bridge *bridge; > + > int ret; > > encoder = devm_kzalloc(drm->dev, sizeof(*encoder), GFP_KERNEL); > @@ -129,16 +79,14 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) > return -EPROBE_DEFER; > } > > - driver = > - to_drm_i2c_encoder_driver(to_i2c_driver(i2c_slave->dev.driver)); > - ret = driver->encoder_init(i2c_slave, drm, encoder); > - if (ret) { > - dev_err(drm->dev, "failed to initialize i2c encoder slave\n"); > - return ret; > - } > + /* Locate drm bridge from the hdmi encoder DT node */ > + bridge = of_drm_find_bridge(np); > + if (!bridge) > + return -EPROBE_DEFER; > > encoder->base.possible_crtcs = 1; > encoder->base.possible_clones = 0; > + encoder->slave_funcs = &arcpgu_drm_encoder_slave_funcs; This should go away too. > ret = drm_encoder_init(drm, &encoder->base, &arcpgu_drm_encoder_funcs, > DRM_MODE_ENCODER_TMDS, NULL); > if (ret) > @@ -147,37 +95,15 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np) > drm_encoder_helper_add(&encoder->base, > &arcpgu_drm_encoder_helper_funcs); > > - arcpgu_connector = devm_kzalloc(drm->dev, sizeof(*arcpgu_connector), > - GFP_KERNEL); > - if (!arcpgu_connector) { > - ret = -ENOMEM; > - goto error_encoder_cleanup; > - } > + /* Link drm_bridge to encoder */ > + bridge->encoder = &encoder->base; This will simply become "bridge->encoder = encoder;" > + encoder->base.bridge = bridge; and this "encoder->bridge = bridge;" You can have a look at the following patches which did the same for rcar-du driver: drm: rcar-du: Remove i2c slave encoder interface for hdmi encoder and drm: rcar-du: Link HDMI encoder with bridge Thanks, Archit > > - connector = &arcpgu_connector->connector; > - drm_connector_helper_add(connector, &arcpgu_drm_connector_helper_funcs); > - ret = drm_connector_init(drm, connector, &arcpgu_drm_connector_funcs, > - DRM_MODE_CONNECTOR_HDMIA); > - if (ret < 0) { > - dev_err(drm->dev, "failed to initialize drm connector\n"); > - goto error_encoder_cleanup; > - } > - > - ret = drm_mode_connector_attach_encoder(connector, &encoder->base); > - if (ret < 0) { > - dev_err(drm->dev, "could not attach connector to encoder\n"); > - drm_connector_unregister(connector); > - goto error_connector_cleanup; > + ret = drm_bridge_attach(drm, bridge); > + if (ret) { > + drm_encoder_cleanup(&encoder->base); > + return ret; > } > > - arcpgu_connector->encoder_slave = encoder; > - > return 0; > - > -error_connector_cleanup: > - drm_connector_cleanup(connector); > - > -error_encoder_cleanup: > - drm_encoder_cleanup(&encoder->base); > - return ret; > } > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project