diff mbox

[v5,1/3] video: fbdev: atyfb: clarify ioremap() base and length used

Message ID 1435196060-27350-2-git-send-email-mcgrof@do-not-panic.com (mailing list archive)
State New, archived
Headers show

Commit Message

Luis R. Rodriguez June 25, 2015, 1:34 a.m. UTC
From: "Luis R. Rodriguez" <mcgrof@suse.com>

This has no functional changes, it just adjusts
the ioremap() call for the framebuffer to use
the same values we later use for the framebuffer,
this will make it easier to review the next change.

The size of the framebuffer varies but since this is
for PCI we *know* this defaults to 0x800000.
atyfb_setup_generic() is *only* used on PCI probe.

Cc: Toshi Kani <toshi.kani@hp.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Antonino Daplas <adaplas@gmail.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Ville Syrjälä <syrjala@sci.fi>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Borislav Petkov <bp@suse.de>
Cc: Davidlohr Bueso <dbueso@suse.de>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/video/fbdev/aty/atyfb_base.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä June 25, 2015, 11:04 p.m. UTC | #1
On Wed, Jun 24, 2015 at 06:34:18PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> This has no functional changes, it just adjusts
> the ioremap() call for the framebuffer to use
> the same values we later use for the framebuffer,
> this will make it easier to review the next change.
> 
> The size of the framebuffer varies but since this is
> for PCI we *know* this defaults to 0x800000.
> atyfb_setup_generic() is *only* used on PCI probe.
> 
> Cc: Toshi Kani <toshi.kani@hp.com>
> Cc: Suresh Siddha <sbsiddha@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Antonino Daplas <adaplas@gmail.com>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Ville Syrjälä <syrjala@sci.fi>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Mathias Krause <minipli@googlemail.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Davidlohr Bueso <dbueso@suse.de>
> Cc: linux-fbdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  drivers/video/fbdev/aty/atyfb_base.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> index 16936bb..8025624 100644
> --- a/drivers/video/fbdev/aty/atyfb_base.c
> +++ b/drivers/video/fbdev/aty/atyfb_base.c
> @@ -3489,7 +3489,9 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
>  
>  	/* Map in frame buffer */
>  	info->fix.smem_start = addr;
> -	info->screen_base = ioremap(addr, 0x800000);
> +	info->fix.smem_len = 0x800000;
> +
> +	info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);

The framebuffer size isn't always 8MB. That's the size of the BAR. So
this change isn't really correct. I suppose it doesn't hurt too much
since smem_len gets overwritten later in aty_init().

>  	if (info->screen_base == NULL) {
>  		ret = -ENOMEM;
>  		goto atyfb_setup_generic_fail;
> -- 
> 2.3.2.209.gd67f9d5.dirty
>
Luis R. Rodriguez June 25, 2015, 11:06 p.m. UTC | #2
On Thu, Jun 25, 2015 at 4:04 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> it doesn't hurt too much
> since smem_len gets overwritten later in aty_init().

That's the idea, we set it with a default as it will be overwritten
later anyway.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ville Syrjälä June 25, 2015, 11:11 p.m. UTC | #3
On Thu, Jun 25, 2015 at 04:06:45PM -0700, Luis R. Rodriguez wrote:
> On Thu, Jun 25, 2015 at 4:04 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> > it doesn't hurt too much
> > since smem_len gets overwritten later in aty_init().
> 
> That's the idea, we set it with a default as it will be overwritten
> later anyway.

Maybe toss in a comment? Otherwise it's a bit dishonest and might give
someone the impression that all PCI cards really have 8MB of memory.
Luis Chamberlain June 26, 2015, 1:09 a.m. UTC | #4
On Fri, Jun 26, 2015 at 02:11:03AM +0300, Ville Syrjälä wrote:
> On Thu, Jun 25, 2015 at 04:06:45PM -0700, Luis R. Rodriguez wrote:
> > On Thu, Jun 25, 2015 at 4:04 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> > > it doesn't hurt too much
> > > since smem_len gets overwritten later in aty_init().
> > 
> > That's the idea, we set it with a default as it will be overwritten
> > later anyway.
> 
> Maybe toss in a comment? Otherwise it's a bit dishonest and might give
> someone the impression that all PCI cards really have 8MB of memory.

Sure, mind this as a follow up patch if its too late?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov June 26, 2015, 7:30 a.m. UTC | #5
On Fri, Jun 26, 2015 at 03:09:27AM +0200, Luis R. Rodriguez wrote:
> Sure, mind this as a follow up patch if its too late?

No need, you can send me an updated one - I'll replace it.

Thanks.
Luis Chamberlain July 2, 2015, 11:23 p.m. UTC | #6
On Fri, Jun 26, 2015 at 12:30 AM, Borislav Petkov <bp@suse.de> wrote:
> On Fri, Jun 26, 2015 at 03:09:27AM +0200, Luis R. Rodriguez wrote:
>> Sure, mind this as a follow up patch if its too late?
>
> No need, you can send me an updated one - I'll replace it.

Will do!

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain July 8, 2015, 12:24 a.m. UTC | #7
On Thu, Jul 2, 2015 at 4:23 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Fri, Jun 26, 2015 at 12:30 AM, Borislav Petkov <bp@suse.de> wrote:
>> On Fri, Jun 26, 2015 at 03:09:27AM +0200, Luis R. Rodriguez wrote:
>>> Sure, mind this as a follow up patch if its too late?
>>
>> No need, you can send me an updated one - I'll replace it.
>
> Will do!

OK the commend I'm adding:

@@ -3489,6 +3489,15 @@ static int atyfb_setup_generic(struct pci_dev
*pdev, struct fb_info *info,

        /* Map in frame buffer */
        info->fix.smem_start = addr;
+
+       /*
+        * The framebuffer is not always 8 MiB that's just the size of the
+        * PCI BAR, this is later corrected for use with write-combining
+        * helpers with aty_fudge_framebuffer_len() which will adjust the
+        * framebuffer accordingly depending on the device. We do this
+        * to match semantics over ioremap calls on framebuffer devices
+        * with with other drivers with the info->fix.smem_len.
+        */
        info->fix.smem_len = 0x800000;

        info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);

Will respin.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ville Syrjälä July 8, 2015, 8:38 a.m. UTC | #8
On Tue, Jul 07, 2015 at 05:24:57PM -0700, Luis R. Rodriguez wrote:
> On Thu, Jul 2, 2015 at 4:23 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Jun 26, 2015 at 12:30 AM, Borislav Petkov <bp@suse.de> wrote:
> >> On Fri, Jun 26, 2015 at 03:09:27AM +0200, Luis R. Rodriguez wrote:
> >>> Sure, mind this as a follow up patch if its too late?
> >>
> >> No need, you can send me an updated one - I'll replace it.
> >
> > Will do!
> 
> OK the commend I'm adding:
> 
> @@ -3489,6 +3489,15 @@ static int atyfb_setup_generic(struct pci_dev
> *pdev, struct fb_info *info,
> 
>         /* Map in frame buffer */
>         info->fix.smem_start = addr;
> +
> +       /*
> +        * The framebuffer is not always 8 MiB that's just the size of the
> +        * PCI BAR, this is later corrected for use with write-combining
> +        * helpers with aty_fudge_framebuffer_len() which will adjust the
> +        * framebuffer accordingly depending on the device.

That somehow gives me the impression that aty_fudge_framebuffer_len()
changes smem_len to match the framebuffer size, which it does
not.

Dunno, maybe something like this?
/*
 * The framebuffer is not always 8 MiB that's just the size of the
 * PCI BAR. We temporarily abuse smem_len here to store the size
 * of the BAR. aty_init() will later correct it to match the actual
 * framebuffer size.
 *
 * On devices that don't have the auxiliary register aperture, the
 * registers are housed at the top end of the framebuffer PCI BAR.
 * aty_fudge_framebuffer_len() is used to reduce smem_len to not
 * overlap with the registers.
 */

> We do this
> +        * to match semantics over ioremap calls on framebuffer devices
> +        * with with other drivers with the info->fix.smem_len.
> +        */
>         info->fix.smem_len = 0x800000;
> 
>         info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
> 
> Will respin.
> 
>  Luis
Luis Chamberlain July 9, 2015, 5:25 p.m. UTC | #9
On Wed, Jul 08, 2015 at 11:38:49AM +0300, Ville Syrjälä wrote:
> On Tue, Jul 07, 2015 at 05:24:57PM -0700, Luis R. Rodriguez wrote:
> > On Thu, Jul 2, 2015 at 4:23 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > On Fri, Jun 26, 2015 at 12:30 AM, Borislav Petkov <bp@suse.de> wrote:
> > >> On Fri, Jun 26, 2015 at 03:09:27AM +0200, Luis R. Rodriguez wrote:
> > >>> Sure, mind this as a follow up patch if its too late?
> > >>
> > >> No need, you can send me an updated one - I'll replace it.
> > >
> > > Will do!
> > 
> > OK the commend I'm adding:
> > 
> > @@ -3489,6 +3489,15 @@ static int atyfb_setup_generic(struct pci_dev
> > *pdev, struct fb_info *info,
> > 
> >         /* Map in frame buffer */
> >         info->fix.smem_start = addr;
> > +
> > +       /*
> > +        * The framebuffer is not always 8 MiB that's just the size of the
> > +        * PCI BAR, this is later corrected for use with write-combining
> > +        * helpers with aty_fudge_framebuffer_len() which will adjust the
> > +        * framebuffer accordingly depending on the device.
> 
> That somehow gives me the impression that aty_fudge_framebuffer_len()
> changes smem_len to match the framebuffer size, which it does
> not.
> 
> Dunno, maybe something like this?
> /*
>  * The framebuffer is not always 8 MiB that's just the size of the
>  * PCI BAR. We temporarily abuse smem_len here to store the size
>  * of the BAR. aty_init() will later correct it to match the actual
>  * framebuffer size.
>  *
>  * On devices that don't have the auxiliary register aperture, the
>  * registers are housed at the top end of the framebuffer PCI BAR.
>  * aty_fudge_framebuffer_len() is used to reduce smem_len to not
>  * overlap with the registers.
>  */

Thanks Ville, I used that. Will send out a v6 series.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index 16936bb..8025624 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -3489,7 +3489,9 @@  static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
 
 	/* Map in frame buffer */
 	info->fix.smem_start = addr;
-	info->screen_base = ioremap(addr, 0x800000);
+	info->fix.smem_len = 0x800000;
+
+	info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
 	if (info->screen_base == NULL) {
 		ret = -ENOMEM;
 		goto atyfb_setup_generic_fail;