Re: [PATCH] drm/imagination: Ensure PVR_MIPS_PT_PAGE_COUNT is never zero

[Frank Binns]

On Tue, 2024-04-23 at 16:09 +, Matt Coster wrote:
> When the host page size was more than 4 times larger than the FW page
> size, this macro evaluated to zero resulting in zero-sized arrays.
> 
> Use DIV_ROUND_UP() to ensure the correct behavior.
> 
> Reported-by: 20240228012313.5934-1-ya...@kylinos.cn
> Closes: 
> https://lore.kernel.org/dri-devel/20240228012313.5934-1-ya...@kylinos.cn
> Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and 
> MMU support")
> Signed-off-by: Matt Coster 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_fw_mips.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_fw_mips.h 
> b/drivers/gpu/drm/imagination/pvr_fw_mips.h
> index 408dbe63a90c..a0c5c41c8aa2 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_mips.h
> +++ b/drivers/gpu/drm/imagination/pvr_fw_mips.h
> @@ -7,13 +7,14 @@
>  #include "pvr_rogue_mips.h"
>  
>  #include 
> +#include 
>  #include 
>  
>  /* Forward declaration from pvr_gem.h. */
>  struct pvr_gem_object;
>  
> -#define PVR_MIPS_PT_PAGE_COUNT ((ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES * 
> ROGUE_MIPSFW_PAGE_SIZE_4K) \
> - >> PAGE_SHIFT)
> +#define PVR_MIPS_PT_PAGE_COUNT 
> DIV_ROUND_UP(ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES * 
> ROGUE_MIPSFW_PAGE_SIZE_4K, PAGE_SIZE)
> +
>  /**
>   * struct pvr_fw_mips_data - MIPS-specific data
>   */
> 
> base-commit: e95752752eaf06c860811ac5ddf9badf6c1b43ca

Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU

[Frank Binns]

On Thu, 2024-03-07 at 07:31 -0600, Adam Ford wrote:
> On Thu, Mar 7, 2024 at 6:37 AM Frank Binns  wrote:
> > On Thu, 2024-03-07 at 12:26 +0000, Frank Binns wrote:
> > > On Tue, 2024-02-27 at 05:50 -0600, Adam Ford wrote:
> > > > On Tue, Feb 27, 2024 at 3:31 AM Matt Coster  
> > > > wrote:
> > > > > Hi Adam,
> > > > > 
> > > > > Thanks for these patches! I'll just reply to this one patch, but my
> > > > > comments apply to them all.
> > > > > 
> > > > > On 27/02/2024 03:45, Adam Ford wrote:
> > > > > > The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware
> > > > > > rogue_4.45.2.58_v1.fw available from Imagination.
> > > > > > 
> > > > > > When enumerated, it appears as:
> > > > > >   powervr fd00.gpu: [drm] loaded firmware 
> > > > > > powervr/rogue_4.45.2.58_v1.fw
> > > > > >   powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS)
> > > > > 
> > > > > These messages are printed after verifying the firmware blob’s 
> > > > > headers,
> > > > > *before* attempting to upload it to the device. Just because they 
> > > > > appear
> > > > > in dmesg does *not* imply the device is functional beyond the handful 
> > > > > of
> > > > > register reads in pvr_load_gpu_id().
> > > > > 
> > > > > Since Mesa does not yet have support for this GPU, there’s not a lot
> > > > > that can be done to actually test these bindings.
> > > > > 
> > > > > When we added upstream support for the first GPU (the AXE core in TI’s
> > > > > AM62), we opted to wait until userspace was sufficiently progressed to
> > > > > the point it could be used for testing. This thought process still
> > > > > applies when adding new GPUs.
> > > > > 
> > > > > Our main concern is that adding bindings for GPUs implies a level of
> > > > > support that cannot be tested. That in turn may make it challenging to
> > > > > justify UAPI changes if/when they’re needed to actually make these 
> > > > > GPUs
> > > > > functional.
> > > > 
> > > > I wrongly assumed that when the firmware was ready, there was some
> > > > preliminary functionality, but it sounds like we need to work for
> > > > Series6XT support to be added to the driver.  I only used the AXE
> > > > compatible since it appeared to the be the only one and the existing
> > > > binding document stated "model/revision is fully discoverable" which I
> > > > interpreted to mean that the AXE compatible was sufficient.
> > > 
> > > The comment is related to there being a few models/revisions of AXE 
> > > [1][2][3],
> > > which we can distinguish between by reading a register.
> > > 
> > > > > > Signed-off-by: Adam Ford 
> > > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi 
> > > > > > b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > > > > index a8a44fe5e83b..8923d9624b39 100644
> > > > > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > > > > @@ -2352,6 +2352,16 @@ gic: interrupt-controller@f101 {
> > > > > >   resets = < 408>;
> > > > > >   };
> > > > > > 
> > > > > > + gpu: gpu@fd00 {
> > > > > > + compatible = "renesas,r8a774a1-gpu", 
> > > > > > "img,img-axe";
> > > > > 
> > > > > The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible
> > > > > with one. For prior art, see [1] where we added support for the MT8173
> > > > > found in Elm Chromebooks R13 (also a Series6XT GPU).
> > > > > 
> > > > > > + reg = <0 0xfd00 0 0x2>;
> > > > > > + clocks = < CPG_MOD 112>;
> > > > > > + clock-names = "core";
> > > > > 
> > > > > Series6XT cores have three clocks (see [1] again). I don’t have a
> > > > > Renesas TRM to hand – do you know if their docs g

Re: [PATCH 1/6] dt-bindings: gpu: powervr-rogue: Add PowerVR support for some Renesas GPUs

[Frank Binns]

Hi Adam,

On Mon, 2024-02-26 at 21:45 -0600, Adam Ford wrote:
> Update the binding to add support for various Renesas SoC's with PowerVR
> Rogue GX6250 and GX6650 GPUs.  These devices only need one clock, so update
> the table to indicate such like what was done for the ti,am62-gpu.
> 
> Signed-off-by: Adam Ford 
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml 
> b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index 256e252f8087..7c75104df09f 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -14,6 +14,11 @@ properties:
>compatible:
>  items:
>- enum:
> +  - renesas,r8a774a1-gpu
> +  - renesas,r8a774e1-gpu
> +  - renesas,r8a77951-gpu
> +  - renesas,r8a77960-gpu
> +  - renesas,r8a77961-gpu
>- ti,am62-gpu
>- const: img,img-axe # IMG AXE GPU model/revision is fully discoverable

A new set of items should be added for 'img,powervr-series6xt' and the Renesas
models along the lines of [1].

Thanks
Frank

[1] 
https://gitlab.freedesktop.org/imagination/linux/-/blob/powervr-next/Documentation/devicetree/bindings/gpu/img,powervr.yaml?ref_type=heads#L16-19

>  
> @@ -51,7 +56,13 @@ allOf:
>properties:
>  compatible:
>contains:
> -const: ti,am62-gpu
> +enum:
> +  - ti,am62-gpu
> +  - renesas,r8a774a1-gpu
> +  - renesas,r8a774e1-gpu
> +  - renesas,r8a77951-gpu
> +  - renesas,r8a77960-gpu
> +  - renesas,r8a77961-gpu
>  then:
>properties:
>  clocks:

Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU

[Frank Binns]

On Thu, 2024-03-07 at 12:26 +, Frank Binns wrote:
> On Tue, 2024-02-27 at 05:50 -0600, Adam Ford wrote:
> > On Tue, Feb 27, 2024 at 3:31 AM Matt Coster  wrote:
> > > Hi Adam,
> > > 
> > > Thanks for these patches! I'll just reply to this one patch, but my
> > > comments apply to them all.
> > > 
> > > On 27/02/2024 03:45, Adam Ford wrote:
> > > > The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware
> > > > rogue_4.45.2.58_v1.fw available from Imagination.
> > > > 
> > > > When enumerated, it appears as:
> > > >   powervr fd00.gpu: [drm] loaded firmware 
> > > > powervr/rogue_4.45.2.58_v1.fw
> > > >   powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS)
> > > 
> > > These messages are printed after verifying the firmware blob’s headers,
> > > *before* attempting to upload it to the device. Just because they appear
> > > in dmesg does *not* imply the device is functional beyond the handful of
> > > register reads in pvr_load_gpu_id().
> > > 
> > > Since Mesa does not yet have support for this GPU, there’s not a lot
> > > that can be done to actually test these bindings.
> > > 
> > > When we added upstream support for the first GPU (the AXE core in TI’s
> > > AM62), we opted to wait until userspace was sufficiently progressed to
> > > the point it could be used for testing. This thought process still
> > > applies when adding new GPUs.
> > > 
> > > Our main concern is that adding bindings for GPUs implies a level of
> > > support that cannot be tested. That in turn may make it challenging to
> > > justify UAPI changes if/when they’re needed to actually make these GPUs
> > > functional.
> > 
> > I wrongly assumed that when the firmware was ready, there was some
> > preliminary functionality, but it sounds like we need to work for
> > Series6XT support to be added to the driver.  I only used the AXE
> > compatible since it appeared to the be the only one and the existing
> > binding document stated "model/revision is fully discoverable" which I
> > interpreted to mean that the AXE compatible was sufficient.
> 
> The comment is related to there being a few models/revisions of AXE [1][2][3],
> which we can distinguish between by reading a register.
> 
> > > > Signed-off-by: Adam Ford 
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi 
> > > > b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > > index a8a44fe5e83b..8923d9624b39 100644
> > > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > > @@ -2352,6 +2352,16 @@ gic: interrupt-controller@f101 {
> > > >   resets = < 408>;
> > > >   };
> > > > 
> > > > + gpu: gpu@fd00 {
> > > > + compatible = "renesas,r8a774a1-gpu", 
> > > > "img,img-axe";
> > > 
> > > The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible
> > > with one. For prior art, see [1] where we added support for the MT8173
> > > found in Elm Chromebooks R13 (also a Series6XT GPU).
> > > 
> > > > + reg = <0 0xfd00 0 0x2>;
> > > > + clocks = < CPG_MOD 112>;
> > > > + clock-names = "core";
> > > 
> > > Series6XT cores have three clocks (see [1] again). I don’t have a
> > > Renesas TRM to hand – do you know if their docs go into detail on the
> > > GPU integration?
> > > 
> > > > + interrupts = ;
> > > > + power-domains = < R8A774A1_PD_3DG_B>;
> > > > + resets = < 112>;
> > > > + };
> > > > +
> > > >   pciec0: pcie@fe00 {
> > > >   compatible = "renesas,pcie-r8a774a1",
> > > >"renesas,pcie-rcar-gen3";
> > > 
> > > As you probably expect by this point, I have to nack this series for
> > > now. I appreciate your effort here and I’ll be happy to help you land
> > 
> > I get that.  I wasn't sure if I should have even pushed this, but I
> > wanted to get a little traction, because I know there are people like
> > myself who want to

Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU

[Frank Binns]

On Tue, 2024-02-27 at 05:50 -0600, Adam Ford wrote:
> On Tue, Feb 27, 2024 at 3:31 AM Matt Coster  wrote:
> > Hi Adam,
> > 
> > Thanks for these patches! I'll just reply to this one patch, but my
> > comments apply to them all.
> > 
> > On 27/02/2024 03:45, Adam Ford wrote:
> > > The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware
> > > rogue_4.45.2.58_v1.fw available from Imagination.
> > > 
> > > When enumerated, it appears as:
> > >   powervr fd00.gpu: [drm] loaded firmware 
> > > powervr/rogue_4.45.2.58_v1.fw
> > >   powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS)
> > 
> > These messages are printed after verifying the firmware blob’s headers,
> > *before* attempting to upload it to the device. Just because they appear
> > in dmesg does *not* imply the device is functional beyond the handful of
> > register reads in pvr_load_gpu_id().
> > 
> > Since Mesa does not yet have support for this GPU, there’s not a lot
> > that can be done to actually test these bindings.
> > 
> > When we added upstream support for the first GPU (the AXE core in TI’s
> > AM62), we opted to wait until userspace was sufficiently progressed to
> > the point it could be used for testing. This thought process still
> > applies when adding new GPUs.
> > 
> > Our main concern is that adding bindings for GPUs implies a level of
> > support that cannot be tested. That in turn may make it challenging to
> > justify UAPI changes if/when they’re needed to actually make these GPUs
> > functional.
> 
> I wrongly assumed that when the firmware was ready, there was some
> preliminary functionality, but it sounds like we need to work for
> Series6XT support to be added to the driver.  I only used the AXE
> compatible since it appeared to the be the only one and the existing
> binding document stated "model/revision is fully discoverable" which I
> interpreted to mean that the AXE compatible was sufficient.

The comment is related to there being a few models/revisions of AXE [1][2][3],
which we can distinguish between by reading a register.

> > > Signed-off-by: Adam Ford 
> > > 
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi 
> > > b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > index a8a44fe5e83b..8923d9624b39 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > @@ -2352,6 +2352,16 @@ gic: interrupt-controller@f101 {
> > >   resets = < 408>;
> > >   };
> > > 
> > > + gpu: gpu@fd00 {
> > > + compatible = "renesas,r8a774a1-gpu", "img,img-axe";
> > 
> > The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible
> > with one. For prior art, see [1] where we added support for the MT8173
> > found in Elm Chromebooks R13 (also a Series6XT GPU).
> > 
> > > + reg = <0 0xfd00 0 0x2>;
> > > + clocks = < CPG_MOD 112>;
> > > + clock-names = "core";
> > 
> > Series6XT cores have three clocks (see [1] again). I don’t have a
> > Renesas TRM to hand – do you know if their docs go into detail on the
> > GPU integration?
> > 
> > > + interrupts = ;
> > > + power-domains = < R8A774A1_PD_3DG_B>;
> > > + resets = < 112>;
> > > + };
> > > +
> > >   pciec0: pcie@fe00 {
> > >   compatible = "renesas,pcie-r8a774a1",
> > >"renesas,pcie-rcar-gen3";
> > 
> > As you probably expect by this point, I have to nack this series for
> > now. I appreciate your effort here and I’ll be happy to help you land
> 
> I get that.  I wasn't sure if I should have even pushed this, but I
> wanted to get a little traction, because I know there are people like
> myself who want to use the 3D in the Renesas boards, but don't want to
> use the closed-source blobs tied to EULA and NDA documents.
> 
> > these once Mesa gains some form of usable support to allow testing.
> 
> Is there a way for your group to add me to the CC list when future
> updates are submitted?  I'd like to follow this and resubmit when it's
> ready.

Sure, we'll keep you updated as things progress.

Thanks
Frank

[1] https://www.imaginationtech.com/product/img-axe-1-16m/
[2] https://www.imaginationtech.com/product/img-axe-1-16/
[3] https://www.imaginationtech.com/product/img-axe-2-16/

> > Cheers,
> > Matt
> > 
> > [1]: 
> > https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-1006

Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU

[Frank Binns]

Hi Geert,

On Tue, 2024-02-27 at 12:04 +0100, Geert Uytterhoeven wrote:
> Hi Matt,
> 
> On Tue, Feb 27, 2024 at 10:31 AM Matt Coster  wrote:
> > Hi Adam,
> > 
> > Thanks for these patches! I'll just reply to this one patch, but my
> > comments apply to them all.
> > 
> > On 27/02/2024 03:45, Adam Ford wrote:
> > > The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware
> > > rogue_4.45.2.58_v1.fw available from Imagination.
> > > 
> > > When enumerated, it appears as:
> > >   powervr fd00.gpu: [drm] loaded firmware 
> > > powervr/rogue_4.45.2.58_v1.fw
> > >   powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS)
> > 
> > These messages are printed after verifying the firmware blob’s headers,
> > *before* attempting to upload it to the device. Just because they appear
> > in dmesg does *not* imply the device is functional beyond the handful of
> > register reads in pvr_load_gpu_id().
> > 
> > Since Mesa does not yet have support for this GPU, there’s not a lot
> > that can be done to actually test these bindings.
> 
> OK.
> 
> > When we added upstream support for the first GPU (the AXE core in TI’s
> > AM62), we opted to wait until userspace was sufficiently progressed to
> > the point it could be used for testing. This thought process still
> > applies when adding new GPUs.
> > 
> > Our main concern is that adding bindings for GPUs implies a level of
> > support that cannot be tested. That in turn may make it challenging to
> > justify UAPI changes if/when they’re needed to actually make these GPUs
> > functional.
> 
> I guess that applies to "[PATCH 00/11] Device tree support for
> Imagination Series5 GPU", too, which has been in linux-next for about
> a month?
> https://lore.kernel.org/all/20240109171950.31010-1-...@ti.com/
> 

The concern Matt has expressed is mostly right. However, adding DT bindings
doesn't prevent us from making UAPI changes. The thing that would prevent this
is adding the compatible(s) to the driver's `struct of_device_id` table, so this
is what we need to avoid doing until sufficient testing has been done.

> > > Signed-off-by: Adam Ford 
> > > 
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi 
> > > b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > index a8a44fe5e83b..8923d9624b39 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> > > @@ -2352,6 +2352,16 @@ gic: interrupt-controller@f101 {
> > >   resets = < 408>;
> > >   };
> > > 
> > > + gpu: gpu@fd00 {
> > > + compatible = "renesas,r8a774a1-gpu", "img,img-axe";
> > 
> > The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible
> > with one. For prior art, see [1] where we added support for the MT8173
> > found in Elm Chromebooks R13 (also a Series6XT GPU).
> 
> IC. And the bindings in [2].
> 
> > > + reg = <0 0xfd00 0 0x2>;
> > > + clocks = < CPG_MOD 112>;
> > > + clock-names = "core";
> > 
> > Series6XT cores have three clocks (see [1] again). I don’t have a
> > Renesas TRM to hand – do you know if their docs go into detail on the
> > GPU integration?
> 
> Not really. The diagram in the Hardware User's Manual just shows the
> following clock inputs:
>   - Clock (ZGϕ) from CPG,
>   - Clock (S3D1ϕ) from CPG,
>   - MSTP (ST112) from CPG.
> 
> ZG is the main (programmable) 3DGE clock, running at up to 600 MHz.
> S3D1 is the fixed 266 MHz AXI bus clock.
> MSTP112 is the gateable module clock (part of the SYSC/CPG clock
> domain), and its parent is ZG.
> 
> According to the sources:
>   - "core" is the primary clock used by the entire GPU core, so we use
> MSTP112 for that.
>   - "sys" is the optional system bus clock, so that could be S3D1,
>   - "mem" is the optional memory clock, no idea what that would map to.
> 

The sys and mem clocks are optional in the driver as AXE can be configured at
integration time to operate with a single clock ("core") or three clocks
("core", "sys" and "mem). Series6XT doesn't have this configurability and
expects all three clocks.

> But IMHO the two optional clocks do not matter at all (the driver
> doesn't care about their rates, and just enables them together with
> the core clock), and S3D1 is always-on, so I'd just limit clocks to
> a single item.
> 

They matter in that, if present, we may be able to make additional power
savings. The driver doesn't attempt to take advantage of this at present (and
can't for AM62x anyway), but this shouldn't influence the DT bindings.

> Just wondering: is the availability of 1 clock specific to AXE, or to
> the AXE integration on AM62x?
> 

This is something TI have configured as part of their integration for AM62x.

Thanks
Frank

> > + interrupts = ;
> > + power-domains = < R8A774A1_PD_3DG_B>;
> > + resets = < 112>;
> > + };
> > [1]: 
> > 

Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3

[Frank Binns]

On Tue, 2024-03-05 at 07:47 -0600, Adam Ford wrote:
> On Tue, Mar 5, 2024 at 5:58 AM Frank Binns  wrote:
> > Hi Adam,
> > 
> > Sorry for not responding sooner. I've recently just returned from paternity
> > leave, so just catching up on everything.
> 
> Congratulations!
> 

Thanks!

> > On Thu, 2024-02-15 at 11:22 -0600, Adam Ford wrote:
> > > On Thu, Feb 15, 2024 at 11:10 AM Adam Ford  wrote:
> > > > On Thu, Feb 15, 2024 at 10:54 AM Geert Uytterhoeven
> > > >  wrote:
> > > > > Hi Maxime,
> > > > > 
> > > > > On Thu, Feb 15, 2024 at 5:18 PM Maxime Ripard  
> > > > > wrote:
> > > > > > On Thu, Feb 15, 2024 at 01:50:09PM +0100, Geert Uytterhoeven wrote:
> > > > > > > Using the Imagination Technologies PowerVR Series 6 GPU requires a
> > > > > > > proprietary firmware image, which is currently only available for 
> > > > > > > Texas
> > > > > > > Instruments K3 AM62x SoCs.  Hence add a dependency on ARCH_K3, to
> > > > > > > prevent asking the user about this driver when configuring a 
> > > > > > > kernel
> > > > > > > without Texas Instruments K3 Multicore SoC support.
> > > > > > 
> > > > > > This wasn't making sense the first time you sent it, and now that 
> > > > > > commit
> > > > > > log is just plain wrong. We have firmwares for the G6110, GX6250,
> > > > > > GX6650, BXE-4-32, and BXS-4-64 models, which can be found on (at 
> > > > > > least)
> > > > > > Renesas, Mediatek, Rockchip, TI and StarFive, so across three
> > > > > 
> > > > > I am so happy to be proven wrong!
> > > > > Yeah, GX6650 is found on e.g. R-Car H3, and GX6250 on e.g. R-Car M3-W.
> > > > > 
> > > > > > architectures and 5 platforms. In two months.
> > > > > 
> > > > > That sounds like great progress, thanks a lot!
> > > > > 
> > > > Geert,
> > > > 
> > > > > Where can I find these firmwares? Linux-firmware[1] seems to lack all
> > > > > but the original K3 AM62x one.
> > > > 
> > > > I think PowerVR has a repo [1], but the last time I checked it, the
> > > > BVNC for the firmware didn't match what was necessary for the GX6250
> > > > on the RZ/G2M.  I can't remember what the corresponding R-Car3 model
> > > > is.  I haven't tried recently because I was told more documentation
> > > > for firmware porting would be delayed until everything was pushed into
> > > > the kernel and Mesa.  Maybe there is a better repo and/or newer
> > > > firmware somewhere else.
> > > > 
> > > I should have doubled checked the repo contents before I sent my last
> > > e-mail , but it appears the firmware  [2] for the RZ/G2M, might be
> > > present now. I don't know if there are driver updates necessary. I
> > > checked my e-mails, but I didn't see any notification, or I would have
> > > tried it earlier.  Either way, thank you Frank for adding it.  I'll
> > > try to test when I have some time.
> > > 
> > 
> > You may have noticed from one of Matt's emails that we now have a set of 
> > repos
> > (linux, linux-firmware and Mesa) in our own area on freedesktop.org GitLab:
> > https://gitlab.freedesktop.org/imagination/
> > 
> > We'll be using this as a staging area for work that isn't ready to be 
> > upstreamed
> > yet (including firmware binaries).
> > 
> 
> I tried to play with these a little, but it seems like there is still
> a fair amount of work to be done on the 6XT series. I tried to add the
> device tree support for several Renesas boards, but the series was
> NAK'd due to an inability to test it.

I've not had a chance to properly read through that thread yet and I seem to
remember that, when I had a quick skim through the DT bindings patch, there was
some feedback I wanted to give. I'll try to get to that sooner rather than
later.

Anyway, in principle I don't think there should be an issue with upstreaming
device tree bindings. The thing that needs to be avoided is baking in the uapi
before sufficient testing has been done (passing the Vulkan conformance test
suite will give us enough confidence). As long as we don't add the compatibles
into the `struct of_device_id` table in the driver, we should be fine in this
regard.

Sorry if this conflicts with anything Matt already said. We're still very new to
workin

Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3

[Frank Binns]

Hi Adam,

Sorry for not responding sooner. I've recently just returned from paternity
leave, so just catching up on everything.

On Thu, 2024-02-15 at 11:22 -0600, Adam Ford wrote:
> On Thu, Feb 15, 2024 at 11:10 AM Adam Ford  wrote:
> > On Thu, Feb 15, 2024 at 10:54 AM Geert Uytterhoeven
> >  wrote:
> > > Hi Maxime,
> > > 
> > > On Thu, Feb 15, 2024 at 5:18 PM Maxime Ripard  wrote:
> > > > On Thu, Feb 15, 2024 at 01:50:09PM +0100, Geert Uytterhoeven wrote:
> > > > > Using the Imagination Technologies PowerVR Series 6 GPU requires a
> > > > > proprietary firmware image, which is currently only available for 
> > > > > Texas
> > > > > Instruments K3 AM62x SoCs.  Hence add a dependency on ARCH_K3, to
> > > > > prevent asking the user about this driver when configuring a kernel
> > > > > without Texas Instruments K3 Multicore SoC support.
> > > > 
> > > > This wasn't making sense the first time you sent it, and now that commit
> > > > log is just plain wrong. We have firmwares for the G6110, GX6250,
> > > > GX6650, BXE-4-32, and BXS-4-64 models, which can be found on (at least)
> > > > Renesas, Mediatek, Rockchip, TI and StarFive, so across three
> > > 
> > > I am so happy to be proven wrong!
> > > Yeah, GX6650 is found on e.g. R-Car H3, and GX6250 on e.g. R-Car M3-W.
> > > 
> > > > architectures and 5 platforms. In two months.
> > > 
> > > That sounds like great progress, thanks a lot!
> > > 
> > Geert,
> > 
> > > Where can I find these firmwares? Linux-firmware[1] seems to lack all
> > > but the original K3 AM62x one.
> > 
> > I think PowerVR has a repo [1], but the last time I checked it, the
> > BVNC for the firmware didn't match what was necessary for the GX6250
> > on the RZ/G2M.  I can't remember what the corresponding R-Car3 model
> > is.  I haven't tried recently because I was told more documentation
> > for firmware porting would be delayed until everything was pushed into
> > the kernel and Mesa.  Maybe there is a better repo and/or newer
> > firmware somewhere else.
> > 
> I should have doubled checked the repo contents before I sent my last
> e-mail , but it appears the firmware  [2] for the RZ/G2M, might be
> present now. I don't know if there are driver updates necessary. I
> checked my e-mails, but I didn't see any notification, or I would have
> tried it earlier.  Either way, thank you Frank for adding it.  I'll
> try to test when I have some time.
> 

You may have noticed from one of Matt's emails that we now have a set of repos
(linux, linux-firmware and Mesa) in our own area on freedesktop.org GitLab:
https://gitlab.freedesktop.org/imagination/

We'll be using this as a staging area for work that isn't ready to be upstreamed
yet (including firmware binaries).


> > adam
> > 
> > [1] 
> > https://gitlab.freedesktop.org/frankbinns/linux-firmware/-/tree/powervr/powervr?ref_type=heads
> 
> [2] - 
> https://gitlab.freedesktop.org/frankbinns/linux-firmware/-/commit/fecb3caebf29f37221fe0a20236e5e1415d39d0b
> 

This is now the place to get the firmware for devices that aren't yet supported
upstream:
https://gitlab.freedesktop.org/imagination/linux-firmware/-/commits/powervr/?ref_type=HEADS

With the firmware for the Renesas variant of GX6250 being found in this commit:
https://gitlab.freedesktop.org/imagination/linux-firmware/-/commit/fecb3caebf29f37221fe0a20236e5e1415d39d0b

Thanks
Frank

> > 
> > > Thanks again!
> > > 
> > > [1] 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/
> > > 
> > > Gr{oetje,eeting}s,
> > > 
> > > Geert
> > > 
> > > --
> > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> > > ge...@linux-m68k.org
> > > 
> > > In personal conversations with technical people, I call myself a hacker. 
> > > But
> > > when I'm talking to journalists I just say "programmer" or something like 
> > > that.
> > > -- Linus Torvalds

Re: [PATCH RFC v2 01/11] dt-bindings: gpu: Rename img,powervr to img,powervr-rogue

[Frank Binns]

Hi Andrew,

On Mon, 2024-01-08 at 12:32 -0600, Andrew Davis wrote:
> Signed-off-by: Andrew Davis 
> ---
>  .../bindings/gpu/{img,powervr.yaml => img,powervr-rogue.yaml} | 4 ++--
>  MAINTAINERS   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>  rename Documentation/devicetree/bindings/gpu/{img,powervr.yaml => 
> img,powervr-rogue.yaml} (91%)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
> b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> similarity index 91%
> rename from Documentation/devicetree/bindings/gpu/img,powervr.yaml
> rename to Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index a13298f1a1827..03a8308b41ae7 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -2,10 +2,10 @@
>  # Copyright (c) 2023 Imagination Technologies Ltd.
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
> +$id: http://devicetree.org/schemas/gpu/img,powervr-rogue.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Imagination Technologies PowerVR and IMG GPU
> +title: Imagination Technologies PowerVR Rogue and IMG GPUs

All the GPUs that will appear in this file will be Rogues, so for me it would be
more natural for 'Rogue' to come after 'IMG'. Can you change the title to:

Imagination Technologies PowerVR and IMG Rogue GPUs
With that changed and Javier's suggestions addressed:
Reviewed-by: Frank Binns 

>  
>  maintainers:
>- Frank Binns 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fa67e2624723f..5b205795da04e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10461,7 +10461,7 @@ M:Donald Robson 
>  M:   Matt Coster 
>  S:   Supported
>  T:   git git://anongit.freedesktop.org/drm/drm-misc
> -F:   Documentation/devicetree/bindings/gpu/img,powervr.yaml
> +F:   Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>  F:   Documentation/gpu/imagination/
>  F:   drivers/gpu/drm/imagination/
>  F:   include/uapi/drm/pvr_drm.h

Re: [PATCH] drm/imagination: Move dereference after NULL check in pvr_mmu_backing_page_init()

[Frank Binns]

Hi Dan,

Thank you for the patch.

On Wed, 2023-12-06 at 17:37 +0300, Dan Carpenter wrote:
> This code dereferences "page->pvr_dev" and then checked for NULL on the
> next line.  Re-order it to avoid a potential NULL pointer dereference.
> 
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Dan Carpenter 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_mmu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c 
> b/drivers/gpu/drm/imagination/pvr_mmu.c
> index c8562bfc0dcd..4fe70610ed94 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> @@ -316,12 +316,14 @@ pvr_mmu_backing_page_init(struct pvr_mmu_backing_page 
> *page,
>  static void
>  pvr_mmu_backing_page_fini(struct pvr_mmu_backing_page *page)
>  {
> - struct device *dev = from_pvr_device(page->pvr_dev)->dev;
> + struct device *dev;
>  
>   /* Do nothing if no allocation is present. */
>   if (!page->pvr_dev)
>   return;
>  
> + dev = from_pvr_device(page->pvr_dev)->dev;
> +
>   dma_unmap_page(dev, page->dma_addr, PVR_MMU_BACKING_PAGE_SIZE,
>  DMA_TO_DEVICE);
>  

[PATCH] MAINTAINERS: Document Imagination PowerVR driver patches go via drm-misc

[Frank Binns]

This is the tree used by nearly all other DRM drivers, so use it for the
PowerVR driver as well.

Signed-off-by: Frank Binns 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0ba904b46efe..d4b46b3db022 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10397,6 +10397,7 @@ M:  Frank Binns 
 M: Donald Robson 
 M: Matt Coster 
 S: Supported
+T: git git://anongit.freedesktop.org/drm/drm-misc
 F: Documentation/devicetree/bindings/gpu/img,powervr.yaml
 F: Documentation/gpu/imagination/
 F: drivers/gpu/drm/imagination/
-- 
2.25.1

Re: [PATCH] drm/imagination: fix off by one in pvr_vm_mips_init() error handling

[Frank Binns]

On Thu, 2023-11-30 at 10:27 +0300, Dan Carpenter wrote:
> If the call to vmap() fails the "page_nr" is one element beyond the end
> of the mips_data->pt_dma_addr[] and mips_data->pt_pages[] arrays.
> 
> The way that this is traditionally written is that we clean up the
> partial loop iteration before the goto and then we can say
> while (--i >= 0).  At that point we know that all the elements thus
> far are initialized so we don't need to have NULL checks.
> 
> Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and 
> MMU support")
> Signed-off-by: Dan Carpenter 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_vm_mips.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c 
> b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> index 7268cf6e630b..2bc7181a4c3e 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> @@ -57,6 +57,7 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
>  PAGE_SIZE, 
> DMA_TO_DEVICE);
>   if (dma_mapping_error(dev, mips_data->pt_dma_addr[page_nr])) {
>   err = -ENOMEM;
> + __free_page(mips_data->pt_pages[page_nr]);
>   goto err_free_pages;
>   }
>   }
> @@ -79,13 +80,11 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
>   return 0;
>  
>  err_free_pages:
> - for (; page_nr >= 0; page_nr--) {
> - if (mips_data->pt_dma_addr[page_nr])
> - dma_unmap_page(from_pvr_device(pvr_dev)->dev,
> -mips_data->pt_dma_addr[page_nr], 
> PAGE_SIZE, DMA_TO_DEVICE);
> + while (--page_nr >= 0) {
> + dma_unmap_page(from_pvr_device(pvr_dev)->dev,
> +mips_data->pt_dma_addr[page_nr], PAGE_SIZE, 
> DMA_TO_DEVICE);
>  
> - if (mips_data->pt_pages[page_nr])
> - __free_page(mips_data->pt_pages[page_nr]);
> + __free_page(mips_data->pt_pages[page_nr]);
>   }
>  
>   return err;

Re: [PATCH 2/2] drm/imagination: Fix IS_ERR() vs NULL bug in pvr_request_firmware()

[Frank Binns]

On Thu, 2023-11-30 at 10:27 +0300, Dan Carpenter wrote:
> The pvr_build_firmware_filename() function returns NULL on error.  It
> doesn't return error pointers.
> 
> Fixes: f99f5f3ea7ef ("drm/imagination: Add GPU ID parsing and firmware 
> loading")
> Signed-off-by: Dan Carpenter 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c 
> b/drivers/gpu/drm/imagination/pvr_device.c
> index e1dcc4e42087..5389aea7ff21 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -286,8 +286,8 @@ pvr_request_firmware(struct pvr_device *pvr_dev)
>  
>   filename = pvr_build_firmware_filename(pvr_dev, "powervr/rogue",
>  PVR_FW_VERSION_MAJOR);
> - if (IS_ERR(filename))
> - return PTR_ERR(filename);
> + if (!filename)
> + return -ENOMEM;
>  
>   /*
>* This function takes a copy of , meaning we can free our

Re: [PATCH 1/2] drm/imagination: Fix error codes in pvr_device_clk_init()

[Frank Binns]

Hi,

Thank you for the patches.

On Thu, 2023-11-30 at 10:26 +0300, Dan Carpenter wrote:
> There is a cut and paste error so this code returns the wrong variable.
> 
> Fixes: 1f88f017e649 ("drm/imagination: Get GPU resources")
> Signed-off-by: Dan Carpenter 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c 
> b/drivers/gpu/drm/imagination/pvr_device.c
> index 8499becf4fbb..e1dcc4e42087 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -105,12 +105,12 @@ static int pvr_device_clk_init(struct pvr_device 
> *pvr_dev)
>  
>   sys_clk = devm_clk_get_optional(drm_dev->dev, "sys");
>   if (IS_ERR(sys_clk))
> - return dev_err_probe(drm_dev->dev, PTR_ERR(core_clk),
> + return dev_err_probe(drm_dev->dev, PTR_ERR(sys_clk),
>"failed to get sys clock\n");
>  
>   mem_clk = devm_clk_get_optional(drm_dev->dev, "mem");
>   if (IS_ERR(mem_clk))
> - return dev_err_probe(drm_dev->dev, PTR_ERR(core_clk),
> + return dev_err_probe(drm_dev->dev, PTR_ERR(mem_clk),
>"failed to get mem clock\n");
>  
>   pvr_dev->core_clk = core_clk;

Re: [PATCH drm-misc-next 0/5] PowerVR VM fixes

[Frank Binns]

Hi,

Thank you for the patches.

On Sat, 2023-11-25 at 00:36 +0100, Danilo Krummrich wrote:
> Hi,
> 
> Some major GPUVM changes landed just before v8 of the PowerVR series. Since v8
> went in rather quickly (review process was finished otherwise) I haven't had 
> the
> chance to review the subsequent code changes.
> 
> Hence, this series with a few fixes in this context. Plus a minor GPUVM patch 
> to
> make the drm_gpuvm_prepare_* helpers useful for PowerVR.
> 
> - Danilo
> 
> 
> Danilo Krummrich (5):
>   drm/imagination: vm: prevent duplicate drm_gpuvm_bo instances
>   drm/imagination: vm: check for drm_gpuvm_range_valid()
>   drm/imagination: vm: fix drm_gpuvm reference count
>   drm/gpuvm: fall back to drm_exec_lock_obj()
>   drm/imagination: vm: make use of GPUVM's drm_exec helper
> 
>  drivers/gpu/drm/drm_gpuvm.c  | 36 --
>  drivers/gpu/drm/imagination/pvr_vm.c | 46 +++-
>  drivers/gpu/drm/imagination/pvr_vm.h |  3 +-
>  include/drm/drm_gpuvm.h  | 23 ++
>  4 files changed, 63 insertions(+), 45 deletions(-)
> 
> 
> base-commit: 46990918f35c1bf6e367cf8e0423e7344fec9fcb

For the series:

Tested-by: Frank Binns 

I'll leave it to Donald to do the review.

Thanks
Frank

Re: [PATCH][next] drm/imagination: Fix a couple of spelling mistakes in literal strings

[Frank Binns]

Hi Colin,

Thank you for the patch.

On Fri, 2023-11-24 at 16:39 +, Colin Ian King wrote:
> There are a couple of spelling mistakes in literal strings in the
> stid_fmts array. Fix these.
> 
> Signed-off-by: Colin Ian King 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_rogue_fwif_sf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_rogue_fwif_sf.h 
> b/drivers/gpu/drm/imagination/pvr_rogue_fwif_sf.h
> index 571954182f33..56e11009e123 100644
> --- a/drivers/gpu/drm/imagination/pvr_rogue_fwif_sf.h
> +++ b/drivers/gpu/drm/imagination/pvr_rogue_fwif_sf.h
> @@ -497,7 +497,7 @@ static const struct rogue_km_stid_fmt stid_fmts[] = {
>   { ROGUE_FW_LOG_CREATESFID(213, ROGUE_FW_GROUP_MAIN, 1),
> "Safety Watchdog threshold period set to 0x%x clock cycles" },
>   { ROGUE_FW_LOG_CREATESFID(214, ROGUE_FW_GROUP_MAIN, 0),
> -   "MTS Safety Event trigged by the safety watchdog." },
> +   "MTS Safety Event triggered by the safety watchdog." },
>   { ROGUE_FW_LOG_CREATESFID(215, ROGUE_FW_GROUP_MAIN, 3),
> "DM%d USC tasks range limit 0 - %d, stride %d" },
>   { ROGUE_FW_LOG_CREATESFID(216, ROGUE_FW_GROUP_MAIN, 1),
> @@ -1114,7 +1114,7 @@ static const struct rogue_km_stid_fmt stid_fmts[] = {
>   { ROGUE_FW_LOG_CREATESFID(39, ROGUE_FW_GROUP_SPM, 2),
> "3DMemFree matches freelist 0x%08x (FL type = %u)" },
>   { ROGUE_FW_LOG_CREATESFID(40, ROGUE_FW_GROUP_SPM, 0),
> -   "Raise the 3DMemFreeDedected flag" },
> +   "Raise the 3DMemFreeDetected flag" },
>   { ROGUE_FW_LOG_CREATESFID(41, ROGUE_FW_GROUP_SPM, 1),
> "Wait for pending grow on Freelist 0x%08x" },
>   { ROGUE_FW_LOG_CREATESFID(42, ROGUE_FW_GROUP_SPM, 1),

Re: [PATCH -next] drm/imagination: Remove unneeded semicolon

[Frank Binns]

Hi,

Thank you for the patch.

On Mon, 2023-11-27 at 09:04 +0800, Yang Li wrote:
> ./drivers/gpu/drm/imagination/pvr_free_list.c:258:2-3: Unneeded semicolon
> 
> Reported-by: Abaci Robot 
> Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=7635
> Signed-off-by: Yang Li 

Reviewed-by: Frank Binns 

> ---
>  drivers/gpu/drm/imagination/pvr_free_list.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_free_list.c 
> b/drivers/gpu/drm/imagination/pvr_free_list.c
> index c61fd417edcb..5e51bc980751 100644
> --- a/drivers/gpu/drm/imagination/pvr_free_list.c
> +++ b/drivers/gpu/drm/imagination/pvr_free_list.c
> @@ -255,7 +255,7 @@ pvr_free_list_insert_pages_locked(struct pvr_free_list 
> *free_list,
>  
>   if (!num_pages)
>   break;
> - };
> + }
>   /* clang-format on */
>  
>   /* Make sure our free_list update is flushed. */

Re: [PATCH v5 09/17] drm/imagination: Implement power management

[Frank Binns]

Hi Paul,

On Wed, 2023-08-16 at 12:56 +0200, Paul Cercueil wrote:
> Hi Sarah,
> 
> Le mercredi 16 août 2023 à 09:25 +0100, Sarah Walker a écrit :
> > Add power management to the driver, using runtime pm. The power off
> > sequence depends on firmware commands which are not implemented in
> > this
> > patch.
> > 
> > Changes since v4:
> > - Suspend runtime PM before unplugging device on rmmod
> > 
> > Changes since v3:
> > - Don't power device when calling pvr_device_gpu_fini()
> > - Documentation for pvr_dev->lost has been improved
> > - pvr_power_init() renamed to pvr_watchdog_init()
> > - Use drm_dev_{enter,exit}
> > 
> > Changes since v2:
> > - Use runtime PM
> > - Implement watchdog
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >  drivers/gpu/drm/imagination/Makefile |   1 +
> >  drivers/gpu/drm/imagination/pvr_device.c |  23 +-
> >  drivers/gpu/drm/imagination/pvr_device.h |  22 ++
> >  drivers/gpu/drm/imagination/pvr_drv.c|  20 +-
> >  drivers/gpu/drm/imagination/pvr_power.c  | 271
> > +++
> >  drivers/gpu/drm/imagination/pvr_power.h  |  39 
> >  6 files changed, 373 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_power.c
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_power.h
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile
> > b/drivers/gpu/drm/imagination/Makefile
> > index 8fcabc1bea36..235e2d329e29 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -10,6 +10,7 @@ powervr-y := \
> > pvr_fw.o \
> > pvr_gem.o \
> > pvr_mmu.o \
> > +   pvr_power.o \
> > pvr_vm.o
> >  
> >  obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c
> > b/drivers/gpu/drm/imagination/pvr_device.c
> > index ef8f7a2ff1a9..5dbd05f21238 100644
> > --- a/drivers/gpu/drm/imagination/pvr_device.c
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -5,6 +5,7 @@
> >  #include "pvr_device_info.h"
> >  
> >  #include "pvr_fw.h"
> > +#include "pvr_power.h"
> >  #include "pvr_rogue_cr_defs.h"
> >  #include "pvr_vm.h"
> >  
> > @@ -357,6 +358,8 @@ pvr_device_gpu_fini(struct pvr_device *pvr_dev)
> >  int
> >  pvr_device_init(struct pvr_device *pvr_dev)
> >  {
> > +   struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +   struct device *dev = drm_dev->dev;
> > int err;
> >  
> > /* Enable and initialize clocks required for the device to
> > operate. */
> > @@ -364,13 +367,29 @@ pvr_device_init(struct pvr_device *pvr_dev)
> > if (err)
> > return err;
> >  
> > +   /* Explicitly power the GPU so we can access control
> > registers before the FW is booted. */
> > +   err = pm_runtime_resume_and_get(dev);
> > +   if (err)
> > +   return err;
> > +
> > /* Map the control registers into memory. */
> > err = pvr_device_reg_init(pvr_dev);
> > if (err)
> > -   return err;
> > +   goto err_pm_runtime_put;
> >  
> > /* Perform GPU-specific initialization steps. */
> > -   return pvr_device_gpu_init(pvr_dev);
> > +   err = pvr_device_gpu_init(pvr_dev);
> > +   if (err)
> > +   goto err_pm_runtime_put;
> > +
> > +   pm_runtime_put(dev);
> > +
> > +   return 0;
> > +
> > +err_pm_runtime_put:
> > +   pm_runtime_put_sync_suspend(dev);
> > +
> > +   return err;
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.h
> > b/drivers/gpu/drm/imagination/pvr_device.h
> > index 990363e433d7..6d58ecfbc838 100644
> > --- a/drivers/gpu/drm/imagination/pvr_device.h
> > +++ b/drivers/gpu/drm/imagination/pvr_device.h
> > @@ -135,6 +135,28 @@ struct pvr_device {
> >  
> > /** @fw_dev: Firmware related data. */
> > struct pvr_fw_device fw_dev;
> > +
> > +   struct {
> > +   /** @work: Work item for watchdog callback. */
> > +   struct delayed_work work;
> > +
> > +   /** @old_kccb_cmds_executed: KCCB command execution
> > count at last watchdog poll. */
> > +   u32 old_kccb_cmds_executed;
> > +
> > +   /** @kccb_stall_count: Number of watchdog polls KCCB
> > has been stalled for. */
> > +   u32 kccb_stall_count;
> > +   } watchdog;
> > +
> > +   /**
> > +* @lost: %true if the device has been lost.
> > +*
> > +* This variable is set if the device has become
> > irretrievably unavailable, e.g. if the
> > +* firmware processor has stopped responding and can not be
> > revived via a hard reset.
> > +*/
> > +   bool lost;
> > +
> > +   /** @sched_wq: Workqueue for schedulers. */
> > +   struct workqueue_struct *sched_wq;
> >  };
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/imagination/pvr_drv.c
> > b/drivers/gpu/drm/imagination/pvr_drv.c
> > index 0d51177b506c..0331dc549116 100644
> > --- 

Re: [PATCH v5 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Linus,

Thank you for your feedback (comments below).

On Fri, 2023-08-18 at 11:36 +0200, Linus Walleij wrote:
> Hi Sarah,
> 
> thanks for your patch!
> 
> Patches adding device tree bindings need to be CC:ed to
> devicet...@vger.kernel.org
> and the DT binding maintainers, I have added it for now.

Thank you - it looks like something went wrong when the patch was sent.

> 
> On Wed, Aug 16, 2023 at 10:26 AM Sarah Walker  wrote:
> 
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Co-developed-by: Frank Binns 
> > Signed-off-by: Frank Binns 
> > Signed-off-by: Sarah Walker 
> (...)
> > +properties:
> > +  compatible:
> > +items:
> > +  - enum:
> > +  - ti,am62-gpu
> > +  - const: img,powervr-seriesaxe
> 
> Should there not at least be a dash there?
> 
> img,powervr-series-axe?
> 
> It is spelled in two words in the commit message,
> Series AXE not SeriesAXE?

We've now changed the string to address your earlier feedback (see below).

> 
> Moreover, if this pertains to the AXE-1-16 and AXE-2-16 it is kind of a 
> wildcard
> and we usually don't do that, I would use the exact version instead,
> such as:
> const: img,powervr-axe-1-16
> any reason not to do this?

The exact GPU model/revision is fully discoverable via a register. We saw the
same is also true for Mali Bifrost, where they have a single string covering
multiple models [1], so we took the same approach. We'll add a comment in v6
along the lines of the one in the Mali Bifrost bindings.

> 
> I asked about the relationship between these strings and the product
> designations earlier I think :/

Sorry about that, I honestly thought we'd addressed that bit of feedback by
changing the compatibility string, but clearly we hadn't :( Thank you for
catching this.

We've now changed the string to "img,img-axe" to align with the marketing name,
along with updating the commit message and various other places to refer to
PowerVR and IMG GPUs (the DRM driver supporting both).

Thanks
Frank

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml?h=v6.5#n29

Re: [PATCH v4 03/17] drm/imagination/uapi: Add PowerVR driver UAPI

[Frank Binns]

Hi Michel,

Thank you for the feedback (comments below).

On Tue, 2023-08-08 at 15:46 +0200, Michel Dänzer wrote:
> On 7/14/23 16:25, Sarah Walker wrote:
> > +/**
> > + * DOC: PowerVR IOCTL CREATE_BO interface
> > + */
> > +
> > +/**
> > + * DOC: Flags for CREATE_BO
> > + *
> > + * The  drm_pvr_ioctl_create_bo_args.flags field is 64 bits wide 
> > and consists
> > + * of three groups of flags: creation, device mapping and CPU mapping.
> > + *
> > + * We use "device" to refer to the GPU here because of the ambiguity 
> > between
> > + * CPU and GPU in some fonts.
> > + *
> > + * Creation options
> > + *These use the prefix ``DRM_PVR_BO_CREATE_``.
> > + *
> > + *:ZEROED: Require the allocated buffer to be zeroed before returning. 
> > Note
> > + *  that this is an active operation, and is never zero cost. Unless 
> > it is
> > + *  explicitly required, this option should not be set.
> 
> Making this optional is kind of problematic from a security standpoint 
> (information leak, at least if the memory was previously used by a different 
> process). See e.g. the discussion starting at 
> https://gitlab.freedesktop.org/mesa/mesa/-/issues/9189#note_1972986 .
> 
> AFAICT the approach I suggested there (Clear freed memory in the background, 
> and make it available for allocation again only once the clear has finished) 
> isn't really possible with gem_shmem in its current state though. There seems 
> to be ongoing work to do something like that for __GFP_ZERO in general, maybe 
> gem_shmem could take advantage of that when it lands. I'm afraid this series 
> can't depend on that though.
> 

Good point! We'll remove this flag and always zero the memory at allocation
time. We can then look to do something better in the future once the driver is
merged.

> 
> > +/**
> > + * DOC: PowerVR IOCTL VM_MAP and VM_UNMAP interfaces
> > + *
> > + * The VM UAPI allows userspace to create buffer object mappings in GPU 
> > virtual address space.
> > + *
> > + * The client is responsible for managing GPU address space. It should 
> > allocate mappings within
> > + * the heaps returned by %DRM_PVR_DEV_QUERY_HEAP_INFO_GET.
> > + *
> > + * %DRM_IOCTL_PVR_VM_MAP creates a new mapping. The client provides the 
> > target virtual address for
> > + * the mapping. Size and offset within the mapped buffer object can be 
> > specified, so the client can
> > + * partially map a buffer.
> > + *
> > + * %DRM_IOCTL_PVR_VM_UNMAP removes a mapping. The entire mapping will be 
> > removed from GPU address
> > + * space. For this reason only the start address is provided by the client.
> > + */
> 
> FWIW, the amdgpu driver uses a single ioctl for VM map & unmap (plus two 
> additional operations for partial residency). Maybe this would make sense for 
> the PowerVR driver as well, in particular if it might support partial 
> residency in the future.
> 
> (amdgpu also uses similar multiplexer ioctls for other things such as context 
> create/destroy/...)
> 
> Just an idea, feel free to ignore.
> 

We originally had a single ioctl for VM map/unmap, which was inspired by amdgpu,
but we were told to split this into separate ioctls:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/15243#note_1283760

> 
> > +/**
> > + * DOC: Flags for SUBMIT_JOB ioctl geometry command.
> > + *
> > + * .. c:macro:: DRM_PVR_SUBMIT_JOB_GEOM_CMD_FIRST
> > + *
> > + *Indicates if this the first command to be issued for a render.
> > + *
> > + * .. c:macro:: DRM_PVR_SUBMIT_JOB_GEOM_CMD_LAST
> 
> Does user space really need to pass in the FIRST/LAST flags, can't the kernel 
> driver determine this implicitly? What happens if user space sets these 
> incorrectly?
> 

I don't think there's a way for the kernel driver to determine when the LAST
flag should be set. The obvious time to do this would be when it first
encounters a fragment job and there are geometry jobs proceeding it. However,
there is a least one case (transform feedback) when we want to submit a geometry
job without a fragment job and we need to set the LAST flag. I can't think of an
obvious way to detect this in the kernel driver.

If the flags aren't set correctly then it can cause the hardware to lockup.

> 
> > + * .. c:macro:: DRM_PVR_SUBMIT_JOB_FRAG_CMD_PREVENT_CDM_OVERLAP
> > + *
> > + *Disallow compute overlapped with this render.
> 
> Does this affect only compute from the same context, or also from other 
> contexts?
> 

Fragment and compute jobs are submitted on separate contexts. This flag is set
on fragment jobs and will prevent the job in question overlapping with compute
jobs across all compute contexts. This is necessary in some cases to avoid
hardware lockups on those GPUs that support compute overlapping with other job
types.

> (Similar question for DRM_PVR_SUBMIT_JOB_COMPUTE_CMD_PREVENT_ALL_OVERLAP)
> 

Likewise, this will prevent compute jobs with this flag set from overlapping
with all other jobs across all contexts. Again, this is to avoid hardware

Re: [PATCH v4 17/17] arm64: dts: ti: k3-am62-main: Add GPU device node [DO NOT MERGE]

[Frank Binns]

Hi Krzysztof,

On Tue, 2023-07-18 at 08:19 +0200, Krzysztof Kozlowski wrote:
> On 14/07/2023 16:30, Sarah Walker wrote:
> > Add the Series AXE GPU node to the AM62 device tree.
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >  arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 13 +
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi 
> > b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> > index b3e4857bbbe4..ad13414acf18 100644
> > --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> > +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> > @@ -892,4 +892,17 @@ mcasp2: audio-controller@2b2 {
> > power-domains = <_pds 192 TI_SCI_PD_EXCLUSIVE>;
> > status = "disabled";
> > };
> > +
> > +gpu: gpu@fd0 {
> > +compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
> > +reg = <0 0x0fd0 0 0x2>;
> > +power-domains = <_pds 187 TI_SCI_PD_EXCLUSIVE>;
> > +clocks = <_clks 187 0>;
> > +clock-names = "core";
> > +interrupts = ;
> > +interrupt-names = "gpu";
> > +#cooling-cells = <2>;
> > +#cooling-min-level = <0>;
> > +#cooling-max-level = <3>;
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).

Thank you for the tip. I'll have a read through.

Frank

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Krzysztof,

On Tue, 2023-07-18 at 08:20 +0200, Krzysztof Kozlowski wrote:
> On 14/07/2023 16:25, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> 
> ...
> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +#include 
> > +#include 
> > +
> > +gpu: gpu@fd0 {
> > +compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
> > +reg = <0x0fd0 0x2>;
> > +power-domains = <_pds 187>;
> > +clocks = <_clks 187 0>;
> > +clock-names = "core";
> > +interrupts = ;
> > +interrupt-names = "gpu";
> 
> Why does it differ from your DTS?

This is just an oversight on our part. We'll make sure they both match in the
next version.

Thanks
Frank

> 
> Best regards,
> Krzysztof
> 

Re: Re: [PATCH v4 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Krzysztof,

On Tue, 2023-07-18 at 13:10 +0200, Krzysztof Kozlowski wrote:
> On 18/07/2023 13:08, Frank Binns wrote:
> > > And this
> > > items:
> > >   - const: gpu
> > > can just be
> > > const: gpu
> > > 
> > > Although, if there is only one interrupt this is probably not
> > > particularly helpful. Are there other implementations of this IP that
> > > have more interrupts?
> > 
> > No, all our current GPUs just have a single interrupt. I assume it's more 
> > future
> > proof to keep the name in case that ever changes? 
> 
> Why do you need name in the first place? If there is single entry, the
> name is pointless, especially if it repeats the name of the IP block.
> 
> > As in, by having the name now
> > we can make it a required property, which I guess we won't be able to do at 
> > some
> > later point.
> 
> Why even making it required?

It seems nicer to look up a resource in the driver based on a name rather than
an index. Happy to drop it though.

Thanks
Frank

> 
> Best regards,
> Krzysztof
> 

Re: Re: [PATCH v4 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Krzysztof,

On Mon, 2023-07-17 at 09:29 +0200, Krzysztof Kozlowski wrote:
> On 14/07/2023 16:25, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> 
> ...
> 
> > +
> > +  clocks:
> > +minItems: 1
> > +maxItems: 3
> > +
> > +  clock-names:
> > +items:
> > +  - const: core
> > +  - const: mem
> > +  - const: sys
> > +minItems: 1
> 
> Why clocks for this device vary? That's really unusual to have a SoC IP
> block which can have a clock physically disconnected, depending on the
> board (not SoC!).

By default, this GPU IP (Series AXE) operates on a single clock (the core
clock), but the SoC vendor can choose at IP integration time to run the memory
and SoC interfaces on separate clocks (mem and sys clocks respectively). We also
have IP, such as the Series 6XT, that requires all 3 clocks.

So the situation here is that Series AXE may have 1 or 3 clocks, but the TI
implementation being added only has 1.

I guess we need to add something like:

  allOf:
- if:
properties:
  compatible:
contains:
  const: ti,am62-gpu
  then:
properties:
  clocks:
maxItems: 1

Or should we be doing something else?

Thanks
Frank

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v4 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Conor,

Thank you for your feedback (comments below).

On Sat, 2023-07-15 at 11:40 +0100, Conor Dooley wrote:
> Hey Sarah,
> 
> Your series does not appear to be threaded. `git send-email` can be
> passed, for example, a directory containing a whole series & will set
> the correct in-reply-to headers so that the series is in a single
> thread.

Thank you pointing this out, we'll make sure we do this for the next iteration.

> 
> On Fri, Jul 14, 2023 at 03:25:26PM +0100, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > Changes since v3:
> > - Remove oneOf in compatible property
> > - Remove power-supply (not used on AM62)
> > 
> > Changes since v2:
> > - Add commit message description
> > - Remove mt8173-gpu support (not currently supported)
> > - Drop quotes from $id and $schema
> > - Remove reg: minItems
> > - Drop _clk suffixes from clock-names
> > - Remove operating-points-v2 property and cooling-cells (not currently
> >   used)
> > - Add additionalProperties: false
> > - Remove stray blank line at the end of file
> 
> The changelog should go below the --- line.

Ack

> 
> > Signed-off-by: Sarah Walker 
> > ---
> >  .../devicetree/bindings/gpu/img,powervr.yaml  | 68 +++
> >  MAINTAINERS   |  7 ++
> >  2 files changed, 75 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
> > b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > new file mode 100644
> > index ..3292a0440465
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2022 Imagination Technologies Ltd.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Imagination Technologies PowerVR GPU
> > +
> > +maintainers:
> > +  - Sarah Walker 
> > +  interrupts:
> > +items:
> > +  - description: GPU interrupt
> 
> The description here doesn't add any value, since there is only one
> interrupt, so you can drop it and do maxItems: 1 as you have done
> elsewhere.

Sure, will make this change.

> 
> > +  interrupt-names:
> > +items:
> > +  - const: gpu
> 
> And this
> items:
>   - const: gpu
> can just be
> const: gpu
> 
> Although, if there is only one interrupt this is probably not
> particularly helpful. Are there other implementations of this IP that
> have more interrupts?

No, all our current GPUs just have a single interrupt. I assume it's more future
proof to keep the name in case that ever changes? As in, by having the name now
we can make it a required property, which I guess we won't be able to do at some
later point.

Thanks
Frank

> 
> Otherwise, this looks good to me.
> 
> Thanks,
> Conor.

Re: [PATCH v3 09/17] drm/imagination: Implement power management

[Frank Binns]

Hi Maxime,

On Fri, 2023-07-07 at 14:48 +0200, Maxime Ripard wrote:
> On Tue, Jun 13, 2023 at 03:47:52PM +0100, Sarah Walker wrote:
> > @@ -503,21 +506,31 @@ pvr_device_init(struct pvr_device *pvr_dev)
> >   if (err)
> >   goto err_device_clk_fini;
> > 
> > + /* Explicitly power the GPU so we can access control registers before 
> > the FW is booted. */
> > + err = pm_runtime_resume_and_get(dev);
> > + if (err)
> > + goto err_device_clk_fini;
> > +
> >   /* Map the control registers into memory. */
> >   err = pvr_device_reg_init(pvr_dev);
> >   if (err)
> > - goto err_device_clk_fini;
> > + goto err_pm_runtime_put;
> > 
> >   /* Perform GPU-specific initialization steps. */
> >   err = pvr_device_gpu_init(pvr_dev);
> >   if (err)
> >   goto err_device_reg_fini;
> > 
> > + pm_runtime_put_autosuspend(dev);
> > +
> 
> You probably can use pm_runtime_put here

Ack

> 
> > @@ -532,12 +545,17 @@ pvr_device_init(struct pvr_device *pvr_dev)
> >  void
> >  pvr_device_fini(struct pvr_device *pvr_dev)
> >  {
> > + struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > + struct device *dev = drm_dev->dev;
> > +
> >   /*
> >* Deinitialization stages are performed in reverse order compared to
> >* the initialization stages in pvr_device_init().
> >*/
> > + pm_runtime_get_sync(dev);
> >   pvr_device_gpu_fini(pvr_dev);
> >   pvr_device_reg_fini(pvr_dev);
> 
> AFAIK gpu_fini releases the firmware and reg_fini drops the register
> mapping address, I don't think you need the device powered up for that.

Very true, we'll fix that :)

> 
> > @@ -130,6 +133,20 @@ struct pvr_device {
> > 
> >   /** @fw_dev: Firmware related data. */
> >   struct pvr_fw_device fw_dev;
> > +
> > + struct {
> > + /** @work: Work item for watchdog callback. */
> > + struct delayed_work work;
> > +
> > + /** @old_kccb_cmds_executed: KCCB command execution count at 
> > last watchdog poll. */
> > + u32 old_kccb_cmds_executed;
> > +
> > + /** @kccb_stall_count: Number of watchdog polls KCCB has been 
> > stalled for. */
> > + u32 kccb_stall_count;
> > + } watchdog;
> > +
> > + /** @lost: %true if the device has been lost. */
> > + bool lost;
> 
> The device being "lost" isn't clear to me. Does it mean it's
> unresponsive or stuck somehow?

The former. For example, this will be set when the firmware has become
unresponsive and hard resetting the GPU doesn't resolve this. We'll update the
docs to clarify.

> 
> > @@ -1285,9 +1303,15 @@ pvr_probe(struct platform_device *plat_dev)
> > 
> >   platform_set_drvdata(plat_dev, drm_dev);
> > 
> > + devm_pm_runtime_enable(_dev->dev);
> > +
> > + pm_runtime_set_autosuspend_delay(_dev->dev, 50);
> > + pm_runtime_use_autosuspend(_dev->dev);
> > + pvr_power_init(pvr_dev);
> 
> The name threw me off a bit. It doesn't look like it's power related but
> you init the watchdog timer?

Yup, we'll update the name to reflect its real purpose.

> 
> I can't really tell from that patch, but if it's not done in a later
> patch you'll probably need a call to sprinkle your driver with a few
> _mark_last_busy calls (at least in the job submission path?)

We weren't doing this before so we'll add some calls in.

Thanks
Frank

> 
> Maxime

Re: [PATCH v3 05/17] drm/imagination: Get GPU resources

[Frank Binns]

Hi Maxime,

On Fri, 2023-07-07 at 14:47 +0200, Maxime Ripard wrote:
> On Tue, Jun 13, 2023 at 03:47:48PM +0100, Sarah Walker wrote:
> > Acquire clock, regulator and register resources, and enable/map as
> > appropriate.
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >  drivers/gpu/drm/imagination/Makefile |   1 +
> >  drivers/gpu/drm/imagination/pvr_device.c | 271 +++
> >  drivers/gpu/drm/imagination/pvr_device.h | 214 ++
> >  drivers/gpu/drm/imagination/pvr_drv.c|  11 +-
> >  4 files changed, 496 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile 
> > b/drivers/gpu/drm/imagination/Makefile
> > index 62ccf0ccbd51..186f920d615b 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -4,6 +4,7 @@
> >  subdir-ccflags-y := -I$(srctree)/$(src)
> > 
> >  powervr-y := \
> > + pvr_device.o \
> >   pvr_drv.o \
> > 
> >  obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c 
> > b/drivers/gpu/drm/imagination/pvr_device.c
> > new file mode 100644
> > index ..790c36cebec1
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#include "pvr_device.h"
> > +
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/**
> > + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->regs.
> > + *
> > + * This method of mapping the device control registers into memory ensures 
> > that
> > + * they are unmapped when the driver is detached (i.e. no explicit cleanup 
> > is
> > + * required).
> > + *
> > + * Return:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_platform_ioremap_resource().
> > + */
> > +static int
> > +pvr_device_reg_init(struct pvr_device *pvr_dev)
> > +{
> > + struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > + struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> > + struct resource *regs_resource;
> > + void __iomem *regs;
> > + int err;
> > +
> > + pvr_dev->regs_resource = NULL;
> > + pvr_dev->regs = NULL;
> > +
> > + regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, 
> > _resource);
> > + if (IS_ERR(regs)) {
> > + err = PTR_ERR(regs);
> > + drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> > + err);
> > + return err;
> > + }
> > +
> > + pvr_dev->regs = regs;
> > + pvr_dev->regs_resource = regs_resource;
> 
> Do you actually need the resources somewhere?

This is needed when setting up the boot data for the MIPS firmware processor in
patch 11 (drm/imagination: Implement MIPS firmware processor and MMU support).
We'll move it to that patch instead.

> 
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * This is essentially a no-op, since pvr_device_reg_init() already 
> > ensures that
> > + * struct pvr_device->regs is unmapped when the device is detached. This
> > + * function just sets struct pvr_device->regs to %NULL.
> > + */
> > +static __always_inline void
> > +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> > +{
> > + pvr_dev->regs = NULL;
> 
> But if you do, I guess clearing the regs_resource pointer would be nice too.

pvr_device_reg_fini() will be gone in the next version.

> 
> > +}
> > +
> > +/**
> > + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> > + * struct pvr_device->mem_clk.
> > + *
> > + * Three clocks are required by the PowerVR device: core, sys and mem. On
> > + * return, this function guarantees that the clocks are in one of the 
> > following
> > + * states:
> > + *
> > + *  * All successfully initialized,
> > + *  * Core errored, sys and mem uninitialized,
> > + *  * Core deinitialized, sys errored, mem uninitialized, or
> > + *  * Core and sys deinitialized, mem errored.
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by devm_clk_get(), or
> > + *  * Any error returned by clk_prepare_enable().
> > + */
> > +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> > +{
> > + struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > + struct clk *core_clk;
> > + struct clk *sys_clk;
> 

Re: [PATCH v3 04/17] drm/imagination: Add skeleton PowerVR driver

[Frank Binns]

Hi Maxime,

Thank you for your feedback (comments below).

On Fri, 2023-07-07 at 14:46 +0200, Maxime Ripard wrote:
> Hi,
> 
> [I just noticed I dropped the Cc list, resending]
> 
> Thanks for contributing this driver, it's awesome to see it moving
> forward.
> 
> And congrats on the documentation too, it's not often we see a driver
> that well documented on its v3.
> 
> I've stripped some parts of the patch that weren't relevant to my
> review.
> 
> On Tue, Jun 13, 2023 at 03:47:47PM +0100, Sarah Walker wrote:
> > +static __always_inline struct pvr_device *
> > +to_pvr_device(struct drm_device *drm_dev)
> > +{
> > + return container_of(drm_dev, struct pvr_device, base);
> > +}
> 
> For that kind of helpers, we're slowly transitioning to using a macro
> and container_of_const. This allows to work with const-ness context.

Ack

> 
> > +static int
> > +pvr_probe(struct platform_device *plat_dev)
> > +{
> > + struct pvr_device *pvr_dev;
> > + struct drm_device *drm_dev;
> > + int err;
> > +
> > + pvr_dev = devm_drm_dev_alloc(_dev->dev, _drm_driver,
> > +  struct pvr_device, base);
> > + if (IS_ERR(pvr_dev)) {
> > + err = IS_ERR(pvr_dev);
> 
> PTR_ERR?

Good catch :)

> 
> > + goto err_out;
> 
> The general pattern here is to return directly here, it's simpler to
> follow.

Ack

> 
> > + }
> > + drm_dev = _dev->base;
> > +
> > + platform_set_drvdata(plat_dev, drm_dev);
> > +
> > + err = drm_dev_register(drm_dev, 0);
> > + if (err)
> > + goto err_out;
> 
> I guess it would be simpler here too, but I think you're going to move
> things around anyway?
> 
> > +static const struct of_device_id dt_match[] = {
> > + { .compatible = "ti,am62-gpu", .data = NULL },
> > + { .compatible = "img,powervr-seriesaxe", .data = NULL },
> 
> Do you really need both? The binding you documented requires both to be
> there, so I think you can either match on the more specific one (and
> extend that list when needed) or match the more generic one and be done
> with it once and for all. Having both is redundant.

We'll drop the more specific one in the next version.

> 
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, dt_match);
> > +
> > +static struct platform_driver pvr_driver = {
> > + .probe = pvr_probe,
> > + .remove = pvr_remove,
> > + .driver = {
> > + .name = PVR_DRIVER_NAME,
> > + .of_match_table = dt_match,
> > + },
> > +};
> > +module_platform_driver(pvr_driver);
> > +
> > +MODULE_AUTHOR("Imagination Technologies Ltd.");
> > +MODULE_DESCRIPTION(PVR_DRIVER_DESC);
> > +MODULE_LICENSE("Dual MIT/GPL");
> > +MODULE_IMPORT_NS(DMA_BUF);
> > +MODULE_FIRMWARE("powervr/rogue_4.40.2.51_v1.fw");
> > +MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");
> 
> That one should probably be moved to the firmware patch?

Ack

> 
> > diff --git a/drivers/gpu/drm/imagination/pvr_drv.h 
> > b/drivers/gpu/drm/imagination/pvr_drv.h
> > new file mode 100644
> > index ..8e6f4a4dde3f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_drv.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#ifndef PVR_DRV_H
> > +#define PVR_DRV_H
> > +
> > +#include "linux/compiler_attributes.h"
> > +#include 
> > +
> > +#define PVR_DRIVER_NAME "powervr"
> > +#define PVR_DRIVER_DESC "Imagination PowerVR Graphics"
> 
> Do you intend to support the SGX and Rogue GPUs with this driver? If
> not, mentioning the generation/architecture name somewhere would be
> nice.

We don't currently have any plans to support SGX ourselves, so we'll update
this, and any other places, to clarify things.

Thanks
Frank

> 
> Maxime

Re: Re: [PATCH v3 07/17] drm/imagination: Add GPU ID parsing and firmware loading

[Frank Binns]

On Mon, 2023-06-26 at 10:38 -0500, Adam Ford wrote:
> On Mon, Jun 26, 2023 at 8:22 AM Frank Binns  wrote:
> > Hi Adam,
> > 
> > On Sat, 2023-06-17 at 07:48 -0500, Adam Ford wrote:
> > > On Tue, Jun 13, 2023 at 10:20 AM Sarah Walker  
> > > wrote:
> > > > Read the GPU ID register at probe time and select the correct
> > > > features/quirks/enhancements. Use the GPU ID to form the firmware
> > > > file name and load the firmware.
> > > 
> > > I have a Rogue 6250 variant, but the BVNC is returning a slightly
> > > different revision than the firmware that's currently available, and
> > > the older firmware for the vendor driver doesn't work with this new
> > > code.
> > > 
> > > Linux responds with Unsupported BVNC: 4.45.2.58.  From what I can
> > > tell, the closest available firmware is 4.40.2.51.
> > > 
> > > Will there be more firmware variants in the future or will there be
> > > some options to build the firmware somehow?
> > 
> > We don't plan to support the SoC vendor provided firmware binaries as this 
> > will
> > mean having to deal with many different versions of the firmware and its
> > interface. Specifically, the interface can change in backwards incompatible 
> > ways
> > between releases, it changes based on the hardware feature set & bugs and 
> > it's
> > split across userspace & the kernel. This makes these SoC provided firmware
> > binaries very difficult to support in this new driver.
> 
> Thanks for the response.
> 
> That makes sense.  I would hope that various SoC vendors would jump on
> the bandwagon to work with your group to get their hardware supported.
> > As an alternative, we'll be releasing firmware binaries as we add support 
> > for
> > more Rogue GPUs. We'll also release binaries upon request, in case others 
> > in the
> > community want to work on support in the meantime - we're just getting 
> > things
> > set up for this at the moment.
> 
> The Mesa side of things appears to be missing some documentation, and
> the power VR stuff still appears listed as experimental.  Is there
> some documentation somewhere that would explain to someone how to go
> about porting the Rogue 6250 to a different hardware variant of the
> 6250?  I don't really know the difference between BVNC of 4.45.2.58
> and 4.40.2.51, but I can't imagine they are drastically different.

One thing I forgot to mention is that, alongside the firmware binaries, we'll
also provide the corresponding device info, e.g. for Mesa:
https://gitlab.freedesktop.org/mesa/mesa/-/blob/e714b35301a33145399f8939ca864ffd14b49de9/src/imagination/common/pvr_device_info.c#L32-125

We don't have any specific porting documentation, but we did just send out a
Mesa MR adding some initial (basic) documentation:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23992

In terms of differences between the two GX6250 variants, it doesn't seem that
there's anything feature-wise that will require any driver changes that are
specific to the 4.45.2.58 variant (the different firmware should in theory be
sufficient). There are still some driver changes required, however.

Assuming the SoC you're interested in is already well supported upstream and the
clocks, power controllers, etc needed by the GPU are also already supported then
the following changes will be required at a minimum:

1. A GPU node will need adding to the device tree source file for your specific
   board
2. The compatible string for the GPU node will need adding to the list of
   supported devices in the kernel driver (grep for "dt_match" in the driver
   code)
3. The device info we provide alongside the firmware binary will need adding to
   the kernel driver and Mesa
4. The compatible string for the GPU and display controller device tree nodes
   will need adding to the Vulkan driver (grep for "pvr_drm_configs" in the Mesa
   code to see existing examples)
   
Hopefully that covers everything, but no doubt I missed something!

With respect to the experimental status of the driver, I think there are three
things that need to happen before we can drop this tag. Firstly, the kernel
driver needs to be merged to the kernel. Secondly, we need to pass Khronos
conformance on at least one of the devices we support (our current focus is on
the AXE-1-16M). Finally, we need to upstream all our Mesa changes. This is
something that we've been chipping away at, but we do have a big backlog in our
public branch [1]. I expect it's going to be quite some time until all of this
work is complete.

While so much code is sitting in downstream branches I think it's going to be
somewhat painful for people to meaningfully contribute to the drive

Re: [PATCH v3 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Linus,

On Fri, 2023-06-16 at 14:48 +0200, Linus Walleij wrote:
> Hi Sarah,
> 
> thanks for your patch!
> 
> On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker  wrote:
> 
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Signed-off-by: Sarah Walker 
> > +properties:
> > +  compatible:
> > +oneOf:
> > +  - items:
> > +  - enum:
> > +  - ti,am62-gpu
> > +  - const: img,powervr-seriesaxe
> 
> I don't see why you need to restrict the bindings to just the stuff
> you happen to
> be writing Linux drivers for right now.
> 
> Add all of them!

The main thinking here was to start off with a single simple case (TI AM62) to
support the initial driver upstreaming rather than trying to cover too many
things at once. This can then be built upon for other GPUs. So, for example, we
can extend the bindings to add a second power domain for those that need it,
such as the GPU in the Mediatek MT8173. The benefit of this approach is that the
bindings, dts and code changes can all be reviewed together so that all the
context is present.

> 
> There is this out-of-tree binding:
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/0ddd308a78926782b8a72f75c74b91417ceb9779
> 
> With these two on top:
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/053346e1933932815066f16ebf6e6bda45d67548
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/1cb62c4cdcad096d438ee7d1d51f6001998acee3
> 
> They are indeed out-of-tree, but H. Nikolaus is a well-known and respected
> contributor to the kernel, and I'm pretty sure he would be contributing
> these upstream if he had the time and incentive.
> 
> To me it seems much more like you should talk to Nikolaus about submitting
> these patches initially, and then add Rogue support with patches on top of it.
> It could be a nice series with just DT bindings.
> 
> I see that your binding uses a power domain rather than a regulator
> (sgx-supply) for power and that is probably a better approach but other
> than that the openpvrsgx binding looks more complete and to the point?
> 
> It will also help them to get these bindings settled so they can merge all
> of the DTS patches adding the SGX block to misc platforms in the
> kernel upstream so they can focus their work on the actual driver.
> 
> When I look at the PowerVR wikipedia page:
> https://en.wikipedia.org/wiki/PowerVR
> there is no correspondence between the product names there and
> "img,powervr-seriesaxe" as used here.
> 
> I think you need to explain if these are internal product names or what
> is going on, and what the relationship is to the marketing names, it could
> be part of the description simply, so that people know what string to use
> somewhat intuitively. Maybe all the strings in the out-of-tree documentation
> are just wrong because internal code names need to be used?

It's been quite some time since we originally wrote these bindings, so we'll
need to go back and check why we settled on "powervr-seriesaxe", but I suspect
it was just a case of us normalising the naming across different series.

Thanks
Frank

> 
> Yours,
> Linus Walleij

Re: [PATCH v3 00/17] Imagination Technologies PowerVR DRM driver

[Frank Binns]

Hi Nikolaus,

On Fri, 2023-06-16 at 16:06 +0200, H. Nikolaus Schaller wrote:
> Hi Linus,
> thanks for sharing this conversation with me.
> 
> > Am 16.06.2023 um 14:29 schrieb Linus Walleij :
> > 
> > Hi Sarah,
> > 
> > thanks for starting this long awaited work!
> > 
> > On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker  
> > wrote:
> > 
> > > This patch series adds the initial DRM driver for Imagination 
> > > Technologies PowerVR
> > > GPUs, starting with those based on our Rogue architecture. It's worth 
> > > pointing
> > > out that this is a new driver, written from the ground up, rather than a
> > > refactored version of our existing downstream driver (pvrsrvkm).
> 
> Great!
> 
> > This seems to be a fairly good starting point, a bit of trade-off
> > between latest-and-greatest
> > and recent enough devices that need aftermarket support.
> > 
> > I assume you are aware of the community existing around Series 5
> > (should be the immediate
> > predecessor to Rogue?):
> > https://github.com/openpvrsgx-devgroup/linux_openpvrsgx
> > 
> > I don't know how active those people are these days, but I can see that a 
> > branch
> 
> Well, there hasn't been much progress due to lack of documentation and 
> compatibility
> issues of user-space code. Just keeping the code compatible to latest 
> upstream kernels.
> 
> But at least for OMAP3 and OMAP5 processors people are actively using this 
> work.
> 
> There is even a gaming console (www.pyra-handheld.com) in active production
> with a strong need for an up-to-date SGX544 driver running on OMAP5.
> 
> > was updated for v6.4-rc3 just three weeks ago.
> > https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/tree/pvrsrvkm-6.4-rc3
> > 
> > I think it would be good for community building to make sure that you get 
> > these
> > people involved in reviewing, especially neutral stuff like device tree 
> > bindings
> > but also to make sure no architectural choices are done that will make it 
> > hard
> > to retrofit a proper driver for the older engines if this community
> > decide to work
> > on it.
> 
> Some questions to the author of the new driver:
> - are there plans to support SGX5 (the predecessor of Rogue6)?

We don't currently have any plans to support SGX. Our main focus is currently on
Rogue and then we'll move onto Volcanic.

> - will this be able to run the existing firmware and user-space code of 
> pvrsrvkm?

I'm afraid not. The interface for existing firmware and userspace code were
designed with different requirements in mind and don't cater to the kernel's
strict compatibility guarantees. As such, the uapi for this new driver is
very different to pvrsrvkm's, although naturally there are similarities:
https://gitlab.freedesktop.org/sarah-walker-imgtec/powervr/-/blob/dev/v3/include/uapi/drm/pvr_drm.h

We've also worked with our firmware team to make changes to the firmware
interface to better support this new driver. Specifically, parts of the firmware
interface are no longer conditional on the GPU feature set / hardware
workarounds, meaning we now have a single interface which can work across all
existing Rogue GPUs, which makes things a lot easier for this new kernel driver.

> - or will it have new firmware and user-space code for these older chips?
> - or will there be open user-space code for SGX (and Rogue)?

We're using the same Rogue firmware as our closed source driver, just with
modifications to the interface to cater for this new kernel driver. In terms of
userspace, we already have a Vulkan driver upstreamed to Mesa:
https://gitlab.freedesktop.org/mesa/mesa/-/tree/main/src/imagination/vulkan

and will be working to enable GLES support via Mesa's Zink GL(ES)-to-Vulkan
translation layer. This naturally limits support to Series 6 onwards, as
anything older isn't capable of supporting Vulkan.

> 
> > Specifically I would ask that the DT bindings include all old and new 
> > PowerVR
> > hardware in one go, unless they have very specific hardware definition 
> > needs,
> > which I doubt.
> 
> Our current bindings for all SoC with a SGX5 GPU inside and which have at 
> least
> some official Linux support are here:
> 
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/blob/letux/omap-pvr-soc-glue-v8/Documentation/devicetree/bindings/gpu/img%2Cpvrsgx.yaml
> 
> There was an attempt to upstream at least this plus glue code (i.e. device 
> tree
> sources) some years ago but there was no consensus about the number and names 
> of
> clocks that should be included in such a bindings document.

I've taken a look and your bindings look very similar to the ones we've come up
with. If you decide to attempt to upstream these again, please feel free to CC
me, Sarah and Donald (all on this email chain) so we can provide some feedback.

> 
> > Also I think they could use your help to get the proper firmware for the 
> > older
> > hardware licensed properly from Imagination and included into linux-firmware
> > so they do not need to ship files on the side.

Re: [PATCH v3 00/17] Imagination Technologies PowerVR DRM driver

[Frank Binns]

Hi Linus,

On Fri, 2023-06-16 at 14:29 +0200, Linus Walleij wrote:
> Hi Sarah,
> 
> thanks for starting this long awaited work!
> 
> On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker  wrote:
> 
> > This patch series adds the initial DRM driver for Imagination Technologies 
> > PowerVR
> > GPUs, starting with those based on our Rogue architecture. It's worth 
> > pointing
> > out that this is a new driver, written from the ground up, rather than a
> > refactored version of our existing downstream driver (pvrsrvkm).
> 
> This seems to be a fairly good starting point, a bit of trade-off
> between latest-and-greatest
> and recent enough devices that need aftermarket support.
> 
> I assume you are aware of the community existing around Series 5
> (should be the immediate
> predecessor to Rogue?):
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx

Actually we were unaware of this community, so thank you for pointing it out.

> 
> I don't know how active those people are these days, but I can see that a 
> branch
> was updated for v6.4-rc3 just three weeks ago.
> https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/tree/pvrsrvkm-6.4-rc3
> 
> I think it would be good for community building to make sure that you get 
> these
> people involved in reviewing, especially neutral stuff like device tree 
> bindings
> but also to make sure no architectural choices are done that will make it hard
> to retrofit a proper driver for the older engines if this community
> decide to work
> on it.

On the face of it, I'd imagine that it will make more sense for SGX to have its
own driver, just because it's different enough to require a different
design. For example, Series6 onwards uses a completely different firmware to
SGX/Series5. Another possible approach might be to share code between this
driver and a future SGX driver by extracting code out into a library. Of course,
we won't know what code to extract, if any, until someone starts working on
upstream SGX support.

> 
> Specifically I would ask that the DT bindings include all old and new PowerVR
> hardware in one go, unless they have very specific hardware definition needs,
> which I doubt.

I'll comment about this on the other thread.

> 
> Also I think they could use your help to get the proper firmware for the older
> hardware licensed properly from Imagination and included into linux-firmware
> so they do not need to ship files on the side.

Sure, we can do this. I've already got approval for the existing SGX firmware to
use the same license as the Rogue firmware, which can be found here:
https://gitlab.freedesktop.org/frankbinns/linux-firmware/-/blob/powervr/LICENSE.powervr

I'll speak to Nikolaus about next steps.

Thanks
Frank

> 
> Yours,
> Linus Walleij

Re: [PATCH v3 01/17] sizes.h: Add entries between 32G and 64T

[Frank Binns]

Hi Linus,

On Fri, 2023-06-16 at 14:10 +0200, Linus Walleij wrote:
> On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker  wrote:
> 
> > From: Matt Coster 
> > 
> > Signed-off-by: Matt Coster 
> 
> Looks useful.
> Reviewed-by: Linus Walleij 

Thank you for the review!

Frank

> 
> Yours,
> Linus Walleij

Re: [PATCH v3 07/17] drm/imagination: Add GPU ID parsing and firmware loading

[Frank Binns]

Hi Adam,

On Sat, 2023-06-17 at 07:48 -0500, Adam Ford wrote:
> On Tue, Jun 13, 2023 at 10:20 AM Sarah Walker  wrote:
> > Read the GPU ID register at probe time and select the correct
> > features/quirks/enhancements. Use the GPU ID to form the firmware
> > file name and load the firmware.
> 
> I have a Rogue 6250 variant, but the BVNC is returning a slightly
> different revision than the firmware that's currently available, and
> the older firmware for the vendor driver doesn't work with this new
> code.
> 
> Linux responds with Unsupported BVNC: 4.45.2.58.  From what I can
> tell, the closest available firmware is 4.40.2.51.
> 
> Will there be more firmware variants in the future or will there be
> some options to build the firmware somehow?

We don't plan to support the SoC vendor provided firmware binaries as this will
mean having to deal with many different versions of the firmware and its
interface. Specifically, the interface can change in backwards incompatible ways
between releases, it changes based on the hardware feature set & bugs and it's
split across userspace & the kernel. This makes these SoC provided firmware
binaries very difficult to support in this new driver.

As an alternative, we'll be releasing firmware binaries as we add support for
more Rogue GPUs. We'll also release binaries upon request, in case others in the
community want to work on support in the meantime - we're just getting things
set up for this at the moment.

Thanks
Frank

> 
> adam
> 
> 
> 
> 
> > The features/quirks/enhancements arrays are currently hardcoded in
> > the driver for the supported GPUs. We are looking at moving this
> > information to the firmware image.
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >  drivers/gpu/drm/imagination/Makefile  |   1 +
> >  drivers/gpu/drm/imagination/pvr_device.c  | 359 
> >  drivers/gpu/drm/imagination/pvr_device.h  | 221 +++
> >  drivers/gpu/drm/imagination/pvr_device_info.c | 223 +++
> >  drivers/gpu/drm/imagination/pvr_device_info.h | 133 +
> >  drivers/gpu/drm/imagination/pvr_drv.c | 553 +-
> >  drivers/gpu/drm/imagination/pvr_drv.h | 108 
> >  drivers/gpu/drm/imagination/pvr_fw.h  |  20 +
> >  8 files changed, 1617 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_device_info.c
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_device_info.h
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_fw.h
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile 
> > b/drivers/gpu/drm/imagination/Makefile
> > index 186f920d615b..d713b1280776 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -5,6 +5,7 @@ subdir-ccflags-y := -I$(srctree)/$(src)
> > 
> >  powervr-y := \
> > pvr_device.o \
> > +   pvr_device_info.o \
> > pvr_drv.o \
> > 
> >  obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c 
> > b/drivers/gpu/drm/imagination/pvr_device.c
> > index 790c36cebec1..2e03763f2eb7 100644
> > --- a/drivers/gpu/drm/imagination/pvr_device.c
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -2,20 +2,32 @@
> >  /* Copyright (c) 2022 Imagination Technologies Ltd. */
> > 
> >  #include "pvr_device.h"
> > +#include "pvr_device_info.h"
> > +
> > +#include "pvr_fw.h"
> > +#include "pvr_rogue_cr_defs.h"
> > 
> >  #include 
> > 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> > +#include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +
> > +/* Major number for the supported version of the firmware. */
> > +#define PVR_FW_VERSION_MAJOR 1
> > 
> >  /**
> >   * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> > @@ -205,6 +217,246 @@ pvr_device_regulator_init(struct pvr_device *pvr_dev)
> > return err;
> >  }
> > 
> > +/**
> > + * pvr_device_clk_core_get_freq - Get current PowerVR device core clock 
> > frequency
> > + * @pvr_dev: Target PowerVR device.
> > + * @freq_out: Pointer to location to store core clock frequency in Hz.
> > + *
> > + * Returns:
> > + *  * 0 on success, or
> > + *  * -%EINVAL if frequency can not be determined.
> > + */
> > +int
> > +pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out)
> > +{
> > +   u32 freq = clk_get_rate(pvr_dev->core_clk);
> > +
> > +   if (!freq)
> > +   return -EINVAL;
> > +
> > +   *freq_out = freq;
> > +   return 0;
> > +}
> > +
> > +/**
> > + * pvr_build_firmware_filename() - Construct a PowerVR firmware filename
> > + * @pvr_dev: Target PowerVR device.
> > + * @base: First part of the filename.
> > + * @major: Major version number.
> > + *
> > + * A PowerVR firmware filename consists of three parts separated by 
> > underscores
> > + * (``'_'``) along with a '.fw' file suffix. The 

Re: [PATCH v3 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Andrew,

Thank you for your feedback (comments below).

On Tue, 2023-06-13 at 12:38 -0500, Andrew Davis wrote:
> On 6/13/23 9:47 AM, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >   .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++
> >   MAINTAINERS   |  7 ++
> >   2 files changed, 78 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpu/img,powervr.yaml 
> > b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > new file mode 100644
> > index ..652343876d1c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > @@ -0,0 +1,71 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2022 Imagination Technologies Ltd.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpu/img,powervr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Imagination Technologies PowerVR GPU
> > +
> > +maintainers:
> > +  - Sarah Walker 
> > +
> > +properties:
> > +  compatible:
> > +oneOf:
> 
> oneOf shouldn't be needed, you can just do the enum followed by const.

Sure, we'll make this change.

> 
> > +  - items:
> > +  - enum:
> > +  - ti,am62-gpu
> > +  - const: img,powervr-seriesaxe
> > +
> > +  reg:
> > +maxItems: 1
> > +
> > +  clocks:
> > +minItems: 1
> > +maxItems: 3
> > +
> > +  clock-names:
> > +items:
> > +  - const: core
> > +  - const: mem
> > +  - const: sys
> > +minItems: 1
> > +
> > +  interrupts:
> > +items:
> > +  - description: GPU interrupt
> > +
> > +  interrupt-names:
> > +items:
> > +  - const: gpu
> > +
> > +  power-domains:
> > +maxItems: 1
> > +
> > +  power-supply: true
> 
> Why do you need power-supply?

We don't need this, so will remove it in the next iteration.

Thanks
Frank

> 
> Andrew
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +#include 
> > +#include 
> > +
> > +gpu: gpu@fd0 {
> > +compatible = "ti,am62-gpu", "img,powervr-seriesaxe";
> > +reg = <0x0fd0 0x2>;
> > +power-domains = <_pds 187>;
> > +clocks = <_clks 187 0>;
> > +clock-names = "core";
> > +interrupts = ;
> > +interrupt-names = "gpu";
> > +};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b344e1318ac3..a41517843a10 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10084,6 +10084,13 @@ IMGTEC IR DECODER DRIVER
> >   S:Orphan
> >   F:drivers/media/rc/img-ir/
> >   
> > +IMGTEC POWERVR DRM DRIVER
> > +M: Frank Binns 
> > +M: Sarah Walker 
> > +M: Donald Robson 
> > +S: Supported
> > +F: Documentation/devicetree/bindings/gpu/img,powervr.yaml
> > +
> >   IMON SOUNDGRAPH USB IR RECEIVER
> >   M:Sean Young 
> >   L:linux-me...@vger.kernel.org

Re: [PATCH v3 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Rob,

On Thu, 2023-06-15 at 14:50 -0600, Rob Herring wrote:
> On Tue, Jun 13, 2023 at 9:20 AM Sarah Walker  wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >  .../devicetree/bindings/gpu/img,powervr.yaml  | 71 +++
> >  MAINTAINERS   |  7 ++
> >  2 files changed, 78 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpu/img,powervr.yaml
> 
> Please use get_maintainers.pl and send your patches to the correct
> people and lists or they won't get reviewed.

Thank you for pointing this out. We'll make sure we do this for the next
iteration of the patches.

Thanks
Frank

> 
> Rob

Re: [PATCH v3 05/17] drm/imagination: Get GPU resources

[Frank Binns]

Hi Andrew,

On Tue, 2023-06-13 at 13:12 -0500, Andrew Davis wrote:
> On 6/13/23 9:47 AM, Sarah Walker wrote:
> > Acquire clock, regulator and register resources, and enable/map as
> > appropriate.
> > 
> > Signed-off-by: Sarah Walker 
> > ---
> >   drivers/gpu/drm/imagination/Makefile |   1 +
> >   drivers/gpu/drm/imagination/pvr_device.c | 271 +++
> >   drivers/gpu/drm/imagination/pvr_device.h | 214 ++
> >   drivers/gpu/drm/imagination/pvr_drv.c|  11 +-
> >   4 files changed, 496 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile 
> > b/drivers/gpu/drm/imagination/Makefile
> > index 62ccf0ccbd51..186f920d615b 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -4,6 +4,7 @@
> >   subdir-ccflags-y := -I$(srctree)/$(src)
> >   
> >   powervr-y := \
> > +   pvr_device.o \
> > pvr_drv.o \
> >   
> >   obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c 
> > b/drivers/gpu/drm/imagination/pvr_device.c
> > new file mode 100644
> > index ..790c36cebec1
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#include "pvr_device.h"
> > +
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/**
> > + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->regs.
> > + *
> > + * This method of mapping the device control registers into memory ensures 
> > that
> > + * they are unmapped when the driver is detached (i.e. no explicit cleanup 
> > is
> > + * required).
> > + *
> > + * Return:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_platform_ioremap_resource().
> > + */
> > +static int
> > +pvr_device_reg_init(struct pvr_device *pvr_dev)
> > +{
> > +   struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +   struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> > +   struct resource *regs_resource;
> > +   void __iomem *regs;
> > +   int err;
> > +
> > +   pvr_dev->regs_resource = NULL;
> > +   pvr_dev->regs = NULL;
> > +
> > +   regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, 
> > _resource);
> > +   if (IS_ERR(regs)) {
> > +   err = PTR_ERR(regs);
> > +   drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> > +   err);
> > +   return err;
> > +   }
> > +
> > +   pvr_dev->regs = regs;
> > +   pvr_dev->regs_resource = regs_resource;
> > +
> > +   return 0;
> > +}
> > +
> > +/**
> > + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * This is essentially a no-op, since pvr_device_reg_init() already 
> > ensures that
> > + * struct pvr_device->regs is unmapped when the device is detached. This
> > + * function just sets struct pvr_device->regs to %NULL.
> > + */
> > +static __always_inline void
> > +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> > +{
> > +   pvr_dev->regs = NULL;
> 
> This function isn't needed, kinda defeats the purpose of using devm_*()
> if you go an manually have no-op functions to unwind it..

Thank you for the suggestions. We'll fix this, as well as your other suggestions
below.

Thanks
Frank

> 
> > +}
> > +
> > +/**
> > + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> > + * struct pvr_device->mem_clk.
> > + *
> > + * Three clocks are required by the PowerVR device: core, sys and mem. On
> > + * return, this function guarantees that the clocks are in one of the 
> > following
> > + * states:
> > + *
> > + *  * All successfully initialized,
> > + *  * Core errored, sys and mem uninitialized,
> > + *  * Core deinitialized, sys errored, mem uninitialized, or
> > + *  * Core and sys deinitialized, mem errored.
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by devm_clk_get(), or
> > + *  * Any error returned by clk_prepare_enable().
> > + */
> > +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> > +{
> > +   struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +   struct clk *core_clk;
> > +   struct clk *sys_clk;
> > +   struct clk *mem_clk;
> > +   int err;
> > +
> > +   pvr_dev->core_clk = NULL;
> > +   pvr_dev->sys_clk = NULL;
> > +   pvr_dev->mem_clk = NULL;
> 
> You could NULL these out on the error path, but is 

Re: [PATCH v3 02/17] dt-bindings: gpu: Add Imagination Technologies PowerVR GPU

[Frank Binns]

Hi Krzysztof,

On Tue, 2023-06-13 at 20:24 +0200, Krzysztof Kozlowski wrote:
> On 13/06/2023 16:47, Sarah Walker wrote:
> > Add the device tree binding documentation for the Series AXE GPU used in
> > TI AM62 SoCs.
> > 
> 
> I don't see improvements. That's a NAK :(
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 

Apologies for not including a change log for this patch and for not highlighting
that we'd made changes in this area in the covering letter as well. This was an
oversight on our part.

The change log for this patch is as follows:
* Added commit message description
* Dropped quotes from $id and $schema
* Dropped reg minItems
* Dropped _clk suffixes from clock-names
* Dropped operating-points-v2 property
* Added missing additionalProperties:false
* Removed stray blank line at end of file

We'll be sure to include this information going forwards.

We've also run 'make dt_binding_check' and there are no reported issues that we
can see.

As far as I'm aware, this should cover all your feedback on the previous version
of the patch. Of course, we may have missed something or unintentionally
introduced more issues. We'd really appreciate if you can highlight anything
else that needs fixing and we'll make sure we address it in the next iteration
and/or respond to individual points where necessary.

Thank you for your feedback so far.

Best regards,
Frank

> Thank you.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH libdrm] xf86drm: Fix operator precedence

[Frank Binns]

Reviewed-by: Frank Binns 

Thierry Reding  writes:

> From: Thierry Reding 
>
> The array subscription operator ([]) has higher precedence than the
> indirection operator (*), so we need to use parentheses to properly
> instruct the compiler to dereference the pointer to an array first,
> and then subscript into the array.
>
> Fixes a crash observed on Tegra.
>
> Signed-off-by: Thierry Reding 
> ---
>  xf86drm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index d006bb38f800..5de37083c9a3 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3606,14 +3606,14 @@ static int drmParseOFDeviceInfo(int maj, int min, 
> char ***compatible)
>  free(value);
>  }
>  
> -*compatible[i] = tmp_name;
> +(*compatible)[i] = tmp_name;
>  }
>  
>  return 0;
>  
>  free:
>  while (i--)
> -free(*compatible[i]);
> +free((*compatible)[i]);
>  
>  free(*compatible);
>  return err;

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drmMode: merge successive mutually-exclusive #ifs

[Frank Binns]

Reviewed-by: Frank Binns <frank.bi...@imgtec.com>

Eric Engestrom <eric.engest...@imgtec.com> writes:

> Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
> ---
>  xf86drmMode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xf86drmMode.c b/xf86drmMode.c
> index e010547602c6f38ba5a0..6111f477ba0ee25c1f05 100644
> --- a/xf86drmMode.c
> +++ b/xf86drmMode.c
> @@ -829,8 +829,7 @@ int drmCheckModesettingSupported(const char *busid)
>   }
>  #elif defined(__DragonFly__)
>   return 0;
> -#endif
> -#ifdef __OpenBSD__
> +#elif defined(__OpenBSD__)
>   int fd;
>   struct drm_mode_card_res res;
>   drmModeResPtr r = 0;

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drmMode: merge successive mutually-exclusive #ifs

[Frank Binns]

Reviewed-by: Frank Binns <frank.bi...@imgtec.com>

Eric Engestrom <eric.engest...@imgtec.com> writes:

> Signed-off-by: Eric Engestrom <eric.engest...@imgtec.com>
> ---
>  xf86drmMode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xf86drmMode.c b/xf86drmMode.c
> index e010547602c6f38ba5a0..6111f477ba0ee25c1f05 100644
> --- a/xf86drmMode.c
> +++ b/xf86drmMode.c
> @@ -829,8 +829,7 @@ int drmCheckModesettingSupported(const char *busid)
>   }
>  #elif defined(__DragonFly__)
>   return 0;
> -#endif
> -#ifdef __OpenBSD__
> +#elif defined(__OpenBSD__)
>   int fd;
>   struct drm_mode_card_res res;
>   drmModeResPtr r = 0;

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] dma-fence: remove duplicate word in comment

[Frank Binns]

Signed-off-by: Frank Binns <frank.bi...@imgtec.com>
---
 include/linux/dma-fence.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index ca97422..efdabbb 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -128,7 +128,7 @@ struct dma_fence_cb {
  * implementation know that there is another driver waiting on
  * the signal (ie. hw->sw case).
  *
- * This function can be called called from atomic context, but not
+ * This function can be called from atomic context, but not
  * from irq context, so normal spinlocks can be used.
  *
  * A return value of false indicates the fence already passed,
-- 
2.7.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Fix locking cargo-cult in encoder/plane init/cleanup

[Frank Binns]

On 29/11/16 09:45, Daniel Vetter wrote:
> Encoders can't be hotplugged, we dont need looking for this

s/looking/locking/

With that changed:
Reviewed-by: Frank Binns 

> since it's all single-threaded driver setup/teardown code. CRTCs
> already don't grab locks.
>
> While at it I noticed that plane's are missing the
> drm_modeset_lock_fini() call, so add it.
>
> Signed-off-by: Daniel Vetter 
> ---
>   drivers/gpu/drm/drm_encoder.c | 9 +
>   drivers/gpu/drm/drm_plane.c   | 4 ++--
>   2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 5c067719164d..992879f15f23 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -110,11 +110,9 @@ int drm_encoder_init(struct drm_device *dev,
>   {
>   int ret;
>   
> - drm_modeset_lock_all(dev);
> -
>   ret = drm_mode_object_get(dev, >base, DRM_MODE_OBJECT_ENCODER);
>   if (ret)
> - goto out_unlock;
> + return ret;
>   
>   encoder->dev = dev;
>   encoder->encoder_type = encoder_type;
> @@ -142,9 +140,6 @@ int drm_encoder_init(struct drm_device *dev,
>   if (ret)
>   drm_mode_object_unregister(dev, >base);
>   
> -out_unlock:
> - drm_modeset_unlock_all(dev);
> -
>   return ret;
>   }
>   EXPORT_SYMBOL(drm_encoder_init);
> @@ -164,12 +159,10 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>* the indices on the drm_encoder after us in the encoder_list.
>*/
>   
> - drm_modeset_lock_all(dev);
>   drm_mode_object_unregister(dev, >base);
>   kfree(encoder->name);
>   list_del(>head);
>   dev->mode_config.num_encoder--;
> - drm_modeset_unlock_all(dev);
>   
>   memset(encoder, 0, sizeof(*encoder));
>   }
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 419ac313c36f..9147aab182c4 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -221,7 +221,8 @@ void drm_plane_cleanup(struct drm_plane *plane)
>   {
>   struct drm_device *dev = plane->dev;
>   
> - drm_modeset_lock_all(dev);
> + drm_modeset_lock_fini(>mutex);
> +
>   kfree(plane->format_types);
>   drm_mode_object_unregister(dev, >base);
>   
> @@ -236,7 +237,6 @@ void drm_plane_cleanup(struct drm_plane *plane)
>   dev->mode_config.num_total_plane--;
>   if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>   dev->mode_config.num_overlay_plane--;
> - drm_modeset_unlock_all(dev);
>   
>   WARN_ON(plane->state && !plane->funcs->atomic_destroy_state);
>   if (plane->state && plane->funcs->atomic_destroy_state)

[PATCH 2/2] drm: Used DRM_LEGACY for all legacy functions

[Frank Binns]

On 04/08/16 09:20, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 08:37:49AM +0100, Frank Binns wrote:
>> On 03/08/16 20:11, Daniel Vetter wrote:
>>> Except for nouveau, only legacy drivers need this really. And nouveau
>>> is already marked up with DRIVER_KMS_LEGACY_CONTEXT as the special
>>> case.
>>>
>>> I've tried to be careful to leave everything related to modeset still
>>> using the DRIVER_MODESET flag. Otherwise it's a direct replacement of
>>> !DRIVER_MODESET with DRIVER_LEGACY checks. Also helps readability
>>> since fewer negative checks overall.
>>>
>>> Signed-off-by: Daniel Vetter 
>> I don't think this goes far enough. For example, the documentation
>> for the 'firstopen' callback says that it's called for legacy UMS drivers
>> but it does a check based on DRIVER_MODESET. There are also some
>> places in drm_pci.c that should be changed as well.
> I switched the check for firstopen from DRIVER_MODESET to DRIVER_LEGACY.
> And which hook in drm_pci.c should be converted in your opinion?
> -Daniel

Whoops, I read the patch without having had my morning coffee
(at least that's my excuse).

Sorry for the noise.

>> Anyway, this is progress so:
>> Reviewed-by: Frank Binns 
>>
>>> ---
>>>drivers/gpu/drm/drm_agpsupport.c |  6 ++
>>>drivers/gpu/drm/drm_auth.c   |  2 +-
>>>drivers/gpu/drm/drm_bufs.c   | 22 +++---
>>>drivers/gpu/drm/drm_context.c| 24 
>>>drivers/gpu/drm/drm_dma.c|  6 ++
>>>drivers/gpu/drm/drm_fops.c   |  6 +++---
>>>drivers/gpu/drm/drm_ioctl.c  |  4 ++--
>>>drivers/gpu/drm/drm_irq.c| 10 +-
>>>drivers/gpu/drm/drm_lock.c   |  4 ++--
>>>drivers/gpu/drm/drm_pci.c|  8 
>>>drivers/gpu/drm/drm_scatter.c|  6 +++---
>>>11 files changed, 47 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_agpsupport.c 
>>> b/drivers/gpu/drm/drm_agpsupport.c
>>> index 605bd243fb36..d621c8a4cf00 100644
>>> --- a/drivers/gpu/drm/drm_agpsupport.c
>>> +++ b/drivers/gpu/drm/drm_agpsupport.c
>>> @@ -430,9 +430,7 @@ struct drm_agp_head *drm_agp_init(struct drm_device 
>>> *dev)
>>> * intact so it can still be used. It is safe to call this if AGP is 
>>> disabled or
>>> * was already removed.
>>> *
>>> - * If DRIVER_MODESET is active, nothing is done to protect the modesetting
>>> - * resources from getting destroyed. Drivers are responsible of cleaning 
>>> them up
>>> - * during device shutdown.
>>> + * Cleanup is only done for drivers who have DRIVER_LEGACY set.
>>> */
>>>void drm_legacy_agp_clear(struct drm_device *dev)
>>>{
>>> @@ -440,7 +438,7 @@ void drm_legacy_agp_clear(struct drm_device *dev)
>>> if (!dev->agp)
>>> return;
>>> -   if (drm_core_check_feature(dev, DRIVER_MODESET))
>>> +   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
>>> return;
>>> list_for_each_entry_safe(entry, tempe, >agp->memory, head) {
>>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
>>> index 4153e8a193af..6b143514a566 100644
>>> --- a/drivers/gpu/drm/drm_auth.c
>>> +++ b/drivers/gpu/drm/drm_auth.c
>>> @@ -251,7 +251,7 @@ void drm_master_release(struct drm_file *file_priv)
>>> if (!drm_is_current_master(file_priv))
>>> goto out;
>>> -   if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
>>> +   if (drm_core_check_feature(dev, DRIVER_LEGACY)) {
>>> /*
>>>  * Since the master is disappearing, so is the
>>>  * possibility to lock.
>>> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
>>> index c3a12cd8bd0d..32191513e82d 100644
>>> --- a/drivers/gpu/drm/drm_bufs.c
>>> +++ b/drivers/gpu/drm/drm_bufs.c
>>> @@ -397,7 +397,7 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, 
>>> void *data,
>>> return -EPERM;
>>> if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
>>> -   drm_core_check_feature(dev, DRIVER_MODESET))
>>> +   !drm_core_check_feature(dev, DRIVER_LEGACY))
>>> return -EINVAL;
>>> err = drm_addmap_core(dev, map->offset, map->size, map->type,
>>> @@ -443,7 +443,7 @@ int drm_legacy_getmap_ioctl(struct 

[PATCH 1/8] drm: rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY

[Frank Binns]

On 03/08/16 19:04, David Herrmann wrote:
> The minor referred to by "DRM_MINOR_LEGACY" is called 'dev->primary' and
> gets 'cardX' as name assigned. Lets reduce this magnificent number of
> names for the same concept by one and rename DRM_MINOR_LEGACY to
> DRM_MINOR_PRIMARY (to match the actual struct-member name).
>
> Furthermore, this is in no way a legacy node, so lets not call it that.
>
> Signed-off-by: David Herrmann 

Reviewed-by: Frank Binns 

> ---
>   drivers/gpu/drm/drm_drv.c | 14 +++---
>   include/drm/drmP.h|  4 ++--
>   2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index be27ed3..57ce973 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -112,7 +112,7 @@ static struct drm_minor **drm_minor_get_slot(struct 
> drm_device *dev,
>unsigned int type)
>   {
>   switch (type) {
> - case DRM_MINOR_LEGACY:
> + case DRM_MINOR_PRIMARY:
>   return >primary;
>   case DRM_MINOR_RENDER:
>   return >render;
> @@ -512,7 +512,7 @@ int drm_dev_init(struct drm_device *dev,
>   goto err_minors;
>   }
>   
> - ret = drm_minor_alloc(dev, DRM_MINOR_LEGACY);
> + ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
>   if (ret)
>   goto err_minors;
>   
> @@ -545,7 +545,7 @@ err_ctxbitmap:
>   drm_legacy_ctxbitmap_cleanup(dev);
>   drm_ht_remove(>map_hash);
>   err_minors:
> - drm_minor_free(dev, DRM_MINOR_LEGACY);
> + drm_minor_free(dev, DRM_MINOR_PRIMARY);
>   drm_minor_free(dev, DRM_MINOR_RENDER);
>   drm_minor_free(dev, DRM_MINOR_CONTROL);
>   drm_fs_inode_free(dev->anon_inode);
> @@ -608,7 +608,7 @@ static void drm_dev_release(struct kref *ref)
>   drm_ht_remove(>map_hash);
>   drm_fs_inode_free(dev->anon_inode);
>   
> - drm_minor_free(dev, DRM_MINOR_LEGACY);
> + drm_minor_free(dev, DRM_MINOR_PRIMARY);
>   drm_minor_free(dev, DRM_MINOR_RENDER);
>   drm_minor_free(dev, DRM_MINOR_CONTROL);
>   
> @@ -684,7 +684,7 @@ int drm_dev_register(struct drm_device *dev, unsigned 
> long flags)
>   if (ret)
>   goto err_minors;
>   
> - ret = drm_minor_register(dev, DRM_MINOR_LEGACY);
> + ret = drm_minor_register(dev, DRM_MINOR_PRIMARY);
>   if (ret)
>   goto err_minors;
>   
> @@ -701,7 +701,7 @@ int drm_dev_register(struct drm_device *dev, unsigned 
> long flags)
>   goto out_unlock;
>   
>   err_minors:
> - drm_minor_unregister(dev, DRM_MINOR_LEGACY);
> + drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
>   drm_minor_unregister(dev, DRM_MINOR_RENDER);
>   drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>   out_unlock:
> @@ -741,7 +741,7 @@ void drm_dev_unregister(struct drm_device *dev)
>   list_for_each_entry_safe(r_list, list_temp, >maplist, head)
>   drm_legacy_rmmap(dev, r_list->map);
>   
> - drm_minor_unregister(dev, DRM_MINOR_LEGACY);
> + drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
>   drm_minor_unregister(dev, DRM_MINOR_RENDER);
>   drm_minor_unregister(dev, DRM_MINOR_CONTROL);
>   }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d377865..d488a72 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -642,7 +642,7 @@ struct drm_driver {
>   };
>   
>   enum drm_minor_type {
> - DRM_MINOR_LEGACY,
> + DRM_MINOR_PRIMARY,
>   DRM_MINOR_CONTROL,
>   DRM_MINOR_RENDER,
>   DRM_MINOR_CNT,
> @@ -856,7 +856,7 @@ static inline bool drm_is_control_client(const struct 
> drm_file *file_priv)
>   
>   static inline bool drm_is_primary_client(const struct drm_file *file_priv)
>   {
> - return file_priv->minor->type == DRM_MINOR_LEGACY;
> + return file_priv->minor->type == DRM_MINOR_PRIMARY;
>   }
>   
>   /**/

[PATCH 2/2] drm: Used DRM_LEGACY for all legacy functions

[Frank Binns]

On 03/08/16 20:11, Daniel Vetter wrote:
> Except for nouveau, only legacy drivers need this really. And nouveau
> is already marked up with DRIVER_KMS_LEGACY_CONTEXT as the special
> case.
>
> I've tried to be careful to leave everything related to modeset still
> using the DRIVER_MODESET flag. Otherwise it's a direct replacement of
> !DRIVER_MODESET with DRIVER_LEGACY checks. Also helps readability
> since fewer negative checks overall.
>
> Signed-off-by: Daniel Vetter 

I don't think this goes far enough. For example, the documentation
for the 'firstopen' callback says that it's called for legacy UMS drivers
but it does a check based on DRIVER_MODESET. There are also some
places in drm_pci.c that should be changed as well.

Anyway, this is progress so:
Reviewed-by: Frank Binns 

> ---
>   drivers/gpu/drm/drm_agpsupport.c |  6 ++
>   drivers/gpu/drm/drm_auth.c   |  2 +-
>   drivers/gpu/drm/drm_bufs.c   | 22 +++---
>   drivers/gpu/drm/drm_context.c| 24 
>   drivers/gpu/drm/drm_dma.c|  6 ++
>   drivers/gpu/drm/drm_fops.c   |  6 +++---
>   drivers/gpu/drm/drm_ioctl.c  |  4 ++--
>   drivers/gpu/drm/drm_irq.c| 10 +-
>   drivers/gpu/drm/drm_lock.c   |  4 ++--
>   drivers/gpu/drm/drm_pci.c|  8 
>   drivers/gpu/drm/drm_scatter.c|  6 +++---
>   11 files changed, 47 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_agpsupport.c 
> b/drivers/gpu/drm/drm_agpsupport.c
> index 605bd243fb36..d621c8a4cf00 100644
> --- a/drivers/gpu/drm/drm_agpsupport.c
> +++ b/drivers/gpu/drm/drm_agpsupport.c
> @@ -430,9 +430,7 @@ struct drm_agp_head *drm_agp_init(struct drm_device *dev)
>* intact so it can still be used. It is safe to call this if AGP is 
> disabled or
>* was already removed.
>*
> - * If DRIVER_MODESET is active, nothing is done to protect the modesetting
> - * resources from getting destroyed. Drivers are responsible of cleaning 
> them up
> - * during device shutdown.
> + * Cleanup is only done for drivers who have DRIVER_LEGACY set.
>*/
>   void drm_legacy_agp_clear(struct drm_device *dev)
>   {
> @@ -440,7 +438,7 @@ void drm_legacy_agp_clear(struct drm_device *dev)
>   
>   if (!dev->agp)
>   return;
> - if (drm_core_check_feature(dev, DRIVER_MODESET))
> + if (!drm_core_check_feature(dev, DRIVER_LEGACY))
>   return;
>   
>   list_for_each_entry_safe(entry, tempe, >agp->memory, head) {
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 4153e8a193af..6b143514a566 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -251,7 +251,7 @@ void drm_master_release(struct drm_file *file_priv)
>   if (!drm_is_current_master(file_priv))
>   goto out;
>   
> - if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> + if (drm_core_check_feature(dev, DRIVER_LEGACY)) {
>   /*
>* Since the master is disappearing, so is the
>* possibility to lock.
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index c3a12cd8bd0d..32191513e82d 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -397,7 +397,7 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void 
> *data,
>   return -EPERM;
>   
>   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> - drm_core_check_feature(dev, DRIVER_MODESET))
> + !drm_core_check_feature(dev, DRIVER_LEGACY))
>   return -EINVAL;
>   
>   err = drm_addmap_core(dev, map->offset, map->size, map->type,
> @@ -443,7 +443,7 @@ int drm_legacy_getmap_ioctl(struct drm_device *dev, void 
> *data,
>   int i;
>   
>   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> - drm_core_check_feature(dev, DRIVER_MODESET))
> + !drm_core_check_feature(dev, DRIVER_LEGACY))
>   return -EINVAL;
>   
>   idx = map->offset;
> @@ -545,7 +545,7 @@ EXPORT_SYMBOL(drm_legacy_rmmap_locked);
>   void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
>   {
>   if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> - drm_core_check_feature(dev, DRIVER_MODESET))
> + !drm_core_check_feature(dev, DRIVER_LEGACY))
>   return;
>   
>   mutex_lock(>struct_mutex);
> @@ -558,7 +558,7 @@ void drm_legacy_master_rmmaps(struct drm_device *dev, 
> struct drm_master *master)
>   {
>   struct drm_map_list *r_list, *list_temp;
>   
> - if (drm_core_check_feature(dev, 

[PATCH 1/2] drm: Mark up legacy/dri1 drivers with DRM_LEGACY

[Frank Binns]

On 03/08/16 20:11, Daniel Vetter wrote:
> It's super confusing that new drivers need to be marked with
> DRIVER_MODESET when really it means DRIVER_MODERN. Much better to
> invert the meaning and rename it to something that's suitably
> off-putting.
>
> Since there's over 100 places using DRIVER_MODESET we need to roll out
> this change without a flag day.
>
> v2: Update docs.
>
> Signed-off-by: Daniel Vetter 

Reviewed-by: Frank Binns 

> ---
>   Documentation/gpu/drm-internals.rst | 9 ++---
>   drivers/gpu/drm/i810/i810_drv.c | 4 +---
>   drivers/gpu/drm/mga/mga_drv.c   | 2 +-
>   drivers/gpu/drm/r128/r128_drv.c | 2 +-
>   drivers/gpu/drm/savage/savage_drv.c | 2 +-
>   drivers/gpu/drm/sis/sis_drv.c   | 2 +-
>   drivers/gpu/drm/tdfx/tdfx_drv.c | 1 +
>   drivers/gpu/drm/via/via_drv.c   | 2 +-
>   include/drm/drmP.h  | 1 +
>   9 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/gpu/drm-internals.rst 
> b/Documentation/gpu/drm-internals.rst
> index 3bb26135971f..37284bcc7764 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -53,9 +53,12 @@ u32 driver_features;
>   DRIVER_USE_AGP
>   Driver uses AGP interface, the DRM core will manage AGP resources.
>   
> -DRIVER_REQUIRE_AGP
> -Driver needs AGP interface to function. AGP initialization failure
> -will become a fatal error.
> +DRIVER_LEGACY
> +Denote a legacy driver using shadow attach. Don't use.
> +
> +DRIVER_KMS_LEGACY_CONTEXT
> +Used only by nouveau for backwards compatibility with existing userspace.
> +Don't use.
>   
>   DRIVER_PCI_DMA
>   Driver is capable of PCI DMA, mapping of PCI DMA buffers to
> diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c
> index 44f4a131c8dd..0be55dc1ef4b 100644
> --- a/drivers/gpu/drm/i810/i810_drv.c
> +++ b/drivers/gpu/drm/i810/i810_drv.c
> @@ -56,9 +56,7 @@ static const struct file_operations i810_driver_fops = {
>   };
>   
>   static struct drm_driver driver = {
> - .driver_features =
> - DRIVER_USE_AGP |
> - DRIVER_HAVE_DMA,
> + .driver_features = DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_LEGACY,
>   .dev_priv_size = sizeof(drm_i810_buf_priv_t),
>   .load = i810_driver_load,
>   .lastclose = i810_driver_lastclose,
> diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c
> index 5e2f131a6a72..25b2a1a424e6 100644
> --- a/drivers/gpu/drm/mga/mga_drv.c
> +++ b/drivers/gpu/drm/mga/mga_drv.c
> @@ -58,7 +58,7 @@ static const struct file_operations mga_driver_fops = {
>   
>   static struct drm_driver driver = {
>   .driver_features =
> - DRIVER_USE_AGP | DRIVER_PCI_DMA |
> + DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_LEGACY |
>   DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
>   .dev_priv_size = sizeof(drm_mga_buf_priv_t),
>   .load = mga_driver_load,
> diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c
> index c57b4de63caf..a982be57d1ef 100644
> --- a/drivers/gpu/drm/r128/r128_drv.c
> +++ b/drivers/gpu/drm/r128/r128_drv.c
> @@ -56,7 +56,7 @@ static const struct file_operations r128_driver_fops = {
>   
>   static struct drm_driver driver = {
>   .driver_features =
> - DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG |
> + DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG | DRIVER_LEGACY |
>   DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
>   .dev_priv_size = sizeof(drm_r128_buf_priv_t),
>   .load = r128_driver_load,
> diff --git a/drivers/gpu/drm/savage/savage_drv.c 
> b/drivers/gpu/drm/savage/savage_drv.c
> index 21aed1febeb4..3b807135a5cd 100644
> --- a/drivers/gpu/drm/savage/savage_drv.c
> +++ b/drivers/gpu/drm/savage/savage_drv.c
> @@ -50,7 +50,7 @@ static const struct file_operations savage_driver_fops = {
>   
>   static struct drm_driver driver = {
>   .driver_features =
> - DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA,
> + DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA | DRIVER_LEGACY,
>   .dev_priv_size = sizeof(drm_savage_buf_priv_t),
>   .load = savage_driver_load,
>   .firstopen = savage_driver_firstopen,
> diff --git a/drivers/gpu/drm/sis/sis_drv.c b/drivers/gpu/drm/sis/sis_drv.c
> index 79bce76cb8f7..ae9839886c4d 100644
> --- a/drivers/gpu/drm/sis/sis_drv.c
> +++ b/drivers/gpu/drm/sis/sis_drv.c
> @@ -102,7 +102,7 @@ static void sis_driver_postclose(struct drm_device *dev, 
> struct drm_file *file)
>   }
>   
>   static struct drm_driver driver = {
> - .driver_features = DRIVER_USE_AGP,
> + .driver_fe

[PATCH 3/6] drm/amd/amdgpu: Set DRIVER_MODESET feature flag at build time

[Frank Binns]

On 24/06/16 23:08, Alex Deucher wrote:
> On Fri, Jun 24, 2016 at 1:15 PM, Frank Binns  
> wrote:
>> This flag was being set unconditionally at runtime so just set it at
>> compile time instead.
>>
>> Signed-off-by: Frank Binns 
> Reviewed-by: Alex Deucher 
>
> Do you want to take this as part of the patch set, or should I apply
> this to my tree?
>
> Alex

This patch is independent of the rest so I'm happy for you to
apply it to your tree.

Thanks
Frank

>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index f888c01..7fe7f3c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -515,7 +515,7 @@ static struct drm_driver kms_driver = {
>>  .driver_features =
>>  DRIVER_USE_AGP |
>>  DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
>> -   DRIVER_PRIME | DRIVER_RENDER,
>> +   DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
>>  .dev_priv_size = 0,
>>  .load = amdgpu_driver_load_kms,
>>  .open = amdgpu_driver_open_kms,
>> @@ -590,7 +590,6 @@ static int __init amdgpu_init(void)
>>  DRM_INFO("amdgpu kernel modesetting enabled.\n");
>>  driver = _driver;
>>  pdriver = _kms_pci_driver;
>> -   driver->driver_features |= DRIVER_MODESET;
>>  driver->num_ioctls = amdgpu_max_kms_ioctl;
>>  amdgpu_register_atpx_handler();
>>  /* let modprobe override vga console setting */
>> --
>> 2.7.4
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/6] drm: Separate DRIVER_MODESET and DRIVER_LEGACY

[Frank Binns]

Support non-legacy drivers without mode-setting functionality by using
the new DRIVER_LEGACY feature to separate out legacy code, rather than
relying on DRIVER_MODESET not being advertised.

Signed-off-by: Thierry Reding 

v2:
- Rebase
- Change a few more places to check against DRIVER_LEGACY
- Move a core check

Signed-off-by: Frank Binns 
---
 drivers/gpu/drm/drm_bufs.c| 22 +++---
 drivers/gpu/drm/drm_context.c | 24 
 drivers/gpu/drm/drm_dma.c |  4 ++--
 drivers/gpu/drm/drm_fops.c| 10 ++
 drivers/gpu/drm/drm_ioctl.c   |  4 ++--
 drivers/gpu/drm/drm_irq.c | 14 +++---
 drivers/gpu/drm/drm_lock.c|  4 ++--
 drivers/gpu/drm/drm_pci.c | 10 +-
 drivers/gpu/drm/drm_scatter.c |  6 +++---
 9 files changed, 50 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index c3a12cd..3219151 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -397,7 +397,7 @@ int drm_legacy_addmap_ioctl(struct drm_device *dev, void 
*data,
return -EPERM;

if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
-   drm_core_check_feature(dev, DRIVER_MODESET))
+   !drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

err = drm_addmap_core(dev, map->offset, map->size, map->type,
@@ -443,7 +443,7 @@ int drm_legacy_getmap_ioctl(struct drm_device *dev, void 
*data,
int i;

if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
-   drm_core_check_feature(dev, DRIVER_MODESET))
+   !drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

idx = map->offset;
@@ -545,7 +545,7 @@ EXPORT_SYMBOL(drm_legacy_rmmap_locked);
 void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
 {
if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
-   drm_core_check_feature(dev, DRIVER_MODESET))
+   !drm_core_check_feature(dev, DRIVER_LEGACY))
return;

mutex_lock(>struct_mutex);
@@ -558,7 +558,7 @@ void drm_legacy_master_rmmaps(struct drm_device *dev, 
struct drm_master *master)
 {
struct drm_map_list *r_list, *list_temp;

-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return;

mutex_lock(>struct_mutex);
@@ -595,7 +595,7 @@ int drm_legacy_rmmap_ioctl(struct drm_device *dev, void 
*data,
int ret;

if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
-   drm_core_check_feature(dev, DRIVER_MODESET))
+   !drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

mutex_lock(>struct_mutex);
@@ -1220,7 +1220,7 @@ int drm_legacy_addbufs(struct drm_device *dev, void *data,
struct drm_buf_desc *request = data;
int ret;

-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA))
@@ -1266,7 +1266,7 @@ int drm_legacy_infobufs(struct drm_device *dev, void 
*data,
int i;
int count;

-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA))
@@ -1347,7 +1347,7 @@ int drm_legacy_markbufs(struct drm_device *dev, void 
*data,
int order;
struct drm_buf_entry *entry;

-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA))
@@ -1395,7 +1395,7 @@ int drm_legacy_freebufs(struct drm_device *dev, void 
*data,
int idx;
struct drm_buf *buf;

-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA))
@@ -1450,7 +1450,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
struct drm_buf_map *request = data;
int i;

-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA))
@@ -1530,7 +1530,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data,
 int drm_legacy_dma_ioctl(struct drm_device *dev, void *data,
  struct drm_file *file_priv)
 {
-   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   if (!drm_core_check_feature(dev, DRIVER_LEGACY))
return -EINVAL;

if (dev->driver->dma_ioctl)
diff --git a/drivers/gpu/drm/drm_context.c b/

[PATCH v2 5/6] drm: Introduce DRIVER_LEGACY feature

[Frank Binns]

From: Thierry Reding <tred...@nvidia.com>

Currently drivers that set the DRIVER_MODESET feature are considered to
be non-legacy drivers. At the same time DRIVER_MODESET implies that the
mode-setting IOCTLs are available. It is therefore not possible to
distinguish between a non-legacy driver with full mode-setting support
and a non-legacy driver without mode-setting functionality.

To separate the meaning of "legacy" and "modeset", a new driver feature
is introduced: DRIVER_LEGACY. The meaning of DRIVER_MODESET can then be
changed to apply to the mode-setting functionality only, irrespective
of whether it is legacy or not.

Mark all legacy drivers appropriately.

Signed-off-by: Thierry Reding 

v2:
- Rebase
- Add legacy flag to r128
- Don't add legacy flag to radeon
- Build fix for tdfx (use the correct field name)
- Add documentation to drm-internals

Signed-off-by: Frank Binns 
---
I wasn't totally sure what to do with regards to authorship and sign-off
as I'd like to give credit where it's due. Feel free to tell me if I got
it wrong :)

 Documentation/gpu/drm-internals.rst | 9 ++---
 drivers/gpu/drm/i810/i810_drv.c | 3 ++-
 drivers/gpu/drm/mga/mga_drv.c   | 3 ++-
 drivers/gpu/drm/r128/r128_drv.c | 3 ++-
 drivers/gpu/drm/savage/savage_drv.c | 2 +-
 drivers/gpu/drm/sis/sis_drv.c   | 2 +-
 drivers/gpu/drm/tdfx/tdfx_drv.c | 1 +
 drivers/gpu/drm/via/via_drv.c   | 2 +-
 include/drm/drmP.h  | 1 +
 9 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/Documentation/gpu/drm-internals.rst 
b/Documentation/gpu/drm-internals.rst
index 4f71765..97b458e 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -95,6 +95,9 @@ DRIVER_ATOMIC
 implement appropriate obj->atomic_get_property() vfuncs for any
 modeset objects with driver specific properties.

+DRIVER_LEGACY
+Driver supports user mode setting interfaces (UMS). Deprecated.
+
 Major, Minor and Patchlevel
 ~~~

@@ -337,9 +340,9 @@ how the ioctl is allowed to be called.
device

 -  DRM_UNLOCKED - The ioctl handler will be called without locking the
-   DRM global mutex. This is the enforced default for kms drivers (i.e.
-   using the DRIVER_MODESET flag) and hence shouldn't be used any more
-   for new drivers.
+   DRM global mutex. This is the enforced default for drivers that don't
+   support UMS (i.e. using the DRIVER_LEGACY flag) and hence shouldn't
+   be used any more for new drivers.

 .. kernel-doc:: drivers/gpu/drm/drm_ioctl.c
:export:
diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c
index 44f4a13..8c3c04a 100644
--- a/drivers/gpu/drm/i810/i810_drv.c
+++ b/drivers/gpu/drm/i810/i810_drv.c
@@ -58,7 +58,8 @@ static const struct file_operations i810_driver_fops = {
 static struct drm_driver driver = {
.driver_features =
DRIVER_USE_AGP |
-   DRIVER_HAVE_DMA,
+   DRIVER_HAVE_DMA |
+   DRIVER_LEGACY,
.dev_priv_size = sizeof(drm_i810_buf_priv_t),
.load = i810_driver_load,
.lastclose = i810_driver_lastclose,
diff --git a/drivers/gpu/drm/mga/mga_drv.c b/drivers/gpu/drm/mga/mga_drv.c
index 5e2f131..06aafdc 100644
--- a/drivers/gpu/drm/mga/mga_drv.c
+++ b/drivers/gpu/drm/mga/mga_drv.c
@@ -59,7 +59,8 @@ static const struct file_operations mga_driver_fops = {
 static struct drm_driver driver = {
.driver_features =
DRIVER_USE_AGP | DRIVER_PCI_DMA |
-   DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
+   DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED |
+   DRIVER_LEGACY,
.dev_priv_size = sizeof(drm_mga_buf_priv_t),
.load = mga_driver_load,
.unload = mga_driver_unload,
diff --git a/drivers/gpu/drm/r128/r128_drv.c b/drivers/gpu/drm/r128/r128_drv.c
index c57b4de..5a63425 100644
--- a/drivers/gpu/drm/r128/r128_drv.c
+++ b/drivers/gpu/drm/r128/r128_drv.c
@@ -57,7 +57,8 @@ static const struct file_operations r128_driver_fops = {
 static struct drm_driver driver = {
.driver_features =
DRIVER_USE_AGP | DRIVER_PCI_DMA | DRIVER_SG |
-   DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
+   DRIVER_HAVE_DMA | DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED |
+   DRIVER_LEGACY,
.dev_priv_size = sizeof(drm_r128_buf_priv_t),
.load = r128_driver_load,
.preclose = r128_driver_preclose,
diff --git a/drivers/gpu/drm/savage/savage_drv.c 
b/drivers/gpu/drm/savage/savage_drv.c
index 21aed1f..3b80713 100644
--- a/drivers/gpu/drm/savage/savage_drv.c
+++ b/drivers/gpu/drm/savage/savage_drv.c
@@ -50,7 +50,7 @@ static const struct file_operations savage_driver_fops = {

 static struct drm_driver driver = {
.driver_features =
-   DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA,
+   DRIVER_USE_AGP | DRIVER_HAVE_DMA | DRIVER_PCI_DMA | DRIVER_LEGACY,
.dev_priv_size =

[PATCH 4/6] drm/qxl: Remove dead code

[Frank Binns]

From: Thierry Reding 

The QXL driver sets DRIVER_MODESET unconditionally, so testing for the
absence of the feature will always fail.

Signed-off-by: Thierry Reding 
---
 drivers/gpu/drm/qxl/qxl_kms.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 2319800..12b8dff 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -310,10 +310,6 @@ int qxl_driver_load(struct drm_device *dev, unsigned long 
flags)
struct qxl_device *qdev;
int r;

-   /* require kms */
-   if (!drm_core_check_feature(dev, DRIVER_MODESET))
-   return -ENODEV;
-
qdev = kzalloc(sizeof(struct qxl_device), GFP_KERNEL);
if (qdev == NULL)
return -ENOMEM;
-- 
2.7.4

[PATCH 3/6] drm/amd/amdgpu: Set DRIVER_MODESET feature flag at build time

[Frank Binns]

This flag was being set unconditionally at runtime so just set it at
compile time instead.

Signed-off-by: Frank Binns 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index f888c01..7fe7f3c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -515,7 +515,7 @@ static struct drm_driver kms_driver = {
.driver_features =
DRIVER_USE_AGP |
DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
-   DRIVER_PRIME | DRIVER_RENDER,
+   DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
.dev_priv_size = 0,
.load = amdgpu_driver_load_kms,
.open = amdgpu_driver_open_kms,
@@ -590,7 +590,6 @@ static int __init amdgpu_init(void)
DRM_INFO("amdgpu kernel modesetting enabled.\n");
driver = _driver;
pdriver = _kms_pci_driver;
-   driver->driver_features |= DRIVER_MODESET;
driver->num_ioctls = amdgpu_max_kms_ioctl;
amdgpu_register_atpx_handler();
/* let modprobe override vga console setting */
-- 
2.7.4

[PATCH 2/6] drm: Rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY

[Frank Binns]

This brings the minor type name into line with how the actual minor,
using this type, is referenced throughout the rest of drm.

Signed-off-by: Frank Binns 
---
 drivers/gpu/drm/drm_drv.c | 16 
 include/drm/drmP.h|  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index aead9ff..7775c8b 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -112,7 +112,7 @@ static struct drm_minor **drm_minor_get_slot(struct 
drm_device *dev,
 unsigned int type)
 {
switch (type) {
-   case DRM_MINOR_LEGACY:
+   case DRM_MINOR_PRIMARY:
return >primary;
case DRM_MINOR_RENDER:
return >render;
@@ -362,7 +362,7 @@ EXPORT_SYMBOL(drm_put_dev);
 void drm_unplug_dev(struct drm_device *dev)
 {
/* for a USB device */
-   drm_minor_unregister(dev, DRM_MINOR_LEGACY);
+   drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
drm_minor_unregister(dev, DRM_MINOR_RENDER);
drm_minor_unregister(dev, DRM_MINOR_CONTROL);

@@ -514,7 +514,7 @@ int drm_dev_init(struct drm_device *dev,
goto err_minors;
}

-   ret = drm_minor_alloc(dev, DRM_MINOR_LEGACY);
+   ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
if (ret)
goto err_minors;

@@ -547,7 +547,7 @@ err_ctxbitmap:
drm_legacy_ctxbitmap_cleanup(dev);
drm_ht_remove(>map_hash);
 err_minors:
-   drm_minor_free(dev, DRM_MINOR_LEGACY);
+   drm_minor_free(dev, DRM_MINOR_PRIMARY);
drm_minor_free(dev, DRM_MINOR_RENDER);
drm_minor_free(dev, DRM_MINOR_CONTROL);
drm_fs_inode_free(dev->anon_inode);
@@ -610,7 +610,7 @@ static void drm_dev_release(struct kref *ref)
drm_ht_remove(>map_hash);
drm_fs_inode_free(dev->anon_inode);

-   drm_minor_free(dev, DRM_MINOR_LEGACY);
+   drm_minor_free(dev, DRM_MINOR_PRIMARY);
drm_minor_free(dev, DRM_MINOR_RENDER);
drm_minor_free(dev, DRM_MINOR_CONTROL);

@@ -686,7 +686,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long 
flags)
if (ret)
goto err_minors;

-   ret = drm_minor_register(dev, DRM_MINOR_LEGACY);
+   ret = drm_minor_register(dev, DRM_MINOR_PRIMARY);
if (ret)
goto err_minors;

@@ -703,7 +703,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long 
flags)
goto out_unlock;

 err_minors:
-   drm_minor_unregister(dev, DRM_MINOR_LEGACY);
+   drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
drm_minor_unregister(dev, DRM_MINOR_RENDER);
drm_minor_unregister(dev, DRM_MINOR_CONTROL);
 out_unlock:
@@ -743,7 +743,7 @@ void drm_dev_unregister(struct drm_device *dev)
list_for_each_entry_safe(r_list, list_temp, >maplist, head)
drm_legacy_rmmap(dev, r_list->map);

-   drm_minor_unregister(dev, DRM_MINOR_LEGACY);
+   drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
drm_minor_unregister(dev, DRM_MINOR_RENDER);
drm_minor_unregister(dev, DRM_MINOR_CONTROL);
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cf918e3e..1d39988 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -642,7 +642,7 @@ struct drm_driver {
 };

 enum drm_minor_type {
-   DRM_MINOR_LEGACY,
+   DRM_MINOR_PRIMARY,
DRM_MINOR_CONTROL,
DRM_MINOR_RENDER,
DRM_MINOR_CNT,
@@ -883,7 +883,7 @@ static inline bool drm_is_control_client(const struct 
drm_file *file_priv)

 static inline bool drm_is_primary_client(const struct drm_file *file_priv)
 {
-   return file_priv->minor->type == DRM_MINOR_LEGACY;
+   return file_priv->minor->type == DRM_MINOR_PRIMARY;
 }

 /**/
-- 
2.7.4

[PATCH 1/6] drm/vmwgfx: Stop checking minor type directly

[Frank Binns]

Use the appropriate drm minor type helper instead.

Cc: Sinclair Yeh 
Cc: Thomas Hellstrom 
Signed-off-by: Frank Binns 
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 6064664..5d5c951 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1041,8 +1041,7 @@ static struct vmw_master *vmw_master_check(struct 
drm_device *dev,
struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
struct vmw_master *vmaster;

-   if (file_priv->minor->type != DRM_MINOR_LEGACY ||
-   !(flags & DRM_AUTH))
+   if (!drm_is_primary_client(file_priv) || !(flags & DRM_AUTH))
return NULL;

ret = mutex_lock_interruptible(>master_mutex);
-- 
2.7.4

[PATCH v2 0/6] Support render only drivers

[Frank Binns]

This series resurrects work originally done by Thierry Reding (hence
the v2) but without taking things quite as far as the original [1].

The motivation for this series is to remove the dual meaning of
the DRIVER_MODESET feature flag, whereby it means "support kernel
modesetting" and "don't expose pre-KMS legacy behaviour". Without
this, it's necessary for render only drivers, such as etnaviv, to
choose between falsely advertising themselves as modesetting drivers
or exposing legacy behaviour. Unlike the original series, it doesn't
restrict the card-node to drivers that set DRIVER_MODESET.

As a couple of years have now passed, a number of the patches from the
original series have now either been merged (mainly clean up), are no
longer relevant or have no code changes in common with the originals.
It also means that some new patches are needed. Those patches that have
been rebased are marked as v2.

[1] https://lwn.net/Articles/588016/

Frank Binns (4):
  drm/vmwgfx: Stop checking minor type directly
  drm: Rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY
  drm/amd/amdgpu: Set DRIVER_MODESET feature flag at build time
  drm: Separate DRIVER_MODESET and DRIVER_LEGACY

Thierry Reding (2):
  drm/qxl: Remove dead code
  drm: Introduce DRIVER_LEGACY feature

 Documentation/gpu/drm-internals.rst |  9 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
 drivers/gpu/drm/drm_bufs.c  | 22 +++---
 drivers/gpu/drm/drm_context.c   | 24 
 drivers/gpu/drm/drm_dma.c   |  4 ++--
 drivers/gpu/drm/drm_drv.c   | 16 
 drivers/gpu/drm/drm_fops.c  | 10 ++
 drivers/gpu/drm/drm_ioctl.c |  4 ++--
 drivers/gpu/drm/drm_irq.c   | 14 +++---
 drivers/gpu/drm/drm_lock.c  |  4 ++--
 drivers/gpu/drm/drm_pci.c   | 10 +-
 drivers/gpu/drm/drm_scatter.c   |  6 +++---
 drivers/gpu/drm/i810/i810_drv.c |  3 ++-
 drivers/gpu/drm/mga/mga_drv.c   |  3 ++-
 drivers/gpu/drm/qxl/qxl_kms.c   |  4 
 drivers/gpu/drm/r128/r128_drv.c |  3 ++-
 drivers/gpu/drm/savage/savage_drv.c |  2 +-
 drivers/gpu/drm/sis/sis_drv.c   |  2 +-
 drivers/gpu/drm/tdfx/tdfx_drv.c |  1 +
 drivers/gpu/drm/via/via_drv.c   |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  3 +--
 include/drm/drmP.h  |  5 +++--
 22 files changed, 79 insertions(+), 75 deletions(-)

-- 
2.7.4

[PATCH] drm/i915: Fix misleading driver debug message

[Frank Binns]

Stop claiming that UMS support is disabled when it's not actually
supported anymore.

Signed-off-by: Frank Binns 
---
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3eb47fb..22ded6b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1807,7 +1807,7 @@ static int __init i915_init(void)

if (!(driver.driver_features & DRIVER_MODESET)) {
/* Silently fail loading to not upset userspace. */
-   DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
+   DRM_DEBUG_DRIVER("KMS disabled.\n");
return 0;
}

-- 
2.7.4

[PATCH] drm: fix some spelling mistakes

[Frank Binns]

Signed-off-by: Frank Binns 
---
 drivers/gpu/drm/drm_irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 8ca3d2b..149453c 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -532,7 +532,7 @@ int drm_irq_uninstall(struct drm_device *dev)

/*
 * Wake up any waiters so they don't hang. This is just to paper over
-* isssues for UMS drivers which aren't in full control of their
+* issues for UMS drivers which aren't in full control of their
 * vblank/irq handling. KMS drivers must ensure that vblanks are all
 * disabled when uninstalling the irq handler.
 */
@@ -594,7 +594,7 @@ int drm_control(struct drm_device *dev, void *data,
return 0;
if (drm_core_check_feature(dev, DRIVER_MODESET))
return 0;
-   /* UMS was only ever support on pci devices. */
+   /* UMS was only ever supported on pci devices. */
if (WARN_ON(!dev->pdev))
return -EINVAL;

-- 
2.7.4

[PATCH v9 14/14] drm/mediatek: Add interface to allocate Mediatek GEM buffer.

[Frank Binns]

Hi Philipp,

Comments below.

On 12/01/16 15:15, Philipp Zabel wrote:
> From: CK Hu 
>
> Add an interface to allocate Mediatek GEM buffers, allow the IOCTLs
> to be used by render nodes.
> This patch also sets the RENDER driver feature.
>
> Signed-off-by: CK Hu 
> Signed-off-by: Nicolas Boichat 
> Signed-off-by: Philipp Zabel 
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 13 +++-
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c | 39 ++
>  drivers/gpu/drm/mediatek/mtk_drm_gem.h | 12 +++
>  include/uapi/drm/mediatek_drm.h| 59 
> ++
>  4 files changed, 122 insertions(+), 1 deletion(-)
>  create mode 100644 include/uapi/drm/mediatek_drm.h
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c 
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index fdb27e9..1f776a9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "mtk_cec.h"
>  #include "mtk_drm_crtc.h"
> @@ -222,6 +223,14 @@ static const struct vm_operations_struct 
> mtk_drm_gem_vm_ops = {
>   .close = drm_gem_vm_close,
>  };
>  
> +static const struct drm_ioctl_desc mtk_ioctls[] = {
> + DRM_IOCTL_DEF_DRV(MTK_GEM_CREATE, mtk_gem_create_ioctl,
> +   DRM_UNLOCKED | DRM_AUTH | DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(MTK_GEM_MAP_OFFSET,
> +   mtk_gem_map_offset_ioctl,
> +   DRM_UNLOCKED | DRM_AUTH | DRM_RENDER_ALLOW),
> +};
> +
>  static const struct file_operations mtk_drm_fops = {
>   .owner = THIS_MODULE,
>   .open = drm_open,
> @@ -237,7 +246,7 @@ static const struct file_operations mtk_drm_fops = {
>  
>  static struct drm_driver mtk_drm_driver = {
>   .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> -DRIVER_ATOMIC,
> +DRIVER_ATOMIC | DRIVER_RENDER,
>   .unload = mtk_drm_unload,
>   .set_busid = drm_platform_set_busid,
>  
> @@ -257,6 +266,8 @@ static struct drm_driver mtk_drm_driver = {
>   .gem_prime_import = drm_gem_prime_import,
>   .gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
>   .gem_prime_mmap = mtk_drm_gem_mmap_buf,
> + .ioctls = mtk_ioctls,
> + .num_ioctls = ARRAY_SIZE(mtk_ioctls),
>   .fops = _drm_fops,
>  
>   .name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c 
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index 96cc980..f726d55 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -13,6 +13,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include "mtk_drm_gem.h"
>  
> @@ -225,3 +226,41 @@ struct sg_table *mtk_gem_prime_get_sg_table(struct 
> drm_gem_object *obj)
>  
>   return sgt;
>  }
> +
> +int mtk_gem_map_offset_ioctl(struct drm_device *drm, void *data,
> +  struct drm_file *file_priv)
> +{
> + struct drm_mtk_gem_map_off *args = data;
> +
You should validate args->pad here.

> + return mtk_drm_gem_dumb_map_offset(file_priv, drm, args->handle,
> +>offset);
> +}
> +
> +int mtk_gem_create_ioctl(struct drm_device *dev, void *data,
> +  struct drm_file *file_priv)
> +{
> + struct mtk_drm_gem_obj *mtk_gem;
> + struct drm_mtk_gem_create *args = data;
> + int ret;
> +
You should validate args->flags here.

> + mtk_gem = mtk_drm_gem_create(dev, args->size, false);
> + if (IS_ERR(mtk_gem))
> + return PTR_ERR(mtk_gem);
> +
> + /*
> +  * allocate a id of idr table where the obj is registered
> +  * and handle has the id what user can see.
> +  */
This comment doesn't seem that useful.

> + ret = drm_gem_handle_create(file_priv, _gem->base, >handle);
> + if (ret)
> + goto err_handle_create;
> +
> + /* drop reference from allocate - handle holds it now. */
> + drm_gem_object_unreference_unlocked(_gem->base);
> +
> + return 0;
> +
> +err_handle_create:
> + mtk_drm_gem_free_object(_gem->base);
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.h 
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> index 9bdeeb3..28b8fa7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> @@ -52,4 +52,16 @@ int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj,
>   struct vm_area_struct *vma);
>  struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj);
>  
> +/*
> + * request gem object creation and buffer allocation as the size
> + * that it is calculated with framebuffer information such as width,
> + * height and bpp.
> + */
> +int mtk_gem_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);
> +
> +/* get buffer offset to map to user space. */
> +int mtk_gem_map_offset_ioctl(struct 

[PATCH] android: fix warning when releasing active sync point

[Frank Binns]

Is this not the issue fixed by 8e43c9c75?

Thanks
Frank

On 15/12/15 13:30, Gustavo Padovan wrote:
> 2015-12-14 Dmitry Torokhov :
>
>> Userspace can close the sync device while there are still active fence
>> points, in which case kernel produces the following warning:
>>
>> [   43.853176] [ cut here ]
>> [   43.857834] WARNING: CPU: 0 PID: 892 at 
>> /mnt/host/source/src/third_party/kernel/v3.18/drivers/staging/android/sync.c:439
>>  android_fence_release+0x88/0x104()
>> [   43.871741] CPU: 0 PID: 892 Comm: Binder_5 Tainted: G U 
>> 3.18.0-07661-g0550ce9 #1
>> [   43.880176] Hardware name: Google Tegra210 Smaug Rev 1+ (DT)
>> [   43.885834] Call trace:
>> [   43.888294] [] dump_backtrace+0x0/0x10c
>> [   43.893697] [] show_stack+0x10/0x1c
>> [   43.898756] [] dump_stack+0x74/0xb8
>> [   43.903814] [] warn_slowpath_common+0x84/0xb0
>> [   43.909736] [] warn_slowpath_null+0x14/0x20
>> [   43.915482] [] android_fence_release+0x84/0x104
>> [   43.921582] [] fence_release+0x104/0x134
>> [   43.927066] [] sync_fence_free+0x74/0x9c
>> [   43.932552] [] sync_fence_release+0x34/0x48
>> [   43.938304] [] __fput+0x100/0x1b8
>> [   43.943185] [] fput+0x8/0x14
>> [   43.947982] [] task_work_run+0xb0/0xe4
>> [   43.953297] [] do_notify_resume+0x44/0x5c
>> [   43.958867] ---[ end trace 5a2aa4027cc5d171 ]---
> This crash report seems to be for a 3.18 kernel. Can you reproduce it
> on upstream kernel as well?
>
>   Gustavo
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm v2] drm: add drmGet(Master|Render)NameFrom(Render|Master)FD functions

[Frank Binns]

Hi Emil,

On 23/02/15 12:22, Emil Velikov wrote:
> Currently most places assume reliable master <> render node mapping.
> Although this may work in some cases, it is not correct.
>
> Add a couple of helpers that hide the details and provide the name of
> the master/render device name, given an render/master FD.
>
> v2:
>  - Rename Device and Primary to Master (aka the /dev/dri/cardX device).
>  - Check for the file via readdir_r() rather than stat().
>  - Wrap the check into a single function.
>  - Return NULL for non-linux platforms.
>
> Cc: Frank Binns 
> Cc: Daniel Vetter 
> Cc: David Herrmann 
> Signed-off-by: Emil Velikov 
> ---
>  xf86drm.c | 82 
> +++
>  xf86drm.h |  3 +++
>  2 files changed, 85 insertions(+)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index e117bc6..d4a4dc6 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -40,6 +40,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -522,6 +524,20 @@ static int drmGetMinorType(int minor)
>  }
>  }
>  
> +static const char *drmGetMinorName(int type)
> +{
> +switch (type) {
> +case DRM_NODE_PRIMARY:
> +return "card";
> +case DRM_NODE_CONTROL:
> +return "controlD";
> +case DRM_NODE_RENDER:
> +return "renderD";
> +default:
> +return NULL;
> +}
> +}
> +
>  /**
>   * Open the device by bus ID.
>   *
> @@ -2736,3 +2752,69 @@ int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t 
> *handle)
>   return 0;
>  }
>  
> +static char *drmGetMinorNameForFD(int fd, int type)
> +{
> +#ifdef __linux__
> + DIR *sysdir;
> + struct dirent *pent, *ent;
> + struct stat sbuf;
> + const char *name = drmGetMinorName(type);
> + const int len = strlen(name);
This will cause a segfault if 'name' is NULL.

> + char dev_name[64], buf[64];
> + long name_max;
> + int maj, min;
> +
> + if (!name)
> + return NULL;
> +
> + if (fstat(fd, ))
> + return NULL;
> +
> + maj = major(sbuf.st_rdev);
> + min = minor(sbuf.st_rdev);
> +
> + if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
> + return NULL;
> +
> + snprintf(buf, sizeof(buf), "/sys/dev/char/%d:%d/device/drm", maj, min);
> +
> + sysdir = opendir(buf);
> + if (!sysdir)
> + return NULL;
> +
> + name_max = fpathconf(dirfd(sysdir), _PC_NAME_MAX);
> + if (name_max == -1)
> + goto out_close_dir;
> +
> + pent = malloc(offsetof(struct dirent, d_name) + name_max + 1);
> + if (pent == NULL)
> +  goto out_close_dir;
> +
> + while (readdir_r(sysdir, pent, ) == 0 && ent != NULL) {
> + if (strncmp(ent->d_name, name, len) == 0) {
> + free(pent);
> + closedir(sysdir);
> +
> + snprintf(dev_name, sizeof(dev_name), DRM_DIR_NAME "/%s",
> +  ent->d_name);
> + return strdup(dev_name);
> + }
> + }
> +
> + free(pent);
> +
> +out_close_dir:
> + closedir(sysdir);
> +#endif
> + return NULL;
> +}
> +
> +char *drmGetMasterNameFromRenderFD(int fd)
I think drmGetPrimaryDeviceNameFromFd would be more appropriate given
the node type is 'primary', the type of the fd doesn't matter afaics and
for consistency with other drmGet* functions. However, given that's a
bit of a mouthful I guess the 'Device' part could be dropped.

> +{
> + return drmGetMinorNameForFD(fd, DRM_NODE_PRIMARY);
> +}
> +
> +char *drmGetRenderNameFromMasterFD(int fd)
As above, maybe drmGetRenderDeviceNameFromFd?

With those things changed/fixed you can have a:
Reviewed-by: Frank Binns 

Thanks
Frank

> +{
> + return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
> +}
> diff --git a/xf86drm.h b/xf86drm.h
> index afd38a1..5fdf27b 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -749,6 +749,9 @@ extern int drmGetNodeTypeFromFd(int fd);
>  extern int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int 
> *prime_fd);
>  extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle);
>  
> +extern char *drmGetRenderNameFromMasterFD(int fd);
> +extern char *drmGetMasterNameFromRenderFD(int fd);
> +
>  #if defined(__cplusplus) || defined(c_plusplus)
>  }
>  #endif

[PATCH libdrm] Add new drmGetNodeTypeFromFd function

[Frank Binns]

Hi Emil,

On 13/02/15 16:38, Emil Velikov wrote:
> Hi Frank,
> On 13/02/15 10:51, Frank Binns wrote:
>> Add a helper function that returns the type of device node from an fd.
>>
>> Signed-off-by: Frank Binns 
> Reviewed-by: Emil Velikov 
>
> Thank you for getting rid of the silly file probing that I went with
> initially.
>
> -Emil

Would you mind pushing this as I don't have commit access.

Thanks
Frank

[RFC PATCH 0/2] libdrm: Add master <> render node helpers

[Frank Binns]

Hi Emil,

On 13/02/15 09:18, Emil Velikov wrote:
> On 12/02/15 18:21, David Herrmann wrote:
>> Hi
>>
>> On Tue, Feb 10, 2015 at 11:37 PM, Emil Velikov  
>> wrote:
>>> On 02/02/15 00:14, Emil Velikov wrote:
 Hi all,

 As mentioned a couple of days ago at #dri-devel some(most) users of
 render nodes tend to rely on strict mapping between the primary and
 render node. I.e. something along the lines of

   fstat(render_fd, );
   sprintf(primary_node, "/dev/dri/card%d",
 ((sbuf.st_rdev & 0x3f) | 0x80));

 Currently the following are (ab)using the above code:
  - xf86-video-nouveau
  - xf86-video-intel
  - libva (vaapi)

 As reminded by David Herrman, this is not the correct solution - thus
 I've added a couple of helpers which walk through sysfs of the
 respecitive device and return the correct device name.

 I'm not 100% happy with the function names, so suggestions are greatly
 appreciated. Any other comments are also welcome :)

 Note: BSD guys - you'll likely need your own version of these functions.

>>> David, Daniel
>>>
>>> Can you please take a look at these two patches. Would be great if we
>>> can minimize the above assumptions before they get too wide spread.
>> The patches look ok. I really dislike the direct file-probing, though.
>> I'd recommend using libudev or readdir_r() on /dev/dri/, but I'm not
>> the one to ask about coding-style for libdrm, so fine with me.
>>
> Thanks for having a look. libudev is no used in libdrm so I will stay
> away for now. I would prefer to use readdir on /dev/dri/ but AFAICT
> there is no way to get the mapping from those alone. Am I missing
> something ?
Can you you not just use use readdir on /dev/dri and stat on each
entry's d_name comparing the result to an fstat on the fd? Once you find
a match you just
strdup the d_name.

>
> I'm also thinking that changing the prototypes, as below, might be
> useful - i.e. return 0 on success, -ENODEV when the corresponding device
> lacks render/primary device, and other negative value on error ?
>
> char *drmGetRenderNameFromDeviceFD(int fd);
> char *drmGetDeviceNameFromRenderFD(int fd);
I personally prefer this as it fits in better with what we already have.

>
> int drmGetRenderNameFromDeviceFD(int fd, char **render);
> int drmGetDeviceNameFromRenderFD(int fd, char **device);
>
>
> Cheers,
> Emil
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm: add drmGet{Device, Render}NameFrom{Render, Device}Fd helpers

[Frank Binns]

Hi Emil,

On 02/02/15 00:14, Emil Velikov wrote:
> Currently most places assume reliable primary <> render node mapping.
> Although this may work in some cases, it is not correct.
>
> Add a couple of helpers that hide the details and safes all the
> guesswork for the user.
>
> Cc: Daniel Vetter 
> Cc: David Herrmann 
> Signed-off-by: Emil Velikov 
> ---
>  xf86drm.c | 54 ++
>  xf86drm.h |  3 +++
>  2 files changed, 57 insertions(+)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 345325a..6af7ac0 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2607,3 +2607,57 @@ int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t 
> *handle)
>   return 0;
>  }
>  
> +char *drmGetDeviceNameFromRenderFD(int fd)
> +{
> +
> + struct stat sbuf;
> + char name[64], buf[64];
> + int maj, min, i;
> +
> + if (!fstat(fd, ))
> + return NULL;
> +
fstat returns 0 on success and -1 otherwise so this should be

if (fstat(fd, ))
return NULL;

> + if (!S_ISCHR(sbuf.st_mode))
> + return NULL;
> +
> + maj = major(sbuf.st_rdev);
> + min = minor(sbuf.st_rdev);
> +
> + snprintf(buf, sizeof(buf), "/sys/dev/char/%d:%d/device/drm", maj, min);
> +
> + for (i = 0; i < DRM_MAX_MINOR; i++) {
> + snprintf(name, sizeof(name), DRM_DEV_NAME, buf, i);
> + if (stat(name, ) == 0) {
> + snprintf(name, sizeof(name), DRM_DEV_NAME, 
> DRM_DIR_NAME, i);
> + return strdup(name);
> + }
> + }
> + return NULL;
> +}
> +
> +char *drmGetRenderNameFromDeviceFD(int fd)
> +{
> + struct stat sbuf;
> + char name[64], buf[64];
> + int maj, min, i;
> +
> + if (!fstat(fd, ))
> + return NULL;
> +
Same here.

> + if (!S_ISCHR(sbuf.st_mode))
> + return NULL;
> +
> + maj = major(sbuf.st_rdev);
> + min = minor(sbuf.st_rdev);
> +
> + snprintf(buf, sizeof(buf), "/sys/dev/char/%d:%d/device/drm", maj, min);
> +
> + for (i = 128; i < (128 + DRM_MAX_MINOR); i++) {
> + snprintf(name, sizeof(name), DRM_DEV_NAME, buf, i);
> + if (stat(name, ) == 0) {
> + snprintf(name, sizeof(name), DRM_RENDER_DEV_NAME, 
> DRM_DIR_NAME, i);
> + return strdup(name);
> + }
> + }
> + return NULL;
> +}
> diff --git a/xf86drm.h b/xf86drm.h
> index bfd0670..bca5887 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -740,6 +740,9 @@ extern char *drmGetDeviceNameFromFd(int fd);
>  extern int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int 
> *prime_fd);
>  extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle);
>  
> +extern char *drmGetRenderNameFromDeviceFD(int fd);
> +extern char *drmGetDeviceNameFromRenderFD(int fd);
> +
>  #if defined(__cplusplus) || defined(c_plusplus)
>  }
>  #endif

[PATCH libdrm] Add new drmGetNodeTypeFromFd function

[Frank Binns]

Add a helper function that returns the type of device node from an fd.

Signed-off-by: Frank Binns 
---
 xf86drm.c | 39 +++
 xf86drm.h |  1 +
 2 files changed, 40 insertions(+)

diff --git a/xf86drm.c b/xf86drm.c
index d85115c..e117bc6 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -505,6 +505,23 @@ static int drmGetMinorBase(int type)
 };
 }

+static int drmGetMinorType(int minor)
+{
+int type = minor >> 6;
+
+if (minor < 0)
+return -1;
+
+switch (type) {
+case DRM_NODE_PRIMARY:
+case DRM_NODE_CONTROL:
+case DRM_NODE_RENDER:
+return type;
+default:
+return -1;
+}
+}
+
 /**
  * Open the device by bus ID.
  *
@@ -2667,6 +2684,28 @@ char *drmGetDeviceNameFromFd(int fd)
return strdup(name);
 }

+int drmGetNodeTypeFromFd(int fd)
+{
+   struct stat sbuf;
+   int maj, min, type;
+
+   if (fstat(fd, ))
+   return -1;
+
+   maj = major(sbuf.st_rdev);
+   min = minor(sbuf.st_rdev);
+
+   if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode)) {
+   errno = EINVAL;
+   return -1;
+   }
+
+   type = drmGetMinorType(min);
+   if (type == -1)
+   errno = ENODEV;
+   return type;
+}
+
 int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int *prime_fd)
 {
struct drm_prime_handle args;
diff --git a/xf86drm.h b/xf86drm.h
index 77937eb..afd38a1 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -744,6 +744,7 @@ typedef struct _drmEventContext {
 extern int drmHandleEvent(int fd, drmEventContextPtr evctx);

 extern char *drmGetDeviceNameFromFd(int fd);
+extern int drmGetNodeTypeFromFd(int fd);

 extern int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int 
*prime_fd);
 extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle);
-- 
1.8.5.4.gfd2

[PATCH 2/2] libdrm: add drmGetNodeType() helper

[Frank Binns]

Hi Emil,

On 02/02/15 00:14, Emil Velikov wrote:
> The add a simple helper which returns the node type of the opened fd.
> Likely to be used in conjunction with the previous two helpers.
>
> Cc: Daniel Vetter 
> Cc: David Herrmann 
> Signed-off-by: Emil Velikov 
> ---
>  xf86drm.c | 43 +++
>  xf86drm.h |  7 +++
>  2 files changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 6af7ac0..a70c0dd 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -85,10 +85,6 @@
>  
>  #define DRM_MSG_VERBOSITY 3
>  
> -#define DRM_NODE_CONTROL 0
> -#define DRM_NODE_PRIMARY 1
> -#define DRM_NODE_RENDER 2
> -
>  static drmServerInfoPtr drm_server_info;
>  
>  void drmSetServerInfo(drmServerInfoPtr info)
> @@ -2607,6 +2603,45 @@ int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t 
> *handle)
>   return 0;
>  }
>  
> +int drmGetNodeType(int fd, drmNodeType *type)
> +{
> + struct stat sbuf;
> + char name[64];
> + dev_t d;
> + int i;
> +
> + if (!fstat(fd, ))
> + return -errno;
> +
> + d = sbuf.st_rdev;
> +
> + for (i = 0; i < DRM_MAX_MINOR; i++) {
> + snprintf(name, sizeof(name), DRM_DEV_NAME, DRM_DIR_NAME, i);
> + if (stat(name, ) == 0 && sbuf.st_rdev == d) {
> + *type = DRM_NODE_PRIMARY;
> + return 0;
> + }
> + }
> +
> + for (i = 64; i < (64 + DRM_MAX_MINOR); i++) {
> + snprintf(name, sizeof(name), DRM_CONTROL_DEV_NAME, 
> DRM_DIR_NAME, i);
> + if (stat(name, ) == 0 && sbuf.st_rdev == d) {
> + *type = DRM_NODE_CONTROL;
> + return 0;
> + }
> + }
> +
> + for (i = 128; i < (128 + DRM_MAX_MINOR); i++) {
> + snprintf(name, sizeof(name), DRM_RENDER_DEV_NAME, DRM_DIR_NAME, 
> i);
> + if (stat(name, ) == 0 && sbuf.st_rdev == d) {
> + *type = DRM_NODE_RENDER;
> + return 0;
> + }
> + }
It would seem easier (and faster) to use minor(sbuf.st_rdev) to
determine the type of node.

I'm not sure what the etiquette is here but I've prepared a patch that
does it this way instead, which will follow this email. Please feel free
to add your own signed-off-by, suggested-by, etc as I don't want to
steal all your credit :)

Thanks
Frank

> +
> + return -EINVAL;
> +}
> +
>  char *drmGetDeviceNameFromRenderFD(int fd)
>  {
>  
> diff --git a/xf86drm.h b/xf86drm.h
> index bca5887..7d67df9 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -740,6 +740,13 @@ extern char *drmGetDeviceNameFromFd(int fd);
>  extern int drmPrimeHandleToFD(int fd, uint32_t handle, uint32_t flags, int 
> *prime_fd);
>  extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle);
>  
> +typedef enum _drmNodeType {
> +DRM_NODE_CONTROL = 0,
> +DRM_NODE_PRIMARY = 1,
> +DRM_NODE_RENDER  = 2
> +} drmNodeType, *drmNodeTypePtr;
> +
> +extern int drmGetNodeType(int fd, drmNodeType *type);
>  extern char *drmGetRenderNameFromDeviceFD(int fd);
>  extern char *drmGetDeviceNameFromRenderFD(int fd);
>  

[PATCH 1/2] Add new drmOpenWithType function (v4)

[Frank Binns]

Reviewed-by: Frank Binns 

On 11/02/15 04:40, Jammy Zhou wrote:
> v2: Add drmGetMinorBase, and call drmOpenWithType in drmOpen
> v3: Pass 'type' to drmOpenByBusid and drmOpenDevice in drmOpenByName
> v4: Renumber node type definitions, and return -1 for unsupported type
>
> Signed-off-by: Jammy Zhou 
> Reviewed-by: Alex Deucher  (v3)
> ---
>  xf86drm.c | 70 
> +--
>  xf86drm.h |  9 +++-
>  2 files changed, 63 insertions(+), 16 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 345325a..998f010 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -85,10 +85,6 @@
>  
>  #define DRM_MSG_VERBOSITY 3
>  
> -#define DRM_NODE_CONTROL 0
> -#define DRM_NODE_PRIMARY 1
> -#define DRM_NODE_RENDER 2
> -
>  static drmServerInfoPtr drm_server_info;
>  
>  void drmSetServerInfo(drmServerInfoPtr info)
> @@ -493,11 +489,25 @@ int drmAvailable(void)
>  return retval;
>  }
>  
> +static int drmGetMinorBase(int type)
> +{
> +switch (type) {
> +case DRM_NODE_PRIMARY:
> +return 0;
> +case DRM_NODE_CONTROL:
> +return 64;
> +case DRM_NODE_RENDER:
> +return 128;
> +default:
> +return -1;
> +};
> +}
>  
>  /**
>   * Open the device by bus ID.
>   *
>   * \param busid bus ID.
> + * \param type device node type.
>   *
>   * \return a file descriptor on success, or a negative value on error.
>   *
> @@ -507,16 +517,20 @@ int drmAvailable(void)
>   *
>   * \sa drmOpenMinor() and drmGetBusid().
>   */
> -static int drmOpenByBusid(const char *busid)
> +static int drmOpenByBusid(const char *busid, int type)
>  {
>  inti, pci_domain_ok = 1;
>  intfd;
>  const char *buf;
>  drmSetVersion sv;
> +intbase = drmGetMinorBase(type);
> +
> +if (base < 0)
> +return -1;
>  
>  drmMsg("drmOpenByBusid: Searching for BusID %s\n", busid);
> -for (i = 0; i < DRM_MAX_MINOR; i++) {
> - fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
> +for (i = base; i < base + DRM_MAX_MINOR; i++) {
> + fd = drmOpenMinor(i, 1, type);
>   drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
>   if (fd >= 0) {
>   /* We need to try for 1.4 first for proper PCI domain support
> @@ -556,6 +570,7 @@ static int drmOpenByBusid(const char *busid)
>   * Open the device by name.
>   *
>   * \param name driver name.
> + * \param type the device node type.
>   * 
>   * \return a file descriptor on success, or a negative value on error.
>   * 
> @@ -566,19 +581,23 @@ static int drmOpenByBusid(const char *busid)
>   * 
>   * \sa drmOpenMinor(), drmGetVersion() and drmGetBusid().
>   */
> -static int drmOpenByName(const char *name)
> +static int drmOpenByName(const char *name, int type)
>  {
>  int   i;
>  int   fd;
>  drmVersionPtr version;
>  char *id;
> +int   base = drmGetMinorBase(type);
> +
> +if (base < 0)
> +return -1;
>  
>  /*
>   * Open the first minor number that matches the driver name and isn't
>   * already in use.  If it's in use it will have a busid assigned already.
>   */
> -for (i = 0; i < DRM_MAX_MINOR; i++) {
> - if ((fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY)) >= 0) {
> +for (i = base; i < base + DRM_MAX_MINOR; i++) {
> + if ((fd = drmOpenMinor(i, 1, type)) >= 0) {
>   if ((version = drmGetVersion(fd))) {
>   if (!strcmp(version->name, name)) {
>   drmFreeVersion(version);
> @@ -620,9 +639,9 @@ static int drmOpenByName(const char *name)
>   for (devstring = ++pt; *pt && *pt != ' '; ++pt)
>   ;
>   if (*pt) { /* Found busid */
> - return drmOpenByBusid(++pt);
> + return drmOpenByBusid(++pt, type);
>   } else { /* No busid */
> - return drmOpenDevice(strtol(devstring, NULL, 0),i, 
> DRM_NODE_PRIMARY);
> + return drmOpenDevice(strtol(devstring, NULL, 0),i, 
> type);
>   }
>   }
>   }
> @@ -652,8 +671,29 @@ static int drmOpenByName(const char *name)
>   */
>  int drmOpen(const char *name, const char *busid)
>  {
> +return drmOpenWithType(name, busid, DRM_NODE_PRIMARY);
> +}
> +
> +/**
> + * Open the DRM device with specified type.
> + *
> + * Looks up the specified name and bus ID, and opens the device found.  The
> + * entry in /dev

[PATCH 1/2] Add new drmOpenWithType function (v3)

[Frank Binns]

On 02/02/15 10:06, Jammy Zhou wrote:
> v2: Add drmGetMinorBase, and call drmOpenWithType in drmOpen
> v3: Pass 'type' to drmOpenByBusid and drmOpenDevice in drmOpenByName
>
> Signed-off-by: Jammy Zhou 
> ---
>  xf86drm.c | 63 
> ---
>  xf86drm.h |  9 -
>  2 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 345325a..810edfa 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -85,10 +85,6 @@
>  
>  #define DRM_MSG_VERBOSITY 3
>  
> -#define DRM_NODE_CONTROL 0
> -#define DRM_NODE_PRIMARY 1
> -#define DRM_NODE_RENDER 2
> -
>  static drmServerInfoPtr drm_server_info;
>  
>  void drmSetServerInfo(drmServerInfoPtr info)
> @@ -493,11 +489,24 @@ int drmAvailable(void)
>  return retval;
>  }
>  
> +static int drmGetMinorBase(int type)
> +{
> +switch (type) {
> +case DRM_NODE_PRIMARY:
> +default:
> +return 0;
> +case DRM_NODE_CONTROL:
> +return 64;
> +case DRM_NODE_RENDER:
> +return 128;
> +};
Wouldn't it be more sensible to return -1 in the default case given that
there's no validation of the 'type' argument in drmOpenWithType? It
feels wrong defaulting to the primary node in this case.

> +}
>  
>  /**
>   * Open the device by bus ID.
>   *
>   * \param busid bus ID.
> + * \param type device node type.
>   *
>   * \return a file descriptor on success, or a negative value on error.
>   *
> @@ -507,16 +516,17 @@ int drmAvailable(void)
>   *
>   * \sa drmOpenMinor() and drmGetBusid().
>   */
> -static int drmOpenByBusid(const char *busid)
> +static int drmOpenByBusid(const char *busid, int type)
>  {
>  inti, pci_domain_ok = 1;
>  intfd;
>  const char *buf;
>  drmSetVersion sv;
> +intbase = drmGetMinorBase(type);
>  
>  drmMsg("drmOpenByBusid: Searching for BusID %s\n", busid);
> -for (i = 0; i < DRM_MAX_MINOR; i++) {
> - fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
> +for (i = base; i < base + DRM_MAX_MINOR; i++) {
> + fd = drmOpenMinor(i, 1, type);
>   drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
>   if (fd >= 0) {
>   /* We need to try for 1.4 first for proper PCI domain support
> @@ -556,6 +566,7 @@ static int drmOpenByBusid(const char *busid)
>   * Open the device by name.
>   *
>   * \param name driver name.
> + * \param type the device node type.
>   * 
>   * \return a file descriptor on success, or a negative value on error.
>   * 
> @@ -566,19 +577,20 @@ static int drmOpenByBusid(const char *busid)
>   * 
>   * \sa drmOpenMinor(), drmGetVersion() and drmGetBusid().
>   */
> -static int drmOpenByName(const char *name)
> +static int drmOpenByName(const char *name, int type)
>  {
>  int   i;
>  int   fd;
>  drmVersionPtr version;
>  char *id;
> +int   base = drmGetMinorBase(type);
>  
>  /*
>   * Open the first minor number that matches the driver name and isn't
>   * already in use.  If it's in use it will have a busid assigned already.
>   */
> -for (i = 0; i < DRM_MAX_MINOR; i++) {
> - if ((fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY)) >= 0) {
> +for (i = base; i < base + DRM_MAX_MINOR; i++) {
> + if ((fd = drmOpenMinor(i, 1, type)) >= 0) {
>   if ((version = drmGetVersion(fd))) {
>   if (!strcmp(version->name, name)) {
>   drmFreeVersion(version);
> @@ -620,9 +632,9 @@ static int drmOpenByName(const char *name)
>   for (devstring = ++pt; *pt && *pt != ' '; ++pt)
>   ;
>   if (*pt) { /* Found busid */
> - return drmOpenByBusid(++pt);
> + return drmOpenByBusid(++pt, type);
>   } else { /* No busid */
> - return drmOpenDevice(strtol(devstring, NULL, 0),i, 
> DRM_NODE_PRIMARY);
> + return drmOpenDevice(strtol(devstring, NULL, 0),i, 
> type);
>   }
>   }
>   }
> @@ -652,8 +664,29 @@ static int drmOpenByName(const char *name)
>   */
>  int drmOpen(const char *name, const char *busid)
>  {
> +return drmOpenWithType(name, busid, DRM_NODE_PRIMARY);
> +}
> +
> +/**
> + * Open the DRM device with specified type.
> + *
> + * Looks up the specified name and bus ID, and opens the device found.  The
> + * entry in /dev/dri is created if necessary and if called by root.
> + *
> + * \param name driver name. Not referenced if bus ID is supplied.
> + * \param busid bus ID. Zero if not known.
> + * \param type the device node type to open, PRIMARY, CONTROL or RENDER
> + *
> + * \return a file descriptor on success, or a negative value on error.
> + *
> + * \internal
> + * It calls drmOpenByBusid() if \p busid is specified or drmOpenByName()
> + * otherwise.
> + */
> +int drmOpenWithType(const char *name, const char *busid, int type)
> +{

[PATCH libdrm 1/2] Rename DRM_NODE_RENDER to DRM_NODE_PRIMARY

[Frank Binns]

Ping

On 14/01/15 14:07, Frank Binns wrote:
> Now that there are render nodes it doesn't seem appropriate for the type of
> the card nodes to be DRM_NODE_RENDER. For this reason, rename this type to
> DRM_NODE_PRIMARY as this name better represents the purpose of these nodes.
>
> Signed-off-by: Frank Binns 
> ---
>  tests/dristat.c |  2 +-
>  xf86drm.c   | 10 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tests/dristat.c b/tests/dristat.c
> index 4f2ee80..449aa24 100644
> --- a/tests/dristat.c
> +++ b/tests/dristat.c
> @@ -268,7 +268,7 @@ int main(int argc, char **argv)
>  
>  for (i = 0; i < 16; i++) if (!minor || i == minor) {
>   sprintf(buf, DRM_DEV_NAME, DRM_DIR_NAME, i);
> - fd = drmOpenMinor(i, 1, DRM_NODE_RENDER);
> + fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
>   if (fd >= 0) {
>   printf("%s\n", buf);
>   if (mask & DRM_BUSID)   getbusid(fd);
> diff --git a/xf86drm.c b/xf86drm.c
> index d900b4b..a23d029 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -86,7 +86,7 @@
>  #define DRM_MSG_VERBOSITY 3
>  
>  #define DRM_NODE_CONTROL 0
> -#define DRM_NODE_RENDER 1
> +#define DRM_NODE_PRIMARY 1
>  
>  static drmServerInfoPtr drm_server_info;
>  
> @@ -444,7 +444,7 @@ int drmAvailable(void)
>  int   retval = 0;
>  int   fd;
>  
> -if ((fd = drmOpenMinor(0, 1, DRM_NODE_RENDER)) < 0) {
> +if ((fd = drmOpenMinor(0, 1, DRM_NODE_PRIMARY)) < 0) {
>  #ifdef __linux__
>   /* Try proc for backward Linux compatibility */
>   if (!access("/proc/dri/0", R_OK))
> @@ -485,7 +485,7 @@ static int drmOpenByBusid(const char *busid)
>  
>  drmMsg("drmOpenByBusid: Searching for BusID %s\n", busid);
>  for (i = 0; i < DRM_MAX_MINOR; i++) {
> - fd = drmOpenMinor(i, 1, DRM_NODE_RENDER);
> + fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
>   drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
>   if (fd >= 0) {
>   /* We need to try for 1.4 first for proper PCI domain support
> @@ -547,7 +547,7 @@ static int drmOpenByName(const char *name)
>   * already in use.  If it's in use it will have a busid assigned already.
>   */
>  for (i = 0; i < DRM_MAX_MINOR; i++) {
> - if ((fd = drmOpenMinor(i, 1, DRM_NODE_RENDER)) >= 0) {
> + if ((fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY)) >= 0) {
>   if ((version = drmGetVersion(fd))) {
>   if (!strcmp(version->name, name)) {
>   drmFreeVersion(version);
> @@ -591,7 +591,7 @@ static int drmOpenByName(const char *name)
>   if (*pt) { /* Found busid */
>   return drmOpenByBusid(++pt);
>   } else { /* No busid */
> - return drmOpenDevice(strtol(devstring, NULL, 0),i, 
> DRM_NODE_RENDER);
> + return drmOpenDevice(strtol(devstring, NULL, 0),i, 
> DRM_NODE_PRIMARY);
>   }
>   }
>   }

[PATCH libdrm 2/2] Add new drmOpenRender function

[Frank Binns]

Add a new function, drmOpenRender, that can be used to open render nodes. This
can be used in the same way that drmOpenControl is used to open control nodes.

Signed-off-by: Frank Binns 
---
 xf86drm.c | 40 ++--
 xf86drm.h |  2 ++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index a23d029..345325a 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -87,6 +87,7 @@

 #define DRM_NODE_CONTROL 0
 #define DRM_NODE_PRIMARY 1
+#define DRM_NODE_RENDER 2

 static drmServerInfoPtr drm_server_info;

@@ -305,6 +306,7 @@ static int chown_check_return(const char *path, uid_t 
owner, gid_t group)
 static int drmOpenDevice(long dev, int minor, int type)
 {
 stat_t  st;
+const char  *dev_name;
 charbuf[64];
 int fd;
 mode_t  devmode = DRM_DEV_MODE, serv_mode;
@@ -312,7 +314,21 @@ static int drmOpenDevice(long dev, int minor, int type)
 uid_t   user= DRM_DEV_UID;
 gid_t   group   = DRM_DEV_GID, serv_group;

-sprintf(buf, type ? DRM_DEV_NAME : DRM_CONTROL_DEV_NAME, DRM_DIR_NAME, 
minor);
+switch (type) {
+case DRM_NODE_PRIMARY:
+   dev_name = DRM_DEV_NAME;
+   break;
+case DRM_NODE_CONTROL:
+   dev_name = DRM_CONTROL_DEV_NAME;
+   break;
+case DRM_NODE_RENDER:
+   dev_name = DRM_RENDER_DEV_NAME;
+   break;
+default:
+   return -EINVAL;
+};
+
+sprintf(buf, dev_name, DRM_DIR_NAME, minor);
 drmMsg("drmOpenDevice: node name is %s\n", buf);

 if (drm_server_info) {
@@ -417,11 +433,26 @@ static int drmOpenMinor(int minor, int create, int type)
 {
 int  fd;
 char buf[64];
+const char *dev_name;

 if (create)
return drmOpenDevice(makedev(DRM_MAJOR, minor), minor, type);

-sprintf(buf, type ? DRM_DEV_NAME : DRM_CONTROL_DEV_NAME, DRM_DIR_NAME, 
minor);
+switch (type) {
+case DRM_NODE_PRIMARY:
+   dev_name = DRM_DEV_NAME;
+   break;
+case DRM_NODE_CONTROL:
+   dev_name = DRM_CONTROL_DEV_NAME;
+   break;
+case DRM_NODE_RENDER:
+   dev_name = DRM_RENDER_DEV_NAME;
+   break;
+default:
+   return -EINVAL;
+};
+
+sprintf(buf, dev_name, DRM_DIR_NAME, minor);
 if ((fd = open(buf, O_RDWR, 0)) >= 0)
return fd;
 return -errno;
@@ -646,6 +677,11 @@ int drmOpenControl(int minor)
 return drmOpenMinor(minor, 0, DRM_NODE_CONTROL);
 }

+int drmOpenRender(int minor)
+{
+return drmOpenMinor(minor, 0, DRM_NODE_RENDER);
+}
+
 /**
  * Free the version information returned by drmGetVersion().
  *
diff --git a/xf86drm.h b/xf86drm.h
index c024cc4..bfd0670 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -79,6 +79,7 @@ extern "C" {
 #define DRM_DIR_NAME  "/dev/dri"
 #define DRM_DEV_NAME  "%s/card%d"
 #define DRM_CONTROL_DEV_NAME  "%s/controlD%d"
+#define DRM_RENDER_DEV_NAME  "%s/renderD%d"
 #define DRM_PROC_NAME "/proc/dri/" /* For backward Linux compatibility */

 #define DRM_ERR_NO_DEVICE  (-1001)
@@ -552,6 +553,7 @@ do {register unsigned int __old __asm("o0");
\
 extern int   drmAvailable(void);
 extern int   drmOpen(const char *name, const char *busid);
 extern int drmOpenControl(int minor);
+extern int   drmOpenRender(int minor);
 extern int   drmClose(int fd);
 extern drmVersionPtr drmGetVersion(int fd);
 extern drmVersionPtr drmGetLibVersion(int fd);
-- 
1.8.5.4.gfd2

[PATCH libdrm 1/2] Rename DRM_NODE_RENDER to DRM_NODE_PRIMARY

[Frank Binns]

Now that there are render nodes it doesn't seem appropriate for the type of
the card nodes to be DRM_NODE_RENDER. For this reason, rename this type to
DRM_NODE_PRIMARY as this name better represents the purpose of these nodes.

Signed-off-by: Frank Binns 
---
 tests/dristat.c |  2 +-
 xf86drm.c   | 10 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/dristat.c b/tests/dristat.c
index 4f2ee80..449aa24 100644
--- a/tests/dristat.c
+++ b/tests/dristat.c
@@ -268,7 +268,7 @@ int main(int argc, char **argv)

 for (i = 0; i < 16; i++) if (!minor || i == minor) {
sprintf(buf, DRM_DEV_NAME, DRM_DIR_NAME, i);
-   fd = drmOpenMinor(i, 1, DRM_NODE_RENDER);
+   fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
if (fd >= 0) {
printf("%s\n", buf);
if (mask & DRM_BUSID)   getbusid(fd);
diff --git a/xf86drm.c b/xf86drm.c
index d900b4b..a23d029 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -86,7 +86,7 @@
 #define DRM_MSG_VERBOSITY 3

 #define DRM_NODE_CONTROL 0
-#define DRM_NODE_RENDER 1
+#define DRM_NODE_PRIMARY 1

 static drmServerInfoPtr drm_server_info;

@@ -444,7 +444,7 @@ int drmAvailable(void)
 int   retval = 0;
 int   fd;

-if ((fd = drmOpenMinor(0, 1, DRM_NODE_RENDER)) < 0) {
+if ((fd = drmOpenMinor(0, 1, DRM_NODE_PRIMARY)) < 0) {
 #ifdef __linux__
/* Try proc for backward Linux compatibility */
if (!access("/proc/dri/0", R_OK))
@@ -485,7 +485,7 @@ static int drmOpenByBusid(const char *busid)

 drmMsg("drmOpenByBusid: Searching for BusID %s\n", busid);
 for (i = 0; i < DRM_MAX_MINOR; i++) {
-   fd = drmOpenMinor(i, 1, DRM_NODE_RENDER);
+   fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY);
drmMsg("drmOpenByBusid: drmOpenMinor returns %d\n", fd);
if (fd >= 0) {
/* We need to try for 1.4 first for proper PCI domain support
@@ -547,7 +547,7 @@ static int drmOpenByName(const char *name)
  * already in use.  If it's in use it will have a busid assigned already.
  */
 for (i = 0; i < DRM_MAX_MINOR; i++) {
-   if ((fd = drmOpenMinor(i, 1, DRM_NODE_RENDER)) >= 0) {
+   if ((fd = drmOpenMinor(i, 1, DRM_NODE_PRIMARY)) >= 0) {
if ((version = drmGetVersion(fd))) {
if (!strcmp(version->name, name)) {
drmFreeVersion(version);
@@ -591,7 +591,7 @@ static int drmOpenByName(const char *name)
if (*pt) { /* Found busid */
return drmOpenByBusid(++pt);
} else { /* No busid */
-   return drmOpenDevice(strtol(devstring, NULL, 0),i, 
DRM_NODE_RENDER);
+   return drmOpenDevice(strtol(devstring, NULL, 0),i, 
DRM_NODE_PRIMARY);
}
}
}
-- 
1.8.5.4.gfd2

RFC on upstreaming of a Mediatek DRM modesetting driver

[Frank Binns]

On 01/12/14 15:28, Daniel Vetter wrote:
> On Mon, Dec 01, 2014 at 10:01:37AM +0000, Frank Binns wrote:
>> Hi,
>>
>> We are currently in negotiations with one of our customers (Mediatek) on
>> a strategy that will allow them to push a DRM modesetting driver into
>> the upstream kernel. We are writing to get people's opinions and
>> feedback on our proposed approach.
>>
>> Currently, our driver is structured in such a way that the display
>> driver is more tightly integrated with the GPU driver than we would
>> like. Although our kernel driver has been shipped with a GPL license for
>> a long time, it is not in a form that would be considered acceptable
>> upstream. Unfortunately, it is going to be a long process to get this
>> part of the driver into a reasonable state. However, in the meantime, we
>> don't want to prevent customer portions of the driver from being
>> upstreamed. With the work done on recent kernels, and with a willing
>> partner in Mediatek, we now think that we can restructure our driver in
>> such a way as to allow this to happen.
>>
>> We see two basic approaches to achieving this:
>> 1) Two independent DRM drivers, i.e. modesetting and render node drivers
>> 2) A single componentised DRM driver
>>
>> Our (IMG's) preferred approach is to have a single componentised DRM
>> driver. This is due to the following reasons:
>>
>> - Existing user space is not fully prepared to handle render nodes.
>>
>> - There is concern that any IMG DRM render node driver will need
>> knowledge about multiple SoCs, each one being from a different vendor.
>> Would this be deemed acceptable?
>>
>> - There is a trend, at least for DRM SoC drivers, towards using the
>> component interface. Although there appears to be very few (one?)
>> examples of GPU component drivers.
>>
>> To give some high level details on how we expect the componentised DRM
>> driver model to work, each vendor (in this case Mediatek) will write
>> their own DRM driver (supporting modesetting, dumb buffers, GEM, prime,
>> etc) and IMG will provide an almost entirely independent component
>> driver that adds in GPU support. Until our GPU driver is in a suitable
>> state this will most likely necessitate a small kernel patch to wire up
>> support, e.g. GPU specific ioctls.
>>
>> Cross-device and cross-process memory allocations will be made using the
>> DRM driver. In order for this memory to be shared with the GPU component
>> driver it will be necessary, at least for the time being, to export it
>> via prime and import it via a GPU ioctl. Synchronisation between the
>> display and GPU will be performed via reservation objects.
>>
>> Does this sound like a sane approach? Questions and/or feedback is very
>> welcome.
> Rule of thumb is that if it's an externally licensed IP block it should be
> a separate driver. Which is the case here. The idea is that the mostly
> generic IMG driver could be reused on other platforms that ship the same
> IP-block, while linking up with the respective display controller driver.
> The end result is 2 drm drivers:
> - Display block drm driver which expose KMS objects for modesetting, but
>   only very basic gem (just enough to allocate dumb framebuffers and
>   import/export dma-bufs).
> - Full-blown gem driver for the img render IP block.
>
> For an example look at the tegra/nouveau combo which can run on TK1.
>
> Plugggin in an IMG driver into each display block like it's currently done
> with all the armsoc stuff on android is imo completely no-go.
>
> Note that the component interface is completely irrelevant wrt the
> interface you expose to userspace. It's just an driver-internal helper
> library useful in certain situation. Not even the drm core really cares
> whether you use component helpers or not.
>
> Thanks, Daniel

OK, so it seems the consensus is that IMG should provide a separate
render-node only DRM driver.

Having not worked directly on the core DRM code I'm not completely
familiar with it but it seems to me that the DRIVER_MODESET flag has a
dual meaning. Firstly it means that the driver supports KMS and secondly
it means that a lot of the legacy stuff isn't supported. It also changes
the way in which driver initialisation is performed. Would it make sense
for the DRIVER_RENDER flag to have a similar effect? In other words,
should it turn off legacy stuff and use the newer method of driver
initialisation?

Thanks
Frank

RFC on upstreaming of a Mediatek DRM modesetting driver

[Frank Binns]

Hi,

We are currently in negotiations with one of our customers (Mediatek) on a 
strategy that will allow them to push a DRM modesetting driver into the 
upstream kernel. We are writing to get people's opinions and feedback on our 
proposed approach.

Currently, our driver is structured in such a way that the display driver is 
more tightly integrated with the GPU driver than we would like. Although our 
kernel driver has been shipped with a GPL license for a long time, it is not in 
a form that would be considered acceptable upstream. Unfortunately, it is going 
to be a long process to get this part of the driver into a reasonable state. 
However, in the meantime, we don't want to prevent customer portions of the 
driver from being upstreamed. With the work done on recent kernels, and with a 
willing partner in Mediatek, we now think that we can restructure our driver in 
such a way as to allow this to happen.

We see two basic approaches to achieving this:
1) Two independent DRM drivers, i.e. modesetting and render node drivers
2) A single componentised DRM driver

Our (IMG's) preferred approach is to have a single componentised DRM driver. 
This is due to the following reasons:

- Existing user space is not fully prepared to handle render nodes.

- There is concern that any IMG DRM render node driver will need knowledge 
about multiple SoCs, each one being from a different vendor. Would this be 
deemed acceptable?

- There is a trend, at least for DRM SoC drivers, towards using the component 
interface. Although there appears to be very few (one?) examples of GPU 
component drivers.

To give some high level details on how we expect the componentised DRM driver 
model to work, each vendor (in this case Mediatek) will write their own DRM 
driver (supporting modesetting, dumb buffers, GEM, prime, etc) and IMG will 
provide an almost entirely independent component driver that adds in GPU 
support. Until our GPU driver is in a suitable
state this will most likely necessitate a small kernel patch to wire up 
support, e.g. GPU specific ioctls.

Cross-device and cross-process memory allocations will be made using the DRM 
driver. In order for this memory to be shared with the GPU component driver it 
will be necessary, at least for the time being, to export it via prime and 
import it via a GPU ioctl. Synchronisation between the display and GPU will be 
performed via reservation objects.

Does this sound like a sane approach? Questions and/or feedback is very welcome.

Thanks
Frank 

[PATCH RESEND] drm: fix case where panic notifier isn't unregistered

[Frank Binns]

The framebuffer helper panic notifier is unregistered, in drm_fb_helper_fini(), 
when kernel_fb_helper_list goes from being non-empty to empty. However, in 
drm_fb_helper_single_fb_probe(), it's possible for the panic notifier to be 
registered without an element being added to this list if a driver's probe 
function returns 0. Make sure that an attempt to add the panic notifier is made 
only when adding an element to kernel_fb_helper_list.

Signed-off-by: Frank Binns 
---
This should hopefully have none of the whitespace damage introduced by my email 
client last time.

 drivers/gpu/drm/drm_fb_helper.c |   21 ++---
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a0d6e89..d3764b3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -807,21 +807,20 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper 
*fb_helper,
printk(KERN_INFO "fb%d: %s frame buffer device\n", info->node,
   info->fix.id);

+   /* Switch back to kernel console on panic */
+   /* multi card linked list maybe */
+   if (list_empty(_fb_helper_list)) {
+   printk(KERN_INFO "drm: registered panic notifier\n");
+   atomic_notifier_chain_register(_notifier_list,
+  );
+   register_sysrq_key('v', 
_drm_fb_helper_restore_op);
+   }
+
+   list_add(_helper->kernel_fb_list, _fb_helper_list);
} else {
drm_fb_helper_set_par(info);
}

-   /* Switch back to kernel console on panic */
-   /* multi card linked list maybe */
-   if (list_empty(_fb_helper_list)) {
-   printk(KERN_INFO "drm: registered panic notifier\n");
-   atomic_notifier_chain_register(_notifier_list,
-  );
-   register_sysrq_key('v', _drm_fb_helper_restore_op);
-   }
-   if (new_fb)
-   list_add(_helper->kernel_fb_list, _fb_helper_list);
-
return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
-- 
1.7.5.4

[PATCH] drm: fix case where panic notifier isn't unregistered

[Frank Binns]

Hi,
I don't know if this patch got missed in the list traffic or if I did something 
wrong but it doesn't appear to have been picked up. This is my first time 
contributing so if I did anything wrong some pointers would be appreciated.

Thanks
Frank


> From: dri-devel-bounces+frank.binns=imgtec.com at lists.freedesktop.org 
> [mailto:dri-devel-bounces+frank.binns=imgtec.com at lists.freedesktop.org] On 
> Behalf Of Frank Binns
> Sent: 14 May 2012 16:28
> To: 'airlied at linux.ie'; 'dri-devel at lists.freedesktop.org'
> Subject: [PATCH] drm: fix case where panic notifier isn't unregistered
>
> The framebuffer helper panic notifier is unregistered, in 
> drm_fb_helper_fini(), when kernel_fb_helper_list goes from being non-empty to 
> empty. However, in drm_fb_helper_single_fb_probe(), it's possible for the 
> panic notifier to be registered without an element being added to this list 
> if a driver's probe function returns 0. Make sure that an attempt to add the 
> panic notifier is made only when adding an element to kernel_fb_helper_list.
>
> Signed-off-by: Frank Binns 
> ---
> drivers/gpu/drm/drm_fb_helper.c |?? 21 ++---
> 1 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a0d6e89..d3764b3 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -807,21 +807,20 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper 
> *fb_helper,
>?? printk(KERN_INFO "fb%d: %s frame buffer 
>device\n", info->node,
>?? ???info->fix.id);
>
> + /* Switch back to kernel console on panic */
> + /* multi card linked list maybe */
> + if (list_empty(_fb_helper_list)) {
> + printk(KERN_INFO "drm: 
> registered panic notifier\n");
> + 
> atomic_notifier_chain_register(_notifier_list,
> +
>  ?? );
> + register_sysrq_key('v', 
> _drm_fb_helper_restore_op);
> + }
> +
> + list_add(_helper->kernel_fb_list, 
> _fb_helper_list);
>?? } else {
>?? drm_fb_helper_set_par(info);
>?? }
>
> -? /* Switch back to kernel console on panic */
> -? /* multi card linked list maybe */
> -? if (list_empty(_fb_helper_list)) {
> -? printk(KERN_INFO "drm: registered panic 
> notifier\n");
> -? 
> atomic_notifier_chain_register(_notifier_list,
> -?
>  ?? );
> -? register_sysrq_key('v', 
> _drm_fb_helper_restore_op);
> -? }
> -? if (new_fb)
> -? list_add(_helper->kernel_fb_list, 
> _fb_helper_list);
> -
>?? return 0;
> }
> EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
> -- 
> 1.7.5.4

RE: [PATCH] drm: fix case where panic notifier isn't unregistered

[Frank Binns]

Hi,
I don't know if this patch got missed in the list traffic or if I did something 
wrong but it doesn't appear to have been picked up. This is my first time 
contributing so if I did anything wrong some pointers would be appreciated.

Thanks
Frank


 From: dri-devel-bounces+frank.binns=imgtec@lists.freedesktop.org 
 [mailto:dri-devel-bounces+frank.binns=imgtec@lists.freedesktop.org] On 
 Behalf Of Frank Binns
 Sent: 14 May 2012 16:28
 To: 'airl...@linux.ie'; 'dri-devel@lists.freedesktop.org'
 Subject: [PATCH] drm: fix case where panic notifier isn't unregistered

 The framebuffer helper panic notifier is unregistered, in 
 drm_fb_helper_fini(), when kernel_fb_helper_list goes from being non-empty to 
 empty. However, in drm_fb_helper_single_fb_probe(), it's possible for the 
 panic notifier to be registered without an element being added to this list 
 if a driver's probe function returns 0. Make sure that an attempt to add the 
 panic notifier is made only when adding an element to kernel_fb_helper_list.

 Signed-off-by: Frank Binns frank.bi...@imgtec.com
 ---
 drivers/gpu/drm/drm_fb_helper.c |   21 ++---
 1 files changed, 10 insertions(+), 11 deletions(-)

 diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
 index a0d6e89..d3764b3 100644
 --- a/drivers/gpu/drm/drm_fb_helper.c
 +++ b/drivers/gpu/drm/drm_fb_helper.c
 @@ -807,21 +807,20 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper 
 *fb_helper,
   printk(KERN_INFO fb%d: %s frame buffer 
device\n, info-node,
      info-fix.id);

 + /* Switch back to kernel console on panic */
 + /* multi card linked list maybe */
 + if (list_empty(kernel_fb_helper_list)) {
 + printk(KERN_INFO drm: 
 registered panic notifier\n);
 + 
 atomic_notifier_chain_register(panic_notifier_list,
 +
     paniced);
 + register_sysrq_key('v', 
 sysrq_drm_fb_helper_restore_op);
 + }
 +
 + list_add(fb_helper-kernel_fb_list, 
 kernel_fb_helper_list);
   } else {
   drm_fb_helper_set_par(info);
   }

 -  /* Switch back to kernel console on panic */
 -  /* multi card linked list maybe */
 -  if (list_empty(kernel_fb_helper_list)) {
 -  printk(KERN_INFO drm: registered panic 
 notifier\n);
 -  
 atomic_notifier_chain_register(panic_notifier_list,
 - 
     paniced);
 -  register_sysrq_key('v', 
 sysrq_drm_fb_helper_restore_op);
 -  }
 -  if (new_fb)
 -  list_add(fb_helper-kernel_fb_list, 
 kernel_fb_helper_list);
 -
   return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
 -- 
 1.7.5.4


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RESEND] drm: fix case where panic notifier isn't unregistered

[Frank Binns]

The framebuffer helper panic notifier is unregistered, in drm_fb_helper_fini(), 
when kernel_fb_helper_list goes from being non-empty to empty. However, in 
drm_fb_helper_single_fb_probe(), it's possible for the panic notifier to be 
registered without an element being added to this list if a driver's probe 
function returns 0. Make sure that an attempt to add the panic notifier is made 
only when adding an element to kernel_fb_helper_list.

Signed-off-by: Frank Binns frank.bi...@imgtec.com
---
This should hopefully have none of the whitespace damage introduced by my email 
client last time.

 drivers/gpu/drm/drm_fb_helper.c |   21 ++---
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a0d6e89..d3764b3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -807,21 +807,20 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper 
*fb_helper,
printk(KERN_INFO fb%d: %s frame buffer device\n, info-node,
   info-fix.id);
 
+   /* Switch back to kernel console on panic */
+   /* multi card linked list maybe */
+   if (list_empty(kernel_fb_helper_list)) {
+   printk(KERN_INFO drm: registered panic notifier\n);
+   atomic_notifier_chain_register(panic_notifier_list,
+  paniced);
+   register_sysrq_key('v', 
sysrq_drm_fb_helper_restore_op);
+   }
+
+   list_add(fb_helper-kernel_fb_list, kernel_fb_helper_list);
} else {
drm_fb_helper_set_par(info);
}
 
-   /* Switch back to kernel console on panic */
-   /* multi card linked list maybe */
-   if (list_empty(kernel_fb_helper_list)) {
-   printk(KERN_INFO drm: registered panic notifier\n);
-   atomic_notifier_chain_register(panic_notifier_list,
-  paniced);
-   register_sysrq_key('v', sysrq_drm_fb_helper_restore_op);
-   }
-   if (new_fb)
-   list_add(fb_helper-kernel_fb_list, kernel_fb_helper_list);
-
return 0;
 }
 EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
-- 
1.7.5.4


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: fix case where panic notifier isn't unregistered

[Frank Binns]

The framebuffer helper panic notifier is unregistered, in drm_fb_helper_fini(), 
when kernel_fb_helper_list goes from being non-empty to empty. However, in 
drm_fb_helper_single_fb_probe(), it's possible for the panic notifier to be 
registered without an element being added to this list if a driver's probe 
function returns 0. Make sure that an attempt to add the panic notifier is made 
only when adding an element to kernel_fb_helper_list.

Signed-off-by: Frank Binns 
---
drivers/gpu/drm/drm_fb_helper.c |   21 ++---
1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a0d6e89..d3764b3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -807,21 +807,20 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper 
*fb_helper,
   printk(KERN_INFO "fb%d: %s frame buffer 
device\n", info->node,
  info->fix.id);
+ /* Switch back to kernel console on panic */
+ /* multi card linked list maybe */
+ if (list_empty(_fb_helper_list)) {
+ printk(KERN_INFO "drm: registered 
panic notifier\n");
+ 
atomic_notifier_chain_register(_notifier_list,
+   
 );
+ register_sysrq_key('v', 
_drm_fb_helper_restore_op);
+ }
+
+ list_add(_helper->kernel_fb_list, 
_fb_helper_list);
   } else {
   drm_fb_helper_set_par(info);
   }
-  /* Switch back to kernel console on panic */
-  /* multi card linked list maybe */
-  if (list_empty(_fb_helper_list)) {
-  printk(KERN_INFO "drm: registered panic 
notifier\n");
-  
atomic_notifier_chain_register(_notifier_list,
-   
  );
-  register_sysrq_key('v', 
_drm_fb_helper_restore_op);
-  }
-  if (new_fb)
-  list_add(_helper->kernel_fb_list, 
_fb_helper_list);
-
   return 0;
}
EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
--
1.7.5.4

-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20120514/184b08ee/attachment-0001.htm>

[PATCH] drm: fix case where panic notifier isn't unregistered

[Frank Binns]

The framebuffer helper panic notifier is unregistered, in drm_fb_helper_fini(), 
when kernel_fb_helper_list goes from being non-empty to empty. However, in 
drm_fb_helper_single_fb_probe(), it's possible for the panic notifier to be 
registered without an element being added to this list if a driver's probe 
function returns 0. Make sure that an attempt to add the panic notifier is made 
only when adding an element to kernel_fb_helper_list.

Signed-off-by: Frank Binns frank.bi...@imgtec.com
---
drivers/gpu/drm/drm_fb_helper.c |   21 ++---
1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a0d6e89..d3764b3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -807,21 +807,20 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper 
*fb_helper,
   printk(KERN_INFO fb%d: %s frame buffer 
device\n, info-node,
  info-fix.id);
+ /* Switch back to kernel console on panic */
+ /* multi card linked list maybe */
+ if (list_empty(kernel_fb_helper_list)) {
+ printk(KERN_INFO drm: registered 
panic notifier\n);
+ 
atomic_notifier_chain_register(panic_notifier_list,
+   
 paniced);
+ register_sysrq_key('v', 
sysrq_drm_fb_helper_restore_op);
+ }
+
+ list_add(fb_helper-kernel_fb_list, 
kernel_fb_helper_list);
   } else {
   drm_fb_helper_set_par(info);
   }
-  /* Switch back to kernel console on panic */
-  /* multi card linked list maybe */
-  if (list_empty(kernel_fb_helper_list)) {
-  printk(KERN_INFO drm: registered panic 
notifier\n);
-  
atomic_notifier_chain_register(panic_notifier_list,
-   
  paniced);
-  register_sysrq_key('v', 
sysrq_drm_fb_helper_restore_op);
-  }
-  if (new_fb)
-  list_add(fb_helper-kernel_fb_list, 
kernel_fb_helper_list);
-
   return 0;
}
EXPORT_SYMBOL(drm_fb_helper_single_fb_probe);
--
1.7.5.4

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel