[Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

It's just open coded and matches.

Note that Thomas said that his version apparently failed for some
reason, but hey maybe we should try again.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 420fc75c240e..3ac24a780f50 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
 
 MODULE_DEVICE_TABLE(pci, ast_pciidlist);
 
-static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
-{
-	bool primary = false;
-	resource_size_t base, size;
-
-	base = pci_resource_start(pdev, 0);
-	size = pci_resource_len(pdev, 0);
-#ifdef CONFIG_X86
-	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
-#endif
-
-	return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
-}
-
 static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct ast_private *ast;
 	struct drm_device *dev;
 	int ret;
 
-	ret = ast_remove_conflicting_framebuffers(pdev);
+	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
 	if (ret)
 		return ret;
 
-- 
2.39.0

[Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

This one nukes all framebuffers, which is a bit much. In reality
gma500 is igpu and never shipped with anything discrete, so there should
not be any difference.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/gma500/psb_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index cd9c73f5a64a..9b0daf90dc50 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
 	 *       might be able to read the framebuffer range from the device.
 	 */
-	ret = drm_aperture_remove_framebuffers(true, &driver);
+	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
 	if (ret)
 		return ret;
 
-- 
2.39.0

Re: [Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1671 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> This one nukes all framebuffers, which is a bit much. In reality
> gma500 is igpu and never shipped with anything discrete, so there should
> not be any difference.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index cd9c73f5a64a..9b0daf90dc50 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
>   	 *       might be able to read the framebuffer range from the device.
>   	 */
> -	ret = drm_aperture_remove_framebuffers(true, &driver);
> +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);

This does not work. The comment just above the changed line explains 
why. The device uses shared memory similar to other integrated Intel 
chips. The console is somewhere in a 16 MiB range, which has been stolen 
by the BIOS from main memory. There's only a 1 MiB memory range on the 
device to program the device. Unless you want to refactor as described, 
this call has to cover the whole memory for now.

Best regards
Thomas

>   	if (ret)
>   		return ret;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > This one nukes all framebuffers, which is a bit much. In reality
> > gma500 is igpu and never shipped with anything discrete, so there should
> > not be any difference.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >   drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> > index cd9c73f5a64a..9b0daf90dc50 100644
> > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >   	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> >   	 *       might be able to read the framebuffer range from the device.
> >   	 */
> > -	ret = drm_aperture_remove_framebuffers(true, &driver);
> > +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> 
> This does not work. The comment just above the changed line explains why.
> The device uses shared memory similar to other integrated Intel chips. The
> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
> from main memory. There's only a 1 MiB memory range on the device to program
> the device. Unless you want to refactor as described, this call has to cover
> the whole memory for now.

Uh. So it's maybe not so pretty, but what if I just call both functions?
That way we get the vga handling through the pci one, and the "make sure
there's no fb left" through the other one. Plus comment of course.

Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
and that just feels all kinds of wrong to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2965 bytes --]

Hi

Am 12.01.23 um 10:59 schrieb Daniel Vetter:
> On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>>> This one nukes all framebuffers, which is a bit much. In reality
>>> gma500 is igpu and never shipped with anything discrete, so there should
>>> not be any difference.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>    drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
>>> index cd9c73f5a64a..9b0daf90dc50 100644
>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
>>> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>    	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
>>>    	 *       might be able to read the framebuffer range from the device.
>>>    	 */
>>> -	ret = drm_aperture_remove_framebuffers(true, &driver);
>>> +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>>
>> This does not work. The comment just above the changed line explains why.
>> The device uses shared memory similar to other integrated Intel chips. The
>> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
>> from main memory. There's only a 1 MiB memory range on the device to program
>> the device. Unless you want to refactor as described, this call has to cover
>> the whole memory for now.
> 
> Uh. So it's maybe not so pretty, but what if I just call both functions?

That's ways more ugly IMHO.

> That way we get the vga handling through the pci one, and the "make sure
> there's no fb left" through the other one. Plus comment of course.
> 
> Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
> and that just feels all kinds of wrong to me.

With your patch applied, aperture_detach_devices() does all the work of 
removing. I'd add the following internal functions:

static void aperture_detach_head(bool is_primary)
{
	/*
	 * lengthy comment here
	 */
	if (is_primary)
		sysfb_disable()
}

static void aperture_detach_tail(bool remove_vga)
{
	if (remove_vga) {
		aperture_detach_devices(VGA_PHYS_)
		vga_remove_vgacon()
	}
}

And call both of them at the beginning/end of 
aperture_remove_conflicting_devices() and 
aperture_remove_conflicting_pci_devices().

You'd still need to primary argument to 
aperture_remove_conflicting_devices(), but there will be no code 
duplication with the aperture helpers and the purpose of each code 
fragment will be clearer.

Best regards
Thomas

> -Daniel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Thu, Jan 12, 2023 at 11:24:13AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.01.23 um 10:59 schrieb Daniel Vetter:
> > On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > > > This one nukes all framebuffers, which is a bit much. In reality
> > > > gma500 is igpu and never shipped with anything discrete, so there should
> > > > not be any difference.
> > > > 
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >    drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> > > > index cd9c73f5a64a..9b0daf90dc50 100644
> > > > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > > > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > > > @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > >    	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> > > >    	 *       might be able to read the framebuffer range from the device.
> > > >    	 */
> > > > -	ret = drm_aperture_remove_framebuffers(true, &driver);
> > > > +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> > > 
> > > This does not work. The comment just above the changed line explains why.
> > > The device uses shared memory similar to other integrated Intel chips. The
> > > console is somewhere in a 16 MiB range, which has been stolen by the BIOS
> > > from main memory. There's only a 1 MiB memory range on the device to program
> > > the device. Unless you want to refactor as described, this call has to cover
> > > the whole memory for now.
> > 
> > Uh. So it's maybe not so pretty, but what if I just call both functions?
> 
> That's ways more ugly IMHO.
> 
> > That way we get the vga handling through the pci one, and the "make sure
> > there's no fb left" through the other one. Plus comment of course.
> > 
> > Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
> > and that just feels all kinds of wrong to me.
> 
> With your patch applied, aperture_detach_devices() does all the work of
> removing. I'd add the following internal functions:
> 
> static void aperture_detach_head(bool is_primary)
> {
> 	/*
> 	 * lengthy comment here
> 	 */
> 	if (is_primary)
> 		sysfb_disable()
> }
> 
> static void aperture_detach_tail(bool remove_vga)
> {
> 	if (remove_vga) {
> 		aperture_detach_devices(VGA_PHYS_)
> 		vga_remove_vgacon()
> 	}
> }
> 
> And call both of them at the beginning/end of
> aperture_remove_conflicting_devices() and
> aperture_remove_conflicting_pci_devices().
> 
> You'd still need to primary argument to
> aperture_remove_conflicting_devices(), but there will be no code duplication
> with the aperture helpers and the purpose of each code fragment will be
> clearer.

Yeah I don't want the primary argument. Aside from this one case here it's
not needed. Also by pushing this special case into the one driver that
needs it we keep it contained, instead of spreading it all around.
Inflicting a parameter on every (and in total we have a lot of callers of
this stuff) just because of gma500 does not seem like a good idea to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4650 bytes --]

Hi

Am 12.01.23 um 11:45 schrieb Daniel Vetter:
> On Thu, Jan 12, 2023 at 11:24:13AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.01.23 um 10:59 schrieb Daniel Vetter:
>>> On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
>>>> Hi
>>>>
>>>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>>>>> This one nukes all framebuffers, which is a bit much. In reality
>>>>> gma500 is igpu and never shipped with anything discrete, so there should
>>>>> not be any difference.
>>>>>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
>>>>> index cd9c73f5a64a..9b0daf90dc50 100644
>>>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
>>>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
>>>>> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>>     	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
>>>>>     	 *       might be able to read the framebuffer range from the device.
>>>>>     	 */
>>>>> -	ret = drm_aperture_remove_framebuffers(true, &driver);
>>>>> +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>>>>
>>>> This does not work. The comment just above the changed line explains why.
>>>> The device uses shared memory similar to other integrated Intel chips. The
>>>> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
>>>> from main memory. There's only a 1 MiB memory range on the device to program
>>>> the device. Unless you want to refactor as described, this call has to cover
>>>> the whole memory for now.
>>>
>>> Uh. So it's maybe not so pretty, but what if I just call both functions?
>>
>> That's ways more ugly IMHO.
>>
>>> That way we get the vga handling through the pci one, and the "make sure
>>> there's no fb left" through the other one. Plus comment of course.
>>>
>>> Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
>>> and that just feels all kinds of wrong to me.
>>
>> With your patch applied, aperture_detach_devices() does all the work of
>> removing. I'd add the following internal functions:
>>
>> static void aperture_detach_head(bool is_primary)
>> {
>> 	/*
>> 	 * lengthy comment here
>> 	 */
>> 	if (is_primary)
>> 		sysfb_disable()
>> }
>>
>> static void aperture_detach_tail(bool remove_vga)
>> {
>> 	if (remove_vga) {
>> 		aperture_detach_devices(VGA_PHYS_)
>> 		vga_remove_vgacon()
>> 	}
>> }
>>
>> And call both of them at the beginning/end of
>> aperture_remove_conflicting_devices() and
>> aperture_remove_conflicting_pci_devices().
>>
>> You'd still need to primary argument to
>> aperture_remove_conflicting_devices(), but there will be no code duplication
>> with the aperture helpers and the purpose of each code fragment will be
>> clearer.
> 
> Yeah I don't want the primary argument. Aside from this one case here it's
> not needed. Also by pushing this special case into the one driver that
> needs it we keep it contained, instead of spreading it all around.
> Inflicting a parameter on every (and in total we have a lot of callers of
> this stuff) just because of gma500 does not seem like a good idea to me.

Unfortunately, vgacon and vgaarb require a PCI device. I don't like the 
proposal, but maybe it's the best for now. So go ahead if you like. I do 
expect that this needs additional work in future, however.

Just some more thoughts.

Grep for drm_aperture_remove_framebuffers(). Within DRM there are really 
just 10 drivers calling this function (vs. 12 callers of 
drm_aperture_remove_conflicting_pci_framebuffers()). In fbdev, there are 
many callers of the PCI helper (~40) and apparently only 3 for the 
non-PCI one. The other drivers are panels, USB, MIPI, etc and don't 
interact with the system framebuffer. Compared to the overall number of 
drivers, we have surprisingly few 'traditional graphics cards' in DRM.

Another thing is that gma500 and the other 9 drivers simply don't bother 
to get the framebuffer range. They should be reworked to fetch the 
configured framebuffer from the device and release that region only. The 
practical impact is close to zero, so it hasn't happened.

Best regards
Thomas


> -Daniel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 02/11] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Thu, 12 Jan 2023 at 13:15, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 12.01.23 um 11:45 schrieb Daniel Vetter:
> > On Thu, Jan 12, 2023 at 11:24:13AM +0100, Thomas Zimmermann wrote:
> >> Hi
> >>
> >> Am 12.01.23 um 10:59 schrieb Daniel Vetter:
> >>> On Thu, Jan 12, 2023 at 10:04:48AM +0100, Thomas Zimmermann wrote:
> >>>> Hi
> >>>>
> >>>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> >>>>> This one nukes all framebuffers, which is a bit much. In reality
> >>>>> gma500 is igpu and never shipped with anything discrete, so there should
> >>>>> not be any difference.
> >>>>>
> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>>>> ---
> >>>>>     drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> >>>>> index cd9c73f5a64a..9b0daf90dc50 100644
> >>>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
> >>>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> >>>>> @@ -429,7 +429,7 @@ static int psb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>>>>            * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> >>>>>            *       might be able to read the framebuffer range from the device.
> >>>>>            */
> >>>>> - ret = drm_aperture_remove_framebuffers(true, &driver);
> >>>>> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> >>>>
> >>>> This does not work. The comment just above the changed line explains why.
> >>>> The device uses shared memory similar to other integrated Intel chips. The
> >>>> console is somewhere in a 16 MiB range, which has been stolen by the BIOS
> >>>> from main memory. There's only a 1 MiB memory range on the device to program
> >>>> the device. Unless you want to refactor as described, this call has to cover
> >>>> the whole memory for now.
> >>>
> >>> Uh. So it's maybe not so pretty, but what if I just call both functions?
> >>
> >> That's ways more ugly IMHO.
> >>
> >>> That way we get the vga handling through the pci one, and the "make sure
> >>> there's no fb left" through the other one. Plus comment of course.
> >>>
> >>> Otherwise we'd need to somehow keep the vga stuff in the non-pci paths,
> >>> and that just feels all kinds of wrong to me.
> >>
> >> With your patch applied, aperture_detach_devices() does all the work of
> >> removing. I'd add the following internal functions:
> >>
> >> static void aperture_detach_head(bool is_primary)
> >> {
> >>      /*
> >>       * lengthy comment here
> >>       */
> >>      if (is_primary)
> >>              sysfb_disable()
> >> }
> >>
> >> static void aperture_detach_tail(bool remove_vga)
> >> {
> >>      if (remove_vga) {
> >>              aperture_detach_devices(VGA_PHYS_)
> >>              vga_remove_vgacon()
> >>      }
> >> }
> >>
> >> And call both of them at the beginning/end of
> >> aperture_remove_conflicting_devices() and
> >> aperture_remove_conflicting_pci_devices().
> >>
> >> You'd still need to primary argument to
> >> aperture_remove_conflicting_devices(), but there will be no code duplication
> >> with the aperture helpers and the purpose of each code fragment will be
> >> clearer.
> >
> > Yeah I don't want the primary argument. Aside from this one case here it's
> > not needed. Also by pushing this special case into the one driver that
> > needs it we keep it contained, instead of spreading it all around.
> > Inflicting a parameter on every (and in total we have a lot of callers of
> > this stuff) just because of gma500 does not seem like a good idea to me.
>
> Unfortunately, vgacon and vgaarb require a PCI device. I don't like the
> proposal, but maybe it's the best for now. So go ahead if you like. I do
> expect that this needs additional work in future, however.
>
> Just some more thoughts.
>
> Grep for drm_aperture_remove_framebuffers(). Within DRM there are really
> just 10 drivers calling this function (vs. 12 callers of
> drm_aperture_remove_conflicting_pci_framebuffers()). In fbdev, there are
> many callers of the PCI helper (~40) and apparently only 3 for the
> non-PCI one. The other drivers are panels, USB, MIPI, etc and don't
> interact with the system framebuffer. Compared to the overall number of
> drivers, we have surprisingly few 'traditional graphics cards' in DRM.

This is largely historical. fbdev is from the 90s, when we had the
huge explosion in largely pci graphics cards, because that was the
place where all the growth and hence drivers were. 80% of these
companies/chipe all died within a short few years.

kms otoh had the huge growth in the 2010s, where there was the tail
end of the SoC mobile explosion, so that's where we have tons of
drivers. If you look at staging there's a pile more fbdev drivers for
SoC, but many of these never got beyond the "vendor hacked some stuff
together and shipped it" stage. So that's probably why they lack
polish like fw -> driver handover (most of these just booted directly
to the driver in real products).

> Another thing is that gma500 and the other 9 drivers simply don't bother
> to get the framebuffer range. They should be reworked to fetch the
> configured framebuffer from the device and release that region only. The
> practical impact is close to zero, so it hasn't happened.

I think that's ok, because trying to figure out the real fb means you
need fairly complete hw state readout (otherwise there's no
motivation), and i915 is the only driver that ever really did that.
Just for fw driver removal "nuke everything" gets the job done.
-Daniel

> Best regards
> Thomas
>
>
> > -Daniel
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 03/11] drm/aperture: Remove primary argument

[Daniel Vetter]

Only really pci devices have a business setting this - it's for
figuring out whether the legacy vga stuff should be nuked too. And
with the preceeding two patches those are all using the pci version of
this.

Which means for all other callers primary == false and we can remove
it now.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Deepak Rawat <drawat.floss@gmail.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Emma Anholt <emma@anholt.net>
Cc: Helge Deller <deller@gmx.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: linux-hyperv@vger.kernel.org
Cc: linux-amlogic@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-tegra@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org
---
 drivers/gpu/drm/arm/hdlcd_drv.c             |  2 +-
 drivers/gpu/drm/armada/armada_drv.c         |  2 +-
 drivers/gpu/drm/drm_aperture.c              | 11 +++--------
 drivers/gpu/drm/hyperv/hyperv_drm_drv.c     |  1 -
 drivers/gpu/drm/meson/meson_drv.c           |  2 +-
 drivers/gpu/drm/msm/msm_fbdev.c             |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
 drivers/gpu/drm/stm/drv.c                   |  2 +-
 drivers/gpu/drm/sun4i/sun4i_drv.c           |  2 +-
 drivers/gpu/drm/tegra/drm.c                 |  2 +-
 drivers/gpu/drm/vc4/vc4_drv.c               |  2 +-
 include/drm/drm_aperture.h                  |  7 +++----
 12 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 7043d1c9ed8f..98267e355918 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -297,7 +297,7 @@ static int hdlcd_drm_bind(struct device *dev)
 	 */
 	if (hdlcd_read(hdlcd, HDLCD_REG_COMMAND)) {
 		hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
-		drm_aperture_remove_framebuffers(false, &hdlcd_driver);
+		drm_aperture_remove_framebuffers(&hdlcd_driver);
 	}
 
 	drm_mode_config_reset(drm);
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 0643887800b4..c99ec7078301 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -95,7 +95,7 @@ static int armada_drm_bind(struct device *dev)
 	}
 
 	/* Remove early framebuffers */
-	ret = drm_aperture_remove_framebuffers(false, &armada_drm_driver);
+	ret = drm_aperture_remove_framebuffers(&armada_drm_driver);
 	if (ret) {
 		dev_err(dev, "[" DRM_NAME ":%s] can't kick out simple-fb: %d\n",
 			__func__, ret);
diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
index 3b8fdeeafd53..697cffbfd603 100644
--- a/drivers/gpu/drm/drm_aperture.c
+++ b/drivers/gpu/drm/drm_aperture.c
@@ -32,17 +32,13 @@
  *
  *	static int remove_conflicting_framebuffers(struct pci_dev *pdev)
  *	{
- *		bool primary = false;
  *		resource_size_t base, size;
  *		int ret;
  *
  *		base = pci_resource_start(pdev, 0);
  *		size = pci_resource_len(pdev, 0);
- *	#ifdef CONFIG_X86
- *		primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
- *	#endif
  *
- *		return drm_aperture_remove_conflicting_framebuffers(base, size, primary,
+ *		return drm_aperture_remove_conflicting_framebuffers(base, size,
  *		                                                    &example_driver);
  *	}
  *
@@ -161,7 +157,6 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
  * drm_aperture_remove_conflicting_framebuffers - remove existing framebuffers in the given range
  * @base: the aperture's base address in physical memory
  * @size: aperture size in bytes
- * @primary: also kick vga16fb if present
  * @req_driver: requesting DRM driver
  *
  * This function removes graphics device drivers which use the memory range described by
@@ -171,9 +166,9 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
  * 0 on success, or a negative errno code otherwise
  */
 int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
-						 bool primary, const struct drm_driver *req_driver)
+						 const struct drm_driver *req_driver)
 {
-	return aperture_remove_conflicting_devices(base, size, primary, req_driver->name);
+	return aperture_remove_conflicting_devices(base, size, false, req_driver->name);
 }
 EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
 
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 427c20ba3404..7e81d58c083f 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -74,7 +74,6 @@ static int hyperv_setup_vram(struct hyperv_drm_device *hv,
 
 	drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
 						     screen_info.lfb_size,
-						     false,
 						     &hyperv_driver);
 
 	hv->fb_size = (unsigned long)hv->mmio_megabytes * 1024 * 1024;
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 79bfe3938d3c..c8d39809d897 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -285,7 +285,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
 	 * Remove early framebuffers (ie. simplefb). The framebuffer can be
 	 * located anywhere in RAM
 	 */
-	ret = drm_aperture_remove_framebuffers(false, &meson_driver);
+	ret = drm_aperture_remove_framebuffers(&meson_driver);
 	if (ret)
 		goto free_drm;
 
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 31e1e30cb52a..84dfbccb6912 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -155,7 +155,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
 	}
 
 	/* the fw fb could be anywhere in memory */
-	ret = drm_aperture_remove_framebuffers(false, dev->driver);
+	ret = drm_aperture_remove_framebuffers(dev->driver);
 	if (ret)
 		goto fini;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 6e0788d14c10..d97f2edc646b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -140,7 +140,7 @@ static int rockchip_drm_bind(struct device *dev)
 	int ret;
 
 	/* Remove existing drivers that may own the framebuffer memory. */
-	ret = drm_aperture_remove_framebuffers(false, &rockchip_drm_driver);
+	ret = drm_aperture_remove_framebuffers(&rockchip_drm_driver);
 	if (ret) {
 		DRM_DEV_ERROR(dev,
 			      "Failed to remove existing framebuffers - %d.\n",
diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 50410bd99dfe..354349c6e085 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -185,7 +185,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
 
 	DRM_DEBUG("%s\n", __func__);
 
-	ret = drm_aperture_remove_framebuffers(false, &drv_driver);
+	ret = drm_aperture_remove_framebuffers(&drv_driver);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index cc94efbbf2d4..6367b89cbab1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -98,7 +98,7 @@ static int sun4i_drv_bind(struct device *dev)
 		goto cleanup_mode_config;
 
 	/* Remove early framebuffers (ie. simplefb) */
-	ret = drm_aperture_remove_framebuffers(false, &sun4i_drv_driver);
+	ret = drm_aperture_remove_framebuffers(&sun4i_drv_driver);
 	if (ret)
 		goto cleanup_mode_config;
 
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 7bd2e65c2a16..d2ff527cf6d7 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1252,7 +1252,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
 
 	drm_mode_config_reset(drm);
 
-	err = drm_aperture_remove_framebuffers(false, &tegra_drm_driver);
+	err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
 	if (err < 0)
 		goto hub;
 
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 0ccaee57fe9a..0a9e922636b1 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -350,7 +350,7 @@ static int vc4_drm_bind(struct device *dev)
 			return -EPROBE_DEFER;
 	}
 
-	ret = drm_aperture_remove_framebuffers(false, driver);
+	ret = drm_aperture_remove_framebuffers(driver);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/drm_aperture.h b/include/drm/drm_aperture.h
index 7096703c3949..cbe33b49fd5d 100644
--- a/include/drm/drm_aperture.h
+++ b/include/drm/drm_aperture.h
@@ -13,14 +13,13 @@ int devm_aperture_acquire_from_firmware(struct drm_device *dev, resource_size_t
 					resource_size_t size);
 
 int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
-						 bool primary, const struct drm_driver *req_driver);
+						 const struct drm_driver *req_driver);
 
 int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
 						     const struct drm_driver *req_driver);
 
 /**
  * drm_aperture_remove_framebuffers - remove all existing framebuffers
- * @primary: also kick vga16fb if present
  * @req_driver: requesting DRM driver
  *
  * This function removes all graphics device drivers. Use this function on systems
@@ -30,9 +29,9 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
  * 0 on success, or a negative errno code otherwise
  */
 static inline int
-drm_aperture_remove_framebuffers(bool primary, const struct drm_driver *req_driver)
+drm_aperture_remove_framebuffers(const struct drm_driver *req_driver)
 {
-	return drm_aperture_remove_conflicting_framebuffers(0, (resource_size_t)-1, primary,
+	return drm_aperture_remove_conflicting_framebuffers(0, (resource_size_t)-1,
 							    req_driver);
 }
 
-- 
2.39.0

Re: [Intel-gfx] [PATCH 03/11] drm/aperture: Remove primary argument

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 11297 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> Only really pci devices have a business setting this - it's for
> figuring out whether the legacy vga stuff should be nuked too. And
> with the preceeding two patches those are all using the pci version of
> this.
> 
> Which means for all other callers primary == false and we can remove
> it now.

AFAICS this patch needs to be merged with patch 4 to build.

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Deepak Rawat <drawat.floss@gmail.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: Emma Anholt <emma@anholt.net>
> Cc: Helge Deller <deller@gmx.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: linux-hyperv@vger.kernel.org
> Cc: linux-amlogic@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-tegra@vger.kernel.org
> Cc: linux-fbdev@vger.kernel.org
> ---
>   drivers/gpu/drm/arm/hdlcd_drv.c             |  2 +-
>   drivers/gpu/drm/armada/armada_drv.c         |  2 +-
>   drivers/gpu/drm/drm_aperture.c              | 11 +++--------
>   drivers/gpu/drm/hyperv/hyperv_drm_drv.c     |  1 -
>   drivers/gpu/drm/meson/meson_drv.c           |  2 +-
>   drivers/gpu/drm/msm/msm_fbdev.c             |  2 +-
>   drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
>   drivers/gpu/drm/stm/drv.c                   |  2 +-
>   drivers/gpu/drm/sun4i/sun4i_drv.c           |  2 +-
>   drivers/gpu/drm/tegra/drm.c                 |  2 +-
>   drivers/gpu/drm/vc4/vc4_drv.c               |  2 +-
>   include/drm/drm_aperture.h                  |  7 +++----
>   12 files changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 7043d1c9ed8f..98267e355918 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -297,7 +297,7 @@ static int hdlcd_drm_bind(struct device *dev)
>   	 */
>   	if (hdlcd_read(hdlcd, HDLCD_REG_COMMAND)) {
>   		hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
> -		drm_aperture_remove_framebuffers(false, &hdlcd_driver);
> +		drm_aperture_remove_framebuffers(&hdlcd_driver);
>   	}
>   
>   	drm_mode_config_reset(drm);
> diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
> index 0643887800b4..c99ec7078301 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -95,7 +95,7 @@ static int armada_drm_bind(struct device *dev)
>   	}
>   
>   	/* Remove early framebuffers */
> -	ret = drm_aperture_remove_framebuffers(false, &armada_drm_driver);
> +	ret = drm_aperture_remove_framebuffers(&armada_drm_driver);
>   	if (ret) {
>   		dev_err(dev, "[" DRM_NAME ":%s] can't kick out simple-fb: %d\n",
>   			__func__, ret);
> diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
> index 3b8fdeeafd53..697cffbfd603 100644
> --- a/drivers/gpu/drm/drm_aperture.c
> +++ b/drivers/gpu/drm/drm_aperture.c
> @@ -32,17 +32,13 @@
>    *
>    *	static int remove_conflicting_framebuffers(struct pci_dev *pdev)
>    *	{
> - *		bool primary = false;
>    *		resource_size_t base, size;
>    *		int ret;
>    *
>    *		base = pci_resource_start(pdev, 0);
>    *		size = pci_resource_len(pdev, 0);
> - *	#ifdef CONFIG_X86
> - *		primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> - *	#endif
>    *
> - *		return drm_aperture_remove_conflicting_framebuffers(base, size, primary,
> + *		return drm_aperture_remove_conflicting_framebuffers(base, size,
>    *		                                                    &example_driver);
>    *	}
>    *
> @@ -161,7 +157,6 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
>    * drm_aperture_remove_conflicting_framebuffers - remove existing framebuffers in the given range
>    * @base: the aperture's base address in physical memory
>    * @size: aperture size in bytes
> - * @primary: also kick vga16fb if present
>    * @req_driver: requesting DRM driver
>    *
>    * This function removes graphics device drivers which use the memory range described by
> @@ -171,9 +166,9 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
>    * 0 on success, or a negative errno code otherwise
>    */
>   int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
> -						 bool primary, const struct drm_driver *req_driver)
> +						 const struct drm_driver *req_driver)
>   {
> -	return aperture_remove_conflicting_devices(base, size, primary, req_driver->name);
> +	return aperture_remove_conflicting_devices(base, size, false, req_driver->name);
>   }
>   EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
>   
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index 427c20ba3404..7e81d58c083f 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -74,7 +74,6 @@ static int hyperv_setup_vram(struct hyperv_drm_device *hv,
>   
>   	drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
>   						     screen_info.lfb_size,
> -						     false,
>   						     &hyperv_driver);
>   
>   	hv->fb_size = (unsigned long)hv->mmio_megabytes * 1024 * 1024;
> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
> index 79bfe3938d3c..c8d39809d897 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -285,7 +285,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
>   	 * Remove early framebuffers (ie. simplefb). The framebuffer can be
>   	 * located anywhere in RAM
>   	 */
> -	ret = drm_aperture_remove_framebuffers(false, &meson_driver);
> +	ret = drm_aperture_remove_framebuffers(&meson_driver);
>   	if (ret)
>   		goto free_drm;
>   
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index 31e1e30cb52a..84dfbccb6912 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -155,7 +155,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
>   	}
>   
>   	/* the fw fb could be anywhere in memory */
> -	ret = drm_aperture_remove_framebuffers(false, dev->driver);
> +	ret = drm_aperture_remove_framebuffers(dev->driver);
>   	if (ret)
>   		goto fini;
>   
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 6e0788d14c10..d97f2edc646b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -140,7 +140,7 @@ static int rockchip_drm_bind(struct device *dev)
>   	int ret;
>   
>   	/* Remove existing drivers that may own the framebuffer memory. */
> -	ret = drm_aperture_remove_framebuffers(false, &rockchip_drm_driver);
> +	ret = drm_aperture_remove_framebuffers(&rockchip_drm_driver);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev,
>   			      "Failed to remove existing framebuffers - %d.\n",
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 50410bd99dfe..354349c6e085 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -185,7 +185,7 @@ static int stm_drm_platform_probe(struct platform_device *pdev)
>   
>   	DRM_DEBUG("%s\n", __func__);
>   
> -	ret = drm_aperture_remove_framebuffers(false, &drv_driver);
> +	ret = drm_aperture_remove_framebuffers(&drv_driver);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index cc94efbbf2d4..6367b89cbab1 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -98,7 +98,7 @@ static int sun4i_drv_bind(struct device *dev)
>   		goto cleanup_mode_config;
>   
>   	/* Remove early framebuffers (ie. simplefb) */
> -	ret = drm_aperture_remove_framebuffers(false, &sun4i_drv_driver);
> +	ret = drm_aperture_remove_framebuffers(&sun4i_drv_driver);
>   	if (ret)
>   		goto cleanup_mode_config;
>   
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 7bd2e65c2a16..d2ff527cf6d7 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1252,7 +1252,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
>   
>   	drm_mode_config_reset(drm);
>   
> -	err = drm_aperture_remove_framebuffers(false, &tegra_drm_driver);
> +	err = drm_aperture_remove_framebuffers(&tegra_drm_driver);
>   	if (err < 0)
>   		goto hub;
>   
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 0ccaee57fe9a..0a9e922636b1 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -350,7 +350,7 @@ static int vc4_drm_bind(struct device *dev)
>   			return -EPROBE_DEFER;
>   	}
>   
> -	ret = drm_aperture_remove_framebuffers(false, driver);
> +	ret = drm_aperture_remove_framebuffers(driver);
>   	if (ret)
>   		return ret;
>   
> diff --git a/include/drm/drm_aperture.h b/include/drm/drm_aperture.h
> index 7096703c3949..cbe33b49fd5d 100644
> --- a/include/drm/drm_aperture.h
> +++ b/include/drm/drm_aperture.h
> @@ -13,14 +13,13 @@ int devm_aperture_acquire_from_firmware(struct drm_device *dev, resource_size_t
>   					resource_size_t size);
>   
>   int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
> -						 bool primary, const struct drm_driver *req_driver);
> +						 const struct drm_driver *req_driver);
>   
>   int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>   						     const struct drm_driver *req_driver);
>   
>   /**
>    * drm_aperture_remove_framebuffers - remove all existing framebuffers
> - * @primary: also kick vga16fb if present
>    * @req_driver: requesting DRM driver
>    *
>    * This function removes all graphics device drivers. Use this function on systems
> @@ -30,9 +29,9 @@ int drm_aperture_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>    * 0 on success, or a negative errno code otherwise
>    */
>   static inline int
> -drm_aperture_remove_framebuffers(bool primary, const struct drm_driver *req_driver)
> +drm_aperture_remove_framebuffers(const struct drm_driver *req_driver)
>   {
> -	return drm_aperture_remove_conflicting_framebuffers(0, (resource_size_t)-1, primary,
> +	return drm_aperture_remove_conflicting_framebuffers(0, (resource_size_t)-1,
>   							    req_driver);
>   }
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[Intel-gfx] [PATCH 04/11] video/aperture: use generic code to figure out the vga default device

[Daniel Vetter]

Since vgaarb has been promoted to be a core piece of the pci subsystem
we don't have to open code random guesses anymore, we actually know
this in a platform agnostic way, and there's no need for an x86
specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
drivers/pci")

This should not result in any functional change, and the non-x86
multi-gpu pci systems are probably rare enough to not matter (I don't
know of any tbh). But it's a nice cleanup, so let's do it.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/video/aperture.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 41e77de1ea82..3d8c925c7365 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
  */
 int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
 {
-	bool primary = false;
+	bool primary;
 	resource_size_t base, size;
 	int bar, ret;
 
-#ifdef CONFIG_X86
-	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
-#endif
+	primary = pdev == vga_default_device();
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
 		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
-- 
2.39.0

Re: [Intel-gfx] [PATCH 04/11] video/aperture: use generic code to figure out the vga default device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2779 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> Since vgaarb has been promoted to be a core piece of the pci subsystem
> we don't have to open code random guesses anymore, we actually know
> this in a platform agnostic way, and there's no need for an x86
> specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
> drivers/pci")
> 
> This should not result in any functional change, and the non-x86
> multi-gpu pci systems are probably rare enough to not matter (I don't
> know of any tbh). But it's a nice cleanup, so let's do it.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>   drivers/video/aperture.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 41e77de1ea82..3d8c925c7365 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
>    */
>   int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
>   {
> -	bool primary = false;
> +	bool primary;
>   	resource_size_t base, size;
>   	int bar, ret;
>   
> -#ifdef CONFIG_X86
> -	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> +	primary = pdev == vga_default_device();

vga_default_device() is part of vgaarb and can return NULL. [1] That new 
test is likely to be incorrect on many systems.

I suggest to implement a helper like fb_is_primary_device() on x86: it 
uses the default VGA if set, or falls back to the original test. [2]

It's noteworthy that on most architectures, fb_is_primary_device() 
returns 0. But at least on Sparc [3] and some Parisc [4] machines, it 
does not.

I've long wanted to rework this helper anyway. So this is a good 
opportunity.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/include/linux/vgaarb.h#L69
[2] 
https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14
[3] 
https://elixir.bootlin.com/linux/latest/source/arch/sparc/include/asm/fb.h#L18
[4] 
https://elixir.bootlin.com/linux/latest/source/drivers/video/console/sticore.c#L1153


>   
>   	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
>   		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 04/11] video/aperture: use generic code to figure out the vga default device

[Daniel Vetter]

On Wed, Jan 11, 2023 at 04:59:30PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > Since vgaarb has been promoted to be a core piece of the pci subsystem
> > we don't have to open code random guesses anymore, we actually know
> > this in a platform agnostic way, and there's no need for an x86
> > specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to
> > drivers/pci")
> > 
> > This should not result in any functional change, and the non-x86
> > multi-gpu pci systems are probably rare enough to not matter (I don't
> > know of any tbh). But it's a nice cleanup, so let's do it.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Cc: Helge Deller <deller@gmx.de>
> > Cc: linux-fbdev@vger.kernel.org
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: linux-pci@vger.kernel.org
> > ---
> >   drivers/video/aperture.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> > index 41e77de1ea82..3d8c925c7365 100644
> > --- a/drivers/video/aperture.c
> > +++ b/drivers/video/aperture.c
> > @@ -324,13 +324,11 @@ EXPORT_SYMBOL(aperture_remove_conflicting_devices);
> >    */
> >   int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name)
> >   {
> > -	bool primary = false;
> > +	bool primary;
> >   	resource_size_t base, size;
> >   	int bar, ret;
> > -#ifdef CONFIG_X86
> > -	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> > -#endif
> > +	primary = pdev == vga_default_device();
> 
> vga_default_device() is part of vgaarb and can return NULL. [1] That new
> test is likely to be incorrect on many systems.
> 
> I suggest to implement a helper like fb_is_primary_device() on x86: it uses
> the default VGA if set, or falls back to the original test. [2]
> 
> It's noteworthy that on most architectures, fb_is_primary_device() returns
> 0. But at least on Sparc [3] and some Parisc [4] machines, it does not.

Afaik these neither do legacy vga nor sysfb, so we should be fine. I'll
augment the commit message.

> I've long wanted to rework this helper anyway. So this is a good
> opportunity.

Hm yeah that sounds like a good thing to copy. I'm honestly not sure
whether it's needed, but at least it shouldn't hurt. I thought at least
that the boot default device should be set on all pci architectures.

I'll also reorder this with the previous patch to avoid the compile fail.
-Daniel

> 
> Best regards
> Thomas
> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/include/linux/vgaarb.h#L69
> [2]
> https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14
> [3] https://elixir.bootlin.com/linux/latest/source/arch/sparc/include/asm/fb.h#L18
> [4] https://elixir.bootlin.com/linux/latest/source/drivers/video/console/sticore.c#L1153
> 
> 
> >   	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
> >   		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 05/11] video/aperture: Only kick vgacon when the pdev is decoding vga

[Daniel Vetter]

Otherwise it's bit silly, and we might throw out the driver for the
screen the user is actually looking at. I haven't found a bug report
for this case yet, but we did get bug reports for the analog case
where we're throwing out the efifb driver.

References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/video/aperture.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 3d8c925c7365..6f351a58f6c6 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -341,6 +341,9 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
 			return ret;
 	}
 
+	if (!primary)
+		return 0;
+
 	/*
 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
 	 * otherwise the vga fbdev driver falls over.
-- 
2.39.0

Re: [Intel-gfx] [PATCH 05/11] video/aperture: Only kick vgacon when the pdev is decoding vga

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1609 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> Otherwise it's bit silly, and we might throw out the driver for the
> screen the user is actually looking at. I haven't found a bug report
> for this case yet, but we did get bug reports for the analog case
> where we're throwing out the efifb driver.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> ---
>   drivers/video/aperture.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 3d8c925c7365..6f351a58f6c6 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -341,6 +341,9 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>   			return ret;
>   	}
>   
> +	if (!primary)
> +		return 0;
> +

The original code from fbdev didn't do this, so this code didn't either.

It appears more to be a special case than an early-out branch. So can we 
write it as

if (primary) {
   // kick_vgacon
}

?

Best regards
Thomas

>   	/*
>   	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
>   	 * otherwise the vga fbdev driver falls over.

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 05/11] video/aperture: Only kick vgacon when the pdev is decoding vga

[Daniel Vetter]

On Wed, Jan 11, 2023 at 05:03:02PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > Otherwise it's bit silly, and we might throw out the driver for the
> > screen the user is actually looking at. I haven't found a bug report
> > for this case yet, but we did get bug reports for the analog case
> > where we're throwing out the efifb driver.
> > 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=216303
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Cc: Helge Deller <deller@gmx.de>
> > Cc: linux-fbdev@vger.kernel.org
> > ---
> >   drivers/video/aperture.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> > index 3d8c925c7365..6f351a58f6c6 100644
> > --- a/drivers/video/aperture.c
> > +++ b/drivers/video/aperture.c
> > @@ -341,6 +341,9 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
> >   			return ret;
> >   	}
> > +	if (!primary)
> > +		return 0;
> > +
> 
> The original code from fbdev didn't do this, so this code didn't either.
> 
> It appears more to be a special case than an early-out branch. So can we
> write it as

Yeah I think this was a mistake going way back to when I added this to
i915 originally. It is a real change, but also I guess the people who have
machines without efifb or vesafb are ... really not many :-) Iirc you had
some very funny kernels going way back when vgacon was considered the only
safe choice to even hit this stuff.

> if (primary) {
>   // kick_vgacon
> }

Yeah, but next patch adds the vga aperture, and then I think it makes a
bit more sense.
-Daniel

> 
> ?
> 
> Best regards
> Thomas
> 
> >   	/*
> >   	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> >   	 * otherwise the vga fbdev driver falls over.
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 06/11] staging/lynxfb: Use pci aperture helper

[Daniel Vetter]

It exists! Note that since this is an exact copy, there shouldn't be
any functional difference here.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Cc: Teddy Wang <teddy.wang@siliconmotion.com>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/staging/sm750fb/sm750.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index effc7fcc3703..22ace3168723 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -989,20 +989,6 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
 	return err;
 }
 
-static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
-{
-	resource_size_t base = pci_resource_start(pdev, 0);
-	resource_size_t size = pci_resource_len(pdev, 0);
-	bool primary = false;
-
-#ifdef CONFIG_X86
-	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
-					IORESOURCE_ROM_SHADOW;
-#endif
-
-	return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");
-}
-
 static int lynxfb_pci_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *ent)
 {
@@ -1011,7 +997,7 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
 	int fbidx;
 	int err;
 
-	err = lynxfb_kick_out_firmware_fb(pdev);
+	err = aperture_remove_conflicting_pci_devices(pdev, "sm750_fb1");
 	if (err)
 		return err;
 
-- 
2.39.0

Re: [Intel-gfx] [PATCH 06/11] staging/lynxfb: Use pci aperture helper

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1916 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It exists! Note that since this is an exact copy, there shouldn't be
> any functional difference here.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Cc: Teddy Wang <teddy.wang@siliconmotion.com>
> Cc: linux-fbdev@vger.kernel.org
> ---
>   drivers/staging/sm750fb/sm750.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index effc7fcc3703..22ace3168723 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -989,20 +989,6 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx)
>   	return err;
>   }
>   
> -static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev)
> -{
> -	resource_size_t base = pci_resource_start(pdev, 0);
> -	resource_size_t size = pci_resource_len(pdev, 0);
> -	bool primary = false;
> -
> -#ifdef CONFIG_X86
> -	primary = pdev->resource[PCI_ROM_RESOURCE].flags &
> -					IORESOURCE_ROM_SHADOW;
> -#endif
> -
> -	return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1");

This still had the primary argument, it needs to be handled in an early 
patch in some way.

> -}
> -
>   static int lynxfb_pci_probe(struct pci_dev *pdev,
>   			    const struct pci_device_id *ent)
>   {
> @@ -1011,7 +997,7 @@ static int lynxfb_pci_probe(struct pci_dev *pdev,
>   	int fbidx;
>   	int err;
>   
> -	err = lynxfb_kick_out_firmware_fb(pdev);
> +	err = aperture_remove_conflicting_pci_devices(pdev, "sm750_fb1");
>   	if (err)
>   		return err;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[Intel-gfx] [PATCH 07/11] fbdev/radeon: use pci aperture helpers

[Daniel Vetter]

It's not exactly the same since the open coded version doesn't set
primary correctly. But that's a bugfix, so shouldn't hurt really.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/video/fbdev/aty/radeon_base.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
index 657064227de8..972c4bbedfa3 100644
--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -2238,14 +2238,6 @@ static const struct bin_attribute edid2_attr = {
 	.read	= radeon_show_edid2,
 };
 
-static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
-{
-	resource_size_t base = pci_resource_start(pdev, 0);
-	resource_size_t size = pci_resource_len(pdev, 0);
-
-	return aperture_remove_conflicting_devices(base, size, false, KBUILD_MODNAME);
-}
-
 static int radeonfb_pci_register(struct pci_dev *pdev,
 				 const struct pci_device_id *ent)
 {
@@ -2296,7 +2288,7 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
 	rinfo->fb_base_phys = pci_resource_start (pdev, 0);
 	rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
 
-	ret = radeon_kick_out_firmware_fb(pdev);
+	ret = aperture_remove_conflicting_pci_devices(pdev, KBUILD_MODNAME);
 	if (ret)
 		goto err_release_fb;
 
-- 
2.39.0

[Intel-gfx] [PATCH 08/11] fbdev/hyperv: use pci aperture helpers

[Daniel Vetter]

Again this just gets setting the primary parameter right, which might
help in some case (but then I guess the hyperv display isn't vga
compatible, I have no idea). It's more consistent for sure.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: linux-hyperv@vger.kernel.org
---
 drivers/video/fbdev/hyperv_fb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index fdbf02b42723..1067a64bbdf3 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1076,9 +1076,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 	info->screen_size = dio_fb_size;
 
 getmem_done:
-	aperture_remove_conflicting_devices(info->apertures->ranges[0].base,
-					    info->apertures->ranges[0].size,
-					    false, KBUILD_MODNAME);
+	aperture_remove_conflicting_pci_devices(pdev, KBUILD_MODNAME);
 
 	if (gen2vm) {
 		/* framebuffer is reallocated, clear screen_info to avoid misuse from kexec */
-- 
2.39.0

[Intel-gfx] [PATCH 09/11] video/aperture: Move vga handling to pci function

[Daniel Vetter]

A few reasons for this:

- It's really the only one where this matters. I tried looking around,
  and I didn't find any non-pci vga-compatible controllers for x86
  (since that's the only platform where we had this until a few
  patches ago), where a driver participating in the aperture claim
  dance would interfere.

- I also don't expect that any future bus anytime soon will
  not just look like pci towards the OS, that's been the case for like
  25+ years by now for practically everything (even non non-x86).

- Also it's a bit funny if we have one part of the vga removal in the
  pci function, and the other in the generic one.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/video/aperture.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 6f351a58f6c6..03f8a5e95238 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -298,14 +298,6 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si
 
 	aperture_detach_devices(base, size);
 
-	/*
-	 * If this is the primary adapter, there could be a VGA device
-	 * that consumes the VGA framebuffer I/O range. Remove this device
-	 * as well.
-	 */
-	if (primary)
-		aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
-
 	return 0;
 }
 EXPORT_SYMBOL(aperture_remove_conflicting_devices);
@@ -344,6 +336,13 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
 	if (!primary)
 		return 0;
 
+	/*
+	 * If this is the primary adapter, there could be a VGA device
+	 * that consumes the VGA framebuffer I/O range. Remove this device
+	 * as well.
+	 */
+	aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
+
 	/*
 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
 	 * otherwise the vga fbdev driver falls over.
-- 
2.39.0

[Intel-gfx] [PATCH 10/11] video/aperture: Drop primary argument

[Daniel Vetter]

With the preceeding patches it's become defunct. Also I'm about to add
a different boolean argument, so it's better to keep the confusion
down to the absolute minimum.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
---
 drivers/gpu/drm/drm_aperture.c | 2 +-
 drivers/video/aperture.c       | 7 +++----
 include/linux/aperture.h       | 9 ++++-----
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
index 697cffbfd603..5729f3bb4398 100644
--- a/drivers/gpu/drm/drm_aperture.c
+++ b/drivers/gpu/drm/drm_aperture.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
 int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
 						 const struct drm_driver *req_driver)
 {
-	return aperture_remove_conflicting_devices(base, size, false, req_driver->name);
+	return aperture_remove_conflicting_devices(base, size, req_driver->name);
 }
 EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
 
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 03f8a5e95238..ba565515480d 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -43,7 +43,7 @@
  *		base = mem->start;
  *		size = resource_size(mem);
  *
- *		ret = aperture_remove_conflicting_devices(base, size, false, "example");
+ *		ret = aperture_remove_conflicting_devices(base, size, "example");
  *		if (ret)
  *			return ret;
  *
@@ -274,7 +274,6 @@ static void aperture_detach_devices(resource_size_t base, resource_size_t size)
  * aperture_remove_conflicting_devices - remove devices in the given range
  * @base: the aperture's base address in physical memory
  * @size: aperture size in bytes
- * @primary: also kick vga16fb if present; only relevant for VGA devices
  * @name: a descriptive name of the requesting driver
  *
  * This function removes devices that own apertures within @base and @size.
@@ -283,7 +282,7 @@ static void aperture_detach_devices(resource_size_t base, resource_size_t size)
  * 0 on success, or a negative errno code otherwise
  */
 int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
-					bool primary, const char *name)
+					const char *name)
 {
 	/*
 	 * If a driver asked to unregister a platform device registered by
@@ -328,7 +327,7 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
 
 		base = pci_resource_start(pdev, bar);
 		size = pci_resource_len(pdev, bar);
-		ret = aperture_remove_conflicting_devices(base, size, primary, name);
+		ret = aperture_remove_conflicting_devices(base, size, name);
 		if (ret)
 			return ret;
 	}
diff --git a/include/linux/aperture.h b/include/linux/aperture.h
index 442f15a57cad..7248727753be 100644
--- a/include/linux/aperture.h
+++ b/include/linux/aperture.h
@@ -14,7 +14,7 @@ int devm_aperture_acquire_for_platform_device(struct platform_device *pdev,
 					      resource_size_t size);
 
 int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
-					bool primary, const char *name);
+					const char *name);
 
 int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name);
 #else
@@ -26,7 +26,7 @@ static inline int devm_aperture_acquire_for_platform_device(struct platform_devi
 }
 
 static inline int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
-						      bool primary, const char *name)
+						      const char *name)
 {
 	return 0;
 }
@@ -39,7 +39,6 @@ static inline int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev,
 
 /**
  * aperture_remove_all_conflicting_devices - remove all existing framebuffers
- * @primary: also kick vga16fb if present; only relevant for VGA devices
  * @name: a descriptive name of the requesting driver
  *
  * This function removes all graphics device drivers. Use this function on systems
@@ -48,9 +47,9 @@ static inline int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev,
  * Returns:
  * 0 on success, or a negative errno code otherwise
  */
-static inline int aperture_remove_all_conflicting_devices(bool primary, const char *name)
+static inline int aperture_remove_all_conflicting_devices(const char *name)
 {
-	return aperture_remove_conflicting_devices(0, (resource_size_t)-1, primary, name);
+	return aperture_remove_conflicting_devices(0, (resource_size_t)-1, name);
 }
 
 #endif
-- 
2.39.0

Re: [Intel-gfx] [PATCH 10/11] video/aperture: Drop primary argument

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5344 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> With the preceeding patches it's become defunct. Also I'm about to add
> a different boolean argument, so it's better to keep the confusion
> down to the absolute minimum.

OK, maybe my earlier comments about the use of 'primary' in some 
function calls were not correct. Ignore them then.

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> ---
>   drivers/gpu/drm/drm_aperture.c | 2 +-
>   drivers/video/aperture.c       | 7 +++----
>   include/linux/aperture.h       | 9 ++++-----
>   3 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c
> index 697cffbfd603..5729f3bb4398 100644
> --- a/drivers/gpu/drm/drm_aperture.c
> +++ b/drivers/gpu/drm/drm_aperture.c
> @@ -168,7 +168,7 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware);
>   int drm_aperture_remove_conflicting_framebuffers(resource_size_t base, resource_size_t size,
>   						 const struct drm_driver *req_driver)
>   {
> -	return aperture_remove_conflicting_devices(base, size, false, req_driver->name);
> +	return aperture_remove_conflicting_devices(base, size, req_driver->name);
>   }
>   EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers);
>   
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index 03f8a5e95238..ba565515480d 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -43,7 +43,7 @@
>    *		base = mem->start;
>    *		size = resource_size(mem);
>    *
> - *		ret = aperture_remove_conflicting_devices(base, size, false, "example");
> + *		ret = aperture_remove_conflicting_devices(base, size, "example");
>    *		if (ret)
>    *			return ret;
>    *
> @@ -274,7 +274,6 @@ static void aperture_detach_devices(resource_size_t base, resource_size_t size)
>    * aperture_remove_conflicting_devices - remove devices in the given range
>    * @base: the aperture's base address in physical memory
>    * @size: aperture size in bytes
> - * @primary: also kick vga16fb if present; only relevant for VGA devices
>    * @name: a descriptive name of the requesting driver
>    *
>    * This function removes devices that own apertures within @base and @size.
> @@ -283,7 +282,7 @@ static void aperture_detach_devices(resource_size_t base, resource_size_t size)
>    * 0 on success, or a negative errno code otherwise
>    */
>   int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
> -					bool primary, const char *name)
> +					const char *name)
>   {
>   	/*
>   	 * If a driver asked to unregister a platform device registered by
> @@ -328,7 +327,7 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>   
>   		base = pci_resource_start(pdev, bar);
>   		size = pci_resource_len(pdev, bar);
> -		ret = aperture_remove_conflicting_devices(base, size, primary, name);
> +		ret = aperture_remove_conflicting_devices(base, size, name);
>   		if (ret)
>   			return ret;
>   	}
> diff --git a/include/linux/aperture.h b/include/linux/aperture.h
> index 442f15a57cad..7248727753be 100644
> --- a/include/linux/aperture.h
> +++ b/include/linux/aperture.h
> @@ -14,7 +14,7 @@ int devm_aperture_acquire_for_platform_device(struct platform_device *pdev,
>   					      resource_size_t size);
>   
>   int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
> -					bool primary, const char *name);
> +					const char *name);
>   
>   int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *name);
>   #else
> @@ -26,7 +26,7 @@ static inline int devm_aperture_acquire_for_platform_device(struct platform_devi
>   }
>   
>   static inline int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t size,
> -						      bool primary, const char *name)
> +						      const char *name)
>   {
>   	return 0;
>   }
> @@ -39,7 +39,6 @@ static inline int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev,
>   
>   /**
>    * aperture_remove_all_conflicting_devices - remove all existing framebuffers
> - * @primary: also kick vga16fb if present; only relevant for VGA devices
>    * @name: a descriptive name of the requesting driver
>    *
>    * This function removes all graphics device drivers. Use this function on systems
> @@ -48,9 +47,9 @@ static inline int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev,
>    * Returns:
>    * 0 on success, or a negative errno code otherwise
>    */
> -static inline int aperture_remove_all_conflicting_devices(bool primary, const char *name)
> +static inline int aperture_remove_all_conflicting_devices(const char *name)
>   {
> -	return aperture_remove_conflicting_devices(0, (resource_size_t)-1, primary, name);
> +	return aperture_remove_conflicting_devices(0, (resource_size_t)-1, name);
>   }
>   
>   #endif

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Daniel Vetter]

This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
sysfb device registration when removing conflicting FBs"), where we
remove the sysfb when loading a driver for an unrelated pci device,
resulting in the user loosing their efifb console or similar.

Note that in practice this only is a problem with the nvidia blob,
because that's the only gpu driver people might install which does not
come with an fbdev driver of it's own. For everyone else the real gpu
driver will restor a working console.

Also note that in the referenced bug there's confusion that this same
bug also happens on amdgpu. But that was just another amdgpu specific
regression, which just happened to happen at roughly the same time and
with the same user-observable symptons. That bug is fixed now, see
https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15

For the above reasons the cc: stable is just notionally, this patch
will need a backport and that's up to nvidia if they care enough.

References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Aaron Plattner <aplattner@nvidia.com>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Helge Deller <deller@gmx.de>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport)
---
 drivers/video/aperture.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index ba565515480d..a1821d369bb1 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
 
 	primary = pdev == vga_default_device();
 
+	if (primary)
+		sysfb_disable();
+
 	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
 		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
 			continue;
 
 		base = pci_resource_start(pdev, bar);
 		size = pci_resource_len(pdev, bar);
-		ret = aperture_remove_conflicting_devices(base, size, name);
-		if (ret)
-			return ret;
+		aperture_detach_devices(base, size);
 	}
 
 	if (!primary)
-- 
2.39.0

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3084 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
> sysfb device registration when removing conflicting FBs"), where we
> remove the sysfb when loading a driver for an unrelated pci device,
> resulting in the user loosing their efifb console or similar.
> 
> Note that in practice this only is a problem with the nvidia blob,
> because that's the only gpu driver people might install which does not
> come with an fbdev driver of it's own. For everyone else the real gpu
> driver will restor a working console.
> 
> Also note that in the referenced bug there's confusion that this same
> bug also happens on amdgpu. But that was just another amdgpu specific
> regression, which just happened to happen at roughly the same time and
> with the same user-observable symptons. That bug is fixed now, see
> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
> 
> For the above reasons the cc: stable is just notionally, this patch
> will need a backport and that's up to nvidia if they care enough.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Aaron Plattner <aplattner@nvidia.com>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport)
> ---
>   drivers/video/aperture.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index ba565515480d..a1821d369bb1 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>   
>   	primary = pdev == vga_default_device();
>   
> +	if (primary)
> +		sysfb_disable();
> +

There's another sysfb_disable() in aperture_remove_conflicting_devices() 
without the branch but with a long comment.  I find this slightly confusing.

I'd rather add a branched sysfb_disable() plus the comment  to 
aperture_detach_devices(). And then add a 'primary' parameter to 
aperture_detach_devices(). In aperture_remove_conflicting_devices() the 
parameter would be unconditionally true.

Best regards
Thomas

>   	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
>   		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>   			continue;
>   
>   		base = pci_resource_start(pdev, bar);
>   		size = pci_resource_len(pdev, bar);
> -		ret = aperture_remove_conflicting_devices(base, size, name);
> -		if (ret)
> -			return ret;
> +		aperture_detach_devices(base, size);
>   	}
>   
>   	if (!primary)

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Daniel Vetter]

On Wed, Jan 11, 2023 at 05:20:00PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
> > sysfb device registration when removing conflicting FBs"), where we
> > remove the sysfb when loading a driver for an unrelated pci device,
> > resulting in the user loosing their efifb console or similar.
> > 
> > Note that in practice this only is a problem with the nvidia blob,
> > because that's the only gpu driver people might install which does not
> > come with an fbdev driver of it's own. For everyone else the real gpu
> > driver will restor a working console.
> > 
> > Also note that in the referenced bug there's confusion that this same
> > bug also happens on amdgpu. But that was just another amdgpu specific
> > regression, which just happened to happen at roughly the same time and
> > with the same user-observable symptons. That bug is fixed now, see
> > https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
> > 
> > For the above reasons the cc: stable is just notionally, this patch
> > will need a backport and that's up to nvidia if they care enough.
> > 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Aaron Plattner <aplattner@nvidia.com>
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Helge Deller <deller@gmx.de>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport)
> > ---
> >   drivers/video/aperture.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> > index ba565515480d..a1821d369bb1 100644
> > --- a/drivers/video/aperture.c
> > +++ b/drivers/video/aperture.c
> > @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
> >   	primary = pdev == vga_default_device();
> > +	if (primary)
> > +		sysfb_disable();
> > +
> 
> There's another sysfb_disable() in aperture_remove_conflicting_devices()
> without the branch but with a long comment.  I find this slightly confusing.
> 
> I'd rather add a branched sysfb_disable() plus the comment  to
> aperture_detach_devices(). And then add a 'primary' parameter to
> aperture_detach_devices(). In aperture_remove_conflicting_devices() the
> parameter would be unconditionally true.

Yeah I was on the fence, but should be easy to redo with all the prep work
out of the way. It does mean we call sysfb_disable once for every bar, but
that shouldn't matter in any reasonable case :-)
-Daniel

> 
> Best regards
> Thomas
> 
> >   	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
> >   		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
> >   			continue;
> >   		base = pci_resource_start(pdev, bar);
> >   		size = pci_resource_len(pdev, bar);
> > -		ret = aperture_remove_conflicting_devices(base, size, name);
> > -		if (ret)
> > -			return ret;
> > +		aperture_detach_devices(base, size);
> >   	}
> >   	if (!primary)
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4113 bytes --]

Hi

Am 11.01.23 um 17:37 schrieb Daniel Vetter:
> On Wed, Jan 11, 2023 at 05:20:00PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>>> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
>>> sysfb device registration when removing conflicting FBs"), where we
>>> remove the sysfb when loading a driver for an unrelated pci device,
>>> resulting in the user loosing their efifb console or similar.
>>>
>>> Note that in practice this only is a problem with the nvidia blob,
>>> because that's the only gpu driver people might install which does not
>>> come with an fbdev driver of it's own. For everyone else the real gpu
>>> driver will restor a working console.
>>>
>>> Also note that in the referenced bug there's confusion that this same
>>> bug also happens on amdgpu. But that was just another amdgpu specific
>>> regression, which just happened to happen at roughly the same time and
>>> with the same user-observable symptons. That bug is fixed now, see
>>> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
>>>
>>> For the above reasons the cc: stable is just notionally, this patch
>>> will need a backport and that's up to nvidia if they care enough.
>>>
>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Aaron Plattner <aplattner@nvidia.com>
>>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Helge Deller <deller@gmx.de>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport)
>>> ---
>>>    drivers/video/aperture.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
>>> index ba565515480d..a1821d369bb1 100644
>>> --- a/drivers/video/aperture.c
>>> +++ b/drivers/video/aperture.c
>>> @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>>>    	primary = pdev == vga_default_device();
>>> +	if (primary)
>>> +		sysfb_disable();
>>> +
>>
>> There's another sysfb_disable() in aperture_remove_conflicting_devices()
>> without the branch but with a long comment.  I find this slightly confusing.
>>
>> I'd rather add a branched sysfb_disable() plus the comment  to
>> aperture_detach_devices(). And then add a 'primary' parameter to
>> aperture_detach_devices(). In aperture_remove_conflicting_devices() the
>> parameter would be unconditionally true.
> 
> Yeah I was on the fence, but should be easy to redo with all the prep work
> out of the way. It does mean we call sysfb_disable once for every bar, but
> that shouldn't matter in any reasonable case :-)

Or leave it as is. It's not so important. The idea of the current design 
was that aperture_remove_conflicting_device() would be the general 
implementation and aperture_remove_conflicting_pci_device() would be a 
helper that only detects the correct PCI BAR. That never really worked 
in practice.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>    	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
>>>    		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>>>    			continue;
>>>    		base = pci_resource_start(pdev, bar);
>>>    		size = pci_resource_len(pdev, bar);
>>> -		ret = aperture_remove_conflicting_devices(base, size, name);
>>> -		if (ret)
>>> -			return ret;
>>> +		aperture_detach_devices(base, size);
>>>    	}
>>>    	if (!primary)
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Javier Martinez Canillas]

On 1/11/23 17:20, Thomas Zimmermann wrote:

[...]

>>
>> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
>> index ba565515480d..a1821d369bb1 100644
>> --- a/drivers/video/aperture.c
>> +++ b/drivers/video/aperture.c
>> @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>>   
>>   	primary = pdev == vga_default_device();
>>   
>> +	if (primary)
>> +		sysfb_disable();
>> +
> 
> There's another sysfb_disable() in aperture_remove_conflicting_devices() 
> without the branch but with a long comment.  I find this slightly confusing.
> 
> I'd rather add a branched sysfb_disable() plus the comment  to 
> aperture_detach_devices(). And then add a 'primary' parameter to 
> aperture_detach_devices(). In aperture_remove_conflicting_devices() the 
> parameter would be unconditionally true.
>

Or just remove that long comment since there's already kernel-doc for the
sysfb_disable() function definition.

This feels to me that any approach to parameterize this will lead to code
that is harder to read.

Since is just a single function call, I would just duplicate like $subject
does to be honest.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Javier Martinez Canillas]

Hello Daniel,

On 1/11/23 16:41, Daniel Vetter wrote:
> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
> sysfb device registration when removing conflicting FBs"), where we
> remove the sysfb when loading a driver for an unrelated pci device,
> resulting in the user loosing their efifb console or similar.
> 
> Note that in practice this only is a problem with the nvidia blob,
> because that's the only gpu driver people might install which does not
> come with an fbdev driver of it's own. For everyone else the real gpu
> driver will restor a working console.

restore

> 
> Also note that in the referenced bug there's confusion that this same
> bug also happens on amdgpu. But that was just another amdgpu specific
> regression, which just happened to happen at roughly the same time and
> with the same user-observable symptons. That bug is fixed now, see

symptoms

> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
> 
> For the above reasons the cc: stable is just notionally, this patch
> will need a backport and that's up to nvidia if they care enough.
> 

Maybe adding a Fixes: ee7a69aa38d8 tag here too ?

> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Aaron Plattner <aplattner@nvidia.com>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport)
> ---
>  drivers/video/aperture.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
> index ba565515480d..a1821d369bb1 100644
> --- a/drivers/video/aperture.c
> +++ b/drivers/video/aperture.c
> @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>  
>  	primary = pdev == vga_default_device();
>  
> +	if (primary)
> +		sysfb_disable();
> +
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
>  		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>  			continue;
>  
>  		base = pci_resource_start(pdev, bar);
>  		size = pci_resource_len(pdev, bar);
> -		ret = aperture_remove_conflicting_devices(base, size, name);
> -		if (ret)
> -			return ret;
> +		aperture_detach_devices(base, size);

Maybe mention in the commit message that you are doing this change, something like:

"Instead of calling aperture_remove_conflicting_devices() to remove the conflicting
devices, just call to aperture_detach_devices() to detach the device that matches
the same PCI BAR / aperture range. Since the former is just a wrapper of the latter
plus a sysfb_disable() call, and now that's done in this function but only for the
primary devices"

Patch looks good to me:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Aaron Plattner]

On 1/11/23 8:58 AM, Javier Martinez Canillas wrote:
> Hello Daniel,
> 
> On 1/11/23 16:41, Daniel Vetter wrote:
>> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
>> sysfb device registration when removing conflicting FBs"), where we
>> remove the sysfb when loading a driver for an unrelated pci device,
>> resulting in the user loosing their efifb console or similar.
>>
>> Note that in practice this only is a problem with the nvidia blob,
>> because that's the only gpu driver people might install which does not
>> come with an fbdev driver of it's own. For everyone else the real gpu
>> driver will restor a working console.
> 
> restore
> 
>>
>> Also note that in the referenced bug there's confusion that this same
>> bug also happens on amdgpu. But that was just another amdgpu specific
>> regression, which just happened to happen at roughly the same time and
>> with the same user-observable symptons. That bug is fixed now, see
> 
> symptoms
> 
>> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
>>
>> For the above reasons the cc: stable is just notionally, this patch
>> will need a backport and that's up to nvidia if they care enough.
>>
> 
> Maybe adding a Fixes: ee7a69aa38d8 tag here too ?
> 
>> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Aaron Plattner <aplattner@nvidia.com>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Helge Deller <deller@gmx.de>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport)
>> ---
>>   drivers/video/aperture.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
>> index ba565515480d..a1821d369bb1 100644
>> --- a/drivers/video/aperture.c
>> +++ b/drivers/video/aperture.c
>> @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na
>>   
>>   	primary = pdev == vga_default_device();
>>   
>> +	if (primary)
>> +		sysfb_disable();
>> +
>>   	for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
>>   		if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>>   			continue;
>>   
>>   		base = pci_resource_start(pdev, bar);
>>   		size = pci_resource_len(pdev, bar);
>> -		ret = aperture_remove_conflicting_devices(base, size, name);
>> -		if (ret)
>> -			return ret;
>> +		aperture_detach_devices(base, size);
> 
> Maybe mention in the commit message that you are doing this change, something like:
> 
> "Instead of calling aperture_remove_conflicting_devices() to remove the conflicting
> devices, just call to aperture_detach_devices() to detach the device that matches
> the same PCI BAR / aperture range. Since the former is just a wrapper of the latter
> plus a sysfb_disable() call, and now that's done in this function but only for the
> primary devices"
> 
> Patch looks good to me:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks Daniel and Javier!

I wasn't able to reproduce the original problem on my hybrid laptop 
since it refuses to boot with the console on an external display, but I 
was able to reproduce it by switching the configuration around: booting 
with i915.modeset=0 and with an experimental version of nvidia-drm that 
registers a framebuffer console. I verified that loading nvidia-drm 
breaks the efi-firmware framebuffer on Intel on Arch's 
linux-6.1.4-arch1-1 kernel and that applying this patch series fixes it. So

Tested-by: Aaron Plattner <aplattner@nvidia.com>

FWIW, the bug ought to be reproducible with i915.modeset=0 + any other 
drm driver that registers a framebuffer.

-- Aaron

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4781 bytes --]

Hi

Am 11.01.23 um 20:21 schrieb Aaron Plattner:
> On 1/11/23 8:58 AM, Javier Martinez Canillas wrote:
>> Hello Daniel,
>>
>> On 1/11/23 16:41, Daniel Vetter wrote:
>>> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
>>> sysfb device registration when removing conflicting FBs"), where we
>>> remove the sysfb when loading a driver for an unrelated pci device,
>>> resulting in the user loosing their efifb console or similar.
>>>
>>> Note that in practice this only is a problem with the nvidia blob,
>>> because that's the only gpu driver people might install which does not
>>> come with an fbdev driver of it's own. For everyone else the real gpu
>>> driver will restor a working console.
>>
>> restore
>>
>>>
>>> Also note that in the referenced bug there's confusion that this same
>>> bug also happens on amdgpu. But that was just another amdgpu specific
>>> regression, which just happened to happen at roughly the same time and
>>> with the same user-observable symptons. That bug is fixed now, see
>>
>> symptoms
>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
>>>
>>> For the above reasons the cc: stable is just notionally, this patch
>>> will need a backport and that's up to nvidia if they care enough.
>>>
>>
>> Maybe adding a Fixes: ee7a69aa38d8 tag here too ?
>>
>>> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Aaron Plattner <aplattner@nvidia.com>
>>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: Helge Deller <deller@gmx.de>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the 
>>> backport)
>>> ---
>>>   drivers/video/aperture.c | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
>>> index ba565515480d..a1821d369bb1 100644
>>> --- a/drivers/video/aperture.c
>>> +++ b/drivers/video/aperture.c
>>> @@ -321,15 +321,16 @@ int 
>>> aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const 
>>> char *na
>>>       primary = pdev == vga_default_device();
>>> +    if (primary)
>>> +        sysfb_disable();
>>> +
>>>       for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
>>>           if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>>>               continue;
>>>           base = pci_resource_start(pdev, bar);
>>>           size = pci_resource_len(pdev, bar);
>>> -        ret = aperture_remove_conflicting_devices(base, size, name);
>>> -        if (ret)
>>> -            return ret;
>>> +        aperture_detach_devices(base, size);
>>
>> Maybe mention in the commit message that you are doing this change, 
>> something like:
>>
>> "Instead of calling aperture_remove_conflicting_devices() to remove 
>> the conflicting
>> devices, just call to aperture_detach_devices() to detach the device 
>> that matches
>> the same PCI BAR / aperture range. Since the former is just a wrapper 
>> of the latter
>> plus a sysfb_disable() call, and now that's done in this function but 
>> only for the
>> primary devices"
>>
>> Patch looks good to me:
>>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Thanks Daniel and Javier!
> 
> I wasn't able to reproduce the original problem on my hybrid laptop 
> since it refuses to boot with the console on an external display, but I 
> was able to reproduce it by switching the configuration around: booting 
> with i915.modeset=0 and with an experimental version of nvidia-drm that 
> registers a framebuffer console. I verified that loading nvidia-drm 

Thank you for testing.

One thing I'd like to note is that using DRM's fbdev emulation is the 
correct way to support a console. Nvidia-drm's current approach of 
utilizing efifb is fragile and requires workarounds from distributions 
(at least here at SUSE). Steps towards fbdev emulation are much appreciated.

Best regards
Thomas

> breaks the efi-firmware framebuffer on Intel on Arch's 
> linux-6.1.4-arch1-1 kernel and that applying this patch series fixes it. So
> 
> Tested-by: Aaron Plattner <aplattner@nvidia.com>
> 
> FWIW, the bug ought to be reproducible with i915.modeset=0 + any other 
> drm driver that registers a framebuffer.
> 
> -- Aaron

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device

[Javier Martinez Canillas]

On 1/12/23 08:55, Thomas Zimmermann wrote:

[...]

>> Thanks Daniel and Javier!
>>
>> I wasn't able to reproduce the original problem on my hybrid laptop 
>> since it refuses to boot with the console on an external display, but I 
>> was able to reproduce it by switching the configuration around: booting 
>> with i915.modeset=0 and with an experimental version of nvidia-drm that 
>> registers a framebuffer console. I verified that loading nvidia-drm 
> 
> Thank you for testing.
> 
> One thing I'd like to note is that using DRM's fbdev emulation is the 
> correct way to support a console. Nvidia-drm's current approach of 
> utilizing efifb is fragile and requires workarounds from distributions 
> (at least here at SUSE). Steps towards fbdev emulation are much appreciated.
>
 
I was meaning to mention the same. Fedora also is carrying a workaround just
for the Nvidia proprietary driver since all other drivers provide a emulated
fbdev device.

So getting this finally fixed will be indeed highly appreciated.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1983 bytes --]

Hi

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It's just open coded and matches.
> 
> Note that Thomas said that his version apparently failed for some
> reason, but hey maybe we should try again.

I'll give this patch a test tomorrow.

Best regards
Thomas

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> ---
>   drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 420fc75c240e..3ac24a780f50 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
>   
>   MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>   
> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> -{
> -	bool primary = false;
> -	resource_size_t base, size;
> -
> -	base = pci_resource_start(pdev, 0);
> -	size = pci_resource_len(pdev, 0);
> -#ifdef CONFIG_X86
> -	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> -
> -	return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> -}
> -
>   static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   {
>   	struct ast_private *ast;
>   	struct drm_device *dev;
>   	int ret;
>   
> -	ret = ast_remove_conflicting_framebuffers(pdev);
> +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
>   	if (ret)
>   		return ret;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Daniel Vetter]

On Wed, Jan 11, 2023 at 04:48:39PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> > It's just open coded and matches.
> > 
> > Note that Thomas said that his version apparently failed for some
> > reason, but hey maybe we should try again.
> 
> I'll give this patch a test tomorrow.

Thanks a lot!
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > Cc: Helge Deller <deller@gmx.de>
> > Cc: linux-fbdev@vger.kernel.org
> > ---
> >   drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
> >   1 file changed, 1 insertion(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index 420fc75c240e..3ac24a780f50 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
> >   MODULE_DEVICE_TABLE(pci, ast_pciidlist);
> > -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> > -{
> > -	bool primary = false;
> > -	resource_size_t base, size;
> > -
> > -	base = pci_resource_start(pdev, 0);
> > -	size = pci_resource_len(pdev, 0);
> > -#ifdef CONFIG_X86
> > -	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> > -#endif
> > -
> > -	return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> > -}
> > -
> >   static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >   {
> >   	struct ast_private *ast;
> >   	struct drm_device *dev;
> >   	int ret;
> > -	ret = ast_remove_conflicting_framebuffers(pdev);
> > +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
> >   	if (ret)
> >   		return ret;
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Patchwork]

== Series Details ==

Series: series starting with [01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers
URL   : https://patchwork.freedesktop.org/series/112678/
State : warning

== Summary ==

Error: dim checkpatch failed
8179e533ce2d drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers
-:50: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 28 lines checked
b6c78450ea70 drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
-:25: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 8 lines checked
6ededaf10dce drm/aperture: Remove primary argument
-:8: WARNING:TYPO_SPELLING: 'preceeding' may be misspelled - perhaps 'preceding'?
#8: 
with the preceeding two patches those are all using the pci version of
         ^^^^^^^^^^

-:239: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 2 warnings, 0 checks, 141 lines checked
f31bb3cc89e7 video/aperture: use generic code to figure out the vga default device
-:10: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to drivers/pci")'
#10: 
specific hack. See also 1d38fe6ee6a8 ("PCI/VGA: Move vgaarb to

-:44: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 1 errors, 1 warnings, 0 checks, 15 lines checked
53af1ee14d29 video/aperture: Only kick vgacon when the pdev is decoding vga
-:32: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 9 lines checked
4b35c67ecf18 staging/lynxfb: Use pci aperture helper
-:47: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 28 lines checked
242c46f69022 fbdev/radeon: use pci aperture helpers
-:40: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 22 lines checked
e532a4323d08 fbdev/hyperv: use pci aperture helpers
-:31: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 10 lines checked
bb9ea9da0922 video/aperture: Move vga handling to pci function
-:59: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 1 warnings, 0 checks, 27 lines checked
3630a240e94e video/aperture: Drop primary argument
-:6: WARNING:TYPO_SPELLING: 'preceeding' may be misspelled - perhaps 'preceding'?
#6: 
With the preceeding patches it's become defunct. Also I'm about to add
         ^^^^^^^^^^

-:109: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 0 errors, 2 warnings, 0 checks, 73 lines checked
1464ac9bdb27 video/aperture: Only remove sysfb on the default vga pci device
-:7: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit ee7a69aa38d8 ("fbdev: Disable sysfb device registration when removing conflicting FBs")'
#7: 
This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable

-:10: WARNING:TYPO_SPELLING: 'loosing' may be misspelled - perhaps 'losing'?
#10: 
resulting in the user loosing their efifb console or similar.
                      ^^^^^^^

-:59: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter <daniel.vetter@ffwll.ch>' != 'Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>'

total: 1 errors, 2 warnings, 0 checks, 19 lines checked

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7408 bytes --]

== Series Details ==

Series: series starting with [01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers
URL   : https://patchwork.freedesktop.org/series/112678/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12573 -> Patchwork_112678v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_112678v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_112678v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/index.html

Participating hosts (34 -> 35)
------------------------------

  Additional (2): fi-kbl-soraka fi-bsw-kefka 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_112678v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-bsw-nick:        [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/fi-bsw-nick/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-bsw-nick/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@guc:
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-kbl-soraka/igt@i915_selftest@live@guc.html

  
Known issues
------------

  Here are the changes found in Patchwork_112678v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#4613]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][6] -> [DMESG-FAIL][7] ([i915#5334])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][8] ([i915#1886])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][9] ([fdo#109271]) +15 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - fi-rkl-guc:         NOTRUN -> [SKIP][10] ([i915#7828])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-rkl-guc/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-n3050:       [PASS][11] -> [FAIL][12] ([i915#6298])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-bsw-kefka:       NOTRUN -> [SKIP][13] ([fdo#109271]) +26 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-bsw-kefka/igt@prime_vgem@basic-fence-flip.html

  * igt@runner@aborted:
    - fi-bsw-nick:        NOTRUN -> [FAIL][14] ([fdo#109271] / [i915#4312])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-bsw-nick/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@fbdev@eof:
    - {bat-atsm-1}:       [SKIP][15] ([i915#2582]) -> [PASS][16] +4 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/bat-atsm-1/igt@fbdev@eof.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/bat-atsm-1/igt@fbdev@eof.html

  * igt@i915_selftest@live@gt_lrc:
    - fi-rkl-guc:         [INCOMPLETE][17] ([i915#4983]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html

  * igt@i915_selftest@live@migrate:
    - {bat-atsm-1}:       [DMESG-FAIL][19] ([i915#7699]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/bat-atsm-1/igt@i915_selftest@live@migrate.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/bat-atsm-1/igt@i915_selftest@live@migrate.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828


Build changes
-------------

  * Linux: CI_DRM_12573 -> Patchwork_112678v1

  CI-20190529: 20190529
  CI_DRM_12573: 05a39ecbd04e67fb44442617819fc138eca0ac61 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7116: 79eb8984acd309108be713a8831e60667db67e21 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112678v1: 05a39ecbd04e67fb44442617819fc138eca0ac61 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

cb06aa7312c4 video/aperture: Only remove sysfb on the default vga pci device
e9aad9aa55b6 video/aperture: Drop primary argument
cdb4dfc78b85 video/aperture: Move vga handling to pci function
c321e71fd956 fbdev/hyperv: use pci aperture helpers
7985c171f29e fbdev/radeon: use pci aperture helpers
a9a6c5c7a737 staging/lynxfb: Use pci aperture helper
927128b2acbe video/aperture: Only kick vgacon when the pdev is decoding vga
d80cfac7c072 video/aperture: use generic code to figure out the vga default device
c35403b915ac drm/aperture: Remove primary argument
178222cca8fe drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
cffab199b6ca drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112678v1/index.html

[-- Attachment #2: Type: text/html, Size: 8655 bytes --]

Re: [Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2089 bytes --]



Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It's just open coded and matches.
> 
> Note that Thomas said that his version apparently failed for some
> reason, but hey maybe we should try again.

It apparently worked this time. Tested on an AST2100 chip.

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org

Tested-by: Thomas Zimmmermann <tzimmermann@suse.de>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 420fc75c240e..3ac24a780f50 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
>   
>   MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>   
> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> -{
> -	bool primary = false;
> -	resource_size_t base, size;
> -
> -	base = pci_resource_start(pdev, 0);
> -	size = pci_resource_len(pdev, 0);
> -#ifdef CONFIG_X86
> -	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> -
> -	return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> -}
> -
>   static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   {
>   	struct ast_private *ast;
>   	struct drm_device *dev;
>   	int ret;
>   
> -	ret = ast_remove_conflicting_framebuffers(pdev);
> +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
>   	if (ret)
>   		return ret;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2055 bytes --]

Hi,

FYI I have merged patches 1, 6 and 7 of this patchset. They look fine 
and are worthwhile fixes on their own.

Best regards
Thomas

Am 11.01.23 um 16:41 schrieb Daniel Vetter:
> It's just open coded and matches.
> 
> Note that Thomas said that his version apparently failed for some
> reason, but hey maybe we should try again.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> ---
>   drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 420fc75c240e..3ac24a780f50 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] = {
>   
>   MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>   
> -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
> -{
> -	bool primary = false;
> -	resource_size_t base, size;
> -
> -	base = pci_resource_start(pdev, 0);
> -	size = pci_resource_len(pdev, 0);
> -#ifdef CONFIG_X86
> -	primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
> -#endif
> -
> -	return drm_aperture_remove_conflicting_framebuffers(base, size, primary, &ast_driver);
> -}
> -
>   static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   {
>   	struct ast_private *ast;
>   	struct drm_device *dev;
>   	int ret;
>   
> -	ret = ast_remove_conflicting_framebuffers(pdev);
> +	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &ast_driver);
>   	if (ret)
>   		return ret;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 01/11] drm/ast: Use drm_aperture_remove_conflicting_pci_framebuffers

[Jammy Huang]

Hi Thomas,

The Intel(x86) CPUs have a separate address space for "IO", but the ARM 
architecture only has "memory", so all IO devices are accessed as if 
they were memory. Which means ARM does not support isolated IO. Here is 
a related discussion on ARM's forum.

https://community.arm.com/support-forums/f/architectures-and-processors-forum/52046/how-to-read-write-an-i-o-port-in-aarch64

Thus, we adapt MMIO only after this patch.


On 2023/4/4 下午 10:45, Thomas Zimmermann wrote:
> Hi,
>
> FYI I have merged patches 1, 6 and 7 of this patchset. They look fine 
> and are worthwhile fixes on their own.
>
> Best regards
> Thomas
>
> Am 11.01.23 um 16:41 schrieb Daniel Vetter:
>> It's just open coded and matches.
>>
>> Note that Thomas said that his version apparently failed for some
>> reason, but hey maybe we should try again.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Helge Deller <deller@gmx.de>
>> Cc: linux-fbdev@vger.kernel.org
>> ---
>>   drivers/gpu/drm/ast/ast_drv.c | 16 +---------------
>>   1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.c 
>> b/drivers/gpu/drm/ast/ast_drv.c
>> index 420fc75c240e..3ac24a780f50 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.c
>> +++ b/drivers/gpu/drm/ast/ast_drv.c
>> @@ -90,27 +90,13 @@ static const struct pci_device_id ast_pciidlist[] 
>> = {
>>     MODULE_DEVICE_TABLE(pci, ast_pciidlist);
>>   -static int ast_remove_conflicting_framebuffers(struct pci_dev *pdev)
>> -{
>> -    bool primary = false;
>> -    resource_size_t base, size;
>> -
>> -    base = pci_resource_start(pdev, 0);
>> -    size = pci_resource_len(pdev, 0);
>> -#ifdef CONFIG_X86
>> -    primary = pdev->resource[PCI_ROM_RESOURCE].flags & 
>> IORESOURCE_ROM_SHADOW;
>> -#endif
>> -
>> -    return drm_aperture_remove_conflicting_framebuffers(base, size, 
>> primary, &ast_driver);
>> -}
>> -
>>   static int ast_pci_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>   {
>>       struct ast_private *ast;
>>       struct drm_device *dev;
>>       int ret;
>>   -    ret = ast_remove_conflicting_framebuffers(pdev);
>> +    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, 
>> &ast_driver);
>>       if (ret)
>>           return ret;
>
-- 
Best Regards
Jammy