Re: [PATCH 2/2] vga_switcheroo: initial implementation (v8)

[RafaÅ MiÅecki]

Oh, sorry, but I don't like such a hidden changes, touching not
directly related things.


2010/2/25 Dave Airlie <airlied@xxxxxxxxx>:
> diff --git a/drivers/gpu/drm/radeon/r600_audio.c b/drivers/gpu/drm/radeon/r600_audio.c
> index 0dcb690..7def989 100644
> --- a/drivers/gpu/drm/radeon/r600_audio.c
> +++ b/drivers/gpu/drm/radeon/r600_audio.c
> @@ -163,6 +163,9 @@ int r600_audio_init(struct radeon_device *rdev)
> Â Â Â Ârdev->audio_status_bits = 0;
> Â Â Â Ârdev->audio_category_code = 0;
>
> + Â Â Â if (!radeon_audio)
> + Â Â Â Â Â Â Â return 0;
> +

Good catch but not for this patch. Plus we could handle audio on/off better.


> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> (...)
> +static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
> +{
> + Â Â Â struct drm_device *dev = pci_get_drvdata(pdev);
> + Â Â Â struct radeon_device *rdev = dev->dev_private;
> + Â Â Â pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
> + Â Â Â if (state == VGA_SWITCHEROO_ON) {
> + Â Â Â Â Â Â Â printk(KERN_ERR "VGA switched radeon on\n");
> + Â Â Â Â Â Â Â /* don't suspend or resume card normally */
> + Â Â Â Â Â Â Â rdev->powered_down = false;
> + Â Â Â Â Â Â Â radeon_resume_kms(dev);
> + Â Â Â Â Â Â Â r600_audio_init(rdev);
> + Â Â Â } else {
> + Â Â Â Â Â Â Â printk(KERN_ERR "VGA switched radeon off\n");
> + Â Â Â Â Â Â Â r600_audio_fini(rdev);
> + Â Â Â Â Â Â Â radeon_suspend_kms(dev, pmm);
> + Â Â Â Â Â Â Â /* don't suspend or resume card normally */
> + Â Â Â Â Â Â Â rdev->powered_down = true;
> + Â Â Â }
> +}

Don't mess with r600_audio_* there. You call if for all chipsets which
is not needed and break nice S&R layout, which is chipset specific.

I guess you needed that because you didn't work on branch containing my patch:
http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=commitdiff;h=38fd2c6ff526e6a59edfa8e08f6f0724646784c4
(you commited it to drm-linus)


> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 8ba3de7..69017f2 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -87,7 +87,7 @@ int radeon_testing = 0;
> Âint radeon_connector_table = 0;
> Âint radeon_tv = 1;
> Âint radeon_new_pll = 1;
> -int radeon_audio = 1;
> +int radeon_audio = 0;

Why?! What for we disable this feature by default?! If you see some
reason for that, explain it to others please. I can change my mind,
but for now I don't like this. It makes audio an option from just
working-out-of-box.

-- 
RafaÅ
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/