* Re: [RFC 0/5] Generic panel framework
From: Tomi Valkeinen @ 2012-08-20 11:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <4948190.AFNtaaFKXQ@avalon>
[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]
On Sat, 2012-08-18 at 03:16 +0200, Laurent Pinchart wrote:
> Hi Tomi,
> mipi-dbi-bus might not belong to include/video/panel/ though, as it can be
> used for non-panel devices (at least in theory). The future mipi-dsi-bus
> certainly will.
They are both display busses. So while they could be used for anything,
I find it quite unlikely as there are much better alternatives for
generic bus needs.
> Would you be able to send incremental patches on top of v2 to implement the
> solution you have in mind ? It would be neat if you could also implement mipi-
> dsi-bus for the OMAP DSS and test the code with a real device :-)
Yes, I'd like to try this out on OMAP, both DBI and DSI. However, I fear
it'll be quite complex due to the dependencies all around we have in the
current driver. We're working on simplifying things so that it'll be
easier to try thing like the panel framework, though, so we're going in
the right direction.
> > Generally about locks, if we define that panel ops may only be called
> > exclusively, does it simplify things? I think we can make such
> > requirements, as there should be only one display framework that handles
> > the panel. Then we don't need locking for things like enable/disable.
>
> Pushing locking to callers would indeed simplify panel drivers, but we need to
> make sure we won't need to expose a panel to several callers in the future.
I have a feeling that would be a bad idea.
Display related stuff are quite sensitive to any delays, so any extra
transactions over, say, DSI bus could cause a noticeable glitch on the
screen. I'm not sure what are all the possible ops that a panel can
offer, but I think all that affect the display or could cause delays
should be handled by one controlling entity (drm or such). The
controlling entity needs to handle locking anyway, so in that sense I
don't think it's an extra burden for it.
The things that come to my mind that could possibly cause calls to the
panel outside drm: debugfs, sysfs, audio, backlight. Of those, I think
backlight should go through drm. Audio, no idea. debugfs and sysfs
locking needs to be handled by the panel driver, and they are a bit
problematic as I guess having them requires full locking.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Mahapatra, Chandrabhanu @ 2012-08-20 10:48 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345452124.2684.7.camel@deskari>
On Mon, Aug 20, 2012 at 2:12 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Fri, 2012-08-17 at 16:54 +0300, Tomi Valkeinen wrote:
>> On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote:
>> > All the cpu_is checks have been moved to dss_init_features function providing a
>> > much more generic and cleaner interface. The OMAP version and revision specific
>> > initializations in various functions are cleaned and the necessary data are
>> > moved to dss_features structure which is local to dss.c.
>> >
>> > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>>
>> > +static int __init dss_init_features(struct device *dev)
>> > +{
>> > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
>> > + if (!dss.feat) {
>> > + dev_err(dev, "Failed to allocate local DSS Features\n");
>> > + return -ENOMEM;
>> > + }
>> > +
>> > + if (cpu_is_omap24xx())
>> > + dss.feat = &omap24xx_dss_features;
>> > + else if (cpu_is_omap34xx())
>> > + dss.feat = &omap34xx_dss_features;
>> > + else if (cpu_is_omap3630())
>> > + dss.feat = &omap3630_dss_features;
>> > + else if (cpu_is_omap44xx())
>> > + dss.feat = &omap44xx_dss_features;
>> > + else
>> > + return -ENODEV;
>> > +
>> > + return 0;
>> > +}
>>
>> This is not correct (and same problem in dispc). You allocate the feat
>> struct and assign the pointer to dss.feat, but then overwrite dss.feat
>> pointer with the pointer to omap24xx_dss_features (which is freed
>> later). You need to memcpy it.
>>
Ok.
>> I also get a crash on omap3 overo board when loading omapdss:
>
During recent tests I never saw the crash happen. The kernel used to
freeze during booting after printing
"Uncompressing Linux... done, booting the kernel."
> The crash happens because dss_get_clocks uses the feat stuff, but the
> dss_init_features is only called later. Did you test this? I can't see
> how that can work on any board.
>
Thanks for pointing that out. Correcting above fixed the problem.
> Also, in the current place you have dss_init_features call, in case
> there's an error you can't just return, you need to release the
> resources. The same problem is in the dispc.c.
>
> Tomi
>
By releasing resources did you mean the dss.feat memory? I think
dss_init_features call is wrongly placed and should be placed at the
very beginning of the probe function.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-20 10:46 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAtePhxTsoCyatfXWR+e4sc3WHmg=Wq_ZXkowoGpOg63FA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
On Mon, 2012-08-20 at 16:06 +0530, Mahapatra, Chandrabhanu wrote:
> By releasing resources did you mean the dss.feat memory? I think
> dss_init_features call is wrongly placed and should be placed at the
> very beginning of the probe function.
I meant e.g. pm_runtime_disable() etc. But if the call is moved t the
beginning, there's nothing to release.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 03/10] video: exynos_dp: Clean up SW link training
From: Jingoo Han @ 2012-08-20 9:35 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-4-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> Clean up some of the SW training code to make it more clear and reduce
> duplicate code.
Clean up code of the SW training code is not necessary.
I cleaned the SW training code, at the patch that I sent as below.
http://www.spinics.net/lists/linux-fbdev/msg06849.html
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 180 ++++++++++++---------------------
> 1 files changed, 67 insertions(+), 113 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index 1836e33..3deded2 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -260,10 +260,8 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
>
> static int exynos_dp_link_start(struct exynos_dp_device *dp)
> {
> - int ret;
> - u8 buf[5];
> - int lane;
> - int lane_count;
> + int ret, lane, lane_count;
> + u8 buf[4];
>
> lane_count = dp->link_train.lane_count;
>
> @@ -286,8 +284,7 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
> /* Setup RX configuration */
> buf[0] = dp->link_train.link_rate;
> buf[1] = dp->link_train.lane_count;
> - ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
> - 2, buf);
> + ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET, 2, buf);
> if (ret)
> return ret;
>
> @@ -300,16 +297,15 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
> exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
>
> /* Set RX training pattern */
> - buf[0] = DPCD_SCRAMBLING_DISABLED |
> - DPCD_TRAINING_PATTERN_1;
> - exynos_dp_write_byte_to_dpcd(dp,
> - DPCD_ADDR_TRAINING_PATTERN_SET, buf[0]);
> + ret = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_TRAINING_PATTERN_SET,
> + DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_1);
> + if (ret)
> + return ret;
>
> for (lane = 0; lane < lane_count; lane++)
> buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
> DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
> - ret = exynos_dp_write_bytes_to_dpcd(dp,
> - DPCD_ADDR_TRAINING_LANE0_SET,
> + ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
> lane_count, buf);
> if (ret)
> return ret;
> @@ -484,67 +480,32 @@ static int exynos_dp_check_max_cr_loop(struct exynos_dp_device *dp,
>
> static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
> {
> - int ret;
> - u8 data;
> - u8 link_status[6];
> - int lane;
> - int lane_count;
> - u8 buf[5];
> -
> - u8 adjust_request[2];
> - u8 voltage_swing;
> - u8 pre_emphasis;
> - u8 training_lane;
> + int ret, lane, lane_count;
> + u8 voltage_swing, pre_emphasis, training_lane, link_status[6];
> + u8 *adjust_request;
>
> usleep_range(100, 101);
>
> - ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
> - 6, link_status);
> + ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS, 6,
> + link_status);
> if (ret)
> return ret;
>
> lane_count = dp->link_train.lane_count;
> + adjust_request = link_status + 4;
>
> if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> /* set training pattern 2 for EQ */
> exynos_dp_set_training_pattern(dp, TRAINING_PTN2);
>
> - adjust_request[0] = link_status[4];
> - adjust_request[1] = link_status[5];
> -
> - exynos_dp_get_adjust_train(dp, adjust_request);
> -
> - buf[0] = DPCD_SCRAMBLING_DISABLED |
> - DPCD_TRAINING_PATTERN_2;
> ret = exynos_dp_write_byte_to_dpcd(dp,
> DPCD_ADDR_TRAINING_PATTERN_SET,
> - buf[0]);
> + DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_2);
> if (ret)
> return ret;
>
> - for (lane = 0; lane < lane_count; lane++) {
> - exynos_dp_set_lane_link_training(dp,
> - dp->link_train.training_lane[lane],
> - lane);
> - buf[lane] = dp->link_train.training_lane[lane];
> - ret = exynos_dp_write_byte_to_dpcd(dp,
> - DPCD_ADDR_TRAINING_LANE0_SET + lane,
> - buf[lane]);
> - if (ret)
> - return ret;
> - }
> dp->link_train.lt_state = EQUALIZER_TRAINING;
> } else {
> - exynos_dp_read_byte_from_dpcd(dp,
> - DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
> - &data);
> - adjust_request[0] = data;
> -
> - exynos_dp_read_byte_from_dpcd(dp,
> - DPCD_ADDR_ADJUST_REQUEST_LANE2_3,
> - &data);
> - adjust_request[1] = data;
> -
> for (lane = 0; lane < lane_count; lane++) {
> training_lane = exynos_dp_get_lane_link_training(
> dp, lane);
> @@ -560,36 +521,31 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
>
> if (exynos_dp_check_max_cr_loop(dp, voltage_swing) != 0) {
> exynos_dp_reduce_link_rate(dp);
> - } else {
> - exynos_dp_get_adjust_train(dp, adjust_request);
> -
> - for (lane = 0; lane < lane_count; lane++) {
> - exynos_dp_set_lane_link_training(dp,
> - dp->link_train.training_lane[lane],
> - lane);
> - buf[lane] = dp->link_train.training_lane[lane];
> - ret = exynos_dp_write_byte_to_dpcd(dp,
> - DPCD_ADDR_TRAINING_LANE0_SET + lane,
> - buf[lane]);
> - if (ret)
> - return ret;
> - }
> + return ret;
> }
> }
>
> + exynos_dp_get_adjust_train(dp, adjust_request);
> +
> + for (lane = 0; lane < lane_count; lane++) {
> + exynos_dp_set_lane_link_training(dp,
> + dp->link_train.training_lane[lane], lane);
> + ret = exynos_dp_write_byte_to_dpcd(dp,
> + DPCD_ADDR_TRAINING_LANE0_SET + lane,
> + dp->link_train.training_lane[lane]);
> + if (ret)
> + return ret;
> + }
> +
> return ret;
> }
>
> static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> {
> - int ret;
> + int ret, lane, lane_count;
> u8 link_status[6];
> - int lane;
> - int lane_count;
> - u8 buf[5];
> u32 reg;
> -
> - u8 adjust_request[2];
> + u8 *adjust_request;
>
> usleep_range(400, 401);
>
> @@ -597,55 +553,53 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> 6, link_status);
> if (ret)
> return ret;
> +
> + adjust_request = link_status + 4;
> lane_count = dp->link_train.lane_count;
>
> - if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> - adjust_request[0] = link_status[4];
> - adjust_request[1] = link_status[5];
> + if (exynos_dp_clock_recovery_ok(link_status, lane_count)) {
> + exynos_dp_reduce_link_rate(dp);
> + return ret;
> + }
> + if (exynos_dp_channel_eq_ok(link_status, lane_count) = 0) {
> + /* traing pattern Set to Normal */
> + exynos_dp_training_pattern_dis(dp);
>
> - if (exynos_dp_channel_eq_ok(link_status, lane_count) = 0) {
> - /* traing pattern Set to Normal */
> - exynos_dp_training_pattern_dis(dp);
> + dev_info(dp->dev, "Link Training success!\n");
>
> - dev_info(dp->dev, "Link Training success!\n");
> + exynos_dp_get_link_bandwidth(dp, ®);
> + dp->link_train.link_rate = reg;
> + dev_dbg(dp->dev, "final bandwidth = %.2x\n",
> + dp->link_train.link_rate);
>
> - exynos_dp_get_link_bandwidth(dp, ®);
> - dp->link_train.link_rate = reg;
> - dev_dbg(dp->dev, "final bandwidth = %.2x\n",
> - dp->link_train.link_rate);
> + exynos_dp_get_lane_count(dp, ®);
> + dp->link_train.lane_count = reg;
> + dev_dbg(dp->dev, "final lane count = %.2x\n",
> + dp->link_train.lane_count);
> + /* set enhanced mode if available */
> + exynos_dp_set_enhanced_mode(dp);
>
> - exynos_dp_get_lane_count(dp, ®);
> - dp->link_train.lane_count = reg;
> - dev_dbg(dp->dev, "final lane count = %.2x\n",
> - dp->link_train.lane_count);
> - /* set enhanced mode if available */
> - exynos_dp_set_enhanced_mode(dp);
> + dp->link_train.lt_state = FINISHED;
> + } else {
> + /* not all locked */
> + dp->link_train.eq_loop++;
>
> - dp->link_train.lt_state = FINISHED;
> + if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
> + exynos_dp_reduce_link_rate(dp);
> } else {
> - /* not all locked */
> - dp->link_train.eq_loop++;
> -
> - if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
> - exynos_dp_reduce_link_rate(dp);
> - } else {
> - exynos_dp_get_adjust_train(dp, adjust_request);
> -
> - for (lane = 0; lane < lane_count; lane++) {
> - exynos_dp_set_lane_link_training(dp,
> - dp->link_train.training_lane[lane],
> - lane);
> - buf[lane] = dp->link_train.training_lane[lane];
> - ret = exynos_dp_write_byte_to_dpcd(dp,
> - DPCD_ADDR_TRAINING_LANE0_SET + lane,
> - buf[lane]);
> - if (ret)
> - return ret;
> - }
> + exynos_dp_get_adjust_train(dp, adjust_request);
> +
> + for (lane = 0; lane < lane_count; lane++) {
> + exynos_dp_set_lane_link_training(dp,
> + dp->link_train.training_lane[lane],
> + lane);
> + ret = exynos_dp_write_byte_to_dpcd(dp,
> + DPCD_ADDR_TRAINING_LANE0_SET + lane,
> + dp->link_train.training_lane[lane]);
> + if (ret)
> + return ret;
> }
> }
> - } else {
> - exynos_dp_reduce_link_rate(dp);
> }
>
> return ret;
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 10/10] video: exynos_dp: Enable hotplug interrupts
From: Jingoo Han @ 2012-08-20 9:29 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-11-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> Enable hotplug interrupts and move the hotplug scheduling into the
> interrupt handler. This allows us to introduce a screen at any time
> while we're running.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Olof Johansson <olofj@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 40 +++++++++++++++++++++++++-------
> drivers/video/exynos/exynos_dp_core.h | 9 +++++++
> drivers/video/exynos/exynos_dp_reg.c | 35 ++++++++++++++++++++++++++++-
> 3 files changed, 74 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index eb5816f..5279027 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -47,10 +47,6 @@ static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
> {
> int timeout_loop = 0;
>
> - exynos_dp_init_hpd(dp);
> -
> - usleep_range(200, 210);
> -
> while (exynos_dp_get_plug_in_status(dp) != 0) {
> timeout_loop++;
> if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> @@ -829,8 +825,32 @@ static void exynos_dp_enable_scramble(struct exynos_dp_device *dp, bool enable)
> static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
> {
> struct exynos_dp_device *dp = arg;
> -
> - dev_err(dp->dev, "exynos_dp_irq_handler\n");
> + enum dp_irq_type irq_type;
> +
> + irq_type = exynos_dp_get_irq_type(dp);
> + switch (irq_type) {
> + case DP_IRQ_TYPE_HP_CABLE_IN:
> + dev_dbg(dp->dev, "Received irq - cable in\n");
> + schedule_work(&dp->hotplug_work);
> + exynos_dp_clear_hotplug_interrupts(dp);
> + break;
> + case DP_IRQ_TYPE_HP_CABLE_OUT:
> + dev_dbg(dp->dev, "Received irq - cable out\n");
> + exynos_dp_clear_hotplug_interrupts(dp);
> + break;
> + case DP_IRQ_TYPE_HP_CHANGE:
> + /*
> + * We get these change notifications once in a while, but there
> + * is nothing we can do with them. Just ignore it for now and
> + * only handle cable changes.
> + */
> + dev_dbg(dp->dev, "Received irq - hotplug change; ignoring.\n");
> + exynos_dp_clear_hotplug_interrupts(dp);
> + break;
> + default:
> + dev_err(dp->dev, "Received irq - unknown type!\n");
> + break;
> + }
> return IRQ_HANDLED;
> }
>
> @@ -843,7 +863,7 @@ static void exynos_dp_hotplug(struct work_struct *work)
>
> ret = exynos_dp_detect_hpd(dp);
> if (ret) {
> - dev_err(dp->dev, "unable to detect hpd\n");
> + /* Cable has been disconnected, we're done */
> return;
> }
>
> @@ -941,7 +961,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> exynos_dp_init_dp(dp);
>
> platform_set_drvdata(pdev, dp);
> - schedule_work(&dp->hotplug_work);
>
> return 0;
>
> @@ -975,6 +994,8 @@ static int exynos_dp_suspend(struct device *dev)
> struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>
> + disable_irq(dp->irq);
> +
> if (work_pending(&dp->hotplug_work))
> flush_work_sync(&dp->hotplug_work);
>
> @@ -998,7 +1019,8 @@ static int exynos_dp_resume(struct device *dev)
> clk_enable(dp->clock);
>
> exynos_dp_init_dp(dp);
> - schedule_work(&dp->hotplug_work);
> +
> + enable_irq(dp->irq);
>
> return 0;
> }
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index cf1010b..a5795dd 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -13,6 +13,13 @@
> #ifndef _EXYNOS_DP_CORE_H
> #define _EXYNOS_DP_CORE_H
>
> +enum dp_irq_type {
> + DP_IRQ_TYPE_HP_CABLE_IN,
> + DP_IRQ_TYPE_HP_CABLE_OUT,
> + DP_IRQ_TYPE_HP_CHANGE,
> + DP_IRQ_TYPE_UNKNOWN,
> +};
> +
> struct link_train {
> int eq_loop;
> int cr_loop[4];
> @@ -51,6 +58,8 @@ void exynos_dp_set_analog_power_down(struct exynos_dp_device *dp,
> bool enable);
> void exynos_dp_init_analog_func(struct exynos_dp_device *dp);
> void exynos_dp_init_hpd(struct exynos_dp_device *dp);
> +enum dp_irq_type exynos_dp_get_irq_type(struct exynos_dp_device *dp);
> +void exynos_dp_clear_hotplug_interrupts(struct exynos_dp_device *dp);
> void exynos_dp_reset_aux(struct exynos_dp_device *dp);
> void exynos_dp_init_aux(struct exynos_dp_device *dp);
> int exynos_dp_get_plug_in_status(struct exynos_dp_device *dp);
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index 389e0f0..5906fc7 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -327,7 +327,7 @@ void exynos_dp_init_analog_func(struct exynos_dp_device *dp)
> writel(reg, dp->reg_base + EXYNOS_DP_FUNC_EN_2);
> }
>
> -void exynos_dp_init_hpd(struct exynos_dp_device *dp)
> +void exynos_dp_clear_hotplug_interrupts(struct exynos_dp_device *dp)
> {
> u32 reg;
>
> @@ -336,10 +336,43 @@ void exynos_dp_init_hpd(struct exynos_dp_device *dp)
>
> reg = INT_HPD;
> writel(reg, dp->reg_base + EXYNOS_DP_INT_STA);
> +}
> +
> +void exynos_dp_init_hpd(struct exynos_dp_device *dp)
> +{
> + u32 reg;
> +
> + exynos_dp_clear_hotplug_interrupts(dp);
>
> reg = readl(dp->reg_base + EXYNOS_DP_SYS_CTL_3);
> reg &= ~(F_HPD | HPD_CTRL);
> writel(reg, dp->reg_base + EXYNOS_DP_SYS_CTL_3);
> +
> + /* Unmask hotplug interrupts */
> + reg = HOTPLUG_CHG | HPD_LOST | PLUG;
> + writel(reg, dp->reg_base + EXYNOS_DP_COMMON_INT_MASK_4);
Please don't handle mask bits in init_hpd.
This masking bits are easily changed by modifying definition of masking bits
as below:
- #define COMMON_INT_MASK_4 (0)
+ #define COMMON_INT_MASK_4 (HOTPLUG_CHG | HPD_LOST | PLUG)
Then, it can be used in exynos_dp_config_interrupt().
> +
> + reg = INT_HPD;
> + writel(reg, dp->reg_base + EXYNOS_DP_INT_STA_MASK);
> +}
> +
> +enum dp_irq_type exynos_dp_get_irq_type(struct exynos_dp_device *dp)
> +{
> + u32 reg;
> +
> + /* Parse hotplug interrupt status register */
> + reg = readl(dp->reg_base + EXYNOS_DP_COMMON_INT_STA_4);
> +
> + if (reg & PLUG)
> + return DP_IRQ_TYPE_HP_CABLE_IN;
> +
> + if (reg & HPD_LOST)
> + return DP_IRQ_TYPE_HP_CABLE_OUT;
> +
> + if (reg & HOTPLUG_CHG)
> + return DP_IRQ_TYPE_HP_CHANGE;
> +
> + return DP_IRQ_TYPE_UNKNOWN;
> }
>
> void exynos_dp_reset_aux(struct exynos_dp_device *dp)
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 09/10] video: exynos_dp: Fix bug when checking dp->irq
From: Jingoo Han @ 2012-08-20 9:24 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-10-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
> -----Original Message-----
> From: Sean Paul [mailto:seanpaul@chromium.org]
> Sent: Wednesday, August 08, 2012 12:54 PM
> To: jg1.han@samsung.com; linux-fbdev@vger.kernel.org
> Cc: Sean Paul
> Subject: [PATCH 09/10] video: exynos_dp: Fix bug when checking dp->irq
>
> Fix a bug where we check !dp->irq instead of the correct check for
> -ENXIO.
>
> BUG=None
> TEST=None
>
> Change-Id: Ibcdd16fb71eb41e4380ac8911ad6063634ff80e2
Remove unnecessary commit message such as 'Change-Id',
'BUG', and 'TEST'.
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-on: https://gerrit.chromium.org/gerrit/29144
> Reviewed-by: Olof Johansson <olofj@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index 68ad494..eb5816f 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -919,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> }
>
> dp->irq = platform_get_irq(pdev, 0);
> - if (!dp->irq) {
> + if (dp->irq = -ENXIO) {
> dev_err(&pdev->dev, "failed to get irq\n");
> ret = -ENODEV;
> goto err_clock;
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 07/10] video: exynos_dp: Improve EDID error handling
From: Jingoo Han @ 2012-08-20 9:22 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-8-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> EDID error handling has 2 problems:
> - It doesn't fail as early as it can
> - The retry counts for i2c and aux transactions are huge
>
> This patch fails if the initial i2c transaction fails, and reduces the
> aux and i2c retry counts down to 3.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Olof Johansson <olofj@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 18 ++++++++++--------
> drivers/video/exynos/exynos_dp_reg.c | 9 ++++-----
> 2 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index 1c998d9..2882362 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -89,9 +89,11 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
> */
>
> /* Read Extension Flag, Number of 128-byte EDID extension blocks */
> - exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> + retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> EDID_EXTENSION_FLAG,
> &extend_block);
> + if (retval)
> + return retval;
>
> if (extend_block > 0) {
> dev_dbg(dp->dev, "EDID data includes a single extension!\n");
> @@ -177,21 +179,21 @@ static int exynos_dp_handle_edid(struct exynos_dp_device *dp)
> {
> u8 buf[12];
> int i;
> - int retval;
> + int ret;
>
> /* Read DPCD DPCD_ADDR_DPCD_REV~RECEIVE_PORT1_CAP_1 */
> - exynos_dp_read_bytes_from_dpcd(dp,
> - DPCD_ADDR_DPCD_REV,
> - 12, buf);
> + ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_DPCD_REV, 12, buf);
> + if (ret)
> + return ret;
Could you use 'retval' instead of 'ret'?
>
> /* Read EDID */
> for (i = 0; i < 3; i++) {
> - retval = exynos_dp_read_edid(dp);
> - if (retval = 0)
> + ret = exynos_dp_read_edid(dp);
> + if (!ret)
> break;
> }
>
> - return retval;
> + return ret;
> }
>
> static void exynos_dp_enable_rx_to_enhanced_mode(struct exynos_dp_device *dp,
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index d7b1494..389e0f0 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -547,7 +547,7 @@ int exynos_dp_write_bytes_to_dpcd(struct exynos_dp_device *dp,
> else
> cur_data_count = count - start_offset;
>
> - for (i = 0; i < 10; i++) {
> + for (i = 0; i < 3; i++) {
> /* Select DPCD device address */
> reg = AUX_ADDR_7_0(reg_addr + start_offset);
> writel(reg, dp->reg_base + EXYNOS_DP_AUX_ADDR_7_0);
> @@ -612,7 +612,7 @@ int exynos_dp_read_bytes_from_dpcd(struct exynos_dp_device *dp,
> cur_data_count = count - start_offset;
>
> /* AUX CH Request Transaction process */
> - for (i = 0; i < 10; i++) {
> + for (i = 0; i < 3; i++) {
> /* Select DPCD device address */
> reg = AUX_ADDR_7_0(reg_addr + start_offset);
> writel(reg, dp->reg_base + EXYNOS_DP_AUX_ADDR_7_0);
> @@ -695,7 +695,7 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
> int i;
> int retval;
>
> - for (i = 0; i < 10; i++) {
> + for (i = 0; i < 3; i++) {
> /* Clear AUX CH data buffer */
> reg = BUF_CLR;
> writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
> @@ -703,7 +703,6 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
> /* Select EDID device */
> retval = exynos_dp_select_i2c_device(dp, device_addr, reg_addr);
> if (retval != 0) {
> - dev_err(dp->dev, "Select EDID device fail!\n");
> continue;
> }
>
> @@ -745,7 +744,7 @@ int exynos_dp_read_bytes_from_i2c(struct exynos_dp_device *dp,
> int retval = 0;
>
> for (i = 0; i < count; i += 16) {
> - for (j = 0; j < 100; j++) {
> + for (j = 0; j < 3; j++) {
> /* Clear AUX CH data buffer */
> reg = BUF_CLR;
> writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 06/10] video: exynos_dp: Fix get_pll_lock_status return value
From: Jingoo Han @ 2012-08-20 9:20 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-7-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> Fix the return value of exynos_dp_get_pll_lock_status to
> reflect what it actually returns.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Olof Johansson <olofj@chromium.org>
Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---
> drivers/video/exynos/exynos_dp_core.h | 2 +-
> drivers/video/exynos/exynos_dp_reg.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 8526e54..6431c65 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -43,7 +43,7 @@ void exynos_dp_init_interrupt(struct exynos_dp_device *dp);
> void exynos_dp_reset(struct exynos_dp_device *dp);
> void exynos_dp_swreset(struct exynos_dp_device *dp);
> void exynos_dp_config_interrupt(struct exynos_dp_device *dp);
> -u32 exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp);
> +enum pll_status exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp);
> void exynos_dp_set_pll_power_down(struct exynos_dp_device *dp, bool enable);
> void exynos_dp_set_analog_power_down(struct exynos_dp_device *dp,
> enum analog_power_block block,
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index a121bed..d7b1494 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -179,7 +179,7 @@ void exynos_dp_config_interrupt(struct exynos_dp_device *dp)
> writel(reg, dp->reg_base + EXYNOS_DP_INT_STA_MASK);
> }
>
> -u32 exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp)
> +enum pll_status exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp)
> {
> u32 reg;
>
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 05/10] video: exynos_dp: Remove sink control to D0
From: Jingoo Han @ 2012-08-20 9:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-6-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> Don't reset the sink power to D0. Removing this for three reasons:
>
> 1) It's not required in the SW link training documentation
OK.
> 2) The comment is incorrect, D0 is normal operation, not "power down"
OK. As you mentioned, D0 is not 'power down', 'normal operation'.
However, if comment is wrong, usually we fix comment, not remove it. :)
> 3) It seems to change things in the link training that causes glitches
Um, it seems that this problem depends on LCD panel.
Other LCDs that I have tested do not have this kind of problem.
Please, modify this comment.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 6 ------
> 1 files changed, 0 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index 207bd7e..1c998d9 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -273,12 +273,6 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
> for (lane = 0; lane < lane_count; lane++)
> dp->link_train.cr_loop[lane] = 0;
>
> - /* Set sink to D0 (Sink Not Ready) mode. */
> - ret = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
> - DPCD_SET_POWER_STATE_D0);
> - if (ret)
> - return ret;
> -
> /* Set link rate and count as you want to establish*/
> exynos_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> exynos_dp_set_lane_count(dp, dp->link_train.lane_count);
> --
> 1.7.7.3
^ permalink raw reply
* RE: [PATCH 04/10] video: exynos_dp: Get pll lock before pattern set
From: Jingoo Han @ 2012-08-20 9:13 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-5-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> According to the exynos datasheet (Figure 49-10), we should wait for PLL
> lock before programming the training pattern when doing software eDP
> link training.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 14 +++++++++++++-
> 1 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index 3deded2..207bd7e 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -23,6 +23,8 @@
>
> #include "exynos_dp_core.h"
>
> +#define PLL_MAX_TRIES 100
> +
> static int exynos_dp_init_dp(struct exynos_dp_device *dp)
> {
> exynos_dp_reset(dp);
> @@ -260,7 +262,7 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
>
> static int exynos_dp_link_start(struct exynos_dp_device *dp)
> {
> - int ret, lane, lane_count;
> + int ret, lane, lane_count, pll_tries;
> u8 buf[4];
>
> lane_count = dp->link_train.lane_count;
> @@ -293,6 +295,16 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
> exynos_dp_set_lane_lane_pre_emphasis(dp,
> PRE_EMPHASIS_LEVEL_0, lane);
>
> + /* Wait for PLL lock */
> + pll_tries = 0;
> + while (exynos_dp_get_pll_lock_status(dp) = PLL_UNLOCKED) {
> + if (pll_tries = PLL_MAX_TRIES)
> + return -ETIMEDOUT;
> +
> + pll_tries++;
> + udelay(100);
> + }
> +
You don't need to add 'PLL_MAX_TRIES', please use 'DP_TIMEOUT_LOOP_COUNT' as below:
/* Wait for PLL lock */
while (exynos_dp_get_pll_lock_status(dp) = PLL_UNLOCKED) {
timeout_loop++;
if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
dev_err(dp->dev, "failed to get pll lock status\n");
return -ETIMEDOUT;
}
udelay(100);
}
Also, could you use usleep_range() instead of udelay()?
> /* Set training pattern 1 */
> exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
>
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 02/10] video: exynos_dp: Check DPCD return codes
From: Jingoo Han @ 2012-08-20 9:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-3-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> Add return code checks to the DPCD transactions in the SW link training.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
> ---
> drivers/video/exynos/exynos_dp_core.c | 65 ++++++++++++++++++++++-----------
> 1 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index c6c016a..1836e33 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -258,8 +258,9 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
> }
> }
>
> -static void exynos_dp_link_start(struct exynos_dp_device *dp)
> +static int exynos_dp_link_start(struct exynos_dp_device *dp)
> {
> + int ret;
> u8 buf[5];
> int lane;
> int lane_count;
> @@ -273,8 +274,10 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
> dp->link_train.cr_loop[lane] = 0;
>
> /* Set sink to D0 (Sink Not Ready) mode. */
> - exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
> + ret = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
> DPCD_SET_POWER_STATE_D0);
> + if (ret)
> + return ret;
Could you use 'retval' instead of 'ret'?
>
> /* Set link rate and count as you want to establish*/
> exynos_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
> @@ -283,8 +286,10 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
> /* Setup RX configuration */
> buf[0] = dp->link_train.link_rate;
> buf[1] = dp->link_train.lane_count;
> - exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
> + ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
> 2, buf);
> + if (ret)
> + return ret;
>
> /* Set TX pre-emphasis to minimum */
> for (lane = 0; lane < lane_count; lane++)
> @@ -303,9 +308,13 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
> for (lane = 0; lane < lane_count; lane++)
> buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
> DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
> - exynos_dp_write_bytes_to_dpcd(dp,
> + ret = exynos_dp_write_bytes_to_dpcd(dp,
> DPCD_ADDR_TRAINING_LANE0_SET,
> lane_count, buf);
> + if (ret)
> + return ret;
> +
> + return ret;
> }
>
> static unsigned char exynos_dp_get_lane_status(u8 link_status[6], int lane)
> @@ -475,6 +484,7 @@ static int exynos_dp_check_max_cr_loop(struct exynos_dp_device *dp,
>
> static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
> {
> + int ret;
> u8 data;
> u8 link_status[6];
> int lane;
> @@ -488,8 +498,11 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
>
> usleep_range(100, 101);
>
> - exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
> + ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
> 6, link_status);
> + if (ret)
> + return ret;
> +
> lane_count = dp->link_train.lane_count;
>
> if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> @@ -503,18 +516,22 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
>
> buf[0] = DPCD_SCRAMBLING_DISABLED |
> DPCD_TRAINING_PATTERN_2;
> - exynos_dp_write_byte_to_dpcd(dp,
> + ret = exynos_dp_write_byte_to_dpcd(dp,
> DPCD_ADDR_TRAINING_PATTERN_SET,
> buf[0]);
> + if (ret)
> + return ret;
>
> for (lane = 0; lane < lane_count; lane++) {
> exynos_dp_set_lane_link_training(dp,
> dp->link_train.training_lane[lane],
> lane);
> buf[lane] = dp->link_train.training_lane[lane];
> - exynos_dp_write_byte_to_dpcd(dp,
> + ret = exynos_dp_write_byte_to_dpcd(dp,
> DPCD_ADDR_TRAINING_LANE0_SET + lane,
> buf[lane]);
> + if (ret)
> + return ret;
> }
> dp->link_train.lt_state = EQUALIZER_TRAINING;
> } else {
> @@ -551,18 +568,21 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
> dp->link_train.training_lane[lane],
> lane);
> buf[lane] = dp->link_train.training_lane[lane];
> - exynos_dp_write_byte_to_dpcd(dp,
> + ret = exynos_dp_write_byte_to_dpcd(dp,
> DPCD_ADDR_TRAINING_LANE0_SET + lane,
> buf[lane]);
> + if (ret)
> + return ret;
> }
> }
> }
>
> - return 0;
> + return ret;
> }
>
> static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> {
> + int ret;
> u8 link_status[6];
> int lane;
> int lane_count;
> @@ -573,8 +593,10 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
>
> usleep_range(400, 401);
>
> - exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
> + ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
> 6, link_status);
> + if (ret)
> + return ret;
> lane_count = dp->link_train.lane_count;
>
> if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> @@ -614,9 +636,11 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> dp->link_train.training_lane[lane],
> lane);
> buf[lane] = dp->link_train.training_lane[lane];
> - exynos_dp_write_byte_to_dpcd(dp,
> + ret = exynos_dp_write_byte_to_dpcd(dp,
> DPCD_ADDR_TRAINING_LANE0_SET + lane,
> buf[lane]);
> + if (ret)
> + return ret;
> }
> }
> }
> @@ -624,7 +648,7 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> exynos_dp_reduce_link_rate(dp);
> }
>
> - return 0;
> + return ret;
> }
>
> static void exynos_dp_get_max_rx_bandwidth(struct exynos_dp_device *dp,
> @@ -692,28 +716,25 @@ static void exynos_dp_init_training(struct exynos_dp_device *dp,
>
> static int exynos_dp_sw_link_training(struct exynos_dp_device *dp)
> {
> - int retval = 0;
> - int training_finished;
> + int ret = 0, training_finished = 0;
>
> /* Turn off unnecessary lane */
> if (dp->link_train.lane_count = 1)
> exynos_dp_set_analog_power_down(dp, CH1_BLOCK, 1);
>
> - training_finished = 0;
> -
> dp->link_train.lt_state = START;
>
> /* Process here */
> - while (!training_finished) {
> + while (!ret && !training_finished) {
> switch (dp->link_train.lt_state) {
> case START:
> - exynos_dp_link_start(dp);
> + ret = exynos_dp_link_start(dp);
> break;
> case CLOCK_RECOVERY:
> - exynos_dp_process_clock_recovery(dp);
> + ret = exynos_dp_process_clock_recovery(dp);
> break;
> case EQUALIZER_TRAINING:
> - exynos_dp_process_equalizer_training(dp);
> + ret = exynos_dp_process_equalizer_training(dp);
> break;
> case FINISHED:
> training_finished = 1;
> @@ -722,8 +743,10 @@ static int exynos_dp_sw_link_training(struct exynos_dp_device *dp)
> return -EREMOTEIO;
> }
> }
> + if (ret)
> + dev_err(dp->dev, "eDP link training failed (%d)\n", ret);
>
> - return retval;
> + return ret;
> }
>
> static int exynos_dp_set_link_train(struct exynos_dp_device *dp,
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH 01/10] video: exynos_dp: Change aux transaction failures
From: Jingoo Han @ 2012-08-20 9:02 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-2-git-send-email-seanpaul@chromium.org>
On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>
> This patch adds the function name to aux transaction failure messages
> so we can tell which transaction is failing. It also changes the level
> of Aux Transaction fail messages from error to debug. We retry the
> transactions a few times and will report errors if warranted outside of
> this function.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
> ---
Acked-by: Jingoo Han <jg1.han@samsung.com>
It looks good.
> drivers/video/exynos/exynos_dp_reg.c | 21 ++++++++++++++-------
> 1 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index ce401c8..a121bed 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -471,7 +471,8 @@ int exynos_dp_write_byte_to_dpcd(struct exynos_dp_device *dp,
> if (retval = 0)
> break;
> else
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> + __func__);
> }
>
> return retval;
> @@ -511,7 +512,8 @@ int exynos_dp_read_byte_from_dpcd(struct exynos_dp_device *dp,
> if (retval = 0)
> break;
> else
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> + __func__);
> }
>
> /* Read data buffer */
> @@ -575,7 +577,8 @@ int exynos_dp_write_bytes_to_dpcd(struct exynos_dp_device *dp,
> if (retval = 0)
> break;
> else
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> + __func__);
> }
>
> start_offset += cur_data_count;
> @@ -632,7 +635,8 @@ int exynos_dp_read_bytes_from_dpcd(struct exynos_dp_device *dp,
> if (retval = 0)
> break;
> else
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> + __func__);
> }
>
> for (cur_data_idx = 0; cur_data_idx < cur_data_count;
> @@ -677,7 +681,7 @@ int exynos_dp_select_i2c_device(struct exynos_dp_device *dp,
> /* Start AUX transaction */
> retval = exynos_dp_start_aux_transaction(dp);
> if (retval != 0)
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n", __func__);
>
> return retval;
> }
> @@ -717,7 +721,8 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
> if (retval = 0)
> break;
> else
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
> + __func__);
> }
>
> /* Read data */
> @@ -777,7 +782,9 @@ int exynos_dp_read_bytes_from_i2c(struct exynos_dp_device *dp,
> if (retval = 0)
> break;
> else
> - dev_err(dp->dev, "Aux Transaction fail!\n");
> + dev_dbg(dp->dev,
> + "%s: Aux Transaction fail!\n",
> + __func__);
> }
> /* Check if Rx sends defer */
> reg = readl(dp->reg_base + EXYNOS_DP_AUX_RX_COMM);
> --
> 1.7.7.3
^ permalink raw reply
* Re: [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-20 8:42 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345211692.3158.160.camel@deskari>
[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]
On Fri, 2012-08-17 at 16:54 +0300, Tomi Valkeinen wrote:
> On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote:
> > All the cpu_is checks have been moved to dss_init_features function providing a
> > much more generic and cleaner interface. The OMAP version and revision specific
> > initializations in various functions are cleaned and the necessary data are
> > moved to dss_features structure which is local to dss.c.
> >
> > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>
> > +static int __init dss_init_features(struct device *dev)
> > +{
> > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
> > + if (!dss.feat) {
> > + dev_err(dev, "Failed to allocate local DSS Features\n");
> > + return -ENOMEM;
> > + }
> > +
> > + if (cpu_is_omap24xx())
> > + dss.feat = &omap24xx_dss_features;
> > + else if (cpu_is_omap34xx())
> > + dss.feat = &omap34xx_dss_features;
> > + else if (cpu_is_omap3630())
> > + dss.feat = &omap3630_dss_features;
> > + else if (cpu_is_omap44xx())
> > + dss.feat = &omap44xx_dss_features;
> > + else
> > + return -ENODEV;
> > +
> > + return 0;
> > +}
>
> This is not correct (and same problem in dispc). You allocate the feat
> struct and assign the pointer to dss.feat, but then overwrite dss.feat
> pointer with the pointer to omap24xx_dss_features (which is freed
> later). You need to memcpy it.
>
> I also get a crash on omap3 overo board when loading omapdss:
The crash happens because dss_get_clocks uses the feat stuff, but the
dss_init_features is only called later. Did you test this? I can't see
how that can work on any board.
Also, in the current place you have dss_init_features call, in case
there's an error you can't just return, you need to release the
resources. The same problem is in the dispc.c.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Alan Stern @ 2012-08-19 19:56 UTC (permalink / raw)
To: Bruno Prémont; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120819182300.63665a0b@neptune.home>
On Sun, 19 Aug 2012, Bruno Prémont wrote:
> > I don't know bout hid_hw_close(). Certainly no more reports should be
> > submitted following usbhid_stop().
>
> Ok, I did just that, prevent new calls to usbhid_submit_report(), after
> calling hid_hw_close(), fixed one bug in my code that triggers the NULL
> pointer dereference (calling hid_set_drvdata(hdev, NULL) too early).
>
> Now I'm still seeing the bad paging request in _mmx_memcpy(), though rather
> sporadically.
> The last ones I saw were during remove() around the time of calling hid_hw_close()
> and hid_hw_stop(). Adding a printk() between the two (at least while picoLCD
> is hosting fbcon) makes it very improbably for the bad page to happen.
>
> It looks like low-level driver did free memory in hid_hw_close() for some
> in-flight URB and thus things break in following USB interrupt.
No, memory is not freed in usbhid_close(). It is freed in
usbhid_stop().
> From mapping trace information to source it seems:
> usbhid/hid-core.c:
> static int hid_submit_out(struct hid_device *hid)
> {
> struct hid_report *report;
> char *raw_report;
> struct usbhid_device *usbhid = hid->driver_data;
> int r;
>
> report = usbhid->out[usbhid->outtail].report;
> raw_report = usbhid->out[usbhid->outtail].raw_report;
>
> usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
> 1 + (report->id > 0);
> usbhid->urbout->dev = hid_to_usb_dev(hid);
> if (raw_report) {
> memcpy(usbhid->outbuf, raw_report,
> usbhid->urbout->transfer_buffer_length);
> ^^^^^^^^^^^^^^^_ this is exploding
Right. Like I said, usbhid->outbuf is freed in hid_free_buffers(),
which is called from usbhid_stop().
Forget about usbhid_close(). The race is between hid_submit_out() and
usbhid_stop().
Alan Stern
^ permalink raw reply
* [PATCH 2/6] HID: picoLCD: rework hid-fbdev interaction
From: Bruno Prémont @ 2012-08-19 17:32 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120819192859.74136bb0@neptune.home>
Split out all FB related data out of struct picolcd_data into a struct
picolcd_fb_data that is allocated with fb_info. This way fb_info may
cleanly outlive struct picolcd_data for as long as needed for its last
user to drop his reference.
Access to struct picolcd_data is now protected with struct
picolcd_fb_data's lock and tile update reports are only generated
while picolcd_fbdata->picolcd is not NULL and is not marked as failed
(which indicates unplug in progress).
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
drivers/hid/hid-picolcd.h | 19 +++-
drivers/hid/hid-picolcd_fb.c | 216 ++++++++++++++++++++++--------------------
2 files changed, 126 insertions(+), 109 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 9200be1..dc4c795 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -90,11 +90,6 @@ struct picolcd_data {
#ifdef CONFIG_HID_PICOLCD_FB
/* Framebuffer stuff */
- u8 fb_update_rate;
- u8 fb_bpp;
- u8 fb_force;
- u8 *fb_vbitmap; /* local copy of what was sent to PicoLCD */
- u8 *fb_bitmap; /* framebuffer */
struct fb_info *fb_info;
#endif /* CONFIG_HID_PICOLCD_FB */
#ifdef CONFIG_HID_PICOLCD_LCD
@@ -119,9 +114,21 @@ struct picolcd_data {
int status;
#define PICOLCD_BOOTLOADER 1
#define PICOLCD_FAILED 2
-#define PICOLCD_READY_FB 4
};
+#ifdef CONFIG_HID_PICOLCD_FB
+struct picolcd_fb_data {
+ /* Framebuffer stuff */
+ spinlock_t lock;
+ struct picolcd_data *picolcd;
+ u8 update_rate;
+ u8 bpp;
+ u8 force;
+ u8 ready;
+ u8 *vbitmap; /* local copy of what was sent to PicoLCD */
+ u8 *bitmap; /* framebuffer */
+};
+#endif /* CONFIG_HID_PICOLCD_FB */
/* Find a given report */
#define picolcd_in_report(id, dev) picolcd_report(id, dev, HID_INPUT_REPORT)
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 4d8e22c..5905bbd 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -98,19 +98,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
};
/* Send a given tile to PicoLCD */
-static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
+static int picolcd_fb_send_tile(struct picolcd_data *data, u8 *vbitmap,
+ int chip, int tile)
{
- struct picolcd_data *data = hid_get_drvdata(hdev);
- struct hid_report *report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, hdev);
- struct hid_report *report2 = picolcd_out_report(REPORT_LCD_DATA, hdev);
+ struct hid_report *report1, *report2;
unsigned long flags;
u8 *tdata;
int i;
- if (!report1 || report1->maxfield != 1 || !report2 || report2->maxfield != 1)
+ report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, data->hdev);
+ if (!report1 || report1->maxfield != 1)
+ return -ENODEV;
+ report2 = picolcd_out_report(REPORT_LCD_DATA, data->hdev);
+ if (!report2 || report2->maxfield != 1)
return -ENODEV;
spin_lock_irqsave(&data->lock, flags);
+ if ((data->status & PICOLCD_FAILED)) {
+ spin_unlock_irqrestore(&data->lock, flags);
+ return -ENODEV;
+ }
hid_set_field(report1->field[0], 0, chip << 2);
hid_set_field(report1->field[0], 1, 0x02);
hid_set_field(report1->field[0], 2, 0x00);
@@ -128,7 +135,7 @@ static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
hid_set_field(report2->field[0], 2, 0x00);
hid_set_field(report2->field[0], 3, 32);
- tdata = data->fb_vbitmap + (tile * 4 + chip) * 64;
+ tdata = vbitmap + (tile * 4 + chip) * 64;
for (i = 0; i < 64; i++)
if (i < 32)
hid_set_field(report1->field[0], 11 + i, tdata[i]);
@@ -189,6 +196,7 @@ void picolcd_fb_refresh(struct picolcd_data *data)
int picolcd_fb_reset(struct picolcd_data *data, int clear)
{
struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i, j;
unsigned long flags;
static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00, 0x64, 0xc0 };
@@ -207,20 +215,19 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
hid_set_field(report->field[0], j, 0);
usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
}
-
- data->status |= PICOLCD_READY_FB;
spin_unlock_irqrestore(&data->lock, flags);
- if (data->fb_bitmap) {
- if (clear) {
- memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE);
- memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp);
- }
- data->fb_force = 1;
+ if (clear) {
+ memset(fbdata->vbitmap, 0, PICOLCDFB_SIZE);
+ memset(fbdata->bitmap, 0, PICOLCDFB_SIZE*fbdata->bpp);
}
+ fbdata->force = 1;
/* schedule first output of framebuffer */
- picolcd_fb_refresh(data);
+ if (fbdata->ready)
+ schedule_delayed_work(&data->fb_info->deferred_work, 0);
+ else
+ fbdata->ready = 1;
return 0;
}
@@ -230,20 +237,15 @@ static void picolcd_fb_update(struct fb_info *info)
{
int chip, tile, n;
unsigned long flags;
+ struct picolcd_fb_data *fbdata = info->par;
struct picolcd_data *data;
mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (!(data->status & PICOLCD_READY_FB)) {
- spin_unlock_irqrestore(&data->lock, flags);
- picolcd_fb_reset(data, 0);
- } else {
- spin_unlock_irqrestore(&data->lock, flags);
- }
+ spin_lock_irqsave(&fbdata->lock, flags);
+ if (!fbdata->ready && fbdata->picolcd)
+ picolcd_fb_reset(fbdata->picolcd, 0);
+ spin_unlock_irqrestore(&fbdata->lock, flags);
/*
* Translate the framebuffer into the format needed by the PicoLCD.
@@ -254,32 +256,38 @@ static void picolcd_fb_update(struct fb_info *info)
*/
n = 0;
for (chip = 0; chip < 4; chip++)
- for (tile = 0; tile < 8; tile++)
- if (picolcd_fb_update_tile(data->fb_vbitmap,
- data->fb_bitmap, data->fb_bpp, chip, tile) ||
- data->fb_force) {
- n += 2;
- if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
- mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
- mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (data->status & PICOLCD_FAILED) {
- spin_unlock_irqrestore(&data->lock, flags);
- goto out;
- }
- spin_unlock_irqrestore(&data->lock, flags);
- n = 0;
- }
- picolcd_fb_send_tile(data->hdev, chip, tile);
+ for (tile = 0; tile < 8; tile++) {
+ if (!fbdata->force && !picolcd_fb_update_tile(
+ fbdata->vbitmap, fbdata->bitmap,
+ fbdata->bpp, chip, tile))
+ continue;
+ n += 2;
+ if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ mutex_unlock(&info->lock);
+ if (!data)
+ return;
+ usbhid_wait_io(data->hdev);
+ mutex_lock(&info->lock);
+ n = 0;
}
- data->fb_force = false;
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ if (!data || picolcd_fb_send_tile(data,
+ fbdata->vbitmap, chip, tile))
+ goto out;
+ }
+ fbdata->force = false;
if (n) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
+ if (data)
+ usbhid_wait_io(data->hdev);
return;
}
out:
@@ -336,20 +344,22 @@ static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,
static int picolcd_fb_blank(int blank, struct fb_info *info)
{
- if (!info->par)
- return -ENODEV;
/* We let fb notification do this for us via lcd/backlight device */
return 0;
}
static void picolcd_fb_destroy(struct fb_info *info)
{
+ struct picolcd_fb_data *fbdata = info->par;
+
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
+ /* No thridparty should ever unregister our framebuffer! */
+ WARN_ON(fbdata->picolcd != NULL);
+
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
- printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
}
static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -376,17 +386,15 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
static int picolcd_set_par(struct fb_info *info)
{
- struct picolcd_data *data = info->par;
+ struct picolcd_fb_data *fbdata = info->par;
u8 *tmp_fb, *o_fb;
- if (!data)
- return -ENODEV;
- if (info->var.bits_per_pixel = data->fb_bpp)
+ if (info->var.bits_per_pixel = fbdata->bpp)
return 0;
/* switch between 1/8 bit depths */
if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
return -EINVAL;
- o_fb = data->fb_bitmap;
+ o_fb = fbdata->bitmap;
tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
if (!tmp_fb)
return -ENOMEM;
@@ -415,7 +423,7 @@ static int picolcd_set_par(struct fb_info *info)
}
kfree(tmp_fb);
- data->fb_bpp = info->var.bits_per_pixel;
+ fbdata->bpp = info->var.bits_per_pixel;
return 0;
}
@@ -453,7 +461,8 @@ static ssize_t picolcd_fb_update_rate_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct picolcd_data *data = dev_get_drvdata(dev);
- unsigned i, fb_update_rate = data->fb_update_rate;
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
+ unsigned i, fb_update_rate = fbdata->update_rate;
size_t ret = 0;
for (i = 1; i <= PICOLCDFB_UPDATE_RATE_LIMIT; i++)
@@ -472,6 +481,7 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct picolcd_data *data = dev_get_drvdata(dev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i;
unsigned u;
@@ -487,8 +497,8 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
else if (u = 0)
u = PICOLCDFB_UPDATE_RATE_DEFAULT;
- data->fb_update_rate = u;
- data->fb_info->fbdefio->delay = HZ / data->fb_update_rate;
+ fbdata->update_rate = u;
+ data->fb_info->fbdefio->delay = HZ / fbdata->update_rate;
return count;
}
@@ -500,30 +510,18 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
{
struct device *dev = &data->hdev->dev;
struct fb_info *info = NULL;
+ struct picolcd_fb_data *fbdata = NULL;
int i, error = -ENOMEM;
- u8 *fb_vbitmap = NULL;
- u8 *fb_bitmap = NULL;
u32 *palette;
- fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
- if (fb_bitmap = NULL) {
- dev_err(dev, "can't get a free page for framebuffer\n");
- goto err_nomem;
- }
-
- fb_vbitmap = kmalloc(PICOLCDFB_SIZE, GFP_KERNEL);
- if (fb_vbitmap = NULL) {
- dev_err(dev, "can't alloc vbitmap image buffer\n");
- goto err_nomem;
- }
-
- data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
/* The extra memory is:
* - 256*u32 for pseudo_palette
* - struct fb_deferred_io
*/
info = framebuffer_alloc(256 * sizeof(u32) +
- sizeof(struct fb_deferred_io), dev);
+ sizeof(struct fb_deferred_io) +
+ sizeof(struct picolcd_fb_data) +
+ PICOLCDFB_SIZE, dev);
if (info = NULL) {
dev_err(dev, "failed to allocate a framebuffer\n");
goto err_nomem;
@@ -531,74 +529,86 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
info->fbdefio = info->par;
*info->fbdefio = picolcd_fb_defio;
- palette = info->par + sizeof(struct fb_deferred_io);
+ info->par += sizeof(struct fb_deferred_io);
+ palette = info->par;
+ info->par += 256 * sizeof(u32);
for (i = 0; i < 256; i++)
palette[i] = i > 0 && i < 16 ? 0xff : 0;
info->pseudo_palette = palette;
- info->screen_base = (char __force __iomem *)fb_bitmap;
info->fbops = &picolcdfb_ops;
info->var = picolcdfb_var;
info->fix = picolcdfb_fix;
info->fix.smem_len = PICOLCDFB_SIZE*8;
- info->fix.smem_start = (unsigned long)fb_bitmap;
- info->par = data;
info->flags = FBINFO_FLAG_DEFAULT;
- data->fb_vbitmap = fb_vbitmap;
- data->fb_bitmap = fb_bitmap;
- data->fb_bpp = picolcdfb_var.bits_per_pixel;
+ fbdata = info->par;
+ spin_lock_init(&fbdata->lock);
+ fbdata->picolcd = data;
+ fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
+ fbdata->bpp = picolcdfb_var.bits_per_pixel;
+ fbdata->force = 1;
+ fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
+ fbdata->bitmap = vmalloc(PICOLCDFB_SIZE*8);
+ if (fbdata->bitmap = NULL) {
+ dev_err(dev, "can't get a free page for framebuffer\n");
+ goto err_nomem;
+ }
+ info->screen_base = (char __force __iomem *)fbdata->bitmap;
+ info->fix.smem_start = (unsigned long)fbdata->bitmap;
+ memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
+ data->fb_info = info;
+
error = picolcd_fb_reset(data, 1);
if (error) {
dev_err(dev, "failed to configure display\n");
goto err_cleanup;
}
+
error = device_create_file(dev, &dev_attr_fb_update_rate);
if (error) {
dev_err(dev, "failed to create sysfs attributes\n");
goto err_cleanup;
}
+
fb_deferred_io_init(info);
- data->fb_info = info;
error = register_framebuffer(info);
if (error) {
dev_err(dev, "failed to register framebuffer\n");
goto err_sysfs;
}
- /* schedule first output of framebuffer */
- data->fb_force = 1;
- schedule_delayed_work(&info->deferred_work, 0);
return 0;
err_sysfs:
- fb_deferred_io_cleanup(info);
device_remove_file(dev, &dev_attr_fb_update_rate);
+ fb_deferred_io_cleanup(info);
err_cleanup:
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
data->fb_info = NULL;
err_nomem:
+ if (fbdata)
+ vfree(fbdata->bitmap);
framebuffer_release(info);
- vfree(fb_bitmap);
- kfree(fb_vbitmap);
return error;
}
void picolcd_exit_framebuffer(struct picolcd_data *data)
{
struct fb_info *info = data->fb_info;
- u8 *fb_vbitmap = data->fb_vbitmap;
-
- if (!info)
- return;
+ struct picolcd_fb_data *fbdata = info->par;
+ unsigned long flags;
device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
- info->par = NULL;
+
+ /* disconnect framebuffer from HID dev */
+ spin_lock_irqsave(&fbdata->lock, flags);
+ fbdata->picolcd = NULL;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+
+ /* make sure there is no running update - thus that fbdata->picolcd
+ * once obtained under lock is guaranteed not to get free() under
+ * the feet of the deferred work */
+ flush_delayed_work_sync(&info->deferred_work);
+
+ data->fb_info = NULL;
unregister_framebuffer(info);
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
- data->fb_info = NULL;
- kfree(fb_vbitmap);
}
--
1.7.8.6
^ permalink raw reply related
* [PATCH 0/6] HID: picoLCD additional fixes + CIR support
From: Bruno Prémont @ 2012-08-19 17:28 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-input, linux-kernel, Mauro Carvalho Chehab, linux-fbdev,
linux-media
In-Reply-To: <20120730213656.0a9f6d30@neptune.home>
Hi,
This series fixes some race conditions in picoLCD driver during remove()
and adds support for IR functionality.
Repeatedly binding/unbinding device at hid-picolcd driver level or at
usbhid level now works properly (except in rare occasions which trigger
a paging error in interrupt context (memcopy) and seem to come from
usbhid internals when usbhid gets interrupt between hid_hw_close() and
hid_hw_stop()).
CIR support is added using RC_CORE for the decoding and handling of
IR-event stream and works for a Sony and a JVC remote.
Bruno
^ permalink raw reply
* Re: [PATCH 6/7 v2] HID: picoLCD: drop version check during probe
From: Bruno Prémont @ 2012-08-19 16:56 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <alpine.LNX.2.00.1208151015220.7026@pobox.suse.cz>
Commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver] introduced
new locking around probe/remove functions that prevents any report/reply
from hardware to reach driver until it returned from probe.
As such, the ask-reply way to checking picoLCD firmware version during
probe is bound to timeout and let probe fail.
Drop the check to let driver successfully probe again (until locking issues
are resolved allowing to reinstate the check).
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
Changes since v1:
- drop version check during probe instead of commenting it out.
drivers/hid/hid-picolcd_core.c | 18 ------------------
1 files changed, 0 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 7b566ee..e08ffd2 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -478,14 +478,6 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
{
int error;
- error = picolcd_check_version(hdev);
- if (error)
- return error;
-
- if (data->version[0] != 0 && data->version[1] != 3)
- hid_info(hdev, "Device with untested firmware revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",
- dev_name(&hdev->dev));
-
/* Setup keypad input device */
error = picolcd_init_keys(data, picolcd_in_report(REPORT_KEY_STATE, hdev));
if (error)
@@ -534,16 +526,6 @@ err:
static int picolcd_probe_bootloader(struct hid_device *hdev, struct picolcd_data *data)
{
- int error;
-
- error = picolcd_check_version(hdev);
- if (error)
- return error;
-
- if (data->version[0] != 1 && data->version[1] != 0)
- hid_info(hdev, "Device with untested bootloader revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",
- dev_name(&hdev->dev));
-
picolcd_init_devfs(data, NULL, NULL,
picolcd_out_report(REPORT_BL_READ_MEMORY, hdev),
picolcd_out_report(REPORT_BL_WRITE_MEMORY, hdev), NULL);
--
1.7.8.6
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-19 16:23 UTC (permalink / raw)
To: Alan Stern; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <Pine.LNX.4.44L0.1208182001320.25537-100000@netrider.rowland.org>
On Sat, 18 August 2012 Alan Stern wrote:
> On Sat, 18 Aug 2012, Bruno Prémont wrote:
>
> > One thing I just though about, how does usbhid handle the calls to
> > usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
> > been called?
>
> Looks like they aren't synchronized at all. That's a bug.
> usbhid_submit_report() should check the HID_DISCONNECTED flag.
>
> > I will attempt to see if it makes a difference to shortcut my
> > usbhid_submit_report() calls from the point on I have called hid_hw_close()...
>
> I don't know bout hid_hw_close(). Certainly no more reports should be
> submitted following usbhid_stop().
Ok, I did just that, prevent new calls to usbhid_submit_report(), after
calling hid_hw_close(), fixed one bug in my code that triggers the NULL
pointer dereference (calling hid_set_drvdata(hdev, NULL) too early).
Now I'm still seeing the bad paging request in _mmx_memcpy(), though rather
sporadically.
The last ones I saw were during remove() around the time of calling hid_hw_close()
and hid_hw_stop(). Adding a printk() between the two (at least while picoLCD
is hosting fbcon) makes it very improbably for the bad page to happen.
It looks like low-level driver did free memory in hid_hw_close() for some
in-flight URB and thus things break in following USB interrupt.
From mapping trace information to source it seems:
usbhid/hid-core.c:
static int hid_submit_out(struct hid_device *hid)
{
struct hid_report *report;
char *raw_report;
struct usbhid_device *usbhid = hid->driver_data;
int r;
report = usbhid->out[usbhid->outtail].report;
raw_report = usbhid->out[usbhid->outtail].raw_report;
usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
1 + (report->id > 0);
usbhid->urbout->dev = hid_to_usb_dev(hid);
if (raw_report) {
memcpy(usbhid->outbuf, raw_report,
usbhid->urbout->transfer_buffer_length);
^^^^^^^^^^^^^^^_ this is exploding
kfree(raw_report);
usbhid->out[usbhid->outtail].raw_report = NULL;
}
dbg_hid("submitting out urb\n");
r = usb_submit_urb(usbhid->urbout, GFP_ATOMIC);
if (r < 0) {
hid_err(hid, "usb_submit_urb(out) failed: %d\n", r);
return r;
}
usbhid->last_out = jiffies;
return 0;
}
Bruno
^ permalink raw reply
* [PATCH 8/14] drivers/video/sunxvr2500.c: fix error return code
From: Julia Lawall @ 2012-08-19 8:44 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: kernel-janitors, linux-fbdev, linux-kernel
In-Reply-To: <1345365870-29831-1-git-send-email-Julia.Lawall@lip6.fr>
From: Julia Lawall <Julia.Lawall@lip6.fr>
Initialize return variable before exiting on an error path.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
drivers/video/sunxvr2500.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/sunxvr2500.c b/drivers/video/sunxvr2500.c
index 5848436..7fbcba8 100644
--- a/drivers/video/sunxvr2500.c
+++ b/drivers/video/sunxvr2500.c
@@ -181,8 +181,10 @@ static int __devinit s3d_pci_register(struct pci_dev *pdev,
sp->fb_size = info->fix.line_length * sp->height;
sp->fb_base = ioremap(sp->fb_base_phys, sp->fb_size);
- if (!sp->fb_base)
+ if (!sp->fb_base) {
+ err = -ENOMEM;
goto err_release_pci;
+ }
err = s3d_set_fbinfo(sp);
if (err)
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Alan Stern @ 2012-08-19 0:11 UTC (permalink / raw)
To: Bruno Prémont; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120818154828.0e992bfe@neptune.home>
On Sat, 18 Aug 2012, Bruno Prémont wrote:
> One thing I just though about, how does usbhid handle the calls to
> usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
> been called?
Looks like they aren't synchronized at all. That's a bug.
usbhid_submit_report() should check the HID_DISCONNECTED flag.
> I will attempt to see if it makes a difference to shortcut my
> usbhid_submit_report() calls from the point on I have called hid_hw_close()...
I don't know bout hid_hw_close(). Certainly no more reports should be
submitted following usbhid_stop().
Alan Stern
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 18:49 UTC (permalink / raw)
To: Alan Stern, Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120818154828.0e992bfe@neptune.home>
On Sat, 18 August 2012 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> One thing I just though about, how does usbhid handle the calls to
> usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
> been called?
> I will attempt to see if it makes a difference to shortcut my
> usbhid_submit_report() calls from the point on I have called hid_hw_close()...
I don't know if I'm progressing or just drawing circles...
I included a check right before calling usbhid_submit_report() to prevent
calling it if hid_hw_close()/hid_hw_stop() has been called and that part
works as expected, just that at some moment it explodes on a NULL-pointer
dereference.
Initially I had usbhid_submit_report macro:
#define usbhid_submit_report(a, b, c) \
do { \
picolcd_debug_out_report(d, a, b); \
usbhid_submit_report(a, b, c); \
} while (0)
Now to make sure no new reports get submitted once hid_hw_stop()
was called I changed it to:
#define usbhid_submit_report(a, b, c) \
do { \
struct picolcd_data *d = hid_get_drvdata(a); \
unsigned long flags; \
picolcd_debug_out_report(d, a, b); \
spin_lock_irqsave(&d->lock, flags); \
if (!(d->status & PICOLCD_FAILED)) \
usbhid_submit_report(a, b, c); \
else \
printk(KERN_INFO "Submitting report after hw_stop!\n"); \
spin_unlock_irqrestore(&d->lock, flags); \
} while (0)
with the printk() to spam dmesg :)
The NULL-pointer dereference I got inside picolcd_fb_deferred_io() was due
to me calling hid_set_drvdata(hdev, NULL) too early in remove(), fixed.
But with that fixed I still encounter the bad page, probably around
the time of hid_hw_stop(), though it always takes the HARD LOCK watchdog
to trigger for the trace to show (probably because or IRQ context):
It seems to happen during the bind/unbind following the unbind that
produces a
hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
either a BUG or even as reboot and happend between hid_hw_close() and
hid_hw_stop() though my extra printks seem to reduce probability of
bad things to happen.
[ 1217.643017] BUG: unable to handle kernel paging request at c7cb0000
[ 1217.643017] IP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c
[ 1217.643017] *pde = 19078063 *pte = 07cb0161
[ 1217.643017] Oops: 0003 [#1]
[ 1217.643017] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 1217.643017] Pid: 0, comm: swapper Not tainted 3.6.0-rc2-jupiter-00008-gd3c2b0c-dirty #2 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 1217.643017] EIP: 0060:[<c11e2be7>] EFLAGS: 00010013 CPU: 0
[ 1217.643017] EIP is at _mmx_memcpy+0x27/0x16c
[ 1217.643017] EAX: dd40a000 EBX: dd6ea010 ECX: 035af77b EDX: d9108c80
[ 1217.643017] ESI: d9121c00 EDI: c7cb0000 EBP: dd40bebc ESP: dd40bea0
[ 1217.643017] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 1217.643017] CR0: 8005003b CR2: c7cb0000 CR3: 1cf1c000 CR4: 000007d0
[ 1217.643017] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 1217.643017] DR6: ffff0ff0 DR7: 00000400
[ 1217.643017] Process swapper (pid: 0, tiÝ40a000 taskÁ5d24c0 task.tiÁ5c6000)
[ 1217.643017] Stack:
[ 1217.643017] 0d6d6d6f dd40beac c109bba4 c7c97080 dd6ea010 c7c97080 dceb3030 dd40bee4
[ 1217.643017] c133b364 c7d40000 c7d40000 dd40bed4 dceb3030 d9108c80 dd6ea010 00000046
[ 1217.643017] dceb3030 dd40bf04 c133bea0 0000d6d6 dbe901b0 fffffffe dbe901b0 fffffffe
[ 1217.643017] Call Trace:
[ 1217.643017] [<c109bba4>] ? free_compound_page+0x14/0x20
[ 1217.643017] [<c133b364>] hid_submit_out+0xa4/0x130
[ 1217.643017] [<c133bea0>] hid_irq_out+0xa0/0x100
[ 1217.643017] [<c12e425e>] usb_hcd_giveback_urb+0x4e/0x90
[ 1217.643017] [<c12f9bd2>] finish_urb+0xb2/0xf0
[ 1217.643017] [<c12fa8a8>] finish_unlinks+0x168/0x320
[ 1217.643017] [<c12fc43f>] ohci_irq+0x27f/0x300
[ 1217.643017] [<c1077790>] ? unmask_irq+0x20/0x20
[ 1217.643017] [<c12e3ace>] usb_hcd_irq+0x1e/0x40
[ 1217.643017] [<c107549d>] handle_irq_event_percpu+0x6d/0x1c0
[ 1217.643017] [<c1077790>] ? unmask_irq+0x20/0x20
[ 1217.643017] [<c1075618>] handle_irq_event+0x28/0x40
[ 1217.643017] [<c107781a>] handle_fasteoi_irq+0x8a/0xe0
[ 1217.643017] <IRQ>
[ 1217.643017] [<c1003e0a>] ? do_IRQ+0x3a/0xb0
[ 1217.643017] [<c140c870>] ? common_interrupt+0x30/0x38
[ 1217.643017] [<c1009028>] ? default_idle+0x68/0xd0
[ 1217.643017] [<c1009a0f>] ? cpu_idle+0x8f/0xc0
[ 1217.643017] [<c13ffe97>] ? rest_init+0x67/0x70
[ 1217.643017] [<c161b8b8>] ? start_kernel+0x2a3/0x2aa
[ 1217.643017] [<c161b3da>] ? repair_env_string+0x51/0x51
[ 1217.643017] [<c161b28e>] ? i386_start_kernel+0x44/0x46
[ 1217.643017] Code: 90 90 90 90 55 89 e5 57 56 89 d6 53 83 ec 10 89 45 f0 89 4d e4 89 e0 25 00 e0 ff ff f7 40 14 00 ff ff 07 74 17 c1 e9 02 8b 7d f0 <f3> a5 8b 4d e4 83 e1 03 74 02 f3 a4 e9 27 01 00 00 8b 45 1
[ 1217.643017] EIP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c SS:ESP 0068:dd40bea0
[ 1217.643017] CR2: 00000000c7cb0000
[ 1217.643017] ---[ end trace fbaa4c5261e1a846 ]---
Bruno
^ permalink raw reply
* Re: [patch] video: mb862xxfb: prevent divide by zero bug
From: Anatolij Gustschin @ 2012-08-18 18:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, H Hartley Sweeten,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Paul Gortmaker, Laurent Pinchart, David Brown
In-Reply-To: <20120818155541.GB22424-mgFCXtclrQlZLf2FXnZxJA@public.gmane.org>
On Sat, 18 Aug 2012 18:55:41 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Do a sanity check on these before using them as divisors.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Anatolij Gustschin <agust@denx.de>
> diff --git a/drivers/video/mb862xx/mb862xxfbdrv.c b/drivers/video/mb862xx/mb862xxfbdrv.c
> index 00ce1f3..57d940b 100644
> --- a/drivers/video/mb862xx/mb862xxfbdrv.c
> +++ b/drivers/video/mb862xx/mb862xxfbdrv.c
> @@ -328,6 +328,8 @@ static int mb862xxfb_ioctl(struct fb_info *fbi, unsigned int cmd,
> case MB862XX_L1_SET_CFG:
> if (copy_from_user(l1_cfg, argp, sizeof(*l1_cfg)))
> return -EFAULT;
> + if (l1_cfg->dh = 0 || l1_cfg->dw = 0)
> + return -EINVAL;
> if ((l1_cfg->sw >= l1_cfg->dw) && (l1_cfg->sh >= l1_cfg->dh)) {
> /* downscaling */
> outreg(cap, GC_CAP_CSC,
Thanks,
Anatolij
^ permalink raw reply
* [patch] video: mb862xxfb: prevent divide by zero bug
From: Dan Carpenter @ 2012-08-18 15:55 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, H Hartley Sweeten,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Paul Gortmaker, Laurent Pinchart, David Brown
Do a sanity check on these before using them as divisors.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/video/mb862xx/mb862xxfbdrv.c b/drivers/video/mb862xx/mb862xxfbdrv.c
index 00ce1f3..57d940b 100644
--- a/drivers/video/mb862xx/mb862xxfbdrv.c
+++ b/drivers/video/mb862xx/mb862xxfbdrv.c
@@ -328,6 +328,8 @@ static int mb862xxfb_ioctl(struct fb_info *fbi, unsigned int cmd,
case MB862XX_L1_SET_CFG:
if (copy_from_user(l1_cfg, argp, sizeof(*l1_cfg)))
return -EFAULT;
+ if (l1_cfg->dh = 0 || l1_cfg->dw = 0)
+ return -EINVAL;
if ((l1_cfg->sw >= l1_cfg->dw) && (l1_cfg->sh >= l1_cfg->dh)) {
/* downscaling */
outreg(cap, GC_CAP_CSC,
^ permalink raw reply related
* viafb PLL/clock tweaking causes XO-1.5 instability
From: Daniel Drake @ 2012-08-18 15:53 UTC (permalink / raw)
To: linux-fbdev
Hi,
I've been investigating a strange regression where in recent kernel
releases, the XO-1.5 cannot reliably suspend/resume when on a virtual
terminal (driven by viafb). Under X there is no instability.
The test case is to suspend and resume a lot (about 10 times a
minute). In less than an hour the system will fail to resume - the
system RAM becomes corrupt on resume and the firmware can't find its
way back into Linux, so it reboots. This is presumably because the GPU
is accessing main memory while the system is suspended, causing the
RAM to come out of self-refresh and lose all its contents before the
CPU wakes up to resume the system.
I have bisected this to find the first bad commit:
commit b692a63af8b63a7a7e84702a713d0072e336b326
Author: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Date: Thu Mar 24 14:25:51 2011 +0000
viafb: add VIA slapping capability
On the XO-1.5 this code tries to set IGA1=off IGA2=on. However, a
later commit causes them both to be turned on, since we enable
X_COMPATIBILITY (we use X, and we find that setting to be necessary
for X to work).
If I comment the code that modifies IGA1, I find that setting IGA2=on
causes the crashyness, setting IGA2=off results in the display not
working at all. The only non-crashy setting here I can find is to not
touch the IGA2 configuration.
If I comment out the code that modifies IGA2, I find that setting
IGA1=on causes the crashyness, and setting IGA1=off is stable.
Going one step lower to find exactly which calls are responsible for
triggering the crashyness, by only enabling one function call at a
time, I find:
set_secondary_pll_state(on) only = stable
set_secondary_clock_state(on) only = crashy
set_primary_pll_state(on) only = stable
set_primary_clock_state(on) only = crashy
Looking at the set_secondary_clock_state(on) case, I find that the
Secondary Display Engine (Gated Clock <LCK>) bits 7:6 in IO Port
3C5.1B are 11 when untouched, and 10 after
set_secondary_clock_state(on) has run. So the code seems to be doing
what it intends to do, but its not clear why modifying this register
effectively switches between 3 behavioural states (stable, no display
at all, display active but crashy suspend/resume). Also is there
really any need to modify it or would the default (enable clock only
if its needed) suffice?
Any ideas?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 13:48 UTC (permalink / raw)
To: Alan Stern; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <Pine.LNX.4.44L0.1208180913440.17394-100000@netrider.rowland.org>
On Sat, 18 August 2012 Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 18 Aug 2012, Bruno Prémont wrote:
> > On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> > > On Thu, 16 Aug 2012, Bruno Prémont wrote:
> > >
> > > > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > > > the URB should be available for future use (and therefore all queues
> > > > > completely drained).
> > > >
> > > > I won't have time today to check, though my guess is that on each
> > > > echo $usb-id > bind; echo $usb-id > unbind
> > > > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > > > not get cleared.
> > > >
> > > > Is usb_kill_urb() called when unbinding just the specific hid driver?
> > >
> > > Yup, through hid_hw_stop() -> usbhid_stop().
> > >
> > > > If so my short timing between bind/unbind must be triggering something
> > > > else...
> > > >
> > > > Otherwise I'm missing something as at first time I got no "output queue full"
> > > > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > > > iteration increased in number.
> > > >
> > > > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > > > findings.
> >
> > Huh, after changing some of the hid-picolcd data in order to have less racy
> > coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
> > and most of the time I don't get a (complete) trace...
>
> There was a similar problem reported recently. It turned out to be
> caused by a __devinitconst annotation attached to a usb_device_id
> table.
>
> If there are any __devinit* annotations in the hid-picolcd driver, you
> should see if removing them helps.
There is no such annotation around in hid-picolcd.
One thing I just though about, how does usbhid handle the calls to
usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
been called?
I will attempt to see if it makes a difference to shortcut my
usbhid_submit_report() calls from the point on I have called hid_hw_close()...
Bruno
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox