From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans Verkuil Subject: Re: [PATCH v6 1/3] ivtv: use arch_phys_wc_add() and require PAT disabled Date: Tue, 09 Jun 2015 08:23:59 +0200 Message-ID: <5576867F.8000003@xs4all.nl> References: <1433809222-28261-1-git-send-email-mcgrof@do-not-panic.com> <1433809222-28261-2-git-send-email-mcgrof@do-not-panic.com> <20150608215625.39544c82@recife.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20150608215625.39544c82@recife.lan> Sender: linux-media-owner@vger.kernel.org To: Mauro Carvalho Chehab , "Luis R. Rodriguez" Cc: bp@suse.de, tomi.valkeinen@ti.com, bhelgaas@google.com, linux-media@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, "Luis R. Rodriguez" , Andy Walls , Doug Ledford , Andy Lutomirski , Suresh Siddha , Ingo Molnar , Thomas Gleixner , Juergen Gross , Daniel Vetter , Dave Airlie , Antonino Daplas , Jean-Christophe Plagniol-Villard , Dave Hansen , Arnd Bergmann , "Michael S. Tsirkin" , Stefan Bader , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Mel Gorman , Vlastimil Babka , Davi List-Id: linux-rdma@vger.kernel.org On 06/09/2015 02:56 AM, Mauro Carvalho Chehab wrote: > Em Mon, 08 Jun 2015 17:20:20 -0700 > "Luis R. Rodriguez" escreveu: >=20 >> From: "Luis R. Rodriguez" >> >> We are burrying direct access to MTRR code support on >> x86 in order to take advantage of PAT. In the future we >> also want to make the default behaviour of ioremap_nocache() >> to use strong UC, use of mtrr_add() on those systems >> would make write-combining void. >> >> In order to help both enable us to later make strong >> UC default and in order to phase out direct MTRR access >> code port the driver over to arch_phys_wc_add() and >> annotate that the device driver requires systems to >> boot with PAT disabled, with the nopat kernel parameter. >> >> This is a worthy comprmise given that the hardware is >> really rare these days, and perhaps only some lost souls >> in some third world country are expected to be using this >> feature of the device driver. >> >> Acked-by: Andy Walls >> Cc: Andy Walls >> Cc: Doug Ledford >> Cc: Mauro Carvalho Chehab >=20 > Provided that you fix the issues below: > Acked-by: Mauro Carvalho Chehab >=20 >> Cc: Andy Lutomirski >> Cc: Suresh Siddha >> Cc: Ingo Molnar >> Cc: Thomas Gleixner >> Cc: Juergen Gross >> Cc: Daniel Vetter >> Cc: Dave Airlie >> Cc: Bjorn Helgaas >> Cc: Antonino Daplas >> Cc: Jean-Christophe Plagniol-Villard >> Cc: Tomi Valkeinen >> Cc: Dave Hansen >> Cc: Arnd Bergmann >> Cc: Michael S. Tsirkin >> Cc: Stefan Bader >> Cc: Ville Syrj=C3=A4l=C3=A4 >> Cc: Mel Gorman >> Cc: Vlastimil Babka >> Cc: Borislav Petkov >> Cc: Davidlohr Bueso >> Cc: konrad.wilk@oracle.com >> Cc: ville.syrjala@linux.intel.com >> Cc: david.vrabel@citrix.com >> Cc: jbeulich@suse.com >> Cc: toshi.kani@hp.com >> Cc: Roger Pau Monn=C3=A9 >> Cc: linux-fbdev@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Cc: ivtv-devel@ivtvdriver.org >> Cc: linux-media@vger.kernel.org >> Cc: xen-devel@lists.xensource.com >> Signed-off-by: Luis R. Rodriguez >> --- >> drivers/media/pci/ivtv/Kconfig | 3 +++ >> drivers/media/pci/ivtv/ivtvfb.c | 58 ++++++++++++++++--------------= ----------- >> 2 files changed, 26 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv= /Kconfig >> index dd6ee57e..b2a7f88 100644 >> --- a/drivers/media/pci/ivtv/Kconfig >> +++ b/drivers/media/pci/ivtv/Kconfig >> @@ -57,5 +57,8 @@ config VIDEO_FB_IVTV >> This is used in the Hauppauge PVR-350 card. There is a driver >> homepage at . >> =20 >> + If you have this hardware you will need to boot with PAT disable= d >> + on your x86 systems, use the nopat kernel parameter. >> + >=20 > Hmm... FB_IVTV is not hardware... it is framebuffer support for IVTV. > It is optional to use FB API for the video output port of this board, > instead of using V4L2 API. That's not true. It is hardware: it drives the video output OSD which o= verlays the video output. The reason it is optional is that it is hard to unloa= d a framebuffer module and most people don't need it. So V4L2 drives the video output and ivtvfb drives the OSD overlay. So i= t is not a case of 'instead of'. >=20 > I would say, instead, something like:=20 >=20 > "In order to use this module, you will need to boot with PAT disable= d > on x86 systems, using the nopat kernel parameter." I do agree with this change, but that's because this module is optional= and not for the reasons you mentioned above. Regards, Hans >=20 >> To compile this driver as a module, choose M here: the >> module will be called ivtvfb. >> diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivt= v/ivtvfb.c >> index 9ff1230..7685ae3 100644 >> --- a/drivers/media/pci/ivtv/ivtvfb.c >> +++ b/drivers/media/pci/ivtv/ivtvfb.c >> @@ -44,8 +44,8 @@ >> #include >> #include >> =20 >> -#ifdef CONFIG_MTRR >> -#include >> +#ifdef CONFIG_X86_64 >> +#include >> #endif >> =20 >> #include "ivtv-driver.h" >> @@ -155,12 +155,11 @@ struct osd_info { >> /* Buffer size */ >> u32 video_buffer_size; >> =20 >> -#ifdef CONFIG_MTRR >> /* video_base rounded down as required by hardware MTRRs */ >> unsigned long fb_start_aligned_physaddr; >> /* video_base rounded up as required by hardware MTRRs */ >> unsigned long fb_end_aligned_physaddr; >> -#endif >> + int wc_cookie; >> =20 >> /* Store the buffer offset */ >> int set_osd_coords_x; >> @@ -1099,6 +1098,8 @@ static int ivtvfb_init_vidmode(struct ivtv *it= v) >> static int ivtvfb_init_io(struct ivtv *itv) >> { >> struct osd_info *oi =3D itv->osd_info; >> + /* Find the largest power of two that maps the whole buffer */ >> + int size_shift =3D 31; >> =20 >> mutex_lock(&itv->serialize_lock); >> if (ivtv_init_on_first_open(itv)) { >> @@ -1132,29 +1133,16 @@ static int ivtvfb_init_io(struct ivtv *itv) >> oi->video_pbase, oi->video_vbase, >> oi->video_buffer_size / 1024); >> =20 >> -#ifdef CONFIG_MTRR >> - { >> - /* Find the largest power of two that maps the whole buffer */ >> - int size_shift =3D 31; >> - >> - while (!(oi->video_buffer_size & (1 << size_shift))) { >> - size_shift--; >> - } >> - size_shift++; >> - oi->fb_start_aligned_physaddr =3D oi->video_pbase & ~((1 << size_= shift) - 1); >> - oi->fb_end_aligned_physaddr =3D oi->video_pbase + oi->video_buffe= r_size; >> - oi->fb_end_aligned_physaddr +=3D (1 << size_shift) - 1; >> - oi->fb_end_aligned_physaddr &=3D ~((1 << size_shift) - 1); >> - if (mtrr_add(oi->fb_start_aligned_physaddr, >> - oi->fb_end_aligned_physaddr - oi->fb_start_aligned_physaddr, >> - MTRR_TYPE_WRCOMB, 1) < 0) { >> - IVTVFB_INFO("disabled mttr\n"); >> - oi->fb_start_aligned_physaddr =3D 0; >> - oi->fb_end_aligned_physaddr =3D 0; >> - } >> - } >> -#endif >> - >> + while (!(oi->video_buffer_size & (1 << size_shift))) >> + size_shift--; >> + size_shift++; >> + oi->fb_start_aligned_physaddr =3D oi->video_pbase & ~((1 << size_s= hift) - 1); >> + oi->fb_end_aligned_physaddr =3D oi->video_pbase + oi->video_buffer= _size; >> + oi->fb_end_aligned_physaddr +=3D (1 << size_shift) - 1; >> + oi->fb_end_aligned_physaddr &=3D ~((1 << size_shift) - 1); >> + oi->wc_cookie =3D arch_phys_wc_add(oi->fb_start_aligned_physaddr, >> + oi->fb_end_aligned_physaddr - >> + oi->fb_start_aligned_physaddr); >> /* Blank the entire osd. */ >> memset_io(oi->video_vbase, 0, oi->video_buffer_size); >> =20 >> @@ -1172,14 +1160,7 @@ static void ivtvfb_release_buffers (struct iv= tv *itv) >> =20 >> /* Release pseudo palette */ >> kfree(oi->ivtvfb_info.pseudo_palette); >> - >> -#ifdef CONFIG_MTRR >> - if (oi->fb_end_aligned_physaddr) { >> - mtrr_del(-1, oi->fb_start_aligned_physaddr, >> - oi->fb_end_aligned_physaddr - oi->fb_start_aligned_physaddr); >> - } >> -#endif >> - >> + arch_phys_wc_del(oi->wc_cookie); >> kfree(oi); >> itv->osd_info =3D NULL; >> } >> @@ -1284,6 +1265,13 @@ static int __init ivtvfb_init(void) >> int registered =3D 0; >> int err; >> =20 >> +#ifdef CONFIG_X86_64 >> + if (WARN(pat_enabled(), >> + "ivtvfb needs PAT disabled, boot with nopat kernel parameter\n")= ) { >> + return EINVAL; >=20 > Errors are always negative. So:=20 > return -EINVAL >=20 > Or, perhaps, -ENODEV. >=20 >> + } >> +#endif >> + >> if (ivtvfb_card_id < -1 || ivtvfb_card_id >=3D IVTV_MAX_CARDS) { >> printk(KERN_ERR "ivtvfb: ivtvfb_card_id parameter is out of rang= e (valid range: -1 - %d)\n", >> IVTV_MAX_CARDS - 1); > -- > To unsubscribe from this list: send the line "unsubscribe linux-media= " in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >=20