Re: [Xen-devel] [PATCH] [media] ivtv: use arch_phys_wc_add() and require PAT disabled

[Andy Walls <awalls@xxxxxxxxxxxxxxxx>]

Hi Luis,

Sorry for the late reply.

Thank you for the patch! See my comments below:

On Wed, 2015-04-22 at 12:33 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
> 
> 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,

I'm OK with the compromise solution.  It makes sense.

>  and perhaps only some lost souls
> in some third world country are expected to be using this
> feature of the device driver.
> 
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> Cc: Andy Walls <awalls@xxxxxxxxxxxxxxxx>
> Cc: Suresh Siddha <sbsiddha@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Dave Airlie <airlied@xxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Antonino Daplas <adaplas@xxxxxxxxx>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@xxxxxxxxxxxx>
> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Cc: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> Cc: Ville SyrjÃlà <syrjala@xxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> Cc: Vlastimil Babka <vbabka@xxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: Davidlohr Bueso <dbueso@xxxxxxx>
> Cc: konrad.wilk@xxxxxxxxxx
> Cc: ville.syrjala@xxxxxxxxxxxxxxx
> Cc: david.vrabel@xxxxxxxxxx
> Cc: jbeulich@xxxxxxxx
> Cc: toshi.kani@xxxxxx
> Cc: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> Cc: linux-fbdev@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: ivtv-devel@xxxxxxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx>
> ---
>  drivers/media/pci/ivtv/Kconfig  |  3 +++
>  drivers/media/pci/ivtv/ivtvfb.c | 59 
> +++++++++++++++++------------------------
>  2 files changed, 27 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 <http://www.ivtvdriver.org>.
>  
> +       If you have this hardware you will need to boot with PAT disabled
> +       on your x86 systems, use the nopat kernel parameter.
> +
>         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/ivtv/ivtvfb.c
> index 9ff1230..552408b 100644
> --- a/drivers/media/pci/ivtv/ivtvfb.c
> +++ b/drivers/media/pci/ivtv/ivtvfb.c
> @@ -44,8 +44,8 @@
>  #include <linux/ivtvfb.h>
>  #include <linux/slab.h>
>  
> -#ifdef CONFIG_MTRR
> -#include <asm/mtrr.h>
> +#ifdef CONFIG_X86_64
> +#include <asm/pat.h>
>  #endif
>  
>  #include "ivtv-driver.h"
> @@ -155,12 +155,11 @@ struct osd_info {
>       /* Buffer size */
>       u32 video_buffer_size;
>  
> -#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;
>  
>       /* Store the buffer offset */
>       int set_osd_coords_x;
> @@ -1099,6 +1098,8 @@ static int ivtvfb_init_vidmode(struct ivtv *itv)
>  static int ivtvfb_init_io(struct ivtv *itv)
>  {
>       struct osd_info *oi = itv->osd_info;
> +     /* Find the largest power of two that maps the whole buffer */
> +     int size_shift = 31;
>  
>       mutex_lock(&itv->serialize_lock);
>       if (ivtv_init_on_first_open(itv)) {
> @@ -1120,6 +1121,7 @@ static int ivtvfb_init_io(struct ivtv *itv)
>       oi->video_buffer_size = 1704960;
>  
>       oi->video_pbase = itv->base_addr + IVTV_DECODER_OFFSET + 
> oi->video_rbase;
> +     /* XXX: split this for PAT */

Please remove this comment.  It is prescriptive of a particular
solution, and probably not the one I'm going to implement.  The ivtv
main driver alreay splits the encoder, decoder, and register regions
into 3 mappings.  The final solution will set the whole 8 MB decoder
region mapping to WC, and then fix up all calls in the ivtvfb and ivtv
drivers where writes to the decoder memory with WC enabled could be a
problem.

Also many other places in the driver need audit in a conversion to work
with PAT, so no need to call out this one location with a comment.

>       oi->video_vbase = itv->dec_mem + oi->video_rbase;
>  
>       if (!oi->video_vbase) {
> @@ -1132,29 +1134,16 @@ static int ivtvfb_init_io(struct ivtv *itv)
>                       oi->video_pbase, oi->video_vbase,
>                       oi->video_buffer_size / 1024);
>  
> -#ifdef CONFIG_MTRR
> -     {
> -             /* Find the largest power of two that maps the whole buffer */
> -             int size_shift = 31;
> -
> -             while (!(oi->video_buffer_size & (1 << size_shift))) {
> -                     size_shift--;
> -             }
> -             size_shift++;
> -             oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << 
> size_shift) - 1);
> -             oi->fb_end_aligned_physaddr = oi->video_pbase + 
> oi->video_buffer_size;
> -             oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
> -             oi->fb_end_aligned_physaddr &= ~((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 = 0;
> -                     oi->fb_end_aligned_physaddr = 0;
> -             }
> -     }
> -#endif
> -
> +     while (!(oi->video_buffer_size & (1 << size_shift)))
> +             size_shift--;
> +     size_shift++;
> +     oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << size_shift) - 
> 1);
> +     oi->fb_end_aligned_physaddr = oi->video_pbase + oi->video_buffer_size;
> +     oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
> +     oi->fb_end_aligned_physaddr &= ~((1 << size_shift) - 1);
> +     oi->wc_cookie = 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);
>  
> @@ -1172,14 +1161,7 @@ static void ivtvfb_release_buffers (struct ivtv *itv)
>  
>       /* 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 = NULL;
>  }
> @@ -1190,6 +1172,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
>  {
>       int rc;
>  
> +#ifdef CONFIG_X86_64
> +     if (WARN(pat_enabled,

This check might be better placed in ivtvfb_init().  This check is going
to have the same result for every PVR-350 card in the system that is
found by ivtvfb.

> +              "ivtv needs PAT disabled, boot with nopat kernel 
> parameter\n")) {

This needs to read "ivtvfb needs [...]" to avoid user confusion with the
main ivtv driver module.

This change is the only one I really care about.  Then I can give my
Ack.

Regards,
Andy

> +             return EINVAL;
> +     }
> +#endif
> +
>       if (itv->osd_info) {
>               IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
>               return -EBUSY;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel