date:20240228

On 28.02.24 22:34, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 09:50:22PM +0100, Javier Carrasco wrote:
>> On 28.02.24 21:41, Matthias Kaehlcke wrote:
>>> On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
 On 28.02.24 19:10, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>> Most of the functionality this driver provides can be used by non-hub
>> devices as well.
>>
>> To account for the hub-specific code, add a flag to the device data
>> structure and check its value for hub-specific code.
>>
>> The 'always_powered_in_supend' attribute is only available for hub
>> devices, keeping the driver's default behavior for non-hub devices (keep
>> on in suspend).
>>
>> Signed-off-by: Javier Carrasco 
>> ---
>>  drivers/usb/misc/onboard_usb_dev.c | 25 +++--
>>  drivers/usb/misc/onboard_usb_dev.h | 10 ++
>>  2 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_dev.c 
>> b/drivers/usb/misc/onboard_usb_dev.c
>> index e1779bd2d126..df0ed172c7ec 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.c
>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct 
>> device *dev)
>>  struct usbdev_node *node;
>>  bool power_off = true;
>>  
>> -if (onboard_dev->always_powered_in_suspend)
>> +if (onboard_dev->always_powered_in_suspend &&
>> +!onboard_dev->pdata->is_hub)
>>  return 0;
>
> With this non-hub devices would always be powered down, since
> 'always_powerd_in_suspend' is not set for them. This should be:
>

 May I ask you what you meant in v4 with this comment?

> Even without the sysfs attribute the field 'always_powered_in_suspend'
> could
> be set to true by probe() for non-hub devices.
>>>
>>> struct onboard_dev always has the field 'always_powered_in_suspend',
>>> even for non-hubs, that don't have the corresponding sysfs attribute.
>>> Currently it is left uninitialized (i.e. false) for non-hubs. Instead
>>> it could be initialized to true by probe() for non-hubs, which would
>>> be semantically correct. With that it wouldn't be necessary to check
>>> here whether a device is hub, because the field would provide the
>>> necessary information.
>>>
>>
>> That is maybe what is confusing me a bit. Should it not be false for
>> non-hub devices? That property is only meant for hubs, so why should
>> non-hub devices be always powered in suspend? I thought it should always
>> be false for non-hub devices, and configurable for hubs.
> 
> I suspect the confusion stems from the sysfs attribute 'always_powered_...'
> vs. the struct field with the same name.
> 
> The sysfs attribute defaults to 'false', which results in USB devices
> being powered down in suspend. That was the desired behavior for a device
> I was working on when I implemented this driver, but in hindsight I think
> the default should have been 'true'.
> 
> We agreed that non-hub devices shouldn't be powered down in suspend. It
> would be unexpected for users and could have side effects like delays
> or losing status. Since (I think) we can't change the default of the
> attribute we said we'd limit it to hubs, and not create it for other
> types of USB devices. Other USB devices would remain powered during
> system suspend.
> 
> Are we in agreement up to this point, in particular that non-hub
> devices should remain powered?
> 
> struct onboard_dev has the field 'always_powered_...', which in the
> existing code is *always* associated with the sysfs attribute of
> the same name. But there is no reason to not to use the field when
> the sysfs attribute does not exist. For any device at any given time
> the field could indicate whether the device should be remain powered
> during suspend. For hubs the value can be changed at runtime
> through the sysfs attribute, for non-hubs it would be static and
> always indicate that the device should remain powered.
> 
> Does that clarify your doubts?

It is crystal clear now, thank you.

Best regards,
Javier Carrasco

Re: [PATCH] dt-bindings: display: atmel,lcdc: convert to dtschema

2024-02-28 Thread Dharma.B

On 28/02/24 3:53 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the 
> content is safe
> 
> On 28/02/2024 11:18, dharm...@microchip.com wrote:
>> On 28/02/24 12:43 pm, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the 
>>> content is safe
>>>
>>> On 28/02/2024 07:59, dharm...@microchip.com wrote:

>
> I don't know what's this exactly, but if embedded display then maybe
> could be part of this device node. If some other display, then maybe you
> need another schema, with compatible? But first I would check how others
> are doing this.

 Okay, then I think the driver also needs to be modified, currently the
 driver parses the phandle and looks for these properties. Also the
 corresponding dts files.
>>>
>>> Driver does not have to be modified in my proposal. You would still have
>>> phandle.
>>
>> If I understand correctly, I could define the dt bindings as below
>>
>> display:
>>   $ref: /schemas/types.yaml#/definitions/phandle
>>   description: A phandle pointing to the display node.
>>
>> panel:
>>   $ref: panel/panel-common.yaml#
>>   properties:
>>
> 
> So these are standard panel bindings? Then the node should live outside
> of lcdc. If current driver needs to poke inside panel and panel could be
> anything, then probably you need peripheral-props-like approach. :/

Thank you so much, so can I use something like this

   display:
 $ref: /schemas/types.yaml#/definitions/phandle
 description: A phandle pointing to the display node.

patternProperties:
   "^panel":
 type: object
 properties:
   atmel,dmacon:
   atmel,lcdcon2:
   :
   :
 required:
   - atmel,dmacon
   - atmel,lcdcon2
   - atmel,guard-time
   - bits-per-pixel

 additionalProperties: false

I tested it by removing the required property in the panel node and it 
flagged it as an issue.

> 
> Best regards,
> Krzysztof
> 

-- 
With Best Regards,
Dharma B.

RE: [PATCH v12 0/8] Enable Adaptive Sync SDP Support for DP

2024-02-28 Thread Golani, Mitulkumar Ajitkumar

> Subject: Re: [PATCH v12 0/8] Enable Adaptive Sync SDP Support for DP
> 
> On Wed, 28 Feb 2024, Mitul Golani 
> wrote:
> > -v12:
> > - Update cover letter
> 
> Did you just send the entire series again within 30 minutes just to update the
> cover letter? You could've just replied to the cover letter...

Sorry, I should have used git send-email --in-reply-to, will take care from 
next time onwards.

Regards,
Mitul

linux-next: build failure after merge of the kunit-next tree

2024-02-28 Thread Stephen Rothwell

Hi all,

After merging the kunit-next tree, today's linux-next build (x86_64
allmodconfig) failed like this:

In file included from drivers/gpu/drm/tests/drm_buddy_test.c:7:
drivers/gpu/drm/tests/drm_buddy_test.c: In function 
'drm_test_buddy_alloc_range_bias':
drivers/gpu/drm/tests/drm_buddy_test.c:191:40: error: format '%u' expects a 
matching 'unsigned int' argument [-Werror=format=]
  191 |"buddy_alloc failed with 
bias(%x-%x), size=%u, ps=%u\n",
  |
^~~
include/kunit/test.h:597:37: note: in definition of macro '_KUNIT_FAILED'
  597 | fmt,
   \
  | ^~~
include/kunit/test.h:662:9: note: in expansion of macro 'KUNIT_UNARY_ASSERTION'
  662 | KUNIT_UNARY_ASSERTION(test, 
   \
  | ^
include/kunit/test.h:1233:9: note: in expansion of macro 
'KUNIT_FALSE_MSG_ASSERTION'
 1233 | KUNIT_FALSE_MSG_ASSERTION(test, 
   \
  | ^
drivers/gpu/drm/tests/drm_buddy_test.c:186:17: note: in expansion of macro 
'KUNIT_ASSERT_FALSE_MSG'
  186 | KUNIT_ASSERT_FALSE_MSG(test,
  | ^~
drivers/gpu/drm/tests/drm_buddy_test.c:191:91: note: format string is defined 
here
  191 |"buddy_alloc failed with 
bias(%x-%x), size=%u, ps=%u\n",
  | 
 ~^
  | 
  |
  | 
  unsigned int
cc1: all warnings being treated as errors

Caused by commit

  806cb2270237 ("kunit: Annotate _MSG assertion variants with gnu printf 
specifiers")

interacting with commit

  c70703320e55 ("drm/tests/drm_buddy: add alloc_range_bias test")

from the drm-misc-fixes tree.

I have applied the following patch for today (this should probably
actually be fixed in the drm-misc-fixes tree).

From: Stephen Rothwell 
Date: Thu, 29 Feb 2024 15:18:36 +1100
Subject: [PATCH] fix up for "drm/tests/drm_buddy: add alloc_range_bias test"

Signed-off-by: Stephen Rothwell 
---
 drivers/gpu/drm/tests/drm_buddy_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c 
b/drivers/gpu/drm/tests/drm_buddy_test.c
index 1e73e3f0d278..369edf587b44 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -188,7 +188,7 @@ static void drm_test_buddy_alloc_range_bias(struct kunit 
*test)
  bias_end, size, 
ps,
  ,
  
DRM_BUDDY_RANGE_ALLOCATION),
-  "buddy_alloc failed with bias(%x-%x), 
size=%u, ps=%u\n",
+  "buddy_alloc failed with bias(%x-%x), 
size=%u\n",
   bias_start, bias_end, size);
bias_rem -= size;
 
-- 
2.43.0

-- 
Cheers,
Stephen Rothwell


pgpdbIfYhJD7t.pgp
Description: OpenPGP digital signature

Re: ECC memory semantics for heaps

2024-02-28 Thread John Stultz

On Wed, Feb 28, 2024 at 7:24 AM Maxime Ripard  wrote:
>
> I'm currently working on a platform that seems to have togglable RAM ECC
> support. Enabling ECC reduces the memory capacity and memory bandwidth,
> so while it's a good idea to protect most of the system, it's not worth
> it for things like framebuffers that won't really be affected by a
> bitflip.
>
> It's currently setup by enabling ECC on the entire memory, and then
> having a region of memory where ECC is disabled and where we're supposed
> to allocate from for allocations that don't need it.
>
> My first thought to support this was to create a reserved memory region
> for the !ECC memory, and to create a heap to allocate buffers from that
> region. That would leave the system protected by ECC, while enabling
> userspace to be nicer to the system by allocating buffers from the !ECC
> region if it doesn't need it.
>
> However, this creates basically a new combination compared to the one we
> already have (ie, physically contiguous vs virtually contiguous), and we
> probably would want to throw in cacheable vs non-cacheable too.
>
> If we had to provide new heaps for each variation, we would have 8 heaps
> (and 6 new ones), which could be fine I guess but would still increase
> quite a lot the number of heaps we have so far.
>
> Is it something that would be a problem? If it is, do you see another
> way to support those kind of allocations (like providing hints through
> the ioctl maybe?)?

So, the dma-buf heaps interface uses chardevs so that we can have a
lot of flexibility in the types of heaps (and don't have the risk of
bitmask exhaustion like ION had). So I don't see adding many
differently named heaps as particularly problematic.

That said, if there are truly generic properties (cacheable vs
non-cachable is maybe one of those) which apply to most heaps, I'm
open to making use of the flags. But I want to avoid having per-heap
flags, it really needs to be a generic attribute.

And I personally don't mind the idea of having things added as heaps
initially, and potentially upgrading them to flags if needed (allowing
heap drivers to optionally enumerate the old chardevs behind a config
option for backwards compatibility).

How common is the hardware that is going to provide this configurable
ECC option, and will you really want the option on all of the heap
types? Will there be any hardware constraint limitations caused by the
ECC/!ECC flags? (ie: Devices that can't use !ECC allocated buffers?)

If not, I wonder if it would make sense to have something more along
the lines using a fcntl()  like how F_SEAL_* is used with memfds?
With some of the discussion around "restricted"/"secure" heaps that
can change state, I've liked this idea of just allocating dmabufs from
normal heaps and then using fcntl or something similar to modify
properties of the buffer that are separate from the type of memory
that is needed to be allocated to satisfy device constraints.

thanks
-john

Re: [PATCH 11/11] drm/mediatek: Rename "pending_needs_vblank" to "needs_vblank"

2024-02-28 Thread 宋孝謙

[PATCH 1/3] dt-bindings: display: mediatek: gamma: Change MT8195 to single enum group

Since MT8195 gamma has multiple bank for 12 bits LUT and it is
different from any other SoC LUT setting.

So we move MT8195 compatible from the one of items to the
single enum group.

Signed-off-by: Jason-JH.Lin 
---
 .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml| 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml 
b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
index c6641acd75d6..3e6cb8f48bcc 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
@@ -24,6 +24,7 @@ properties:
   - enum:
   - mediatek,mt8173-disp-gamma
   - mediatek,mt8183-disp-gamma
+  - mediatek,mt8195-disp-gamma
   - items:
   - enum:
   - mediatek,mt6795-disp-gamma
@@ -33,7 +34,6 @@ properties:
   - mediatek,mt8186-disp-gamma
   - mediatek,mt8188-disp-gamma
   - mediatek,mt8192-disp-gamma
-  - mediatek,mt8195-disp-gamma
   - const: mediatek,mt8183-disp-gamma
 
   reg:
-- 
2.18.0

[PATCH 2/3] dt-bindings: display: mediatek: gamma: Add support for MT8188

The gamma LUT setting of MT8188 and MT8195 are the same, so we create
a one of items for MT8188 to reuse the driver data settings of MT8195.

Signed-off-by: Jason-JH.Lin 
---
 .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml  | 4 
 1 file changed, 4 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml 
b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
index 3e6cb8f48bcc..90c454eea06f 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
@@ -29,6 +29,10 @@ properties:
   - enum:
   - mediatek,mt6795-disp-gamma
   - const: mediatek,mt8173-disp-gamma
+  - items:
+  - enum:
+  - mediatek,mt8188-disp-gamma
+  - const: mediatek,mt8195-disp-gamma
   - items:
   - enum:
   - mediatek,mt8186-disp-gamma
-- 
2.18.0

[PATCH 3/3] drm/mediatek: Add gamma support for MT8195

Since MT8195 compatible is in the single enum group, we have to add its
compatible into mediatek-drm component binding table to ensure that
it can be bound as a ddp_comp.

Signed-off-by: Jason-JH.Lin 
---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c 
b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 14a1e0157cc4..93303bff8f34 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -707,6 +707,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
  .data = (void *)MTK_DISP_GAMMA, },
{ .compatible = "mediatek,mt8183-disp-gamma",
  .data = (void *)MTK_DISP_GAMMA, },
+   { .compatible = "mediatek,mt8195-disp-gamma",
+ .data = (void *)MTK_DISP_GAMMA, },
{ .compatible = "mediatek,mt8195-disp-merge",
  .data = (void *)MTK_DISP_MERGE },
{ .compatible = "mediatek,mt2701-disp-mutex",
-- 
2.18.0

[PATCH 0/3] Add GAMMA 12-bit LUT support for MT8188

From: Jason-jh Lin 

Since MT8195 supports GAMMA 12-bit LUT after the landing of [1] series,
we can now add support for MT8188.

[1] MediaTek DDP GAMMA - 12-bit LUT support
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=792516

Jason-JH.Lin (3):
  dt-bindings: display: mediatek: gamma: Change MT8195 to single enum
group
  dt-bindings: display: mediatek: gamma: Add support for MT8188
  drm/mediatek: Add gamma support for MT8195

 .../bindings/display/mediatek/mediatek,gamma.yaml   | 6 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.18.0

Re: [PATCH] drm/imagination: Kconfig: add 'PAGE_SIZE=4K' dependency

2024-02-28 Thread yaolu



在 2024/2/28 18:47, Matt Coster 写道:

Hi, thanks for the patch!

On 28/02/2024 01:23, Lu Yao wrote:

When 'PAGE_SIZE=64K',the following compilation error occurs:
"
   ../drivers/gpu/drm/imagination/pvr_fw_mips.c: In function
‘pvr_mips_fw_process’:
   ../drivers/gpu/drm/imagination/pvr_fw_mips.c:140:60: error: array
subscript 0 is outside the bounds of an interior zero-length array
‘dma_addr_t[0]’ {aka ‘long long unsigned int[]’}
[-Werror=zero-length-bounds]
   140 |   boot_data->pt_phys_addr[page_nr] =
mips_data->pt_dma_addr[src_page_nr] +
~~^
   In file included from ../drivers/gpu/drm/imagination/pvr_fw_mips.c:6:
   ../drivers/gpu/drm/imagination/pvr_fw_mips.h:30:13: note: while
referencing ‘pt_dma_addr’
30 |  dma_addr_t pt_dma_addr[PVR_MIPS_PT_PAGE_COUNT];
"

This is because 'PVR_MIPS_PT_PAGE_COUNT' is defined as
'(ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES * ROGUE_MIPSFW_PAGE_SIZE_4K) \

PAGE_SHIFT', and under the above conditions, 'PAGE_SHIFT' is '16',

'ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES' is '4','ROGUE_MIPSFW_PAGE_SIZE_4K'
is '4096',so 'PVR_MIPS_PT_PAGE_COUNT' is '0' causing the member
'pt_dma_addr' to be incorrectly defined.

The whole MIPS page table system is supposed to be host page-size
agnostic. In practice, we don’t regularly test on non-4K platforms so
this may well be broken in subtle or not-so-subtle (as in this instance)
ways. There has been at least some testing with 64K host pages – the
configs from TI for the AM62-SK dev boards have that as the default (or
at least they did when we started working with them).

I’m loathed to accept this patch without at least investigating how deep
the problems go; the true fix here should be to fix the problems causing
this build error rather than just gating off non-4K hosts.

I’ll have a dig this afternoon to see what I can find. Did you try
anything to fix this yourself? It would be nice to not duplicate effort
on this if you have.


No, I haven't tried, and I'm currently disabling the DRM_POWERVR driver. 
Actually,
I don't have any IMG graphics cards. This error was found when  compiling the 
kernel
randomly.Looks like it's up to you to sort this out reasonably.;)

Regards,
Lu


Cheers,
Matt


Signed-off-by: Lu Yao
---
  drivers/gpu/drm/imagination/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/Kconfig 
b/drivers/gpu/drm/imagination/Kconfig
index 3bfa2ac212dc..e585922f634d 100644
--- a/drivers/gpu/drm/imagination/Kconfig
+++ b/drivers/gpu/drm/imagination/Kconfig
@@ -3,7 +3,7 @@
  
  config DRM_POWERVR

tristate "Imagination Technologies PowerVR (Series 6 and later) & IMG 
Graphics"
-   depends on ARM64
+   depends on (ARM64 && ARM64_PAGE_SHIFT=12)
depends on DRM
depends on PM
select DRM_EXEC

Re: [PATCH v2 6/9] drm/vkms: Add YUV support

2024-02-28 Thread Arthur Grillo




On 27/02/24 17:01, Arthur Grillo wrote:
> 
> 
> On 27/02/24 12:02, Louis Chauvet wrote:
>> Hi Pekka,
>>
>> For all the comment related to the conversion part, maybe Arthur have an 
>> opinion on it, I took his patch as a "black box" (I did not want to 
>> break (and debug) it).
>>
>> Le 26/02/24 - 14:19, Pekka Paalanen a écrit :
>>> On Fri, 23 Feb 2024 12:37:26 +0100
>>> Louis Chauvet  wrote:
>>>
 From: Arthur Grillo 

 Add support to the YUV formats bellow:

 - NV12
 - NV16
 - NV24
 - NV21
 - NV61
 - NV42
 - YUV420
 - YUV422
 - YUV444
 - YVU420
 - YVU422
 - YVU444

 The conversion matrices of each encoding and range were obtained by
 rounding the values of the original conversion matrices multiplied by
 2^8. This is done to avoid the use of fixed point operations.

 Signed-off-by: Arthur Grillo 
 [Louis Chauvet: Adapted Arthur's work and implemented the read_line_t
 callbacks for yuv formats]
 Signed-off-by: Louis Chauvet 
 ---
  drivers/gpu/drm/vkms/vkms_composer.c |   2 +-
  drivers/gpu/drm/vkms/vkms_drv.h  |   6 +-
  drivers/gpu/drm/vkms/vkms_formats.c  | 289 
 +--
  drivers/gpu/drm/vkms/vkms_formats.h  |   4 +
  drivers/gpu/drm/vkms/vkms_plane.c|  14 +-
  5 files changed, 295 insertions(+), 20 deletions(-)

 diff --git a/drivers/gpu/drm/vkms/vkms_composer.c 
 b/drivers/gpu/drm/vkms/vkms_composer.c
 index e555bf9c1aee..54fc5161d565 100644
 --- a/drivers/gpu/drm/vkms/vkms_composer.c
 +++ b/drivers/gpu/drm/vkms/vkms_composer.c
 @@ -312,7 +312,7 @@ static void blend(struct vkms_writeback_job *wb,
 * buffer [1]
 */
current_plane->pixel_read_line(
 -  current_plane->frame_info,
 +  current_plane,
x_start,
y_start,
direction,
 diff --git a/drivers/gpu/drm/vkms/vkms_drv.h 
 b/drivers/gpu/drm/vkms/vkms_drv.h
 index ccc5be009f15..a4f6456cb971 100644
 --- a/drivers/gpu/drm/vkms/vkms_drv.h
 +++ b/drivers/gpu/drm/vkms/vkms_drv.h
 @@ -75,6 +75,8 @@ enum pixel_read_direction {
READ_RIGHT
  };
  
 +struct vkms_plane_state;
 +
  /**
  <<< HEAD
   * typedef pixel_read_line_t - These functions are used to read a pixel 
 line in the source frame,
 @@ -87,8 +89,8 @@ enum pixel_read_direction {
   * @out_pixel: Pointer where to write the pixel value. Pixels will be 
 written between x_start and
   *  x_end.
   */
 -typedef void (*pixel_read_line_t)(struct vkms_frame_info *frame_info, int 
 x_start, int y_start, enum
 -  pixel_read_direction direction, int count, struct pixel_argb_u16 
 out_pixel[]);
 +typedef void (*pixel_read_line_t)(struct vkms_plane_state *frame_info, 
 int x_start, int y_start,
 +  enum pixel_read_direction direction, int count, struct pixel_argb_u16 
 out_pixel[]);
>>>
>>> This is the second or third time in this one series changing this type.
>>> Could you not do the change once, in its own patch if possible?
>>
>> Sorry, this is not a change here, but a wrong formatting (missed when 
>> rebasing).
>>
>> Do you think that it make sense to re-order my patches and put this 
>> typedef at the end? This way it is never updated.
>>
  
  /**
   * vkms_plane_state - Driver specific plane state
 diff --git a/drivers/gpu/drm/vkms/vkms_formats.c 
 b/drivers/gpu/drm/vkms/vkms_formats.c
 index 46daea6d3ee9..515c80866a58 100644
 --- a/drivers/gpu/drm/vkms/vkms_formats.c
 +++ b/drivers/gpu/drm/vkms/vkms_formats.c
 @@ -33,7 +33,8 @@ static size_t packed_pixels_offset(const struct 
 vkms_frame_info *frame_info, int
 */
return fb->offsets[plane_index] +
   (y / drm_format_info_block_width(format, plane_index)) * 
 fb->pitches[plane_index] +
 - (x / drm_format_info_block_height(format, plane_index)) * 
 format->char_per_block[plane_index];
 + (x / drm_format_info_block_height(format, plane_index)) *
 + format->char_per_block[plane_index];
>>>
>>> Shouldn't this be in the patch that added this code in the first place?
>>
>> Same as above, a wrong formatting, I will remove this change and keep 
>> everything on one line (even if it's more than 100 chars, it is easier to 
>> read).
>>
  }
  
  /**
 @@ -84,6 +85,32 @@ static int get_step_1x1(struct drm_framebuffer *fb, 
 enum pixel_read_direction di
}
  }
  
 +/**
 + * get_subsampling() - Get the subsampling value on a specific direction
>>>
>>> subsampling divisor
>>
>> Thanks for this precision.
>>
 + */
 +static int get_subsampling(const struct drm_format_info *format,

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

2024-02-28 Thread Hsin-Yi Wang

On Wed, Feb 28, 2024 at 5:13 PM Dmitry Baryshkov
 wrote:
>
> On Thu, 29 Feb 2024 at 03:05, Hsin-Yi Wang  wrote:
> >
> > On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson  wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
> > > >
> > > > There are 2 different AUO panels using the same panel id. One of the
> > > > variants requires using overridden modes to resolve glitching issue as
> > > > described in commit 70e0d5550f5c ("drm/panel-edp: Add 
> > > > auo_b116xa3_mode").
> > > > Other variants should use the modes parsed from EDID.
> > > >
> > > > Signed-off-by: Hsin-Yi Wang 
> > > > ---
> > > > v2: new
> > > > ---
> > > >  drivers/gpu/drm/panel/panel-edp.c | 17 -
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > The previous version of this patch that we reverted also had an
> > > override for AUO 0x615c. Is that one no longer needed?
> > >
> > >
> > > > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] 
> > > > = {
> > > > EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, _200_500_e50, 
> > > > "B116XAN06.1"),
> > > > EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, _200_500_e50, 
> > > > "B116XTN02.5"),
> > > > EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, _200_500_e50, 
> > > > "B140HAN04.0"),
> > > > -   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > > > "B116XAK01.0"),
> > > > +   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > > > "B116XAN04.0 "),
> > > > +   EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > > > "B116XAK01.0 ",
> > > > +_b116xa3_mode),
> > >
> > > The name string now has a space at the end of it. I _guess_ that's OK.
> > > Hmmm, but I guess you should update the kernel doc for "struct
> > > edp_panel_entry". The name field is described as "Name of this panel
> > > (for printing to logs)". Now it should include that it's also used for
> > > matching EDIDs in some cases too.
> >
> > The space here is because in the EDID, there is space at the end,
> > before 0x0a (\n).
> > Okay I will update the kernel doc to mention that the same should be
> > exactly the same as the panel name.
>
> Maybe it would be better to strip all the whitespace on the right?
>

Sounds good too.

> --
> With best wishes
> Dmitry

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

2024-02-28 Thread Dmitry Baryshkov

On Thu, 29 Feb 2024 at 03:05, Hsin-Yi Wang  wrote:
>
> On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson  wrote:
> >
> > Hi,
> >
> > On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
> > >
> > > There are 2 different AUO panels using the same panel id. One of the
> > > variants requires using overridden modes to resolve glitching issue as
> > > described in commit 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode").
> > > Other variants should use the modes parsed from EDID.
> > >
> > > Signed-off-by: Hsin-Yi Wang 
> > > ---
> > > v2: new
> > > ---
> > >  drivers/gpu/drm/panel/panel-edp.c | 17 -
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > The previous version of this patch that we reverted also had an
> > override for AUO 0x615c. Is that one no longer needed?
> >
> >
> > > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> > > EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, _200_500_e50, 
> > > "B116XAN06.1"),
> > > EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, _200_500_e50, 
> > > "B116XTN02.5"),
> > > EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, _200_500_e50, 
> > > "B140HAN04.0"),
> > > -   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > > "B116XAK01.0"),
> > > +   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > > "B116XAN04.0 "),
> > > +   EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > > "B116XAK01.0 ",
> > > +_b116xa3_mode),
> >
> > The name string now has a space at the end of it. I _guess_ that's OK.
> > Hmmm, but I guess you should update the kernel doc for "struct
> > edp_panel_entry". The name field is described as "Name of this panel
> > (for printing to logs)". Now it should include that it's also used for
> > matching EDIDs in some cases too.
>
> The space here is because in the EDID, there is space at the end,
> before 0x0a (\n).
> Okay I will update the kernel doc to mention that the same should be
> exactly the same as the panel name.

Maybe it would be better to strip all the whitespace on the right?

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

2024-02-28 Thread Hsin-Yi Wang

On Wed, Feb 28, 2024 at 4:22 PM Doug Anderson  wrote:
>
> Hi,
>
> On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
> >
> > There are 2 different AUO panels using the same panel id. One of the
> > variants requires using overridden modes to resolve glitching issue as
> > described in commit 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode").
> > Other variants should use the modes parsed from EDID.
> >
> > Signed-off-by: Hsin-Yi Wang 
> > ---
> > v2: new
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 17 -
> >  1 file changed, 16 insertions(+), 1 deletion(-)
>
> The previous version of this patch that we reverted also had an
> override for AUO 0x615c. Is that one no longer needed?
>
>
> > @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> > EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, _200_500_e50, 
> > "B116XAN06.1"),
> > EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, _200_500_e50, 
> > "B116XTN02.5"),
> > EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, _200_500_e50, 
> > "B140HAN04.0"),
> > -   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > "B116XAK01.0"),
> > +   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > "B116XAN04.0 "),
> > +   EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> > "B116XAK01.0 ",
> > +_b116xa3_mode),
>
> The name string now has a space at the end of it. I _guess_ that's OK.
> Hmmm, but I guess you should update the kernel doc for "struct
> edp_panel_entry". The name field is described as "Name of this panel
> (for printing to logs)". Now it should include that it's also used for
> matching EDIDs in some cases too.

The space here is because in the EDID, there is space at the end,
before 0x0a (\n).
Okay I will update the kernel doc to mention that the same should be
exactly the same as the panel name.

Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

2024-02-28 Thread Hsin-Yi Wang

On Wed, Feb 28, 2024 at 4:21 PM Doug Anderson  wrote:
>
> Hi,
>
> On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
> >
> > Some panels are interested in the EDID during early probe when connector
> > is still unknown.
> >
> > Add a function drm_get_edid_no_connector() to get edid without connector.
> > No functional change for existing usage.
> >
> > Signed-off-by: Hsin-Yi Wang 
> > ---
> > v1->v2:
> > add a function to return the entire edid without updating connector.
> > ---
> >  drivers/gpu/drm/drm_edid.c | 45 --
> >  include/drm/drm_edid.h |  1 +
> >  2 files changed, 34 insertions(+), 12 deletions(-)
>
> I'll respond in the discussion in v1 too, but overall I'm not a fan of
> reading the whole EDID twice at bootup. Personally I'd love to see us
> to back to just reading the base block like in v1, but I guess we can
> see what Jani and others say.
>
>
> > @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct 
> > drm_connector *connector,
> > if (status == EDID_BLOCK_READ_FAIL)
> > goto fail;
> >
> > -   /* FIXME: Clarify what a corrupt EDID actually means. */
> > -   if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> > -   connector->edid_corrupt = false;
> > -   else
> > -   connector->edid_corrupt = true;
> > +   if (connector) {
> > +   /* FIXME: Clarify what a corrupt EDID actually means. */
> > +   if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> > +   connector->edid_corrupt = false;
> > +   else
> > +   connector->edid_corrupt = true;
> >
> > -   if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> > -   if (status == EDID_BLOCK_ZERO)
> > -   connector->null_edid_counter++;
> > +   if (!edid_block_status_valid(status, edid_block_tag(edid))) 
> > {
> > +   if (status == EDID_BLOCK_ZERO)
> > +   connector->null_edid_counter++;
> >
> > -   connector_bad_edid(connector, edid, 1);
> > -   goto fail;
> > +   connector_bad_edid(connector, edid, 1);
> > +   goto fail;
>
> This "goto fail" is now only run "if (connector)" which means that
> you're not properly checking if the EDID is valid when "connector ==
> NULL", right? That seems like a bug unless I missed something...

We can't check with connector_bad_edid() since there's no connector.
But we still check with edid_block_read() status, similar to what the
original drm_edid_get_panel_id() checks.

Re: [PATCH v2 3/3] drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
>
> There are 2 different AUO panels using the same panel id. One of the
> variants requires using overridden modes to resolve glitching issue as
> described in commit 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode").
> Other variants should use the modes parsed from EDID.
>
> Signed-off-by: Hsin-Yi Wang 
> ---
> v2: new
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 17 -
>  1 file changed, 16 insertions(+), 1 deletion(-)

The previous version of this patch that we reverted also had an
override for AUO 0x615c. Is that one no longer needed?


> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, _200_500_e50, 
> "B116XAN06.1"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, _200_500_e50, 
> "B116XTN02.5"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, _200_500_e50, 
> "B140HAN04.0"),
> -   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> "B116XAK01.0"),
> +   EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> "B116XAN04.0 "),
> +   EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, _b116xak01.delay, 
> "B116XAK01.0 ",
> +_b116xa3_mode),

The name string now has a space at the end of it. I _guess_ that's OK.
Hmmm, but I guess you should update the kernel doc for "struct
edp_panel_entry". The name field is described as "Name of this panel
(for printing to logs)". Now it should include that it's also used for
matching EDIDs in some cases too.

Re: [PATCH v2 2/3] drm/panel: panel-edp: Match edp_panels with panel name

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
>
> @@ -2107,13 +2113,41 @@ static const struct edp_panel_entry edp_panels[] = {
> { /* sentinal */ }
>  };
>
> -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> +static bool edid_has_name(struct edid *edid, const char *panel_name)
> +{
> +   int i, j;
> +   char buf[13];
> +

Should have some comment about why this can't use
drm_edid_get_monitor_name(). AKA because panels seem to be storing the
monitor name tagged as raw strings instead of as the name. Should
probably also have some of the other checks from
is_display_descriptor() like checking for clock of 0 and pad1 of 0.

> +   for (i = 0; i < 4; i++) {

Instead of 4, I think this can be ARRAY_SIZE(edid->detailed_timings), right?

> +   strncpy(buf, 
> edid->detailed_timings[i].data.other_data.data.str.str,
> +   sizeof(buf));

I can never quite remember which of the strXcpy() routines are frowned
upon and which are the golden children at the moment. ...but I don't
think we really need the copy nor the local buffer anyway, right?
You're already going through this thing 1 byte at a time so just
compare it straight away.

> +   for (j = 0; j < 13; j++) {
> +   if (buf[j] == 0x0a) {

Instead of hardcoding 0x0a, I think you want '\n', no?

> +   buf[j] = '\0';
> +   break;
> +   }
> +   }
> +   buf[12] = '\0';
> +   if (strncmp(panel_name, buf, strlen(panel_name)) == 0)
> +   return true;

Untested, but I think with my suggestions above the function becomes this:

static bool edid_has_name(struct edid *edid, const char *panel_name)
{
int i, j;

/*
 * We can't use drm_edid_get_monitor_name() since many eDP panels store
 * their name as a raw string. We'll accept either a string or name
 * match as long as the panel ID also matches.
 */
for (i = 0; i < ARRAY_SIZE(edid->detailed_timings); i++) {
struct detailed_timing *timing = >detailed_timings[i];

if (timing->pixel_clock != 0 ||
timing->data.other_data.pad1 != 0 ||
(timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
 timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
continue;

for (j = 0; j < ARRAY_SIZE(timing->data.other_data.data.str.str); j++) {
const char *str = timing->data.other_data.data.str.str;

if (panel_name[j] == '\0') {
if (str[j] == '\0' || str[j] == '\n')
return true;
break;
}
}
if (j == ARRAY_SIZE(timing->data.other_data.data.str.str) &&
panel_name[j] == '\0')
return true;
}

return false;
}

Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang  wrote:
>
> Some panels are interested in the EDID during early probe when connector
> is still unknown.
>
> Add a function drm_get_edid_no_connector() to get edid without connector.
> No functional change for existing usage.
>
> Signed-off-by: Hsin-Yi Wang 
> ---
> v1->v2:
> add a function to return the entire edid without updating connector.
> ---
>  drivers/gpu/drm/drm_edid.c | 45 --
>  include/drm/drm_edid.h |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)

I'll respond in the discussion in v1 too, but overall I'm not a fan of
reading the whole EDID twice at bootup. Personally I'd love to see us
to back to just reading the base block like in v1, but I guess we can
see what Jani and others say.


> @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct 
> drm_connector *connector,
> if (status == EDID_BLOCK_READ_FAIL)
> goto fail;
>
> -   /* FIXME: Clarify what a corrupt EDID actually means. */
> -   if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> -   connector->edid_corrupt = false;
> -   else
> -   connector->edid_corrupt = true;
> +   if (connector) {
> +   /* FIXME: Clarify what a corrupt EDID actually means. */
> +   if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> +   connector->edid_corrupt = false;
> +   else
> +   connector->edid_corrupt = true;
>
> -   if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> -   if (status == EDID_BLOCK_ZERO)
> -   connector->null_edid_counter++;
> +   if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> +   if (status == EDID_BLOCK_ZERO)
> +   connector->null_edid_counter++;
>
> -   connector_bad_edid(connector, edid, 1);
> -   goto fail;
> +   connector_bad_edid(connector, edid, 1);
> +   goto fail;

This "goto fail" is now only run "if (connector)" which means that
you're not properly checking if the EDID is valid when "connector ==
NULL", right? That seems like a bug unless I missed something...

Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block

Hi,

On Tue, Feb 27, 2024 at 5:27 PM Hsin-Yi Wang  wrote:
>
> On Tue, Feb 27, 2024 at 1:09 AM Jani Nikula  
> wrote:
> >
> > On Fri, 23 Feb 2024, Hsin-Yi Wang  wrote:
> > > It's found that some panels have variants that they share the same panel 
> > > id
> > > although their EDID and names are different. Besides panel id, now we need
> > > the hash of entire EDID base block to distinguish these panel variants.
> > >
> > > Add drm_edid_get_base_block to returns the EDID base block, so caller can
> > > further use it to get panel id and/or the hash.
> >
> > Please reconsider the whole approach here.
> >
> > Please let's not add single-use special case functions to read an EDID
> > base block.
> >
> > Please consider reading the whole EDID, using the regular EDID reading
> > functions, and use that instead.
> >
> > Most likely you'll only have 1-2 blocks anyway. And you might consider
> > caching the EDID in struct panel_edp if reading the entire EDID is too
> > slow. (And if it is, this is probably sensible even if the EDID only
> > consists of one block.)
> >
> > Anyway, please do *not* merge this as-is.
> >
>
> hi Jani,
>
> I sent a v2 here implementing this method:
> https://lore.kernel.org/lkml/20240228011133.1238439-2-hsi...@chromium.org/
>
> We still have to read edid twice due to:
> 1. The first caller is in panel probe, at that time, connector is
> still unknown, so we can't update connector status (eg. update
> edid_corrupt).
> 2. It's possible that the connector can have some override
> (drm_edid_override_get) to EDID, that is still unknown during the
> first read.

I'll also comment in Hsin-Yi's v2, but given Hsin-Yi's digging and the
fact that we can't cache the EDID (because we don't yet have a
"drm_connector"), I'd much prefer Hsin-Yi's solution here from v1 that
allows reading just the first block. If we try to boot a device with a
multi-block EDID we're now wastefully reading all the blocks of the
EDID twice at bootup which will slow boot time.

If you can see a good solution to avoid reading the EDID twice then
that would be amazing, but if not it seems like we should go back to
what's here in v1. What do you think? Anyone else have any opinions?



-Doug

Re: [PATCH v2 0/7] Add YUV formats to VKMS

2024-02-28 Thread Arthur Grillo




On 15/01/24 12:06, Sebastian Wick wrote:
> On Wed, Jan 10, 2024 at 02:44:00PM -0300, Arthur Grillo wrote:
>> This patchset aims to add support for additional buffer YUV formats.
>> More specifically, it adds support to:
>>
>> Semi-planar formats:
>>
>> - NV12
>> - NV16
>> - NV24
>> - NV21
>> - NV61
>> - NV42
>>
>> Planar formats:
>>
>> - YUV440
>> - YUV422
>> - YUV444
>> - YVU440
>> - YVU422
>> - YVU444
>>
>> These formats have more than one plane, and most have chroma
>> subsampling. These properties don't have support on VKMS, so I had to
>> work on this before.
>>
>> To ensure that the conversions from YUV to RGB are working, I wrote a
>> KUnit test. As the work from Harry on creating KUnit tests on VKMS[1] is
>> not yet merged, I took the setup part (Kconfig entry and .kunitfile)
>> from it.
>>
>> Furthermore, I couldn't find any sources with the conversion matrices,
>> so I had to work out the values myself based on the ITU papers[2][3][4].
>> So, I'm not 100% sure if the values are accurate. I'd appreciate some
>> input if anyone has more knowledge in this area.
> 
> H.273 CICP [1] has concise descriptions of the required values for most
> widely used formats and the colour python framework [2] also can be used
> to quickly get to the desired information most of the time.
> 
> [1]: https://www.itu.int/rec/T-REC-H.273
> [2]: https://www.colour-science.org/

I want to thank you for suggesting the python framework, it helped
immensely now that I'm changing the precision from 8-bit to 32-bit[1].

If I'd known about this framework while developing this patch, I
would've saved myself a few weeks of pain and suffering recreating a
part of this library XD.

[1]: 
https://lore.kernel.org/all/b23da076-0bfb-48b2-9386-383a6dec1...@riseup.net/

Best Regards,
~Arthur Grillo

> 
>> Also, I used two IGT tests to check if the formats were having a correct
>> conversion (all with the --extended flag):
>>
>> - kms_plane@pixel_format
>> - kms_plane@pixel_format_source_clamping.
>>
>> The nonsubsampled formats don't have support on IGT, so I sent a patch
>> fixing this[5].
>>
>> Currently, this patchset does not add those formats to the writeback, as
>> it would require a rewrite of how the conversions are done (similar to
>> what was done on a previous patch[6]). So, I would like to review this
>> patchset before I start the work on this other part.
>>
>> [1]: 
>> https://lore.kernel.org/all/20231108163647.106853-5-harry.wentl...@amd.com/
>> [2]: https://www.itu.int/rec/R-REC-BT.601-7-201103-I/en
>> [3]: https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
>> [4]: https://www.itu.int/rec/R-REC-BT.2020-2-201510-I/en
>> [5]: https://lists.freedesktop.org/archives/igt-dev/2024-January/066937.html
>> [6]: 
>> https://lore.kernel.org/dri-devel/20230414135151.75975-2-mca...@igalia.com/
>>
>> Signed-off-by: Arthur Grillo 
>> ---
>> Changes in v2:
>> - Use EXPORT_SYMBOL_IF_KUNIT instead of including the .c test
>>   file (Maxime)
>> - Link to v1: 
>> https://lore.kernel.org/r/20240105-vkms-yuv-v1-0-34c4cd345...@riseup.net
>>
>> ---
>> Arthur Grillo (7):
>>   drm/vkms: Use drm_frame directly
>>   drm/vkms: Add support for multy-planar framebuffers
>>   drm/vkms: Add range and encoding properties to pixel_read function
>>   drm/vkms: Add chroma subsampling
>>   drm/vkms: Add YUV support
>>   drm/vkms: Drop YUV formats TODO
>>   drm/vkms: Create KUnit tests for YUV conversions
>>
>>  Documentation/gpu/vkms.rst|   3 +-
>>  drivers/gpu/drm/vkms/Kconfig  |  15 ++
>>  drivers/gpu/drm/vkms/Makefile |   1 +
>>  drivers/gpu/drm/vkms/tests/.kunitconfig   |   4 +
>>  drivers/gpu/drm/vkms/tests/Makefile   |   3 +
>>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 
>>  drivers/gpu/drm/vkms/vkms_drv.h   |   6 +-
>>  drivers/gpu/drm/vkms/vkms_formats.c   | 247 
>> ++
>>  drivers/gpu/drm/vkms/vkms_formats.h   |   9 +
>>  drivers/gpu/drm/vkms/vkms_plane.c |  26 ++-
>>  drivers/gpu/drm/vkms/vkms_writeback.c |   5 -
>>  11 files changed, 426 insertions(+), 49 deletions(-)
>> ---
>> base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
>> change-id: 20231226-vkms-yuv-6f7859f32df8
>>
>> Best regards,
>> -- 
>> Arthur Grillo 
>>
>

Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks


On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:

On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:

On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi  
wrote:
>
> ...
>
> > > +#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE)
>
> Can sizeof() be used in assembly?
>
> ...
>
> > > -#define __GENMASK(h, l) \
> > > -   (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > > -(~UL(0) >> (BITS_PER_LONG - 1 - (h
> > > -#define GENMASK(h, l) \
> > > -   (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
> > > +#define __GENMASK(t, h, l) \
> > > +   (GENMASK_INPUT_CHECK(h, l) + \
> > > +(((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > > +((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)
>
> Nevertheless, the use ~0ULL is not proper assembly, this broke initial
> implementation using UL() / ULL().

indeed.

>
>
> > > -#define __GENMASK_ULL(h, l) \
> > > -   (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > > -(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h
> > > -#define GENMASK_ULL(h, l) \
> > > -   (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
> Ditto.

problem here seems actually because of the cast to the final type. My
previous impl was avoiding that, but was too verbose compared to this.

I will look at reverting this.

Lucas De Marchi


The fix is quite straightforward. Can you consider the following
patch? I tested it for C and x86_64 asm parts, and it compiles well.

Thanks,
Yury

From 78b2887eea26f208aac50ae283ba9a4d062bb997 Mon Sep 17 00:00:00 2001
From: Yury Norov 
Date: Wed, 7 Feb 2024 23:45:19 -0800
Subject: [PATCH v2] bits: introduce fixed-type GENMASKs

Generalize __GENMASK() to support different types, and implement
fixed-types versions of GENMASK() based on it. The fixed-type version
allows more strict checks to the min/max values accepted, which is
useful for defining registers like implemented by i915 and xe drivers
with their REG_GENMASK*() macros.

The strict checks rely on shift-count-overflow compiler check to
fail the build if a number outside of the range allowed is passed.
Example:

#define FOO_MASK GENMASK_U32(33, 4)

will generate a warning like:

../include/linux/bits.h:41:31: error: left shift count >= width of type 
[-Werror=shift-count-overflow]
   41 |  (((t)~0ULL - ((t)(1) << (l)) + 1) & \
  |   ^~

CC: Dmitry Baryshkov 
Signed-off-by: Yury Norov 
Acked-by: Jani Nikula 
Reviewed-by: Andi Shyti 


I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
need to fork the __GENMASK() implementation on the 2 sides of the ifdef
since I think the GENMASK_INPUT_CHECK() should be the one covering the
input checks. However to make it common we'd need to solve 2 problems:
the casts and the sizeof. The sizeof can be passed as arg to
__GENMASK(), however the casts I think would need a __CAST_U8(x)
or the like and sprinkle it everywhere, which would hurt readability.
Not pretty. Or go back to the original submission and make it less
horrible :-/



---
include/linux/bitops.h |  1 -
include/linux/bits.h   | 41 -
2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 2ba557e067fe..1db50c69cfdb 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -15,7 +15,6 @@
#  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
#endif

-#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE)
#define BITS_TO_LONGS(nr)   __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
#define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
#define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7c0cf5031abe..f3cf8d5f2b55 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -6,6 +6,8 @@
#include 
#include 

+#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE)
+
#define BIT_MASK(nr)(UL(1) << ((nr) % BITS_PER_LONG))
#define BIT_WORD(nr)((nr) / BITS_PER_LONG)
#define BIT_ULL_MASK(nr)(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
@@ -22,24 +24,37 @@
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define __GENMASK(t, h, l) \
+   (GENMASK_INPUT_CHECK(h, l) + \
+(((t)~0ULL - ((t)(1) << (l)) + 1) & \
+((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)
#else
/*
- * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
- * disable the input check if that is the case.
+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files.
+ * Similarly, assembler lacks for C types. So no parameters check in asm.
+ * It's users'

Re: [PATCH] soc: qcom: pmic_glink_altmode: Use common error handling code in pmic_glink_altmode_probe()

2024-02-28 Thread Konrad Dybcio

On 2/28/24 19:05, Markus Elfring wrote:

From: Markus Elfring 
Date: Wed, 28 Feb 2024 18:45:13 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function implementation.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---

(+CC Peter)

Hmm.. this looks very similar to the problem that __free solves
with ..

I know no better, but wouldn't the same mechanism, down to the
usage of DEFINE_FREE work fine for _put-like functions?

Konrad

Re: [PATCH] drm/tidss: Use dev_err_probe() over dev_dbg() when failing to probe the port

2024-02-28 Thread Enric Balletbo i Serra

Hello Andrew,

Many thanks for proposing this.

On Wed, Feb 28, 2024 at 11:02 PM Javier Martinez Canillas
 wrote:
>
> Andrew Halaney  writes:
>
> Hello Andrew,
>
> > This gets logged out to /sys/kernel/debug/devices_deferred in the
> > -EPROBE_DEFER case and as an error otherwise. The message here provides
> > useful information to the user when troubleshooting why their display is
> > not working in either case, so let's make it output appropriately.
> >
> > Signed-off-by: Andrew Halaney 
> > ---
> > There's definitely more spots in this driver that could be upgraded from
> > dev_dbg() to something more appropriate, but this one burned me today so
> > I thought I'd send a patch for it specifically before I forget.
> > ---
>
> Makes sense to me and I agree that's useful to have that information there.
>
> Reviewed-by: Javier Martinez Canillas 
>

Logging in  /sys/kernel/debug/devices_deferred was useful for me, so

Tested-by: Enric Balletbo i Serra 

Cheers,
  Enric

> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>

Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing

Hi Helen,

I appreciate the amount of work you've put into this, to improve testing
of the kernel as a whole. I'll need more time to answer, but please see
below for a quick comment already.

On Wed, Feb 28, 2024 at 07:55:24PM -0300, Helen Koike wrote:
> Dear Kernel Community,
> 
> This patch introduces a `.gitlab-ci` file along with a `ci/` folder, defining 
> a
> basic test pipeline triggered by code pushes to a GitLab-CI instance. This
> initial version includes static checks (checkpatch and smatch for now) and 
> build
> tests across various architectures and configurations. It leverages an
> integrated cache for efficient build times and introduces a flexible 
> 'scenarios'
> mechanism for subsystem-specific extensions.
> 
> tl;dr: check this video to see a quick demo: https://youtu.be/TWiTjhjOuzg,
> but don't forget to check the "Motivation for this work" below. Your feedback,
> whether a simple thumbs up or down, is crucial to determine if it is 
> worthwhile
> to pursue this initiative.
> 
> GitLab is an Open Source platform that includes integrated CI/CD. The pipeline
> provided in this patch is designed to work out-of-the-box with any GitLab
> instance, including the gitlab.com Free Tier. If you reach the limits of the
> Free Tier, consider using community instances like 
> https://gitlab.freedesktop.org/.
> Alternatively, you can set up a local runner for more flexibility. The
> bootstrap-gitlab-runner.sh script included with this patch simplifies this
> process, enabling you to run tests on your preferred infrastructure, including
> your own machine.
> 
> For detailed information, please refer to the documentation included in the
> patch, or check the rendered version here: 
> https://koike.pages.collabora.com/-/linux/-/jobs/298498/artifacts/artifacts/Documentation-output/ci/gitlab-ci/gitlab-ci.html
>  .
> 
> 
> Motivation for this Work
> 
> 
> We all know tests are a major topic in the community, so let's mention the
> specificities of this approach:
> 
> 1. **Built-in User Interface:** GitLab CI/CD is growing in popularity and has 
> an
> user-friendly interface. Our experience with the upstream DRM-CI in the kernel
> tree (see this blog post 
> [https://www.collabora.com/news-and-blog/blog/2024/02/08/drm-ci-a-gitlab-ci-pipeline-for-linux-kernel-testing/]
>  )
> has provided insights into how such a system can benefit the wider community.
> 
> 2. **Distributed Infrastructure:**
> The proposed GitLab-CI pipeline is designed with a distributed infrastructure
> model, being possible to run in any gitlab instance. 
> 
> 3. **Reduce regressions:** Fostering a culture where people habitually run
> validated tests and post their results can prevent many issues in post-merge
> tests.
> 
> 4. **Collaborative Testing Environment:** The kernel community is already
> engaged in numerous testing efforts, including various GitLab-CI pipelines 
> such
> as DRM-CI, which I maintain, along with other solutions like KernelCI and
> BPF-CI. This proposal is designed to further stimulate contributions to the
> evolving testing landscape. Our goal is to establish a comprehensive suite of
> common tools and files.
> 
> 5. **Ownership of QA:** 
> Discrepancies between kernel code and outdated tests often lead to 
> misattributed
> failures, complicating regression tracking. This issue, often arising from
> neglected or deprioritized test updates, creates uncertainty about the source 
> of
> failures. Adopting an "always green pipeline" approach, as detailed in this
> patch's documentation, encourages timely maintenance and validation of tests.
> This ensures that testing accurately reflects the current state of the kernel,
> thereby improving the effectiveness of our QA processes.
> 
> Additionally, if we discover that this method isn't working for us, we can
> easily remove it from the codebase, as it is primarily contained within the 
> ci/
> folder.
> 
> 
> Future Work
> ===
> 
> **Expanding Static Checks:**
> We have the opportunity to integrate a variety of static analysis tools,
> including:
> - dtbs_checks
> - sparse
> - yamllint
> - dt-doc-validate
> - coccicheck
> 
> **Adding Userspace Tests on VMs:**
> To further our testing, we can implement userspace tests that run on virtual
> machines (VMs), such as:
> - kselftests
> - kunit tests
> - Subsystem-specific tests, customizable in the scenarios.
> 
> **Leveraging External Test Labs:**
> We can extend our testing to external labs, similar to what DRM-CI currently
> does. This includes:
> - Lava labs
> - Bare metal labs
> - Using KernelCI-provided labs
> 
> **Other integrations**
> - Submit results to KCIDB
> 
> **Lightweight Implementation for All Developers:**
> We aim to design these tests to be lightweight, ensuring developers with 
> limited
> computing resources can still run essential tests. Resource-intensive tests 
> can
> be set to trigger manually, rather than automatically, to accommodate diverse
> development

[PATCH 2/3] kci-gitlab: Add documentation

2024-02-28 Thread Helen Koike

Add documentation of kci-gitlab.

Signed-off-by: Helen Koike 
---
 Documentation/ci/gitlab-ci/gitlab-ci.rst | 404 +++
 Documentation/index.rst  |   7 +
 MAINTAINERS  |   1 +
 3 files changed, 412 insertions(+)
 create mode 100644 Documentation/ci/gitlab-ci/gitlab-ci.rst

diff --git a/Documentation/ci/gitlab-ci/gitlab-ci.rst 
b/Documentation/ci/gitlab-ci/gitlab-ci.rst
new file mode 100644
index 0..4f7ef03cca95c
--- /dev/null
+++ b/Documentation/ci/gitlab-ci/gitlab-ci.rst
@@ -0,0 +1,404 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+=
+Automated Testing with GitLab CI/CD
+=
+
+This documentation outlines the GitLab CI/CD workflow for the Linux Kernel. The
+workflow is designed to simplify testing for developers, allowing tests to be
+run on any branch at any time, without the need for specific infrastructure.
+Tests are automatically triggered on each `git push`, with results displayed in
+the GitLab UI.
+
+.. image:: images/the-pipeline.png
+   :alt: GitLab-CI pipeline for kernel testing
+   :align: center
+
+Customizations and extensions of the pipeline are possible through the
+scenarios. Scenarios can override existing jobs, change configurations, or
+define new jobs and stages. See :ref:`extending-the-ci` section.
+
+.. note:: If you are unfamiliar with GitLab CI/CD basic concepts, please check
+   the `official documentation `_.
+
+.. only:: subproject and html
+
+   Indices
+   ===
+
+   * :ref:`genindex`
+
+Setup
+-
+
+The GitLab CI pipeline is configured for **"out-of-the-box"** use. Pushing 
code to a
+GitLab repository automatically triggers the pipeline.
+
+.. code-block:: bash
+
+  # Download the Linux kernel source code
+  git clone 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
+  # Create a repository on GitLab and add it as a remote
+  git remote add gitlab 
https://gitlab.yourinstance.com/your-username/your-repo.git
+  # Push the code to GitLab
+  git push gitlab
+
+.. image:: images/pipelines-on-push.png
+   :alt: Pipeline triggered on push
+   :align: center
+
+Troubleshooting
+---
+
+If the pipeline doesn't trigger automatically, check the following:
+
+1. **Enable CI/CD in Project Settings:**
+
+   - Go to `Settings > General > Visibility, project features, permissions`.
+   - Under `Repository`, ensure the `CI/CD` toggle is enabled.
+
+2. **Enable Container Registry:**
+
+   - Still in `Settings`, find the `Container Registry` section.
+   - Enable the `Container Registry` toggle.
+
+3. **CI Minutes and Resources:**
+
+   - If you've exhausted CI minutes or other resources on the Free Tier,
+ consider setting up a local GitLab runner (see below).
+
+Setting Up a Local GitLab Runner
+
+
+You can use your own machine as a runner, instead of the shared runners 
provided
+by your GitLab instance.
+
+1. **Generate a GitLab Runner Token:**
+
+   - Navigate to `Settings > CI/CD > Runners`.
+   - Expand the `Runners` section and click on "New project runner".
+   - Choose "Run untagged jobs" and click "Create runner".
+   - Copy the provided token.
+
+.. image:: images/new-project-runner.png
+   :alt: New project runner button
+   :align: center
+
+2. **Launch the Runner:**
+
+   - Ensure Docker is installed and your user is added to the Docker group:
+
+.. code-block:: bash
+
+sudo usermod -aG docker 
+
+   - Log in again to apply the changes.
+   - Set up the runner:
+
+.. code-block:: bash
+
+ export GITLAB_RUNNER_TOKEN=
+ export GITLAB_URL=https://gitlab.yourinstance.com  # Use this for 
instances other than gitlab.com
+ cd ci/gitlab-ci
+ ./bootstrap-gitlab-runner.sh
+
+
+Current Pipeline Jobs
+-
+
+stage: container
+
+
+**job: debian/x86_64_build**
+
+This job prepares the container used by subsequent jobs. It starts from a base
+Debian image, installing necessary tools for building the kernel and running
+tests. The resulting image is pushed to the project registry and cached. If the
+image already exists in the registry, it won't be rebuilt.
+
+To force a rebuild, update the `FDO_DISTRIBUTION_TAG` variable in the
+`container.yml` file.
+
+stage: static-checks
+
+
+**job: checkpatch**
+
+Runs the `checkpatch.pl` script on the last `$ICI_PATCH_SERIES_SIZE` commits.
+This variable is determined by:
+
+- `ICI_PATCH_SERIES_SIZE=` The number of differing patches between target and
+  source branches for merge requests; Or
+- `ICI_PATCH_SERIES_SIZE=$KCI_PATCH_SERIES_SIZE` if `KCI_PATCH_SERIES_SIZE` is
+  set (see :ref:`how-to-set-variables` below).
+
+Defaults to 1 and raises a GitLab warning if unable to identify the number of
+commits.
+
+**job: smatch**
+
+Checks `.c` files in the last `$ICI_PATCH_SERIES_SIZE` commits.

[PATCH 1/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing

2024-02-28 Thread Helen Koike

This patch introduces a `.gitlab-ci` file along with a `ci/` folder,
defininga basic test pipeline triggered by code pushes to a GitLab-CI
instance. This initial version includes static checks (checkpatch and
smatch for now) and build tests across various architectures and
configurations. It leverages an integrated cache for efficient build
times and introduces a flexible 'scenarios' mechanism for
subsystem-specific extensions.

[ci: add prerequisites to run check-patch on MRs]
Co-developed-by: Tales Aparecida 
Signed-off-by: Tales Aparecida 
Signed-off-by: Helen Koike 

---

Hey all,

You can check the validation of this patchset on:
https://gitlab.collabora.com/koike/linux/-/pipelines/87035

I would appreciate your feedback on this work, what do you think?

If you would rate from 0 to 5, where:

[ ] 0. I don't think this is useful at all, and I doubt it will ever be. It 
doesn't seem worthwhile.
[ ] 1. I don't find it useful in its current form.
[ ] 2. It might be useful to others, but not for me.
[ ] 3. It has potential, but it's not yet something I can incorporate into my 
workflow.
[ ] 4. This is useful, but it needs some adjustments before I can include it in 
my workflow.
[ ] 5. This is really useful! I'm eager to start using it right away. Why 
didn't you send this earlier? :)

Which rating would you select?

---
 .gitlab-ci.yml|   2 +
 MAINTAINERS   |   8 ++
 ci/gitlab-ci/bootstrap-gitlab-runner.sh   |  55 +
 ci/gitlab-ci/ci-scripts/build-docs.sh |  35 ++
 ci/gitlab-ci/ci-scripts/build-kernel.sh   |  35 ++
 ci/gitlab-ci/ci-scripts/ici-functions.sh  | 104 ++
 ci/gitlab-ci/ci-scripts/install-smatch.sh |  13 +++
 .../ci-scripts/parse_commit_message.sh|  27 +
 ci/gitlab-ci/ci-scripts/run-checkpatch.sh |  20 
 ci/gitlab-ci/ci-scripts/run-smatch.sh |  45 
 ci/gitlab-ci/docker-compose.yaml  |  18 +++
 ci/gitlab-ci/linux.code-workspace |  11 ++
 ci/gitlab-ci/yml/build.yml|  43 
 ci/gitlab-ci/yml/cache.yml|  26 +
 ci/gitlab-ci/yml/container.yml|  36 ++
 ci/gitlab-ci/yml/gitlab-ci.yml|  71 
 ci/gitlab-ci/yml/kernel-combinations.yml  |  18 +++
 ci/gitlab-ci/yml/scenarios.yml|  12 ++
 ci/gitlab-ci/yml/scenarios/file-systems.yml   |  21 
 ci/gitlab-ci/yml/scenarios/media.yml  |  21 
 ci/gitlab-ci/yml/scenarios/network.yml|  21 
 ci/gitlab-ci/yml/static-checks.yml|  21 
 22 files changed, 663 insertions(+)
 create mode 100644 .gitlab-ci.yml
 create mode 100755 ci/gitlab-ci/bootstrap-gitlab-runner.sh
 create mode 100755 ci/gitlab-ci/ci-scripts/build-docs.sh
 create mode 100755 ci/gitlab-ci/ci-scripts/build-kernel.sh
 create mode 100644 ci/gitlab-ci/ci-scripts/ici-functions.sh
 create mode 100755 ci/gitlab-ci/ci-scripts/install-smatch.sh
 create mode 100755 ci/gitlab-ci/ci-scripts/parse_commit_message.sh
 create mode 100755 ci/gitlab-ci/ci-scripts/run-checkpatch.sh
 create mode 100755 ci/gitlab-ci/ci-scripts/run-smatch.sh
 create mode 100644 ci/gitlab-ci/docker-compose.yaml
 create mode 100644 ci/gitlab-ci/linux.code-workspace
 create mode 100644 ci/gitlab-ci/yml/build.yml
 create mode 100644 ci/gitlab-ci/yml/cache.yml
 create mode 100644 ci/gitlab-ci/yml/container.yml
 create mode 100644 ci/gitlab-ci/yml/gitlab-ci.yml
 create mode 100644 ci/gitlab-ci/yml/kernel-combinations.yml
 create mode 100644 ci/gitlab-ci/yml/scenarios.yml
 create mode 100644 ci/gitlab-ci/yml/scenarios/file-systems.yml
 create mode 100644 ci/gitlab-ci/yml/scenarios/media.yml
 create mode 100644 ci/gitlab-ci/yml/scenarios/network.yml
 create mode 100644 ci/gitlab-ci/yml/static-checks.yml

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 0..fac523bb86866
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,2 @@
+include:
+  - ci/gitlab-ci/yml/gitlab-ci.yml
diff --git a/MAINTAINERS b/MAINTAINERS
index 716b2e22598c8..aa0f65791c2ee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4998,6 +4998,14 @@ T:   git git://linuxtv.org/media_tree.git
 F: Documentation/devicetree/bindings/media/i2c/chrontel,ch7322.yaml
 F: drivers/media/cec/i2c/ch7322.c
 
+CI AUTOMATED TESTING
+M: Helen Koike 
+L: linux-ker...@vger.kernel.org
+S: Maintained
+T: git https://gitlab.collabora.com/koike/linux.git
+F: .gitlab-ci.yml
+F: ci/
+
 CIRRUS LOGIC AUDIO CODEC DRIVERS
 M: James Schulman 
 M: David Rhodes 
diff --git a/ci/gitlab-ci/bootstrap-gitlab-runner.sh 
b/ci/gitlab-ci/bootstrap-gitlab-runner.sh
new file mode 100755
index 0..73238960d0880
--- /dev/null
+++ b/ci/gitlab-ci/bootstrap-gitlab-runner.sh
@@ -0,0 +1,55 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Copyright (C) 2024 Collabora, Helen Koike 
+
+set -eo pipefail
+

[PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing

2024-02-28 Thread Helen Koike

Dear Kernel Community,

This patch introduces a `.gitlab-ci` file along with a `ci/` folder, defining a
basic test pipeline triggered by code pushes to a GitLab-CI instance. This
initial version includes static checks (checkpatch and smatch for now) and build
tests across various architectures and configurations. It leverages an
integrated cache for efficient build times and introduces a flexible 'scenarios'
mechanism for subsystem-specific extensions.

tl;dr: check this video to see a quick demo: https://youtu.be/TWiTjhjOuzg,
but don't forget to check the "Motivation for this work" below. Your feedback,
whether a simple thumbs up or down, is crucial to determine if it is worthwhile
to pursue this initiative.

GitLab is an Open Source platform that includes integrated CI/CD. The pipeline
provided in this patch is designed to work out-of-the-box with any GitLab
instance, including the gitlab.com Free Tier. If you reach the limits of the
Free Tier, consider using community instances like
https://gitlab.freedesktop.org/.
Alternatively, you can set up a local runner for more flexibility. The
bootstrap-gitlab-runner.sh script included with this patch simplifies this
process, enabling you to run tests on your preferred infrastructure, including
your own machine.

For detailed information, please refer to the documentation included in the
patch, or check the rendered version here:
https://koike.pages.collabora.com/-/linux/-/jobs/298498/artifacts/artifacts/Documentation-output/ci/gitlab-ci/gitlab-ci.html
.

Motivation for this Work

We all know tests are a major topic in the community, so let's mention the
specificities of this approach:

1. **Built-in User Interface:** GitLab CI/CD is growing in popularity and has an
user-friendly interface. Our experience with the upstream DRM-CI in the kernel
tree (see this blog post
[https://www.collabora.com/news-and-blog/blog/2024/02/08/drm-ci-a-gitlab-ci-pipeline-for-linux-kernel-testing/]
)
has provided insights into how such a system can benefit the wider community.

2. **Distributed Infrastructure:**
The proposed GitLab-CI pipeline is designed with a distributed infrastructure
model, being possible to run in any gitlab instance.

3. **Reduce regressions:** Fostering a culture where people habitually run
validated tests and post their results can prevent many issues in post-merge
tests.

4. **Collaborative Testing Environment:** The kernel community is already
engaged in numerous testing efforts, including various GitLab-CI pipelines such
as DRM-CI, which I maintain, along with other solutions like KernelCI and
BPF-CI. This proposal is designed to further stimulate contributions to the
evolving testing landscape. Our goal is to establish a comprehensive suite of
common tools and files.

5. **Ownership of QA:**
Discrepancies between kernel code and outdated tests often lead to misattributed
failures, complicating regression tracking. This issue, often arising from
neglected or deprioritized test updates, creates uncertainty about the source of
failures. Adopting an "always green pipeline" approach, as detailed in this
patch's documentation, encourages timely maintenance and validation of tests.
This ensures that testing accurately reflects the current state of the kernel,
thereby improving the effectiveness of our QA processes.

Additionally, if we discover that this method isn't working for us, we can
easily remove it from the codebase, as it is primarily contained within the ci/
folder.

Future Work
===

**Expanding Static Checks:**
We have the opportunity to integrate a variety of static analysis tools,
including:
- dtbs_checks
- sparse
- yamllint
- dt-doc-validate
- coccicheck

**Adding Userspace Tests on VMs:**
To further our testing, we can implement userspace tests that run on virtual
machines (VMs), such as:
- kselftests
- kunit tests
- Subsystem-specific tests, customizable in the scenarios.

**Leveraging External Test Labs:**
We can extend our testing to external labs, similar to what DRM-CI currently
does. This includes:
- Lava labs
- Bare metal labs
- Using KernelCI-provided labs

**Other integrations**
- Submit results to KCIDB

**Lightweight Implementation for All Developers:**
We aim to design these tests to be lightweight, ensuring developers with limited
computing resources can still run essential tests. Resource-intensive tests can
be set to trigger manually, rather than automatically, to accommodate diverse
development environments.

Chat Discussions

For those interested in further discussions:

**Join Our Slack Channel:**
We have a Slack channel, #gitlab-ci, on the KernelCI Slack instance
https://kernelci.slack.com/ .
Feel free to join and contribute to the conversation. The KernelCI team has
weekly calls where we also discuss the GitLab-CI pipeline.

**Acknowledgments:**
A special thanks to Nikolai Kondrashov, Tales da Aparecida - both from Red Hat -
and KernelCI community for their

Re: [PATCH] drm/msm/dp: fix runtime_pm handling in dp_wait_hpd_asserted

2024-02-28 Thread Stephen Boyd

Quoting Dmitry Baryshkov (2024-02-26 14:34:45)
> The function dp_wait_hpd_asserted() uses pm_runtime_get_sync() and
> doesn't care about the return value. Potentially this can lead to
> unclocked access if for some reason resuming of the DP controller fails.
>
> Change the function to use pm_runtime_resume_and_get() and return an
> error if resume fails.
>
> Fixes: e2969ee30252 ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP 
> probe()")
> Signed-off-by: Dmitry Baryshkov 
> ---

Reviewed-by: Stephen Boyd

RE: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format

2024-02-28 Thread Klymenko, Anatoliy

Hi Laurent,

Thanks a lot for the review.

> -Original Message-
> From: Laurent Pinchart 
> Sent: Wednesday, February 28, 2024 8:08 AM
> To: Klymenko, Anatoliy 
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> Hi Anatoliy,
> 
> Thank you for the patch.
> 
> On Mon, Feb 26, 2024 at 08:44:44PM -0800, Anatoliy Klymenko wrote:
> > Program live video input format according to selected media bus format.
> >
> > In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> > almost certainly supports a single media bus format as its output.
> > Expect this to be delivered via the new bridge atomic state. Program
> > DPSUB registers accordingly.
> >
> > Signed-off-by: Anatoliy Klymenko 
> > ---
> >  drivers/gpu/drm/xlnx/zynqmp_disp.c  | 52
> +
> >  drivers/gpu/drm/xlnx/zynqmp_disp.h  |  2 ++
> >  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  8 ++---
> >  drivers/gpu/drm/xlnx/zynqmp_dp.c| 13 ++---
> >  4 files changed, 67 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index ee99aad915ba..1c3ffdee6b8e 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -416,6 +416,34 @@ static bool zynqmp_disp_layer_is_video(const struct
> zynqmp_disp_layer *layer)
> >   return layer->id == ZYNQMP_DPSUB_LAYER_VID;  }
> >
> > +/**
> > + * zynqmp_disp_avbuf_set_live_format - Set live input format for a
> > +layer
> > + * @disp: Display controller
> > + * @layer: The layer
> > + * @fmt: The format information
> > + *
> > + * Set the live video input format for @layer to @fmt.
> > + */
> > +static void zynqmp_disp_avbuf_set_live_format(struct zynqmp_disp *disp,
> > +   struct zynqmp_disp_layer *layer,
> > +   const struct
> > +zynqmp_disp_format *fmt) {
> > + u32 reg, i;
> > +
> > + reg = zynqmp_disp_layer_is_video(layer)
> > + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
> > + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
> > + zynqmp_disp_avbuf_write(disp, reg, fmt->buf_fmt);
> > +
> > + for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; ++i) {
> > + reg = zynqmp_disp_layer_is_video(layer)
> > + ? ZYNQMP_DISP_AV_BUF_LIVD_VID_COMP_SF(i)
> > + : ZYNQMP_DISP_AV_BUF_LIVD_GFX_COMP_SF(i);
> > + zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
> > + }
> 
> This is identical to zynqmp_disp_avbuf_set_format(), you should avoid 
> duplicating
> code.
> 

Yeah, there are similarities - let me think on how to refactor this properly.

> > + layer->disp_fmt = fmt;
> > +}
> > +
> >  /**
> >   * zynqmp_disp_avbuf_set_format - Set the input format for a layer
> >   * @disp: Display controller
> > @@ -979,6 +1007,30 @@ void zynqmp_disp_layer_disable(struct
> zynqmp_disp_layer *layer)
> >   zynqmp_disp_blend_layer_disable(layer->disp, layer);  }
> >
> > +/**
> > + * zynqmp_disp_layer_set_live_format - Set live layer input format
> > + * @layer: The layer
> > + * @info: Input media bus format
> > + *
> > + * Set the live @layer input bus format. The layer must be disabled.
> > + */
> > +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> > +u32 bus_format)
> 
> I'd prefer reusing zynqmp_disp_layer_set_format(), and handling the 
> differences
> between live and non-live input there. There's already a dma_enabled check in
> that function.
> 

There is a difference between setting format for dma-backed layer vs live input 
layer. In the first case we have memory layout in fourcc format, but in the 
second case we have video signal described by media bus format. Anyways, let me 
check if I can unify both cases.

> > +{
> > + int i;
> > + const struct zynqmp_disp_format *fmt;
> > +
> > + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) {
> > + fmt = _live_fmts[i];
> > + if (fmt->bus_fmt == bus_format) {
> > + layer->disp_fmt = fmt;
> > + layer->drm_fmt = drm_format_info(fmt->drm_fmt);
> > + zynqmp_disp_avbuf_set_live_format(layer->disp, layer, 
> > fmt);
> > + return;
> > + }
> > + }
> > +}
> > +
> >  /**
> >   * zynqmp_disp_layer_set_format - Set the layer format
> >   * @layer: The layer
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> >

Re: [PATCH 2/3] drm/panel: add samsung s6e3fa7 panel driver

2024-02-28 Thread Jessica Zhang





On 2/9/2024 3:17 PM, Richard Acayan wrote:

On Thu, Feb 08, 2024 at 05:34:57PM -0800, Jessica Zhang wrote:

On 2/8/2024 4:16 PM, Richard Acayan wrote:

The S6E3FA7 display controller is enabled in every Pixel 3a (non-XL)
variant. Add the driver for it, generated by
linux-mdss-dsi-panel-driver-generator.

There are other panels connected to the same S6E3FA7 display controller,
such as the AMS604NL01 panel, which are incompatible with this driver.
Name the device tree compatible after the panel model according to
iFixit.

Link: https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator
Link: 
https://android.googlesource.com/kernel/msm/+/7fda1cd7b64710dafac5f34899611c6d35eb4cd2/arch/arm64/boot/dts/google/dsi-panel-s6e3fa7-1080p-cmd.dtsi
Link: 
https://github.com/msm8953-mainline/linux/blob/v6.6.12-r0/drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c
Link: https://www.ifixit.com/Guide/Image/meta/muyjtLQTHu6MDkhK
Signed-off-by: Richard Acayan 
---
   drivers/gpu/drm/panel/Kconfig |   9 +
   drivers/gpu/drm/panel/Makefile|   1 +
   drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c | 285 ++
   3 files changed, 295 insertions(+)
   create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 8f3783742208..a693b03f680e 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -577,6 +577,15 @@ config DRM_PANEL_SAMSUNG_DB7430
  DB7430 DPI display controller used in such devices as the
  LMS397KF04 480x800 DPI panel.
+config DRM_PANEL_SAMSUNG_S6E3FA7
+   tristate "Samsung S6E3FA7 panel driver"
+   depends on OF
+   depends on DRM_MIPI_DSI
+   depends on BACKLIGHT_CLASS_DEVICE
+   help
+ Say Y here if you want to enable support for the Samsung S6E3FA7
+ 1920x2220 panel.
+
   config DRM_PANEL_SAMSUNG_S6D16D0
tristate "Samsung S6D16D0 DSI video mode panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index d94a644d0a6c..560b62129f68 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += 
panel-samsung-ld9040.o
   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D27A1) += panel-samsung-s6d27a1.o
   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D7AA0) += panel-samsung-s6d7aa0.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3FA7) += panel-samsung-s6e3fa7.o
   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
   obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c 
b/drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c
new file mode 100644
index ..10bc8fb5f1f9
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the Samsung S6E3FA7 panel.
+ *
+ * Copyright (c) 2022-2024, The Linux Foundation. All rights reserved.



Hi Richard,

Not really sure about the copyright dates -- since this is a completely new
file to this tree, wouldn't the year be just 2024?


That would be more concise, but the original driver was generated and
added to a kernel fork [1] in 2022 and amendments have been made since then.


Ah, got it. Sounds good.

In that case

Reviewed-by: Jessica Zhang 

Thanks,

Jessica Zhang



[1] 
https://gitlab.com/sdm670-mainline/linux/-/blob/sdm670-v6.2.6/drivers/gpu/drm/panel/panel-samsung-s6e3fa7.c?ref_type=tags



The rest LGTM.

Thanks,

Jessica Zhang


+ * Generated with linux-mdss-dsi-panel-driver-generator from vendor device 
tree:
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ */ > +
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+
+struct s6e3fa7_panel {
+   struct drm_panel panel;
+   struct mipi_dsi_device *dsi;
+   struct gpio_desc *reset_gpio;
+};
+
+static inline struct s6e3fa7_panel *to_s6e3fa7_panel(struct drm_panel *panel)
+{
+   return container_of(panel, struct s6e3fa7_panel, panel);
+}
+
+static void s6e3fa7_panel_reset(struct s6e3fa7_panel *ctx)
+{
+   gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+   usleep_range(1000, 2000);
+   gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+   usleep_range(1, 11000);
+}
+
+static int s6e3fa7_panel_on(struct s6e3fa7_panel *ctx)
+{
+   struct mipi_dsi_device *dsi = ctx->dsi;
+   struct device *dev = >dev;
+   int ret;
+
+   ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+   if (ret < 0) {
+   dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
+   return ret;
+   }
+   msleep(120);
+
+   ret = mipi_dsi_dcs_set_tear_on(dsi,

Re: [linux-next:master] BUILD REGRESSION 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e


On Thu, Feb 29, 2024 at 01:31:53AM +0800, kernel test robot wrote:

drivers/gpu/drm/xe/xe_mmio.c:109:23: error: result of comparison of constant 
4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is 
always false [-Werror,-Wtautological-constant-out-of-range-compare]


this is fixed now in drm-xe-next.

Lucas De Marchi

Re: [PATCH] drm/tidss: Use dev_err_probe() over dev_dbg() when failing to probe the port

2024-02-28 Thread Javier Martinez Canillas

Andrew Halaney  writes:

Hello Andrew,

> This gets logged out to /sys/kernel/debug/devices_deferred in the
> -EPROBE_DEFER case and as an error otherwise. The message here provides
> useful information to the user when troubleshooting why their display is
> not working in either case, so let's make it output appropriately.
>
> Signed-off-by: Andrew Halaney 
> ---
> There's definitely more spots in this driver that could be upgraded from
> dev_dbg() to something more appropriate, but this one burned me today so
> I thought I'd send a patch for it specifically before I forget.
> ---

Makes sense to me and I agree that's useful to have that information there.

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

RE: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format callback

2024-02-28 Thread Klymenko, Anatoliy

Hi Maxime,

Thanks for the review.

> -Original Message-
> From: Maxime Ripard 
> Sent: Wednesday, February 28, 2024 7:30 AM
> To: Klymenko, Anatoliy 
> Cc: Laurent Pinchart ; Maarten Lankhorst
> ; Thomas Zimmermann
> ; David Airlie ; Daniel Vetter
> ; Simek, Michal ; Andrzej Hajda
> ; Neil Armstrong ; Robert
> Foss ; Jonas Karlman ; Jernej Skrabec
> ; dri-devel@lists.freedesktop.org; linux-arm-
> ker...@lists.infradead.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format
> callback
> 
> Hi,
> 
> On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> > Add select_output_bus_format to CRTC atomic helpers callbacks. This
> > callback Will allow CRTC to participate in media bus format
> > negotiation over connected DRM bridge chain and impose CRTC-specific
> restrictions.
> > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC
> > will most certainly support a single output media bus format, as
> > supporting multiple runtime options consumes extra FPGA resources. A
> > variety of options for FPGA are usually achieved by synthesizing IP
> > with different parameters.
> >
> > Incorporate select_output_bus_format callback into the format
> > negotiation stage to fix the input bus format of the first DRM bridge in the
> chain.
> >
> > Signed-off-by: Anatoliy Klymenko 
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 19 +--
> >  include/drm/drm_modeset_helper_vtables.h | 31
> > +++
> >  2 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c
> > b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..453ae3d174b4 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -32,6 +32,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge
> *first_bridge,
> > unsigned int i, num_in_bus_fmts = 0;
> > struct drm_bridge_state *cur_state;
> > struct drm_bridge *prev_bridge;
> > -   u32 *in_bus_fmts;
> > +   struct drm_crtc *crtc = crtc_state->crtc;
> > +   u32 *in_bus_fmts, in_fmt;
> > int ret;
> >
> > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge
> *first_bridge,
> > return -ENOMEM;
> >
> > if (first_bridge == cur_bridge) {
> > -   cur_state->input_bus_cfg.format = in_bus_fmts[0];
> > +   in_fmt = in_bus_fmts[0];
> > +   if (crtc->helper_private &&
> > +   crtc->helper_private->select_output_bus_format) {
> > +   in_fmt = crtc->helper_private-
> >select_output_bus_format(
> > +   crtc,
> > +   crtc->state,
> > +   in_bus_fmts,
> > +   num_in_bus_fmts);
> > +   if (!in_fmt) {
> > +   kfree(in_bus_fmts);
> > +   return -ENOTSUPP;
> > +   }
> > +   }
> > +   cur_state->input_bus_cfg.format = in_fmt;
> 
> I don't think we should start poking at the CRTC internals, but we should 
> rather
> provide a helper here.

Makes sense, thank you. ACK.

> 
> > cur_state->output_bus_cfg.format = out_bus_fmt;
> > kfree(in_bus_fmts);
> > return 0;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 881b03e4dc28..7c21ae1fe3ad 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> >  bool in_vblank_irq, int *vpos, int *hpos,
> >  ktime_t *stime, ktime_t *etime,
> >  const struct drm_display_mode *mode);
> > +
> > +   /**
> > +* @select_output_bus_format
> > +*
> > +* Called by the first connected DRM bridge to negotiate input media
> > +* bus format. CRTC is expected to pick preferable media formats from
> > +* the list supported by the DRM bridge chain.
> 
> There's nothing restricting it to bridges here. Please rephrase this to 
> remove the
> bridge mention. The user is typically going to be the encoder, and bridges 
> are just
> an automagic implementation of an encoder.
> 

OK. I'll fix than in the next version.

> And generally speaking, I'd really like to have an implementation available 
> before
> merging this.
>

Well, 2 instances of this callback implementations exist as drafts, as this is 
the new API. A little bit of a chicken and egg problem. I'll try to groom at 
least one of them into upstreamable shape and attach

Re: drm-misc migration to Gitlab server

2024-02-28 Thread Stephen Rothwell

Hi Daniel,

On Wed, 28 Feb 2024 17:33:28 +0100 Daniel Vetter  wrote:
>
> > git://git.freedesktop.org/git/drm/drm.git#drm-fixes
> > git://git.freedesktop.org/git/drm/drm.git#drm-next  
> 
> To test out the process we've moved drm.git first. It's now here:
> 
> https://gitlab.freedesktop.org/drm/kernel.git
> 
> Still the same two branches. Can you please update the url? We haven't
> enabled the auto-mirror for this one, since we want to make sure the
> upgrade path in the tooling works and people do switch over to the new
> repo.

Done.  They seem to be fine.

-- 
Cheers,
Stephen Rothwell


pgp8RJ1W7oaUD.pgp
Description: OpenPGP digital signature

Re: [PATCH RFC 0/4] Support for Simulated Panels

2024-02-28 Thread Jessica Zhang





On 2/2/2024 2:15 AM, Maxime Ripard wrote:

On Tue, Jan 30, 2024 at 09:53:13AM +0100, Daniel Vetter wrote:

Wouldn't it be simpler if we had a vkms-like panel that we could either
configure from DT or from debugfs that would just be registered the
usual way and would be the only panel we register?




No, we need to have validate actual hardware pipeline with the simulated
panel. With vkms, actual display pipeline will not be validated. With
incorrect display pipeline misconfigurations arising from different panel
combinations, this can easily be caught with any existing IGT CRC testing.
In addition, all performance related bugs can also be easily caught by
simulating high resolution displays.


That's not what I meant. What I meant was that something like a
user-configurable, generic, panel driver would be a good idea. Just like
vkms (with the debugfs patches) is for a full blown KMS device.



Let me respond for both this question and the one below from you/Jani.

Certainly having user-configurable information is a goal here. The end-goal
is to make everything there in the existing panels such as below like I
wrote:

1) Display resolution with timings (drm_display_mode)
2) Compression/non-compression
3) Command mode/Video mode
4) MIPI mode flags
5) DCS commands for panel enable/disable and other panel sequences
6) Power-up/Power-down sequence for the panel

But, we also have to see what all is feasible today from the DRM fwk
standpoint. There are some limitations about what is boot-time configurable
using bootparams and what is runtime configurable (across a modeset) using
debugfs.

1) Today, everything part of struct mipi_dsi_device needs to be available at
boot time from what I can see as we need that while calling
mipi_dsi_attach(). So for that we went with boot-params.

2) For the list of modes, we can move this to a debugfs like
"populate_modes" which the client using a sim panel can call before picking
a mode and triggering a commit.

But we need to have some default mode and configuration.


Uh, at the risk of sounding a bit like I'm just chasing the latest
buzzwords, but this sounds like something that's screaming for ebpf.


I make a half-joke to Jani on IRC about it, but I was also being
half-serious. If the goal we want to have is to fully emulate any panel
variation, ebpf really looks like the best and most flexible way
forward.


Hi Maxime and Daniel,

For our current sim panel requirements, we can go with implementing the 
configfs first then add ebpf if requirements get more complex.


Thanks,

Jessica Zhang



Maxime

Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

On Wed, Feb 28, 2024 at 09:50:22PM +0100, Javier Carrasco wrote:
> On 28.02.24 21:41, Matthias Kaehlcke wrote:
> > On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
> >> On 28.02.24 19:10, Matthias Kaehlcke wrote:
> >>> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>  Most of the functionality this driver provides can be used by non-hub
>  devices as well.
> 
>  To account for the hub-specific code, add a flag to the device data
>  structure and check its value for hub-specific code.
> 
>  The 'always_powered_in_supend' attribute is only available for hub
>  devices, keeping the driver's default behavior for non-hub devices (keep
>  on in suspend).
> 
>  Signed-off-by: Javier Carrasco 
>  ---
>   drivers/usb/misc/onboard_usb_dev.c | 25 +++--
>   drivers/usb/misc/onboard_usb_dev.h | 10 ++
>   2 files changed, 33 insertions(+), 2 deletions(-)
> 
>  diff --git a/drivers/usb/misc/onboard_usb_dev.c 
>  b/drivers/usb/misc/onboard_usb_dev.c
>  index e1779bd2d126..df0ed172c7ec 100644
>  --- a/drivers/usb/misc/onboard_usb_dev.c
>  +++ b/drivers/usb/misc/onboard_usb_dev.c
>  @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct 
>  device *dev)
>   struct usbdev_node *node;
>   bool power_off = true;
>   
>  -if (onboard_dev->always_powered_in_suspend)
>  +if (onboard_dev->always_powered_in_suspend &&
>  +!onboard_dev->pdata->is_hub)
>   return 0;
> >>>
> >>> With this non-hub devices would always be powered down, since
> >>> 'always_powerd_in_suspend' is not set for them. This should be:
> >>>
> >>
> >> May I ask you what you meant in v4 with this comment?
> >>
> >>> Even without the sysfs attribute the field 'always_powered_in_suspend'
> >>> could
> >>> be set to true by probe() for non-hub devices.
> > 
> > struct onboard_dev always has the field 'always_powered_in_suspend',
> > even for non-hubs, that don't have the corresponding sysfs attribute.
> > Currently it is left uninitialized (i.e. false) for non-hubs. Instead
> > it could be initialized to true by probe() for non-hubs, which would
> > be semantically correct. With that it wouldn't be necessary to check
> > here whether a device is hub, because the field would provide the
> > necessary information.
> > 
> 
> That is maybe what is confusing me a bit. Should it not be false for
> non-hub devices? That property is only meant for hubs, so why should
> non-hub devices be always powered in suspend? I thought it should always
> be false for non-hub devices, and configurable for hubs.

I suspect the confusion stems from the sysfs attribute 'always_powered_...'
vs. the struct field with the same name.

The sysfs attribute defaults to 'false', which results in USB devices
being powered down in suspend. That was the desired behavior for a device
I was working on when I implemented this driver, but in hindsight I think
the default should have been 'true'.

We agreed that non-hub devices shouldn't be powered down in suspend. It
would be unexpected for users and could have side effects like delays
or losing status. Since (I think) we can't change the default of the
attribute we said we'd limit it to hubs, and not create it for other
types of USB devices. Other USB devices would remain powered during
system suspend.

Are we in agreement up to this point, in particular that non-hub
devices should remain powered?

struct onboard_dev has the field 'always_powered_...', which in the
existing code is *always* associated with the sysfs attribute of
the same name. But there is no reason to not to use the field when
the sysfs attribute does not exist. For any device at any given time
the field could indicate whether the device should be remain powered
during suspend. For hubs the value can be changed at runtime
through the sysfs attribute, for non-hubs it would be static and
always indicate that the device should remain powered.

Does that clarify your doubts?

Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are not powered

Hi,

On Wed, Feb 28, 2024 at 8:52 AM  wrote:
>
> On 28/02/2024 17:40, Doug Anderson wrote:
> > Neil,
> >
> > On Thu, Feb 15, 2024 at 8:53 AM Neil Armstrong
> >  wrote:
> >>
> >> Hi Doug,
> >>
> >> On 15/02/2024 16:08, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Thu, Feb 15, 2024 at 2:24 AM Jani Nikula  wrote:
> 
>  On Wed, 14 Feb 2024, Doug Anderson  wrote:
> > Hi,
> >
> > On Tue, Feb 13, 2024 at 10:25 PM Hsin-Yi Wang  
> > wrote:
> >>
> >> On Wed, Feb 14, 2024 at 2:23 PM Douglas Anderson 
> >>  wrote:
> >>>
> >>> If an eDP panel is not powered on then any attempts to talk to it over
> >>> the DP AUX channel will timeout. Unfortunately these attempts may be
> >>> quite slow. Userspace can initiate these attempts either via a
> >>> /dev/drm_dp_auxN device or via the created i2c device.
> >>>
> >>> Making the DP AUX drivers timeout faster is a difficult proposition.
> >>> In theory we could just poll the panel's HPD line in the AUX transfer
> >>> function and immediately return an error there. However, this is
> >>> easier said than done. For one thing, there's no hard requirement to
> >>> hook the HPD line up for eDP panels and it's OK to just delay a fixed
> >>> amount. For another thing, the HPD line may not be fast to probe. On
> >>> parade-ps8640 we need to wait for the bridge chip's firmware to boot
> >>> before we can get the HPD line and this is a slow process.
> >>>
> >>> The fact that the transfers are taking so long to timeout is causing
> >>> real problems. The open source fwupd daemon sometimes scans DP busses
> >>> looking for devices whose firmware need updating. If it happens to
> >>> scan while a panel is turned off this scan can take a long time. The
> >>> fwupd daemon could try to be smarter and only scan when eDP panels are
> >>> turned on, but we can also improve the behavior in the kernel.
> >>>
> >>> Let's let eDP panels drivers specify that a panel is turned off and
> >>> then modify the common AUX transfer code not to attempt a transfer in
> >>> this case.
> >>>
> >>> Signed-off-by: Douglas Anderson 
> >>> ---
> >>
> >> Reviewed-by: Hsin-Yi Wang 
> >
> > Thanks for the review!
> >
> > Given that this touches core DRM code and that I never got
> > confirmation that Jani's concerns were addressed with my previous
> > response, I'm still going to wait a little while before applying. I'm
> > on vacation for most of next week, but if there are no further replies
> > between now and then I'll plan to apply this to "drm-misc-next" the
> > week of Feb 26th. If someone else wants to apply this before I do then
> > I certainly won't object. Jani: if you feel this needs more discussion
> > or otherwise object to this patch landing then please yell. Likewise
> > if anyone else in the community wants to throw in their opinion, feel
> > free.
> 
>  Sorry for dropping the ball after my initial response. I simply have not
>  had the time to look into this.
> 
>  It would be great to get, say, drm-misc maintainer ack on this before
>  merging. It's not fair for me to stall this any longer, I'll trust their
>  judgement.
> 
>  Reasonable?
> >>>
> >>> I'd be more than happy for one of the drm-misc maintainers to Ack.
> >>> I'll move Maxime, Thomas, and Maarten to the "To:" line to see if that
> >>> helps get through their filters.
> >>
> >> I'll like some test reports to be sure it doesn't break anything,
> >> then I'll be happy to give my ack !
> >
> > Are you looking for any more test reports at this point? Eizan did
> > some testing and provided a tag, though this was also on ChromeOS.
> > Steev also tested on two non-ChromeOS environments and provided his
> > tag. It's also been another two weeks of this being rolled out to some
> > Chromebook users and I haven't heard any reports of problems. If
> > somehow something was missed, I'm happy to follow-up and provide
> > additional fixes if some report comes in later.
>
> Sure, thx I think you can apply it now
>
> Acked-by: Neil Armstrong 

Pushed to drm-misc-next.

8df1ddb5bf11 drm/dp: Don't attempt AUX transfers when eDP panels are not powered

Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

On 28.02.24 21:41, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
>> On 28.02.24 19:10, Matthias Kaehlcke wrote:
>>> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
 Most of the functionality this driver provides can be used by non-hub
 devices as well.

 To account for the hub-specific code, add a flag to the device data
 structure and check its value for hub-specific code.

 The 'always_powered_in_supend' attribute is only available for hub
 devices, keeping the driver's default behavior for non-hub devices (keep
 on in suspend).

 Signed-off-by: Javier Carrasco 
 ---
  drivers/usb/misc/onboard_usb_dev.c | 25 +++--
  drivers/usb/misc/onboard_usb_dev.h | 10 ++
  2 files changed, 33 insertions(+), 2 deletions(-)

 diff --git a/drivers/usb/misc/onboard_usb_dev.c 
 b/drivers/usb/misc/onboard_usb_dev.c
 index e1779bd2d126..df0ed172c7ec 100644
 --- a/drivers/usb/misc/onboard_usb_dev.c
 +++ b/drivers/usb/misc/onboard_usb_dev.c
 @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct 
 device *dev)
struct usbdev_node *node;
bool power_off = true;
  
 -  if (onboard_dev->always_powered_in_suspend)
 +  if (onboard_dev->always_powered_in_suspend &&
 +  !onboard_dev->pdata->is_hub)
return 0;
>>>
>>> With this non-hub devices would always be powered down, since
>>> 'always_powerd_in_suspend' is not set for them. This should be:
>>>
>>
>> May I ask you what you meant in v4 with this comment?
>>
>>> Even without the sysfs attribute the field 'always_powered_in_suspend'
>>> could
>>> be set to true by probe() for non-hub devices.
> 
> struct onboard_dev always has the field 'always_powered_in_suspend',
> even for non-hubs, that don't have the corresponding sysfs attribute.
> Currently it is left uninitialized (i.e. false) for non-hubs. Instead
> it could be initialized to true by probe() for non-hubs, which would
> be semantically correct. With that it wouldn't be necessary to check
> here whether a device is hub, because the field would provide the
> necessary information.
> 

That is maybe what is confusing me a bit. Should it not be false for
non-hub devices? That property is only meant for hubs, so why should
non-hub devices be always powered in suspend? I thought it should always
be false for non-hub devices, and configurable for hubs.

>>>   if (!onboard_dev->pdata->is_hub ||
>>>onboard_dev->always_powered_in_suspend)
>>>
>>> Checking for the (non-)hub status first is clearer IMO, also it avoids
>>> an unneccessary check of 'always_powered' for non-hub devices.
>>>
>>
>> That makes sense and will be fixed.
>>
>>> Without code context: for hubs there can be multiple device tree nodes
>>> for the same physical hub chip (e.g. one for the USB2 and another for
>>> the USB3 part). I suppose this could also be the case for non-hub
>>> devices. For hubs there is the 'peer-hub' device tree property to
>>> establish a link between the two USB devices, as a result the onboard
>>> driver only creates a single platform device (which is desired,
>>> otherwise two platform devices would be in charge for power sequencing
>>> the same phyiscal device. For non-hub devices there is currently no such
>>> link. In many cases I expect there will be just one DT entry even though
>>> the device has multiple USB interfaces, but it could happen and would
>>> actually be a more accurate representation.
>>>
>>> General support is already there (the code dealing with 'peer-hub'), but
>>> we'd have to come up with a suitable name. 'peer-device' is the first
>>> thing that comes to my mind, but there might be better options. If such
>>> a generic property is added then we should deprecate 'peer-hub', but
>>> maintain backwards compatibility.
>>
>> I have nothing against that, but the first non-hub device that will be
>> added does not have multiple DT nodes, so I have nothing to test that
>> extension with real hardware.
> 
> I see, the XVF3500 is USB 2.0 only, so it isn't suitable for testing.
> 
>> That could be added in the future, though, if the need ever arises.
> 
> I expect it will, when a DT maintainer asks the hardware to be
> represented correctly for a device that is connected to more than one USB
> bus. IIRC that's how 'peer-hub' was born :)
> 
> Ok, we can leave it out for now. I might send a dedicated patch after your
> series landed. If a switch to 'peer-device' or similar is anticipated then
> it's probably best to deprecate 'peer-hub' ASAP, to avoid it from getting
> added to more bindings.

Best regards,
Javier Carrasco

Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
> On 28.02.24 19:10, Matthias Kaehlcke wrote:
> > On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
> >> Most of the functionality this driver provides can be used by non-hub
> >> devices as well.
> >>
> >> To account for the hub-specific code, add a flag to the device data
> >> structure and check its value for hub-specific code.
> >>
> >> The 'always_powered_in_supend' attribute is only available for hub
> >> devices, keeping the driver's default behavior for non-hub devices (keep
> >> on in suspend).
> >>
> >> Signed-off-by: Javier Carrasco 
> >> ---
> >>  drivers/usb/misc/onboard_usb_dev.c | 25 +++--
> >>  drivers/usb/misc/onboard_usb_dev.h | 10 ++
> >>  2 files changed, 33 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c 
> >> b/drivers/usb/misc/onboard_usb_dev.c
> >> index e1779bd2d126..df0ed172c7ec 100644
> >> --- a/drivers/usb/misc/onboard_usb_dev.c
> >> +++ b/drivers/usb/misc/onboard_usb_dev.c
> >> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct 
> >> device *dev)
> >>struct usbdev_node *node;
> >>bool power_off = true;
> >>  
> >> -  if (onboard_dev->always_powered_in_suspend)
> >> +  if (onboard_dev->always_powered_in_suspend &&
> >> +  !onboard_dev->pdata->is_hub)
> >>return 0;
> > 
> > With this non-hub devices would always be powered down, since
> > 'always_powerd_in_suspend' is not set for them. This should be:
> > 
> 
> May I ask you what you meant in v4 with this comment?
> 
> > Even without the sysfs attribute the field 'always_powered_in_suspend'
> > could
> > be set to true by probe() for non-hub devices.

struct onboard_dev always has the field 'always_powered_in_suspend',
even for non-hubs, that don't have the corresponding sysfs attribute.
Currently it is left uninitialized (i.e. false) for non-hubs. Instead
it could be initialized to true by probe() for non-hubs, which would
be semantically correct. With that it wouldn't be necessary to check
here whether a device is hub, because the field would provide the
necessary information.

> >   if (!onboard_dev->pdata->is_hub ||
> >onboard_dev->always_powered_in_suspend)
> > 
> > Checking for the (non-)hub status first is clearer IMO, also it avoids
> > an unneccessary check of 'always_powered' for non-hub devices.
> > 
> 
> That makes sense and will be fixed.
> 
> > Without code context: for hubs there can be multiple device tree nodes
> > for the same physical hub chip (e.g. one for the USB2 and another for
> > the USB3 part). I suppose this could also be the case for non-hub
> > devices. For hubs there is the 'peer-hub' device tree property to
> > establish a link between the two USB devices, as a result the onboard
> > driver only creates a single platform device (which is desired,
> > otherwise two platform devices would be in charge for power sequencing
> > the same phyiscal device. For non-hub devices there is currently no such
> > link. In many cases I expect there will be just one DT entry even though
> > the device has multiple USB interfaces, but it could happen and would
> > actually be a more accurate representation.
> > 
> > General support is already there (the code dealing with 'peer-hub'), but
> > we'd have to come up with a suitable name. 'peer-device' is the first
> > thing that comes to my mind, but there might be better options. If such
> > a generic property is added then we should deprecate 'peer-hub', but
> > maintain backwards compatibility.
> 
> I have nothing against that, but the first non-hub device that will be
> added does not have multiple DT nodes, so I have nothing to test that
> extension with real hardware.

I see, the XVF3500 is USB 2.0 only, so it isn't suitable for testing.

> That could be added in the future, though, if the need ever arises.

I expect it will, when a DT maintainer asks the hardware to be
represented correctly for a device that is connected to more than one USB
bus. IIRC that's how 'peer-hub' was born :)

Ok, we can leave it out for now. I might send a dedicated patch after your
series landed. If a switch to 'peer-device' or similar is anticipated then
it's probably best to deprecate 'peer-hub' ASAP, to avoid it from getting
added to more bindings.

Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

On 28.02.24 19:10, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>> Most of the functionality this driver provides can be used by non-hub
>> devices as well.
>>
>> To account for the hub-specific code, add a flag to the device data
>> structure and check its value for hub-specific code.
>>
>> The 'always_powered_in_supend' attribute is only available for hub
>> devices, keeping the driver's default behavior for non-hub devices (keep
>> on in suspend).
>>
>> Signed-off-by: Javier Carrasco 
>> ---
>>  drivers/usb/misc/onboard_usb_dev.c | 25 +++--
>>  drivers/usb/misc/onboard_usb_dev.h | 10 ++
>>  2 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_dev.c 
>> b/drivers/usb/misc/onboard_usb_dev.c
>> index e1779bd2d126..df0ed172c7ec 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.c
>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct 
>> device *dev)
>>  struct usbdev_node *node;
>>  bool power_off = true;
>>  
>> -if (onboard_dev->always_powered_in_suspend)
>> +if (onboard_dev->always_powered_in_suspend &&
>> +!onboard_dev->pdata->is_hub)
>>  return 0;
> 
> With this non-hub devices would always be powered down, since
> 'always_powerd_in_suspend' is not set for them. This should be:
> 

May I ask you what you meant in v4 with this comment?

> Even without the sysfs attribute the field 'always_powered_in_suspend'
> could
> be set to true by probe() for non-hub devices.

>   if (!onboard_dev->pdata->is_hub ||
>onboard_dev->always_powered_in_suspend)
> 
> Checking for the (non-)hub status first is clearer IMO, also it avoids
> an unneccessary check of 'always_powered' for non-hub devices.
> 

That makes sense and will be fixed.

> Without code context: for hubs there can be multiple device tree nodes
> for the same physical hub chip (e.g. one for the USB2 and another for
> the USB3 part). I suppose this could also be the case for non-hub
> devices. For hubs there is the 'peer-hub' device tree property to
> establish a link between the two USB devices, as a result the onboard
> driver only creates a single platform device (which is desired,
> otherwise two platform devices would be in charge for power sequencing
> the same phyiscal device. For non-hub devices there is currently no such
> link. In many cases I expect there will be just one DT entry even though
> the device has multiple USB interfaces, but it could happen and would
> actually be a more accurate representation.
> 
> General support is already there (the code dealing with 'peer-hub'), but
> we'd have to come up with a suitable name. 'peer-device' is the first
> thing that comes to my mind, but there might be better options. If such
> a generic property is added then we should deprecate 'peer-hub', but
> maintain backwards compatibility.

I have nothing against that, but the first non-hub device that will be
added does not have multiple DT nodes, so I have nothing to test that
extension with real hardware.

That could be added in the future, though, if the need ever arises.

Best regards,
Javier Carrasco

[PATCH] drm/tidss: Use dev_err_probe() over dev_dbg() when failing to probe the port

2024-02-28 Thread Andrew Halaney

This gets logged out to /sys/kernel/debug/devices_deferred in the
-EPROBE_DEFER case and as an error otherwise. The message here provides
useful information to the user when troubleshooting why their display is
not working in either case, so let's make it output appropriately.

Signed-off-by: Andrew Halaney 
---
There's definitely more spots in this driver that could be upgraded from
dev_dbg() to something more appropriate, but this one burned me today so
I thought I'd send a patch for it specifically before I forget.
---
 drivers/gpu/drm/tidss/tidss_kms.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_kms.c 
b/drivers/gpu/drm/tidss/tidss_kms.c
index a0e494c806a96..f371518f86971 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -135,8 +135,7 @@ static int tidss_dispc_modeset_init(struct tidss_device 
*tidss)
dev_dbg(dev, "no panel/bridge for port %d\n", i);
continue;
} else if (ret) {
-   dev_dbg(dev, "port %d probe returned %d\n", i, ret);
-   return ret;
+   return dev_err_probe(dev, ret, "port %d probe 
failed\n", i);
}
 
if (panel) {

---
base-commit: 22ba90670a51a18c6b36d285fddf92b9887c0bc3
change-id: 20240228-tidss-dev-err-probe-fa61fb057029

Best regards,
-- 
Andrew Halaney

Re: [PATCH v5 2/8] usb: misc: onboard_hub: rename to onboard_dev

On 28.02.24 19:18, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 02:51:29PM +0100, Javier Carrasco wrote:
>> This patch prepares onboad_hub to support non-hub devices by renaming
>> the driver files and their content, the headers and their references.
>>
>> The comments and descriptions have been slightly modified to keep
>> coherence and account for the specific cases that only affect onboard
>> hubs (e.g. peer-hub).
>>
>> The "hub" variables in functions where "dev" (and similar names) variables
>> already exist have been renamed to onboard_dev for clarity, which adds a
>> few lines in cases where more than 80 characters are used.
>>
>> No new functionality has been added.
>>
>> Signed-off-by: Javier Carrasco 
>> ---
>>  ...-usb-hub => sysfs-bus-platform-onboard-usb-dev} |   3 +-
>>  MAINTAINERS|   4 +-
>>  drivers/usb/core/Makefile  |   4 +-
>>  drivers/usb/core/hub.c |   8 +-
>>  drivers/usb/core/hub.h |   2 +-
>>  drivers/usb/misc/Kconfig   |  16 +-
>>  drivers/usb/misc/Makefile  |   2 +-
>>  drivers/usb/misc/onboard_usb_dev.c | 519 
>> +
>>  .../misc/{onboard_usb_hub.h => onboard_usb_dev.h}  |  28 +-
>>  ...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} |  47 +-
>>  include/linux/usb/onboard_dev.h|  18 +
>>  include/linux/usb/onboard_hub.h|  18 -
>>  12 files changed, 595 insertions(+), 74 deletions(-)
> 
> This does not rename/delete onboard_usb_hub.c. With a rename there would
> probably be an actual diff for onboard_usb_dev.c instead of a new file,
> which would help with reviewing.

Thanks, I noticed that when I started working on v6 and it has been
fixed. I must have lost that one when fixing conflicts during the patch
re-ordering.

Best regards,
Javier Carrasco

Re: [PATCH v2] drm/i915/guc: Use context hints for GT freq

2024-02-28 Thread Belgaumkar, Vinay




On 2/28/2024 4:54 AM, Tvrtko Ursulin wrote:


On 27/02/2024 23:51, Vinay Belgaumkar wrote:

Allow user to provide a low latency context hint. When set, KMD
sends a hint to GuC which results in special handling for this
context. SLPC will ramp the GT frequency aggressively every time
it switches to this context. The down freq threshold will also be
lower so GuC will ramp down the GT freq for this context more slowly.
We also disable waitboost for this context as that will interfere with
the strategy.

We need to enable the use of SLPC Compute strategy during init, but
it will apply only to contexts that set this bit during context
creation.

Userland can check whether this feature is supported using a new param-
I915_PARAM_HAS_CONTEXT_FREQ_HINTS. This flag is true for all guc 
submission

enabled platforms as they use SLPC for frequency management.

The Mesa usage model for this flag is here -
https://gitlab.freedesktop.org/sushmave/mesa/-/commits/compute_hint

v2: Rename flags as per review suggestions (Rodrigo, Tvrtko).
Also, use flag bits in intel_context as it allows finer control for
toggling per engine if needed (Tvrtko).

Cc: Rodrigo Vivi 
Cc: Tvrtko Ursulin 
Cc: Sushma Venkatesh Reddy 
Signed-off-by: Vinay Belgaumkar 
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 +++--
  .../gpu/drm/i915/gem/i915_gem_context_types.h |  1 +
  drivers/gpu/drm/i915/gt/intel_context_types.h |  1 +
  drivers/gpu/drm/i915/gt/intel_rps.c   |  5 +
  .../drm/i915/gt/uc/abi/guc_actions_slpc_abi.h | 21 +++
  drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c   | 17 +++
  drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h   |  1 +
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  6 ++
  drivers/gpu/drm/i915/i915_getparam.c  | 12 +++
  include/uapi/drm/i915_drm.h   | 15 +
  10 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c

index dcbfe32fd30c..0799cb0b2803 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -879,6 +879,7 @@ static int set_proto_ctx_param(struct 
drm_i915_file_private *fpriv,

 struct i915_gem_proto_context *pc,
 struct drm_i915_gem_context_param *args)
  {
+    struct drm_i915_private *i915 = fpriv->i915;
  int ret = 0;
    switch (args->param) {
@@ -904,6 +905,13 @@ static int set_proto_ctx_param(struct 
drm_i915_file_private *fpriv,

  pc->user_flags &= ~BIT(UCONTEXT_BANNABLE);
  break;
  +    case I915_CONTEXT_PARAM_LOW_LATENCY:
+    if (intel_uc_uses_guc_submission(_gt(i915)->uc))
+    pc->user_flags |= BIT(UCONTEXT_LOW_LATENCY);
+    else
+    ret = -EINVAL;
+    break;
+
  case I915_CONTEXT_PARAM_RECOVERABLE:
  if (args->size)
  ret = -EINVAL;
@@ -992,6 +1000,9 @@ static int intel_context_set_gem(struct 
intel_context *ce,
  if (sseu.slice_mask && !WARN_ON(ce->engine->class != 
RENDER_CLASS))

  ret = intel_context_reconfigure_sseu(ce, sseu);
  +    if (test_bit(UCONTEXT_LOW_LATENCY, >user_flags))
+    set_bit(CONTEXT_LOW_LATENCY, >flags);


Does not need to be atomic so can use __set_bit as higher up in the 
function.

ok.



+
  return ret;
  }
  @@ -1630,6 +1641,8 @@ i915_gem_create_context(struct 
drm_i915_private *i915,

  if (vm)
  ctx->vm = vm;
  +    ctx->user_flags = pc->user_flags;
+


Given how most ctx->something assignments are at the bottom of the 
function I would stick a comment here saying along the lines of 
"assign early for intel_context_set_gem called when creating engines".

ok.



mutex_init(>engines_mutex);
  if (pc->num_user_engines >= 0) {
  i915_gem_context_set_user_engines(ctx);
@@ -1652,8 +1665,6 @@ i915_gem_create_context(struct drm_i915_private 
*i915,

   * is no remap info, it will be a NOP. */
  ctx->remap_slice = ALL_L3_SLICES(i915);
  -    ctx->user_flags = pc->user_flags;
-
  for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
  ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
  diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h

index 03bc7f9d191b..b6d97da63d1f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -338,6 +338,7 @@ struct i915_gem_context {
  #define UCONTEXT_BANNABLE    2
  #define UCONTEXT_RECOVERABLE    3
  #define UCONTEXT_PERSISTENCE    4
+#define UCONTEXT_LOW_LATENCY    5
    /**
   * @flags: small set of booleans
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h 
b/drivers/gpu/drm/i915/gt/intel_context_types.h

index 7eccbd70d89f..ed95a7b57cbb 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++

RE: Making drm_gpuvm work across gpu devices

2024-02-28 Thread Zeng, Oak

The mail wasn’t indent/preface correctly. Manually format it.

From: Christian König 
mailto:christian.koe...@amd.com>>
Sent: Tuesday, February 27, 2024 1:54 AM
To: Zeng, Oak mailto:oak.z...@intel.com>>; Danilo Krummrich 
mailto:d...@redhat.com>>; Dave Airlie 
mailto:airl...@redhat.com>>; Daniel Vetter 
mailto:dan...@ffwll.ch>>; Felix Kuehling 
mailto:felix.kuehl...@amd.com>>; 
jgli...@redhat.com
Cc: Welty, Brian mailto:brian.we...@intel.com>>; 
dri-devel@lists.freedesktop.org; 
intel...@lists.freedesktop.org; Bommu, 
Krishnaiah mailto:krishnaiah.bo...@intel.com>>; 
Ghimiray, Himal Prasad 
mailto:himal.prasad.ghimi...@intel.com>>; 
thomas.hellst...@linux.intel.com; 
Vishwanathapura, Niranjana 
mailto:niranjana.vishwanathap...@intel.com>>;
 Brost, Matthew mailto:matthew.br...@intel.com>>; 
Gupta, saurabhg mailto:saurabhg.gu...@intel.com>>
Subject: Re: Making drm_gpuvm work across gpu devices

Hi Oak,
Am 23.02.24 um 21:12 schrieb Zeng, Oak:
Hi Christian,

I go back this old email to ask a question.

sorry totally missed that one.

Quote from your email:
“Those ranges can then be used to implement the SVM feature required for higher 
level APIs and not something you need at the UAPI or even inside the low level 
kernel memory management.”
“SVM is a high level concept of OpenCL, Cuda, ROCm etc.. This should not have 
any influence on the design of the kernel UAPI.”

There are two category of SVM:

1.   driver svm allocator: this is implemented in user space,  i.g., 
cudaMallocManaged (cuda) or zeMemAllocShared (L0) or clSVMAlloc(openCL). Intel 
already have gem_create/vm_bind in xekmd and our umd implemented clSVMAlloc and 
zeMemAllocShared on top of gem_create/vm_bind. Range A..B of the process 
address space is mapped into a range C..D of the GPU address space, exactly as 
you said.

2.   system svm allocator:  This doesn’t introduce extra driver API for 
memory allocation. Any valid CPU virtual address can be used directly 
transparently in a GPU program without any extra driver API call. Quote from 
kernel Documentation/vm/hmm.hst: “Any application memory region (private 
anonymous, shared memory, or regular file backed memory) can be used by a 
device transparently” and “to share the address space by duplicating the CPU 
page table in the device page table so the same address points to the same 
physical memory for any valid main memory address in the process address 
space”. In system svm allocator, we don’t need that A..B C..D mapping.

It looks like you were talking of 1). Were you?

No, even when you fully mirror the whole address space from a process into the 
GPU you still need to enable this somehow with an IOCTL.

And while enabling this you absolutely should specify to which part of the 
address space this mirroring applies and where it maps to.

[Zeng, Oak]
Lets say we have a hardware platform where both CPU and GPU support 57bit(use 
it for example. The statement apply to any address range) virtual address 
range, how do you decide “which part of the address space this mirroring 
applies”? You have to mirror the whole address space [0~2^57-1], do you? As you 
designed it, the gigantic vm_bind/mirroring happens at the process 
initialization time, and at that time, you don’t know which part of the address 
space will be used for gpu program. Remember for system allocator, *any* valid 
CPU address can be used for GPU program.  If you add an offset to [0~2^57-1], 
you get an address out of 57bit address range. Is this a valid concern?

I see the system svm allocator as just a special case of the driver allocator 
where not fully backed buffer objects are allocated, but rather sparse one 
which are filled and migrated on demand.

[Zeng, Oak]
Above statement is true to me. We don’t have BO for system svm allocator. It is 
a sparse one as we can sparsely map vma to GPU. Our migration policy decide 
which pages/how much of the vma is migrated/mapped to GPU page table.

The difference b/t your mind and mine is, you want a gigantic vma (created 
during the gigantic vm_bind) to be sparsely populated to gpu. While I thought 
vma (xe_vma in xekmd codes) is a place to save memory attributes (such as 
caching, user preferred placement etc). All those memory attributes are range 
based, i.e., user can specify range1 is cached while range2 is uncached. So I 
don’t see how you can manage it with the gigantic vma. Do you split your 
gigantic vma later to save range based memory attributes?

Regards,
Oak

Regards,
Christian.

[PATCH 2/2] drm/msm/dpu: drop dpu_kms from _dpu_kms_initialize_writeback

2024-02-28 Thread Abhinav Kumar

Following the pattern of other interfaces, lets align writeback
as well by dropping the dpu_kms parameter in its _dpu_kms_initialize_*
function.

Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ab924ac78c9b..382ef4c8e8eb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -620,13 +620,12 @@ static int _dpu_kms_initialize_hdmi(struct drm_device 
*dev,
 }
 
 static int _dpu_kms_initialize_writeback(struct drm_device *dev,
-   struct msm_drm_private *priv, struct dpu_kms *dpu_kms,
+   struct msm_drm_private *priv, u32 maxlinewidth,
const u32 *wb_formats, int n_formats)
 {
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
const enum dpu_wb wb_idx = WB_2;
-   u32 maxlinewidth;
int rc;
 
memset(, 0, sizeof(info));
@@ -636,8 +635,6 @@ static int _dpu_kms_initialize_writeback(struct drm_device 
*dev,
info.h_tile_instance[0] = wb_idx;
info.intf_type = INTF_WB;
 
-   maxlinewidth = dpu_rm_get_wb(_kms->rm, 
info.h_tile_instance[0])->caps->maxlinewidth;
-
encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, );
if (IS_ERR(encoder)) {
DPU_ERROR("encoder init failed for dsi display\n");
@@ -690,9 +687,12 @@ static int _dpu_kms_setup_displays(struct drm_device *dev,
if (dpu_kms->catalog->wb_count) {
for (i = 0; i < dpu_kms->catalog->wb_count; i++) {
if (dpu_kms->catalog->wb[i].id == WB_2) {
-   rc = _dpu_kms_initialize_writeback(dev, priv, 
dpu_kms,
-   
dpu_kms->catalog->wb[i].format_list,
-   
dpu_kms->catalog->wb[i].num_formats);
+   u32 wb_maxlinewidth = 
dpu_kms->catalog->wb[i].maxlinewidth;
+   const u32 *wb_formats = 
dpu_kms->catalog->wb[i].format_list;
+   u32 n_formats = 
dpu_kms->catalog->wb[i].num_formats;
+
+   rc = _dpu_kms_initialize_writeback(dev, priv, 
wb_maxlinewidth,
+  wb_formats, 
n_formats);
if (rc) {
DPU_ERROR("initialize_WB failed, rc = 
%d\n", rc);
return rc;
-- 
2.34.1

[PATCH 1/2] drm/msm/dpu: drop unused dpu_kms from interface initialization

2024-02-28 Thread Abhinav Kumar

dpu_kms seems unused while initializing DSI, HDMI and DP through
their respective _dpu_kms_initialize_* functions.

Hence lets drop the parameter altogether.

Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 2af62d8fa9a7..ab924ac78c9b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -494,8 +494,7 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, 
unsigned crtc_mask)
 }
 
 static int _dpu_kms_initialize_dsi(struct drm_device *dev,
-   struct msm_drm_private *priv,
-   struct dpu_kms *dpu_kms)
+  struct msm_drm_private *priv)
 {
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
@@ -558,8 +557,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 }
 
 static int _dpu_kms_initialize_displayport(struct drm_device *dev,
-   struct msm_drm_private *priv,
-   struct dpu_kms *dpu_kms)
+  struct msm_drm_private *priv)
 {
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
@@ -592,8 +590,7 @@ static int _dpu_kms_initialize_displayport(struct 
drm_device *dev,
 }
 
 static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
-   struct msm_drm_private *priv,
-   struct dpu_kms *dpu_kms)
+   struct msm_drm_private *priv)
 {
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
@@ -671,19 +668,19 @@ static int _dpu_kms_setup_displays(struct drm_device *dev,
int rc = 0;
int i;
 
-   rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
+   rc = _dpu_kms_initialize_dsi(dev, priv);
if (rc) {
DPU_ERROR("initialize_dsi failed, rc = %d\n", rc);
return rc;
}
 
-   rc = _dpu_kms_initialize_displayport(dev, priv, dpu_kms);
+   rc = _dpu_kms_initialize_displayport(dev, priv);
if (rc) {
DPU_ERROR("initialize_DP failed, rc = %d\n", rc);
return rc;
}
 
-   rc = _dpu_kms_initialize_hdmi(dev, priv, dpu_kms);
+   rc = _dpu_kms_initialize_hdmi(dev, priv);
if (rc) {
DPU_ERROR("initialize HDMI failed, rc = %d\n", rc);
return rc;
-- 
2.34.1

Re: [PATCH 1/3] [v2] drm/xe/kunit: fix link failure with built-in xe



On Mon, 26 Feb 2024 13:46:36 +0100, Arnd Bergmann wrote:
> When the driver is built-in but the tests are in loadable modules,
> the helpers don't actually get put into the driver:
> 
> ERROR: modpost: "xe_kunit_helper_alloc_xe_device" 
> [drivers/gpu/drm/xe/tests/xe_test.ko] undefined!
> 
> Change the Makefile to ensure they are always part of the driver
> even when the rest of the kunit tests are in loadable modules.
> 
> [...]

All 3 patches applied to drm-xe-next. Thanks!

[1/3] drm/xe/kunit: fix link failure with built-in xe
  commit: 0e6fec6da25167a568fbaeb8401d8172069124ad
[2/3] drm/xe/mmio: fix build warning for BAR resize on 32-bit
  commit: f5d3983366c0b88ec388b3407b29c1c0862ee2b8
[3/3] drm/xe/xe2: fix 64-bit division in pte_update_size
  commit: 1408784b599927d2f361bac6dc5170d2ee275f17

Best regards,
-- 
Lucas De Marchi

Re: [PATCH v5 8/8] usb: misc: onboard_hub: add support for XMOS XVF3500

On Wed, Feb 28, 2024 at 02:51:35PM +0100, Javier Carrasco wrote:
> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
> multicore controller for voice processing.
> 
> This device requires a specific power sequence, which consists of
> enabling the regulators that control the 3V3 and 1V0 device supplies,
> and a reset de-assertion after a delay of at least 100ns. Such power
> sequence is already supported by the onboard_hub driver, and it can be
> reused for non-hub USB devices as well.

Please update the commit message, the onboard_hub driver no longer
exists as such with the other patches of this series.

> Once in normal operation, the XVF3500 registers itself as a USB device,
> and it does not require any device-specific operations in the driver.
> 
> [1] https://www.xmos.com/xvf3500/
> 
> Signed-off-by: Javier Carrasco 

Acked-by: Matthias Kaehlcke 

> ---
>  drivers/usb/misc/onboard_usb_dev.c | 2 ++
>  drivers/usb/misc/onboard_usb_dev.h | 8 
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_dev.c 
> b/drivers/usb/misc/onboard_usb_dev.c
> index df0ed172c7ec..50f84c5278a2 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -405,6 +405,7 @@ static struct platform_driver onboard_dev_driver = {
>  #define VENDOR_ID_REALTEK0x0bda
>  #define VENDOR_ID_TI 0x0451
>  #define VENDOR_ID_VIA0x2109
> +#define VENDOR_ID_XMOS   0x20B1
>  
>  /*
>   * Returns the onboard_dev platform device that is associated with the USB
> @@ -497,6 +498,7 @@ static const struct usb_device_id onboard_dev_id_table[] 
> = {
>   { USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
>   { USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */
>   { USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */
> + { USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XVF3500 */

nit: be a bit more specific? e.g. XVF3500 Voice Processor, or XMOS XVF3500

The other entries were implicitly hubs since this was the 'onboard_hub'
driver. It wouldn't be a bad idea to add 'hub' to these entries in the
patch that 'renames' the driver.

[pull] drm/msm: drm-msm-fixes-2024-02-28 for v6.8-rc7

2024-02-28 Thread Rob Clark

Hi Dave,

A late revert to address a displayport hpd regression.

The following changes since commit 8c7bfd8262319fd3f127a5380f593ea76f1b88a2:

  drm/msm: Wire up tlb ops (2024-02-15 08:51:31 -0800)

are available in the Git repository at:

  https://gitlab.freedesktop.org/drm/msm.git tags/drm-msm-fixes-2024-02-28

for you to fetch changes up to 664bad6af3cbe01d6804b7264bee674b3e7dae7e:

  Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status
changes" (2024-02-28 15:32:29 +0200)


Fixes for v6.8-rc7

DP:
- Revert a change which was causing a HDP regression


Dmitry Baryshkov (1):
  Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD
status changes"

 drivers/gpu/drm/msm/dp/dp_display.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Re: [PATCH v5 2/8] usb: misc: onboard_hub: rename to onboard_dev

On Wed, Feb 28, 2024 at 02:51:29PM +0100, Javier Carrasco wrote:
> This patch prepares onboad_hub to support non-hub devices by renaming
> the driver files and their content, the headers and their references.
> 
> The comments and descriptions have been slightly modified to keep
> coherence and account for the specific cases that only affect onboard
> hubs (e.g. peer-hub).
> 
> The "hub" variables in functions where "dev" (and similar names) variables
> already exist have been renamed to onboard_dev for clarity, which adds a
> few lines in cases where more than 80 characters are used.
> 
> No new functionality has been added.
> 
> Signed-off-by: Javier Carrasco 
> ---
>  ...-usb-hub => sysfs-bus-platform-onboard-usb-dev} |   3 +-
>  MAINTAINERS|   4 +-
>  drivers/usb/core/Makefile  |   4 +-
>  drivers/usb/core/hub.c |   8 +-
>  drivers/usb/core/hub.h |   2 +-
>  drivers/usb/misc/Kconfig   |  16 +-
>  drivers/usb/misc/Makefile  |   2 +-
>  drivers/usb/misc/onboard_usb_dev.c | 519 
> +
>  .../misc/{onboard_usb_hub.h => onboard_usb_dev.h}  |  28 +-
>  ...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} |  47 +-
>  include/linux/usb/onboard_dev.h|  18 +
>  include/linux/usb/onboard_hub.h|  18 -
>  12 files changed, 595 insertions(+), 74 deletions(-)

This does not rename/delete onboard_usb_hub.c. With a rename there would
probably be an actual diff for onboard_usb_dev.c instead of a new file,
which would help with reviewing.

Re: [PATCH v5 07/14] drm/panthor: Add the MMU/VM logical block

2024-02-28 Thread Boris Brezillon

On Sun, 18 Feb 2024 22:41:21 +0100
Boris Brezillon  wrote:

> +int panthor_vm_active(struct panthor_vm *vm)
> +{
> + struct panthor_device *ptdev = vm->ptdev;
> + struct io_pgtable_cfg *cfg = 
> _pgtable_ops_to_pgtable(vm->pgtbl_ops)->cfg;
> + int ret = 0, as, cookie;
> + u64 transtab, transcfg;
> +
> + if (!drm_dev_enter(>base, ))
> + return -ENODEV;
> +
> + if (refcount_inc_not_zero(>as.active_cnt))
> + goto out_dev_exit;
> +
> + mutex_lock(>mmu->as.slots_lock);
> +
> + if (refcount_inc_not_zero(>as.active_cnt))
> + goto out_unlock;
> +
> + as = vm->as.id;
> + if (as >= 0) {
> + /* Unhandled pagefault on this AS, the MMU was disabled. We 
> need to
> +  * re-enable the MMU after clearing+unmasking the AS interrupts.
> +  */
> + if (ptdev->mmu->as.faulty_mask & 
> panthor_mmu_as_fault_mask(ptdev, as))
> + goto out_enable_as;
> +
> + goto out_make_active;
> + }
> +
> + /* Check for a free AS */
> + if (vm->for_mcu) {
> + drm_WARN_ON(>base, ptdev->mmu->as.alloc_mask & BIT(0));
> + as = 0;
> + } else {
> + as = ffz(ptdev->mmu->as.alloc_mask | BIT(0));
> + }
> +
> + if (!(BIT(as) & ptdev->gpu_info.as_present)) {
> + struct panthor_vm *lru_vm;
> +
> + lru_vm = list_first_entry_or_null(>mmu->as.lru_list,
> +   struct panthor_vm,
> +   as.lru_node);
> + if (drm_WARN_ON(>base, !lru_vm)) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
> +
> + drm_WARN_ON(>base, 
> refcount_read(_vm->as.active_cnt));
> + as = lru_vm->as.id;
> + panthor_vm_release_as_locked(lru_vm);
> + }
> +
> + /* Assign the free or reclaimed AS to the FD */
> + vm->as.id = as;
> + set_bit(as, >mmu->as.alloc_mask);
> + ptdev->mmu->as.slots[as].vm = vm;
> +
> +out_enable_as:
> + transtab = cfg->arm_lpae_s1_cfg.ttbr;
> + transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
> +AS_TRANSCFG_PTW_RA |
> +AS_TRANSCFG_ADRMODE_AARCH64_4K;

We also need

   AS_TRANSCFG_INA_BITS(55 - va_bits);

otherwise we lack one page table level on 32-bit platforms where the
virtual address space is artificially limited to 32bits.


> + if (ptdev->coherent)
> + transcfg |= AS_TRANSCFG_PTW_SH_OS;
> +
> + /* If the VM is re-activated, we clear the fault. */
> + vm->unhandled_fault = false;
> +
> + /* Unhandled pagefault on this AS, clear the fault and re-enable 
> interrupts
> +  * before enabling the AS.
> +  */
> + if (ptdev->mmu->as.faulty_mask & panthor_mmu_as_fault_mask(ptdev, as)) {
> + gpu_write(ptdev, MMU_INT_CLEAR, 
> panthor_mmu_as_fault_mask(ptdev, as));
> + ptdev->mmu->as.faulty_mask &= ~panthor_mmu_as_fault_mask(ptdev, 
> as);
> + gpu_write(ptdev, MMU_INT_MASK, ~ptdev->mmu->as.faulty_mask);
> + }
> +
> + ret = panthor_mmu_as_enable(vm->ptdev, vm->as.id, transtab, transcfg, 
> vm->memattr);
> +
> +out_make_active:
> + if (!ret) {
> + refcount_set(>as.active_cnt, 1);
> + list_del_init(>as.lru_node);
> + }
> +
> +out_unlock:
> + mutex_unlock(>mmu->as.slots_lock);
> +
> +out_dev_exit:
> + drm_dev_exit(cookie);
> + return ret;
> +}

Re: [PATCH v5 10/14] drm/panthor: Add the scheduler logical block

2024-02-28 Thread Boris Brezillon

On Sun, 18 Feb 2024 22:41:24 +0100
Boris Brezillon  wrote:

> +static void panthor_sched_immediate_tick(struct panthor_device *ptdev)
> +{
> + struct panthor_scheduler *sched = ptdev->scheduler;
> +
> + sched_queue_delayed_work(sched, tick, 0);
> +}
> +
> +/**
> + * panthor_sched_report_mmu_fault() - Report MMU faults to the scheduler.
> + */
> +void panthor_sched_report_mmu_fault(struct panthor_device *ptdev)
> +{
> + /* Force a tick to immediately kill faulty groups. */
> + panthor_sched_immediate_tick(ptdev);

We need

if (ptdev->scheduler)
panthor_sched_immediate_tick(ptdev);

to prevent a NULL deref in panthor_sched_immediate_tick() if the MMU
fault handler is called before the scheduler block is initialized (FW
boot might trigger MMU faults on AS0).

> +}

Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
> Most of the functionality this driver provides can be used by non-hub
> devices as well.
> 
> To account for the hub-specific code, add a flag to the device data
> structure and check its value for hub-specific code.
> 
> The 'always_powered_in_supend' attribute is only available for hub
> devices, keeping the driver's default behavior for non-hub devices (keep
> on in suspend).
> 
> Signed-off-by: Javier Carrasco 
> ---
>  drivers/usb/misc/onboard_usb_dev.c | 25 +++--
>  drivers/usb/misc/onboard_usb_dev.h | 10 ++
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/misc/onboard_usb_dev.c 
> b/drivers/usb/misc/onboard_usb_dev.c
> index e1779bd2d126..df0ed172c7ec 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct 
> device *dev)
>   struct usbdev_node *node;
>   bool power_off = true;
>  
> - if (onboard_dev->always_powered_in_suspend)
> + if (onboard_dev->always_powered_in_suspend &&
> + !onboard_dev->pdata->is_hub)
>   return 0;

With this non-hub devices would always be powered down, since
'always_powerd_in_suspend' is not set for them. This should be:

  if (!onboard_dev->pdata->is_hub ||
   onboard_dev->always_powered_in_suspend)

Checking for the (non-)hub status first is clearer IMO, also it avoids
an unneccessary check of 'always_powered' for non-hub devices.

Without code context: for hubs there can be multiple device tree nodes
for the same physical hub chip (e.g. one for the USB2 and another for
the USB3 part). I suppose this could also be the case for non-hub
devices. For hubs there is the 'peer-hub' device tree property to
establish a link between the two USB devices, as a result the onboard
driver only creates a single platform device (which is desired,
otherwise two platform devices would be in charge for power sequencing
the same phyiscal device. For non-hub devices there is currently no such
link. In many cases I expect there will be just one DT entry even though
the device has multiple USB interfaces, but it could happen and would
actually be a more accurate representation.

General support is already there (the code dealing with 'peer-hub'), but
we'd have to come up with a suitable name. 'peer-device' is the first
thing that comes to my mind, but there might be better options. If such
a generic property is added then we should deprecate 'peer-hub', but
maintain backwards compatibility.

Re: [PATCH] Revert "drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes"

2024-02-28 Thread Abhinav Kumar





On 2/28/2024 5:28 AM, Johan Hovold wrote:

On Wed, Feb 28, 2024 at 01:08:04PM +0200, Dmitry Baryshkov wrote:

On Wed, 28 Feb 2024 at 11:50, Johan Hovold  wrote:


On Tue, Feb 27, 2024 at 02:11:56PM -0800, Abhinav Kumar wrote:

On 2/27/2024 2:08 PM, Dmitry Baryshkov wrote:

This reverts commit e467e0bde881 ("drm/msm/dp: use
drm_bridge_hpd_notify() to report HPD status changes").

The commit changed the way how the MSM DP driver communicates
HPD-related events to the userspace. The mentioned commit made some of
the HPD events being reported earlier. This way userspace starts poking
around. It interacts in a bad way with the dp_bridge_detect and the
driver's state machine, ending up either with the very long delays
during hotplug detection or even inability of the DP driver to report
the display as connected.

A proper fix will involve redesigning of the HPD handling in the MSM DP
driver. It is underway, but it will be intrusive and can not be thought
about as a simple fix for the issue. Thus, revert the offending commit.


Yes, for fixing this on 6.9 I am fine with this.


Since this is a regression in 6.8-rc1, I hope you meant to say 6.8 here?


In the worst case it will land to 6.8.x via the stable tree process.


This is a fix for a user-visible regression that was reported formally
two weeks ago and informally (e.g. to you) soon after rc1 came out, and
which now also has an identified cause and an analysis of the problem.
And we're at rc6 so there should be no reason to delay fixing this (e.g.
even if you want to run some more tests for a couple of days).



Yup, we landed it in msm-fixes now, so this will go as a late -fixes PR 
for 6.8.




Johan

Re: [PATCH v2 8/9] media: dt-bindings: Add Intel Displayport RX IP

2024-02-28 Thread Rob Herring

On Wed, Feb 28, 2024 at 02:09:33PM +0100, Paweł Anikiel wrote:
> On Wed, Feb 28, 2024 at 1:18 PM Krzysztof Kozlowski
>  wrote:
> >
> > On 28/02/2024 12:05, Paweł Anikiel wrote:
> > > On Tue, Feb 27, 2024 at 3:29 PM Rob Herring  wrote:
> > >>
> > >> On Mon, Feb 26, 2024 at 11:59:42AM +0100, Paweł Anikiel wrote:
> > >>> On Mon, Feb 26, 2024 at 10:13 AM Krzysztof Kozlowski
> > >>>  wrote:
> > 
> >  On 21/02/2024 17:02, Paweł Anikiel wrote:
> > > The Intel Displayport RX IP is a part of the DisplayPort Intel FPGA IP
> > > Core. It implements a DisplayPort 1.4 receiver capable of HBR3 video
> > > capture and Multi-Stream Transport. The user guide can be found here:
> > >
> > > https://www.intel.com/programmable/technical-pdfs/683273.pdf
> > >
> > > Signed-off-by: Paweł Anikiel 
> > > ---
> > >  .../devicetree/bindings/media/intel,dprx.yaml | 160 
> > > ++
> > >  1 file changed, 160 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/media/intel,dprx.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/intel,dprx.yaml 
> > > b/Documentation/devicetree/bindings/media/intel,dprx.yaml
> > > new file mode 100644
> > > index ..31025f2d5dcd
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/intel,dprx.yaml
> > > @@ -0,0 +1,160 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/intel,dprx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Intel DisplayPort RX IP
> > > +
> > > +maintainers:
> > > +  - Paweł Anikiel 
> > > +
> > > +description: |
> > > +  The Intel Displayport RX IP is a part of the DisplayPort Intel 
> > > FPGA IP
> > > +  Core. It implements a DisplayPort 1.4 receiver capable of HBR3 
> > > video
> > > +  capture and Multi-Stream Transport.
> > > +
> > > +  The IP features a large number of configuration parameters, found 
> > > at:
> > > +  
> > > https://www.intel.com/content/www/us/en/docs/programmable/683273/23-3-20-0-1/sink-parameters.html
> > > +
> > > +  The following parameters have to be enabled:
> > > +- Support DisplayPort sink
> > > +- Enable GPU control
> > > +  The following parameters' values have to be set in the devicetree:
> > > +- RX maximum link rate
> > > +- Maximum lane count
> > > +- Support MST
> > > +- Max stream count (only if Support MST is enabled)
> > > +
> > > +properties:
> > > +  compatible:
> > > +const: intel,dprx-20.0.1
> > > +
> > > +  reg:
> > > +maxItems: 1
> > > +
> > > +  interrupts:
> > > +maxItems: 1
> > > +
> > > +  intel,max-link-rate:
> > > +$ref: /schemas/types.yaml#/definitions/uint32
> > > +description: Max link rate configuration parameter
> > 
> >  Please do not duplicate property name in description. It's useless.
> >  Instead explain what is this responsible for.
> > 
> >  Why max-link-rate would differ for the same dprx-20.0.1? And why
> >  standard properties cannot be used?
> > 
> >  Same for all questions below.
> > >>>
> > >>> These four properties are the IP configuration parameters mentioned in
> > >>> the device description. When generating the IP core you can set these
> > >>> parameters, which could make them differ for the same dprx-20.0.1.
> > >>> They are documented in the user guide, for which I also put a link in
> > >>> the description. Is that enough? Or should I also document these
> > >>> parameters here?
> > >>
> > >> Use the standard properties: link-frequencies and data-lanes. Those go
> > >> under the port(s) because they are inheritly per logical link.
> > >
> > > The DP receiver has one input interface (a deserialized DP stream),
> > > and up to four output interfaces (the decoded video streams). The "max
> > > link rate" and "max lane count" parameters only describe the input
> > > interface to the receiver. However, the port(s) I am using here are
> > > for the output streams. They are not affected by those parameters, so
> > > I don't think these properties should go under the output port(s).
> > >
> > > The receiver doesn't have an input port in the DT, because there isn't
> > > any controllable entity on the other side - the deserializer doesn't
> > > have any software interface. Since these standard properties
> > > (link-frequencies and data-lanes) are only defined in
> > > video-interfaces.yaml (which IIUC describes a graph endpoint), I can't
> > > use them directly in the device node.
> >
> > DT describes the hardware, so where does the input come? From something,
> > right? Regardless if you have a driver or not. There is dp-connector
> > binding, if

Re: [PATCH 2/2] fbcon: Defer console takeover for splash screens to first switch

2024-02-28 Thread Mario Limonciello


On 2/28/2024 05:54, Hans de Goede wrote:

Hi Daniel,

On 2/28/24 03:00, Daniel van Vugt wrote:

On 27/2/24 21:47, Hans de Goede wrote:





I think some boot failures also take you to the grub menu automatically next 
time?


In Fedora all boot failures will unhide the grub menu on
the next boot. This unfortunately relies on downstream changes
so I don't know what Ubuntu does here.




The kernel itself will be quiet as long as you set
CONFIG_CONSOLE_LOGLEVEL_QUIET=3 Ubuntu atm has set this
to 4 which means any kernel pr_err() or dev_err()
messages will get through and since there are quite
a few false positives of those Ubuntu really needs
to set CONFIG_CONSOLE_LOGLEVEL_QUIET=3 to fix part of:
https://bugs.launchpad.net/bugs/1970069


Incorrect. In my testing some laptops needed log level as low as 2 to go quiet.
And the Ubuntu kernel team is never going to fix all those for non-sponsored
devices.


Notice that atm Ubuntu's kernel is using the too high
CONFIG_CONSOLE_LOGLEVEL_QUIET=4 with
CONFIG_CONSOLE_LOGLEVEL_QUIET=3 getting any errors logged
to the console should be very very rare.

The only thing I can think of is if the kernel oopses
/ WARN()s early on but the cause is innocent enough
that the boot happily continues.

In that case actually showing the oops/WARN() is a good
thing.

For all the years Fedora has had flickerfree boot I have
seen zero bug reports about this. If you have examples
of this actually being a problem please file bugs for
them (launchpad or bugzilla.kernel.org is fine) and
then lets take a look at those bugs and fix them.

These should be so rare that I'm not worried about this
becoming a never ending list of bugs (unlike pr_err() /
dev_err() messages of which there are simply too many).


I personally own many laptops with so many different boot messages that it's
overwhelming for me already to report bugs for each of them. Hence this patch.

Also I don't own all the laptops in the world, so fixing all the errors just
for my collection wouldn't solve all cases. Whereas this patch does.


Almost all of those boot messages are because Ubuntu has
set CONFIG_CONSOLE_LOGLEVEL_QUIET too high. Once that is fixed
there should be very little of not no messages left.

I too own many laptops and I'm not seeing this issue on
any of them.

You claim you are still seeing errors with
CONFIG_CONSOLE_LOGLEVEL_QUIET=3 yet you have not provided
a single example!


Sorry, but your real problem here seems to be your
noisy downstream systemd patch. I'm not going to ack
a kernel patch which I consider a bad idea because
Ubuntu has a non standard systemd patch which is
to trigger happy with spamming the console.


Indeed the systemd patch is a big problem. We seem to have had it for 9 years
or so. I only just discovered it recently and would love to drop it, but was
told we can't. Its main problem is that it uses the console as a communication
pipe to plymouth. So simply making it less noisy isn't possible without
disabling its functionality. It was seemingly intended to run behind the
splash, but since it does fsck it tends to run before the splash (because DRM
startup takes a few more seconds).


This comes back to what I was saying before - Ubuntu (and anyone else 
that wants a flicker free boot for that matter) should adopt simpledrm.


When simpledrm is compiled into the kernel DRM will be up long before 
the splash screen comes up.  When drivers do fastboot (Intel) or 
seamless (AMD) handoff you /should/ be able to get the splash screen 
without a modeset.


I think between doing that and changing the default log level not to 
show console err messages will go a long way.


If there is a concern that people need to see those; how about changing 
the kernel command line for the recovery kernel so that they only come 
up in the recovery kernel?




This does indeed sound like it is a non trivial problem to fix,
but that is still not a good reason to add this (IMHO) hack
to the kernel.

The issue deferred fbcon takeover was designed to fix is that
the fbcon would mess up the framebuffer contents even if
nothing was ever logged to the console.

The whole idea being that to still have the fbcon come up
as soon as there are any messages.

Actively hiding messages was never part of the design, so
this is still a NACK from me.

Also note that this matches how things work in grub
and shim when I first implemented flickerfree boot
I also had to patch shim and grub to not make EFI
text output protocol calls (including init()) until
they actually had some text to show.

So the whole design here for shim, grub and the kernel
has always been to not mess with the framebuffer until
there is some text (any text) to output and then show
that text immediately.

I do not think that deviating from this design is a good
idea.

Regards,

Hans




Closes: https://bugs.launchpad.net/bugs/1970069
Cc: Mario Limonciello 
Signed-off-by: Daniel van Vugt 
---
  drivers/video/fbdev/core/fbcon.c | 32

[PATCH] soc: qcom: pmic_glink_altmode: Use common error handling code in pmic_glink_altmode_probe()

2024-02-28 Thread Markus Elfring

From: Markus Elfring 
Date: Wed, 28 Feb 2024 18:45:13 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function implementation.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/soc/qcom/pmic_glink_altmode.c | 57 +--
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c 
b/drivers/soc/qcom/pmic_glink_altmode.c
index b3808fc24c69..c987a45e1703 100644
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -434,8 +434,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device 
*adev,
ret = fwnode_property_read_u32(fwnode, "reg", );
if (ret < 0) {
dev_err(dev, "missing reg property of %pOFn\n", fwnode);
-   fwnode_handle_put(fwnode);
-   return ret;
+   goto put_fwnode;
}

if (port >= ARRAY_SIZE(altmode->ports)) {
@@ -445,8 +444,8 @@ static int pmic_glink_altmode_probe(struct auxiliary_device 
*adev,

if (altmode->ports[port].altmode) {
dev_err(dev, "multiple connector definition for port 
%u\n", port);
-   fwnode_handle_put(fwnode);
-   return -EINVAL;
+   ret = -EINVAL;
+   goto put_fwnode;
}

alt_port = >ports[port];
@@ -456,8 +455,8 @@ static int pmic_glink_altmode_probe(struct auxiliary_device 
*adev,

alt_port->bridge = devm_drm_dp_hpd_bridge_alloc(dev, 
to_of_node(fwnode));
if (IS_ERR(alt_port->bridge)) {
-   fwnode_handle_put(fwnode);
-   return PTR_ERR(alt_port->bridge);
+   ret = PTR_ERR(alt_port->bridge);
+   goto put_fwnode;
}

alt_port->dp_alt.svid = USB_TYPEC_DP_SID;
@@ -466,48 +465,42 @@ static int pmic_glink_altmode_probe(struct 
auxiliary_device *adev,

alt_port->typec_mux = fwnode_typec_mux_get(fwnode);
if (IS_ERR(alt_port->typec_mux)) {
-   fwnode_handle_put(fwnode);
-   return dev_err_probe(dev, PTR_ERR(alt_port->typec_mux),
-"failed to acquire mode-switch for 
port: %d\n",
-port);
+   ret = dev_err_probe(dev, PTR_ERR(alt_port->typec_mux),
+   "failed to acquire mode-switch for 
port: %d\n",
+   port);
+   goto put_fwnode;
}

ret = devm_add_action_or_reset(dev, pmic_glink_altmode_put_mux,
   alt_port->typec_mux);
-   if (ret) {
-   fwnode_handle_put(fwnode);
-   return ret;
-   }
+   if (ret)
+   goto put_fwnode;

alt_port->typec_retimer = fwnode_typec_retimer_get(fwnode);
if (IS_ERR(alt_port->typec_retimer)) {
-   fwnode_handle_put(fwnode);
-   return dev_err_probe(dev, 
PTR_ERR(alt_port->typec_retimer),
-"failed to acquire retimer-switch 
for port: %d\n",
-port);
+   ret = dev_err_probe(dev, 
PTR_ERR(alt_port->typec_retimer),
+   "failed to acquire retimer-switch 
for port: %d\n",
+   port);
+   goto put_fwnode;
}

ret = devm_add_action_or_reset(dev, 
pmic_glink_altmode_put_retimer,
   alt_port->typec_retimer);
-   if (ret) {
-   fwnode_handle_put(fwnode);
-   return ret;
-   }
+   if (ret)
+   goto put_fwnode;

alt_port->typec_switch = fwnode_typec_switch_get(fwnode);
if (IS_ERR(alt_port->typec_switch)) {
-   fwnode_handle_put(fwnode);
-   return dev_err_probe(dev, 
PTR_ERR(alt_port->typec_switch),
-"failed to acquire 
orientation-switch for port: %d\n",
-port);
+   ret = dev_err_probe(dev, 
PTR_ERR(alt_port->typec_switch),
+   "failed to acquire 
orientation-switch for port: %d\n",
+   port);
+   goto put_fwnode;
}

ret =

[linux-next:master] BUILD REGRESSION 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e

2024-02-28 Thread kernel test robot

tree/branch: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch HEAD: 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e  Add linux-next specific 
files for 20240228

Error/Warning reports:

https://lore.kernel.org/oe-kbuild-all/202402281921.gr4yw253-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202402282106.ou7dstpq-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202402282202.yv6gmmju-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202402282343.fvkeq0vs-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202402282356.8hzbimag-...@intel.com
https://lore.kernel.org/oe-kbuild-all/20240229.tx1ed1x7-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202402290152.fpueak3i-...@intel.com

Error/Warning: (recently discovered and may have been fixed)

drivers/gpu/drm/display/drm_dp_tunnel.c:1185: warning: expecting prototype for 
drm_dp_tunnel_atomic_get_allocated_bw(). Prototype was for 
drm_dp_tunnel_get_allocated_bw() instead
drivers/gpu/drm/display/drm_dp_tunnel.c:1903: warning: Function parameter or 
struct member 'max_group_count' not described in 'drm_dp_tunnel_mgr_create'
drivers/gpu/drm/display/drm_dp_tunnel.c:447: warning: Function parameter or 
struct member 'tracker' not described in 'drm_dp_tunnel_put'
drivers/gpu/drm/display/drm_dp_tunnel.c:447: warning: Function parameter or 
struct member 'tunnel' not described in 'drm_dp_tunnel_put'
drivers/gpu/drm/xe/xe_mmio.c:109:23: error: result of comparison of constant 
4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is 
always false [-Werror,-Wtautological-constant-out-of-range-compare]
drivers/leds/leds-gpio-register.c:23:25: warning: attribute declaration must 
precede definition [-Wignored-attributes]
drivers/power/supply/power_supply_core.c:1630:9: error: too few arguments to 
function 'power_supply_init_attrs'
include/linux/dpll.h:179:1: warning: control reaches end of non-void function 
[-Wreturn-type]
lib/../mm/internal.h:144:14: error: implicit declaration of function 
'pte_batch_hint' [-Werror=implicit-function-declaration]
lib/../mm/internal.h:145:50: error: implicit declaration of function 
'pte_advance_pfn' [-Werror=implicit-function-declaration]
lib/../mm/internal.h:145:50: error: incompatible type for argument 1 of 
'__pte_batch_clear_ignored'
lib/../mm/internal.h:149:23: error: implicit declaration of function 'ptep_get' 
[-Werror=implicit-function-declaration]
lib/../mm/internal.h:149:23: error: incompatible types when assigning to type 
'pte_t' from type 'int'
lib/../mm/internal.h:154:22: error: implicit declaration of function 
'pte_same'; did you mean 'pte_page'? [-Werror=implicit-function-declaration]
mm/internal.h:101:16: error: implicit declaration of function 'pte_wrprotect' 
[-Werror=implicit-function-declaration]
mm/internal.h:101:16: error: incompatible types when returning type 'int' but 
'pte_t' was expected
mm/internal.h:101:30: error: implicit declaration of function 'pte_mkold' 
[-Werror=implicit-function-declaration]
mm/internal.h:140:27: error: implicit declaration of function 'pte_present'; 
did you mean 'pgd_present'? [-Werror=implicit-function-declaration]
mm/internal.h:142:49: error: implicit declaration of function 'pte_pfn' 
[-Werror=implicit-function-declaration]
mm/internal.h:144:14: error: implicit declaration of function 'pte_batch_hint' 
[-Werror=implicit-function-declaration]
mm/internal.h:145:50: error: implicit declaration of function 'pte_advance_pfn' 
[-Werror=implicit-function-declaration]
mm/internal.h:145:50: error: incompatible type for argument 1 of 
'__pte_batch_clear_ignored'
mm/internal.h:149:23: error: implicit declaration of function 'ptep_get' 
[-Werror=implicit-function-declaration]
mm/internal.h:149:23: error: incompatible types when assigning to type 'pte_t' 
from type 'int'
mm/internal.h:151:38: error: implicit declaration of function 'pte_write'; did 
you mean 'pgd_write'? [-Werror=implicit-function-declaration]
mm/internal.h:154:22: error: implicit declaration of function 'pte_same'; did 
you mean 'pte_page'? [-Werror=implicit-function-declaration]
mm/internal.h:98:23: error: implicit declaration of function 'pte_mkclean'; did 
you mean 'page_mkclean'? [-Werror=implicit-function-declaration]
mm/zsmalloc.c:742:23: warning: 'get_zspage_lockless' defined but not used 
[-Wunused-function]

Error/Warning ids grouped by kconfigs:

gcc_recent_errors
|-- alpha-allyesconfig
|   |-- 
drivers-gpu-drm-nouveau-nvkm-subdev-gsp-r535.c:warning:Function-parameter-or-struct-member-gsp-not-described-in-nvkm_gsp_radix3_sg
|   `-- 
fs-ubifs-journal.c:warning:expecting-prototype-for-wake_up_reservation().-Prototype-was-for-add_or_start_queue()-instead
|-- alpha-randconfig-r016-20220129
|   `-- 
drivers-power-supply-power_supply_core.c:error:too-few-arguments-to-function-power_supply_init_attrs
|-- arc-allmodconfig
|   |-- 
drivers-gpu-drm-nouveau-nvkm-subdev-gsp-r535.c:warning:Function-parameter-or-struct-member-gsp-not-described

Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are not powered

2024-02-28 Thread neil . armstrong


On 28/02/2024 17:40, Doug Anderson wrote:

Neil,

On Thu, Feb 15, 2024 at 8:53 AM Neil Armstrong
 wrote:


Hi Doug,

On 15/02/2024 16:08, Doug Anderson wrote:

Hi,

On Thu, Feb 15, 2024 at 2:24 AM Jani Nikula  wrote:


On Wed, 14 Feb 2024, Doug Anderson  wrote:

Hi,

On Tue, Feb 13, 2024 at 10:25 PM Hsin-Yi Wang  wrote:


On Wed, Feb 14, 2024 at 2:23 PM Douglas Anderson  wrote:


If an eDP panel is not powered on then any attempts to talk to it over
the DP AUX channel will timeout. Unfortunately these attempts may be
quite slow. Userspace can initiate these attempts either via a
/dev/drm_dp_auxN device or via the created i2c device.

Making the DP AUX drivers timeout faster is a difficult proposition.
In theory we could just poll the panel's HPD line in the AUX transfer
function and immediately return an error there. However, this is
easier said than done. For one thing, there's no hard requirement to
hook the HPD line up for eDP panels and it's OK to just delay a fixed
amount. For another thing, the HPD line may not be fast to probe. On
parade-ps8640 we need to wait for the bridge chip's firmware to boot
before we can get the HPD line and this is a slow process.

The fact that the transfers are taking so long to timeout is causing
real problems. The open source fwupd daemon sometimes scans DP busses
looking for devices whose firmware need updating. If it happens to
scan while a panel is turned off this scan can take a long time. The
fwupd daemon could try to be smarter and only scan when eDP panels are
turned on, but we can also improve the behavior in the kernel.

Let's let eDP panels drivers specify that a panel is turned off and
then modify the common AUX transfer code not to attempt a transfer in
this case.

Signed-off-by: Douglas Anderson 
---


Reviewed-by: Hsin-Yi Wang 


Thanks for the review!

Given that this touches core DRM code and that I never got
confirmation that Jani's concerns were addressed with my previous
response, I'm still going to wait a little while before applying. I'm
on vacation for most of next week, but if there are no further replies
between now and then I'll plan to apply this to "drm-misc-next" the
week of Feb 26th. If someone else wants to apply this before I do then
I certainly won't object. Jani: if you feel this needs more discussion
or otherwise object to this patch landing then please yell. Likewise
if anyone else in the community wants to throw in their opinion, feel
free.


Sorry for dropping the ball after my initial response. I simply have not
had the time to look into this.

It would be great to get, say, drm-misc maintainer ack on this before
merging. It's not fair for me to stall this any longer, I'll trust their
judgement.

Reasonable?


I'd be more than happy for one of the drm-misc maintainers to Ack.
I'll move Maxime, Thomas, and Maarten to the "To:" line to see if that
helps get through their filters.


I'll like some test reports to be sure it doesn't break anything,
then I'll be happy to give my ack !


Are you looking for any more test reports at this point? Eizan did
some testing and provided a tag, though this was also on ChromeOS.
Steev also tested on two non-ChromeOS environments and provided his
tag. It's also been another two weeks of this being rolled out to some
Chromebook users and I haven't heard any reports of problems. If
somehow something was missed, I'm happy to follow-up and provide
additional fixes if some report comes in later.


Sure, thx I think you can apply it now

Acked-by: Neil Armstrong 

Thanks,
Neil



Thanks!

-Doug

[PATCH] drm/dp: Fix documentation of DP tunnel functions

2024-02-28 Thread Imre Deak

Fix the documentation issues below, also reported by 'make htmldocs':

drivers/gpu/drm/display/drm_dp_tunnel.c:447: warning: Function parameter or 
struct member 'tunnel' not described in 'drm_dp_tunnel_put'
drivers/gpu/drm/display/drm_dp_tunnel.c:447: warning: Function parameter or 
struct member 'tracker' not described in 'drm_dp_tunnel_put'
drivers/gpu/drm/display/drm_dp_tunnel.c:1185: warning: expecting prototype for 
drm_dp_tunnel_atomic_get_allocated_bw(). Prototype was for 
drm_dp_tunnel_get_allocated_bw() instead
drivers/gpu/drm/display/drm_dp_tunnel.c:1903: warning: Function parameter or 
struct member 'max_group_count' not described in 'drm_dp_tunnel_mgr_create'

Fixes: 295654f7e554 ("drm/dp: Add support for DP tunneling")
Reported-by: kernel test robot 
Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/display/drm_dp_tunnel.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_tunnel.c 
b/drivers/gpu/drm/display/drm_dp_tunnel.c
index 120e0de674c19..017f1d4c63415 100644
--- a/drivers/gpu/drm/display/drm_dp_tunnel.c
+++ b/drivers/gpu/drm/display/drm_dp_tunnel.c
@@ -436,8 +436,8 @@ EXPORT_SYMBOL(drm_dp_tunnel_get);
 
 /**
  * drm_dp_tunnel_put - Put a reference for a DP tunnel
- * @tunnel - Tunnel object
- * @tracker - Debug tracker for the reference
+ * @tunnel: Tunnel object
+ * @tracker: Debug tracker for the reference
  *
  * Put a reference for @tunnel along with its debug *@tracker, which
  * was obtained with drm_dp_tunnel_get().
@@ -1170,7 +1170,7 @@ int drm_dp_tunnel_alloc_bw(struct drm_dp_tunnel *tunnel, 
int bw)
 EXPORT_SYMBOL(drm_dp_tunnel_alloc_bw);
 
 /**
- * drm_dp_tunnel_atomic_get_allocated_bw - Get the BW allocated for a DP tunnel
+ * drm_dp_tunnel_get_allocated_bw - Get the BW allocated for a DP tunnel
  * @tunnel: Tunnel object
  *
  * Get the current BW allocated for @tunnel. After the tunnel is created /
@@ -1892,6 +1892,7 @@ static void destroy_mgr(struct drm_dp_tunnel_mgr *mgr)
 /**
  * drm_dp_tunnel_mgr_create - Create a DP tunnel manager
  * @dev: DRM device object
+ * @max_group_count: Maximum number of tunnel groups
  *
  * Creates a DP tunnel manager for @dev.
  *
-- 
2.43.3

Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are not powered

Neil,

On Thu, Feb 15, 2024 at 8:53 AM Neil Armstrong
 wrote:
>
> Hi Doug,
>
> On 15/02/2024 16:08, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Feb 15, 2024 at 2:24 AM Jani Nikula  wrote:
> >>
> >> On Wed, 14 Feb 2024, Doug Anderson  wrote:
> >>> Hi,
> >>>
> >>> On Tue, Feb 13, 2024 at 10:25 PM Hsin-Yi Wang  wrote:
> 
>  On Wed, Feb 14, 2024 at 2:23 PM Douglas Anderson  
>  wrote:
> >
> > If an eDP panel is not powered on then any attempts to talk to it over
> > the DP AUX channel will timeout. Unfortunately these attempts may be
> > quite slow. Userspace can initiate these attempts either via a
> > /dev/drm_dp_auxN device or via the created i2c device.
> >
> > Making the DP AUX drivers timeout faster is a difficult proposition.
> > In theory we could just poll the panel's HPD line in the AUX transfer
> > function and immediately return an error there. However, this is
> > easier said than done. For one thing, there's no hard requirement to
> > hook the HPD line up for eDP panels and it's OK to just delay a fixed
> > amount. For another thing, the HPD line may not be fast to probe. On
> > parade-ps8640 we need to wait for the bridge chip's firmware to boot
> > before we can get the HPD line and this is a slow process.
> >
> > The fact that the transfers are taking so long to timeout is causing
> > real problems. The open source fwupd daemon sometimes scans DP busses
> > looking for devices whose firmware need updating. If it happens to
> > scan while a panel is turned off this scan can take a long time. The
> > fwupd daemon could try to be smarter and only scan when eDP panels are
> > turned on, but we can also improve the behavior in the kernel.
> >
> > Let's let eDP panels drivers specify that a panel is turned off and
> > then modify the common AUX transfer code not to attempt a transfer in
> > this case.
> >
> > Signed-off-by: Douglas Anderson 
> > ---
> 
>  Reviewed-by: Hsin-Yi Wang 
> >>>
> >>> Thanks for the review!
> >>>
> >>> Given that this touches core DRM code and that I never got
> >>> confirmation that Jani's concerns were addressed with my previous
> >>> response, I'm still going to wait a little while before applying. I'm
> >>> on vacation for most of next week, but if there are no further replies
> >>> between now and then I'll plan to apply this to "drm-misc-next" the
> >>> week of Feb 26th. If someone else wants to apply this before I do then
> >>> I certainly won't object. Jani: if you feel this needs more discussion
> >>> or otherwise object to this patch landing then please yell. Likewise
> >>> if anyone else in the community wants to throw in their opinion, feel
> >>> free.
> >>
> >> Sorry for dropping the ball after my initial response. I simply have not
> >> had the time to look into this.
> >>
> >> It would be great to get, say, drm-misc maintainer ack on this before
> >> merging. It's not fair for me to stall this any longer, I'll trust their
> >> judgement.
> >>
> >> Reasonable?
> >
> > I'd be more than happy for one of the drm-misc maintainers to Ack.
> > I'll move Maxime, Thomas, and Maarten to the "To:" line to see if that
> > helps get through their filters.
>
> I'll like some test reports to be sure it doesn't break anything,
> then I'll be happy to give my ack !

Are you looking for any more test reports at this point? Eizan did
some testing and provided a tag, though this was also on ChromeOS.
Steev also tested on two non-ChromeOS environments and provided his
tag. It's also been another two weeks of this being rolled out to some
Chromebook users and I haven't heard any reports of problems. If
somehow something was missed, I'm happy to follow-up and provide
additional fixes if some report comes in later.

Thanks!

-Doug

Re: drm-misc migration to Gitlab server

2024-02-28 Thread Daniel Vetter

Hi Stephen!

On Wed, Feb 21, 2024 at 09:46:43AM +1100, Stephen Rothwell wrote:
> On Tue, 20 Feb 2024 11:25:05 + Daniel Stone  wrote:
> >
> > On Tue, 20 Feb 2024 at 09:05, Maxime Ripard  wrote:
> > > On Tue, Feb 20, 2024 at 09:49:25AM +0100, Maxime Ripard wrote:  
> > > > This will be mostly transparent to current committers and users: we'll
> > > > still use dim, in the exact same way, the only change will be the URL of
> > > > the repo. This will also be transparent to linux-next, since the
> > > > linux-next branch lives in its own repo and is pushed by dim when
> > > > pushing a branch.  
> > >
> > > Actually, I double-checked and linux-next pulls our branches directly,
> > > so once the transition is over we'll have to notify them too.  
> > 
> > cc sfr - once we move the DRM repos to a different location, what's
> > the best way to update linux-next?
> > 
> > That being said, we could set up read-only pull mirrors in the old
> > location ... something I want to do in March (because what else are
> > you going to do on holiday?) is to kill the write repos on kemper
> > (git.fd.o), move them to being on molly (cgit/anongit.fd.o) only, and
> > just have a cronjob that regularly pulls from all the gl.fd.o repos,
> > rather than pushing from GitLab.
> 
> These are (I think) all the drm trees/branches that I fetch every day:
> 
> git://anongit.freedesktop.org/drm-intel#for-linux-next
> git://anongit.freedesktop.org/drm-intel#for-linux-next-fixes
> git://anongit.freedesktop.org/drm/drm-misc#for-linux-next
> git://anongit.freedesktop.org/drm/drm-misc#for-linux-next-fixes
> git://git.freedesktop.org/git/drm/drm.git#drm-fixes
> git://git.freedesktop.org/git/drm/drm.git#drm-next

To test out the process we've moved drm.git first. It's now here:

https://gitlab.freedesktop.org/drm/kernel.git

Still the same two branches. Can you please update the url? We haven't
enabled the auto-mirror for this one, since we want to make sure the
upgrade path in the tooling works and people do switch over to the new
repo.

For the others the plan is keep the old places automatically mirrored, at
least until the dust has settled.

Thanks!


> git://git.freedesktop.org/git/drm/drm.git#topic/drm-ci
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git#for-linux-next
> https://gitlab.freedesktop.org/agd5f/linux#drm-next
> https://gitlab.freedesktop.org/drm/msm.git#msm-next
> https://gitlab.freedesktop.org/drm/tegra.git#for-next
> https://gitlab.freedesktop.org/lumag/msm.git#msm-next-lumag
> 
> If someone could just send me all the new equivalent URLs when the
> change happens, I will fix them up in my config.
> 
> -- 
> Cheers,
> Stephen Rothwell



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

Re: [PATCH 2/2] arm64: dts: imx8mp-beacon-kit: Enable HDMI bridge HPD

Hi Adam,

Thank you for the patch.

On Wed, Feb 28, 2024 at 05:37:36AM -0600, Adam Ford wrote:
> The DSI to HDMI bridge supports hot-plut-detect, but the
> driver didn't previously support a shared IRQ GPIO.  With
> the driver updated, the interrupt can be added to the bridge.

You can reflow the commit message to 72 columns.

> Signed-off-by: Adam Ford 

Reviewed-by: Laurent Pinchart 

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts 
> b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> index 8de4dd268908..d854c94ec997 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> @@ -405,6 +405,8 @@ adv_bridge: hdmi@3d {
>   compatible = "adi,adv7535";
>   reg = <0x3d>, <0x3c>, <0x3e>, <0x3f>;
>   reg-names = "main", "cec", "edid", "packet";
> + interrupt-parent = <>;
> + interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
>   adi,dsi-lanes = <4>;
>   #sound-dai-cells = <0>;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins

Hi Adam,

Thank you for the patch.

On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> The IRQ registration currently assumes that the GPIO is
> dedicated to it, but that may not necessarily be the case.
> If the board has another device sharing the IRQ, it won't be
> registered and the hot-plug detect fails.  This is easily
> fixed by add the IRQF_SHARED flag.
> 
> Signed-off-by: Adam Ford 
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index b5518ff97165..21f08b2ae265 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
>  
>   ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
>   adv7511_irq_handler,
> - IRQF_ONESHOT, dev_name(dev),
> + IRQF_ONESHOT | IRQF_SHARED,
> + dev_name(dev),

This looks fine, but the IRQ handler doesn't.

static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
{
unsigned int irq0, irq1;
int ret;

ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
if (ret < 0)
return ret;

ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), );
if (ret < 0)
return ret;

regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);

if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
schedule_work(>hpd_work);

if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
adv7511->edid_read = true;

if (adv7511->i2c_main->irq)
wake_up_all(>wq);
}

#ifdef CONFIG_DRM_I2C_ADV7511_CEC
adv7511_cec_irq_process(adv7511, irq1);
#endif

return 0;
}

static irqreturn_t adv7511_irq_handler(int irq, void *devid)
{
struct adv7511 *adv7511 = devid;
int ret;

ret = adv7511_irq_process(adv7511, true);
return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
}

The function will return IRQ_HANDLED as long as the registers can be
read, even if they don't report any interrupt.

>   adv7511);
>   if (ret)
>   goto err_unregister_audio;

-- 
Regards,

Laurent Pinchart

Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME

2024-02-28 Thread Simon Ser

On Wednesday, February 28th, 2024 at 17:14, Maxime Ripard  
wrote:

> > I don't know what the rules were 8 years ago, but the current uAPI rules
> > are what they are, and a new enum entry is new uAPI.
> 
> TBF, and even if the wayland compositors support is missing, this
> property is perfectly usable as it is with upstream, open-source code,
> through either the command-line or X.org, and it's documented.
> 
> So it's fine by me from a UAPI requirement side.

That is not a valid way to pass the uAPI requirements IMHO. Yes, one
can program any KMS property via modetest or xrandr. Does that mean that
none of the new uAPI need a "real" implementation anymore? Does that mean
that the massive patch adding a color pipeline uAPI doesn't need
user-space anymore?

The only thing I'm saying is that this breaks the usual DRM requirements.
If, as a maintainer, you're fine with breaking the rules and have a good
motivation to do so, that's fine by me. Rules are meant to be broken from
time to time depending on the situation. But please don't pretend that
modetest/xrandr is valid user-space to pass the rules.

Re: [PATCH] drm: xlnx: dp: Reset DisplayPort IP

Hello Rohit,

Thank you for the patch.

On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote:
> Assert DisplayPort reset signal before deasserting,
> it is to clear out any registers programmed before booting kernel.
> 
> Signed-off-by: Rohit Visavalia 
> ---
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 1846c4971fd8..5a40aa1d4283 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   goto err_free;
>   }
>  
> + ret = zynqmp_dp_reset(dp, true);
> + if (ret < 0)
> + return ret;
> +

This looks fine to me, so

Reviewed-by: Laurent Pinchart 

However, looking at zynqmp_dp_reset(), we have

static int zynqmp_dp_reset(struct zynqmp_dp *dp, bool assert)
{
unsigned long timeout;

if (assert)
reset_control_assert(dp->reset);
else
reset_control_deassert(dp->reset);

/* Wait for the (de)assert to complete. */
timeout = jiffies + msecs_to_jiffies(RST_TIMEOUT_MS);
while (!time_after_eq(jiffies, timeout)) {
bool status = !!reset_control_status(dp->reset);

if (assert == status)
return 0;

cpu_relax();
}

dev_err(dp->dev, "reset %s timeout\n", assert ? "assert" : "deassert");
return -ETIMEDOUT;
}

That doesn't seem quite right. Aren't reset_control_assert() and
reset_control_deassert() supposed to be synchronous ? If so there should
be no need to wait, and if there's a need to wait, it could be a bug in
the reset controller driver. This should be fixed, and then the code in
probe could be replaced with a call to reset_control_reset().

>   ret = zynqmp_dp_reset(dp, false);
>   if (ret < 0)
>   goto err_free;

-- 
Regards,

Laurent Pinchart

Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME

Hi,

On Tue, Feb 27, 2024 at 09:51:06AM +, Simon Ser wrote:
> On Monday, February 26th, 2024 at 18:23, Dave Stevenson 
>  wrote:
> > > and I've also completely missed any kernel command line
> > > arguments manipulating KMS properties.
> > 
> > "tv_mode" on the command line is handled in
> > drm_mode_parse_cmdline_options() [3], as are rotate, reflect_x,
> > reflect_y, margin_[left|right|top|bottom], and panel_orientation all
> > to set the relevant KMS properties.
> > 
> > Having "video=Composite-1:PAL,tv_mode=Mono" on the kernel command line
> > will set up connector Composite-1 with the standard 720x576 50Hz
> > interlaced timings, and DRM_MODE_TV_MODE_MONOCHROME selected on the
> > tv_mode property. Swap in different tv_mode descriptions as required
> > (eg PAL,tv_mode=SECAM), although some make little sense.
> > 
> > That's the main route I'm looking at for configuring this property,
> > for situations such as having a black and white TV connected. You
> > don't get the opportunity to interrogate a composite display over what
> > it supports, so it has to be configured manually somewhere in the
> > system. If your monitor doesn't support the system default, then you
> > can't see a GUI in order to change the option, and there is no
> > guaranteed supported configuration so the command line is about the
> > only option.
> > 
> > The use cases for runtime switching of the "tv_mode" are exceedingly
> > rare, so IMHO the property doesn't need exposing through the UAPI.
> > However it was added to the UAPI about 8 years ago for vc4 and sunxi,
> > and is also now used by other drivers, so can't be reverted. Does that
> > mean it can now never be changed without jumping through the hoop of
> > creating some userspace user?
> 
> I don't know what the rules were 8 years ago, but the current uAPI rules
> are what they are, and a new enum entry is new uAPI.

TBF, and even if the wayland compositors support is missing, this
property is perfectly usable as it is with upstream, open-source code,
through either the command-line or X.org, and it's documented.

So it's fine by me from a UAPI requirement side.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v5 1/8] usb: misc: onboard_hub: use device supply names

On Wed, Feb 28, 2024 at 05:02:10PM +0100, Javier Carrasco wrote:
> On 28.02.24 16:37, Matthias Kaehlcke wrote:
> > Hi Javier,
> > 
> > Thanks for moving this patch to the front of the series!
> > 
> > A few more comments inline.
> > 
> > On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
> >> The current implementation uses generic names for the power supplies,
> >> which conflicts with proper name definitions in the device bindings.
> >>
> >> Add a per-device property to include real supply names and keep generic
> >> names for existing devices to keep backward compatibility.
> >>
> >> Signed-off-by: Javier Carrasco 
> >> ---
> >>  drivers/usb/misc/onboard_usb_hub.c | 49 
> >> --
> >>  drivers/usb/misc/onboard_usb_hub.h | 12 ++
> >>  2 files changed, 38 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/onboard_usb_hub.c 
> >> b/drivers/usb/misc/onboard_usb_hub.c
> >> index 0dd2b032c90b..3755f6cc1eda 100644
> >> --- a/drivers/usb/misc/onboard_usb_hub.c
> >> +++ b/drivers/usb/misc/onboard_usb_hub.c
> >> @@ -29,17 +29,6 @@
> >>  
> >>  #include "onboard_usb_hub.h"
> >>  
> >> -/*
> >> - * Use generic names, as the actual names might differ between hubs. If a 
> >> new
> >> - * hub requires more than the currently supported supplies, add a new one 
> >> here.
> >> - */
> >> -static const char * const supply_names[] = {
> >> -  "vdd",
> >> -  "vdd2",
> >> -};
> >> -
> >> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
> >> -
> >>  static void onboard_hub_attach_usb_driver(struct work_struct *work);
> >>  
> >>  static struct usb_device_driver onboard_hub_usbdev_driver;
> >> @@ -65,6 +54,30 @@ struct onboard_hub {
> >>struct clk *clk;
> >>  };
> >>  
> >> +static int onboard_hub_get_regulator_bulk(struct device *dev,
> >> +struct onboard_hub *onboard_hub)
> > 
> > Let's call this onboard_hub_get_regulators(), it's an implementation detail
> > that regulator_bulk_get() is used for getting them.
> > 
> > no need to pass 'dev', there is onboard_hub->dev
> > 
> 
> Not at this point, though. The hub->dev = dev assignment happens a few
> lines below, but there is no reason not to move the line up. I will
> modify this for v6.
> 
> >>  static int onboard_hub_power_on(struct onboard_hub *hub)
> >>  {
> >>int err;
> >> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device 
> >> *pdev)
> >>  {
> >>struct device *dev = >dev;
> >>struct onboard_hub *hub;
> >> -  unsigned int i;
> >>int err;
> >>  
> >>hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> >> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device 
> >> *pdev)
> >>if (!hub->pdata)
> >>return -EINVAL;
> >>  
> >> -  if (hub->pdata->num_supplies > MAX_SUPPLIES)
> >> -  return dev_err_probe(dev, -EINVAL, "max %zu supplies 
> >> supported!\n",
> >> -   MAX_SUPPLIES);
> >> -
> >> -  for (i = 0; i < hub->pdata->num_supplies; i++)
> >> -  hub->supplies[i].supply = supply_names[i];
> >> -
> >> -  err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, 
> >> hub->supplies);
> >> -  if (err) {
> >> -  dev_err(dev, "Failed to get regulator supplies: %pe\n", 
> >> ERR_PTR(err));
> >> +  err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
> > 
> > The local variable is called 'hub', not 'onboard_hub'.
> > 
> 
> Good catch! Actually this patch alone would have not compiled, but once
> the renaming is done, everything is ok again. I will fix this for v6.
> 
> >> diff --git a/drivers/usb/misc/onboard_usb_hub.h 
> >> b/drivers/usb/misc/onboard_usb_hub.h
> >> index f360d5cf8d8a..ea24bd6790f0 100644
> >> --- a/drivers/usb/misc/onboard_usb_hub.h
> >> +++ b/drivers/usb/misc/onboard_usb_hub.h
> >> @@ -6,54 +6,66 @@
> >>  #ifndef _USB_MISC_ONBOARD_USB_HUB_H
> >>  #define _USB_MISC_ONBOARD_USB_HUB_H
> >>  
> >> +#define MAX_SUPPLIES 2
> >> +
> >>  struct onboard_hub_pdata {
> >>unsigned long reset_us; /* reset pulse width in us */
> >>unsigned int num_supplies;  /* number of supplies */
> >> +  const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
> > 
> > The comment isn't particularly useful or accurate. Not in all cases
> > real names are used and outside of the context of this change the
> > comment is hard to understand.
> > 
> > I'd say just omit it, the name of the field is self-documenting enough,
> > there is no need to repeat the same in a comment (as for 'num_supplies'
> > ...)
> 
> I added tthe comment because I can foresee what is going to happen:
> people will copy the names from existing devices, we will have to ask if
> the supplies are actually called vdd and vdd2 in the datasheet, and then
> the real names will be sent in v2. Especially at the beginning, when the
> supported devices are using vdd and vdd2.

It will probably happen, but I don't expect the comment to prevent that
in

Re: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format callback

On Wed, Feb 28, 2024 at 04:29:33PM +0100, Maxime Ripard wrote:
> On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> > Add select_output_bus_format to CRTC atomic helpers callbacks. This
> > callback Will allow CRTC to participate in media bus format negotiation
> > over connected DRM bridge chain and impose CRTC-specific restrictions.
> > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC will
> > most certainly support a single output media bus format, as supporting
> > multiple runtime options consumes extra FPGA resources. A variety of
> > options for FPGA are usually achieved by synthesizing IP with different
> > parameters.
> > 
> > Incorporate select_output_bus_format callback into the format negotiation
> > stage to fix the input bus format of the first DRM bridge in the chain.
> > 
> > Signed-off-by: Anatoliy Klymenko 
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 19 +--
> >  include/drm/drm_modeset_helper_vtables.h | 31 
> > +++
> >  2 files changed, 48 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 521a71c61b16..453ae3d174b4 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -32,6 +32,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge 
> > *first_bridge,
> > unsigned int i, num_in_bus_fmts = 0;
> > struct drm_bridge_state *cur_state;
> > struct drm_bridge *prev_bridge;
> > -   u32 *in_bus_fmts;
> > +   struct drm_crtc *crtc = crtc_state->crtc;
> > +   u32 *in_bus_fmts, in_fmt;
> > int ret;
> >  
> > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge 
> > *first_bridge,
> > return -ENOMEM;
> >  
> > if (first_bridge == cur_bridge) {
> > -   cur_state->input_bus_cfg.format = in_bus_fmts[0];
> > +   in_fmt = in_bus_fmts[0];
> > +   if (crtc->helper_private &&
> > +   crtc->helper_private->select_output_bus_format) {
> > +   in_fmt = crtc->helper_private->select_output_bus_format(
> > +   crtc,
> > +   crtc->state,
> > +   in_bus_fmts,
> > +   num_in_bus_fmts);
> > +   if (!in_fmt) {
> > +   kfree(in_bus_fmts);
> > +   return -ENOTSUPP;
> > +   }
> > +   }
> > +   cur_state->input_bus_cfg.format = in_fmt;
> 
> I don't think we should start poking at the CRTC internals, but we
> should rather provide a helper here.

It would probably look cleaner, yes.

> > cur_state->output_bus_cfg.format = out_bus_fmt;
> > kfree(in_bus_fmts);
> > return 0;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h 
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 881b03e4dc28..7c21ae1fe3ad 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> >  bool in_vblank_irq, int *vpos, int *hpos,
> >  ktime_t *stime, ktime_t *etime,
> >  const struct drm_display_mode *mode);
> > +
> > +   /**
> > +* @select_output_bus_format
> > +*
> > +* Called by the first connected DRM bridge to negotiate input media
> > +* bus format. CRTC is expected to pick preferable media formats from
> > +* the list supported by the DRM bridge chain.
> 
> There's nothing restricting it to bridges here. Please rephrase this to
> remove the bridge mention. The user is typically going to be the
> encoder, and bridges are just an automagic implementation of an encoder.
> 
> And generally speaking, I'd really like to have an implementation
> available before merging this.

There's a downstream implementation in the Xilinx kernel, which would
indeed be nice to upstream. This shouldn't block patches 1/4 to 3/4
though, those can be merged once review comments are taken into account.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format

Hi Anatoliy,

Thank you for the patch.

On Mon, Feb 26, 2024 at 08:44:44PM -0800, Anatoliy Klymenko wrote:
> Program live video input format according to selected media bus format.
> 
> In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> almost certainly supports a single media bus format as its output. Expect
> this to be delivered via the new bridge atomic state. Program DPSUB
> registers accordingly.
> 
> Signed-off-by: Anatoliy Klymenko 
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c  | 52 
> +
>  drivers/gpu/drm/xlnx/zynqmp_disp.h  |  2 ++
>  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  8 ++---
>  drivers/gpu/drm/xlnx/zynqmp_dp.c| 13 ++---
>  4 files changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index ee99aad915ba..1c3ffdee6b8e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -416,6 +416,34 @@ static bool zynqmp_disp_layer_is_video(const struct 
> zynqmp_disp_layer *layer)
>   return layer->id == ZYNQMP_DPSUB_LAYER_VID;
>  }
>  
> +/**
> + * zynqmp_disp_avbuf_set_live_format - Set live input format for a layer
> + * @disp: Display controller
> + * @layer: The layer
> + * @fmt: The format information
> + *
> + * Set the live video input format for @layer to @fmt.
> + */
> +static void zynqmp_disp_avbuf_set_live_format(struct zynqmp_disp *disp,
> +   struct zynqmp_disp_layer *layer,
> +   const struct zynqmp_disp_format 
> *fmt)
> +{
> + u32 reg, i;
> +
> + reg = zynqmp_disp_layer_is_video(layer)
> + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
> + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
> + zynqmp_disp_avbuf_write(disp, reg, fmt->buf_fmt);
> +
> + for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; ++i) {
> + reg = zynqmp_disp_layer_is_video(layer)
> + ? ZYNQMP_DISP_AV_BUF_LIVD_VID_COMP_SF(i)
> + : ZYNQMP_DISP_AV_BUF_LIVD_GFX_COMP_SF(i);
> + zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
> + }

This is identical to zynqmp_disp_avbuf_set_format(), you should avoid
duplicating code.

> + layer->disp_fmt = fmt;
> +}
> +
>  /**
>   * zynqmp_disp_avbuf_set_format - Set the input format for a layer
>   * @disp: Display controller
> @@ -979,6 +1007,30 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer 
> *layer)
>   zynqmp_disp_blend_layer_disable(layer->disp, layer);
>  }
>  
> +/**
> + * zynqmp_disp_layer_set_live_format - Set live layer input format
> + * @layer: The layer
> + * @info: Input media bus format
> + *
> + * Set the live @layer input bus format. The layer must be disabled.
> + */
> +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> +u32 bus_format)

I'd prefer reusing zynqmp_disp_layer_set_format(), and handling the
differences between live and non-live input there. There's already a
dma_enabled check in that function.

> +{
> + int i;
> + const struct zynqmp_disp_format *fmt;
> +
> + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i) {
> + fmt = _live_fmts[i];
> + if (fmt->bus_fmt == bus_format) {
> + layer->disp_fmt = fmt;
> + layer->drm_fmt = drm_format_info(fmt->drm_fmt);
> + zynqmp_disp_avbuf_set_live_format(layer->disp, layer, 
> fmt);
> + return;
> + }
> + }
> +}
> +
>  /**
>   * zynqmp_disp_layer_set_format - Set the layer format
>   * @layer: The layer
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index c2c8dd4896ba..f244b7d2346a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -66,6 +66,8 @@ void zynqmp_disp_layer_enable(struct zynqmp_disp_layer 
> *layer);
>  void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
>  void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> const struct drm_format_info *info);
> +void zynqmp_disp_layer_set_live_format(struct zynqmp_disp_layer *layer,
> +u32 bus_format);
>  int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
>struct drm_plane_state *state);
>  
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h 
> b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..fa3935384834 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -165,10 +165,10 @@
>  #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_100x2
>  #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_120x3
>  #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK  GENMASK(2, 0)
> -#define

Re: [PATCH v5 1/8] usb: misc: onboard_hub: use device supply names

On 28.02.24 16:37, Matthias Kaehlcke wrote:
> Hi Javier,
> 
> Thanks for moving this patch to the front of the series!
> 
> A few more comments inline.
> 
> On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
>> The current implementation uses generic names for the power supplies,
>> which conflicts with proper name definitions in the device bindings.
>>
>> Add a per-device property to include real supply names and keep generic
>> names for existing devices to keep backward compatibility.
>>
>> Signed-off-by: Javier Carrasco 
>> ---
>>  drivers/usb/misc/onboard_usb_hub.c | 49 
>> --
>>  drivers/usb/misc/onboard_usb_hub.h | 12 ++
>>  2 files changed, 38 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_hub.c 
>> b/drivers/usb/misc/onboard_usb_hub.c
>> index 0dd2b032c90b..3755f6cc1eda 100644
>> --- a/drivers/usb/misc/onboard_usb_hub.c
>> +++ b/drivers/usb/misc/onboard_usb_hub.c
>> @@ -29,17 +29,6 @@
>>  
>>  #include "onboard_usb_hub.h"
>>  
>> -/*
>> - * Use generic names, as the actual names might differ between hubs. If a 
>> new
>> - * hub requires more than the currently supported supplies, add a new one 
>> here.
>> - */
>> -static const char * const supply_names[] = {
>> -"vdd",
>> -"vdd2",
>> -};
>> -
>> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
>> -
>>  static void onboard_hub_attach_usb_driver(struct work_struct *work);
>>  
>>  static struct usb_device_driver onboard_hub_usbdev_driver;
>> @@ -65,6 +54,30 @@ struct onboard_hub {
>>  struct clk *clk;
>>  };
>>  
>> +static int onboard_hub_get_regulator_bulk(struct device *dev,
>> +  struct onboard_hub *onboard_hub)
> 
> Let's call this onboard_hub_get_regulators(), it's an implementation detail
> that regulator_bulk_get() is used for getting them.
> 
> no need to pass 'dev', there is onboard_hub->dev
> 

Not at this point, though. The hub->dev = dev assignment happens a few
lines below, but there is no reason not to move the line up. I will
modify this for v6.

>>  static int onboard_hub_power_on(struct onboard_hub *hub)
>>  {
>>  int err;
>> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device 
>> *pdev)
>>  {
>>  struct device *dev = >dev;
>>  struct onboard_hub *hub;
>> -unsigned int i;
>>  int err;
>>  
>>  hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
>> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device 
>> *pdev)
>>  if (!hub->pdata)
>>  return -EINVAL;
>>  
>> -if (hub->pdata->num_supplies > MAX_SUPPLIES)
>> -return dev_err_probe(dev, -EINVAL, "max %zu supplies 
>> supported!\n",
>> - MAX_SUPPLIES);
>> -
>> -for (i = 0; i < hub->pdata->num_supplies; i++)
>> -hub->supplies[i].supply = supply_names[i];
>> -
>> -err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, 
>> hub->supplies);
>> -if (err) {
>> -dev_err(dev, "Failed to get regulator supplies: %pe\n", 
>> ERR_PTR(err));
>> +err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
> 
> The local variable is called 'hub', not 'onboard_hub'.
> 

Good catch! Actually this patch alone would have not compiled, but once
the renaming is done, everything is ok again. I will fix this for v6.

>> diff --git a/drivers/usb/misc/onboard_usb_hub.h 
>> b/drivers/usb/misc/onboard_usb_hub.h
>> index f360d5cf8d8a..ea24bd6790f0 100644
>> --- a/drivers/usb/misc/onboard_usb_hub.h
>> +++ b/drivers/usb/misc/onboard_usb_hub.h
>> @@ -6,54 +6,66 @@
>>  #ifndef _USB_MISC_ONBOARD_USB_HUB_H
>>  #define _USB_MISC_ONBOARD_USB_HUB_H
>>  
>> +#define MAX_SUPPLIES 2
>> +
>>  struct onboard_hub_pdata {
>>  unsigned long reset_us; /* reset pulse width in us */
>>  unsigned int num_supplies;  /* number of supplies */
>> +const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
> 
> The comment isn't particularly useful or accurate. Not in all cases
> real names are used and outside of the context of this change the
> comment is hard to understand.
> 
> I'd say just omit it, the name of the field is self-documenting enough,
> there is no need to repeat the same in a comment (as for 'num_supplies'
> ...)

I added tthe comment because I can foresee what is going to happen:
people will copy the names from existing devices, we will have to ask if
the supplies are actually called vdd and vdd2 in the datasheet, and then
the real names will be sent in v2. Especially at the beginning, when the
supported devices are using vdd and vdd2.

But if you think the field name is self-documenting, I am fine with it
too. I will remove the comment for v6.

Thanks again and best regards,
Javier Carrasco

Re: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Anounce supported input formats

Hi Anatoliy,

Thank you for the patch.

On Mon, Feb 26, 2024 at 08:44:43PM -0800, Anatoliy Klymenko wrote:
> DPSUB in bridge mode supports multiple input media bus formats.
> 
> Announce the list of supported input media bus formats via
> drm_bridge.atomic_get_input_bus_fmts callback.
> 
> Signed-off-by: Anatoliy Klymenko 
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c | 37 +
>  drivers/gpu/drm/xlnx/zynqmp_disp.h | 10 ++
>  drivers/gpu/drm/xlnx/zynqmp_dp.c   |  1 +
>  3 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index e6d26ef60e89..ee99aad915ba 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -18,6 +18,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
>  /**
>   * struct zynqmp_disp_format - Display subsystem format information
>   * @drm_fmt: DRM format (4CC)
> + * @bus_fmt: Media bus format
>   * @buf_fmt: AV buffer format
>   * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
>   * @sf: Scaling factors for color components
>   */
>  struct zynqmp_disp_format {
>   u32 drm_fmt;
> + u32 bus_fmt;
>   u32 buf_fmt;
>   bool swap;
>   const u32 *sf;
> @@ -364,6 +367,40 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] 
> = {
>   },
>  };
>  
> +/* List of live video layer formats */
> +static const struct zynqmp_disp_format avbuf_live_fmts[] = {
> + {
> + .drm_fmt= DRM_FORMAT_VYUY,
> + .bus_fmt= MEDIA_BUS_FMT_VYUY8_1X16,
> + .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> +   ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> + .sf = scaling_factors_888,

Is there a reason to have a separate array, instead of populating
.bus_fmt in the existing arrays for the formats that can be supported
with the live input, and only reporting those from
zynqmp_disp_get_input_bus_fmts() ?

> + },
> +};
> +
> +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state,
> + u32 output_fmt,
> + unsigned int *num_input_fmts)
> +{
> + int i;
> + u32 *input_fmts;
> +
> + input_fmts = kcalloc(ARRAY_SIZE(avbuf_live_fmts), sizeof(*input_fmts), 
> GFP_KERNEL);
> + if (!input_fmts) {
> + *num_input_fmts = 0;
> + return input_fmts;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(avbuf_live_fmts); ++i)
> + input_fmts[i] =  avbuf_live_fmts[i].bus_fmt;

Extra space.

> + *num_input_fmts = ARRAY_SIZE(avbuf_live_fmts);
> +
> + return input_fmts;
> +}
> +
>  static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
>  {
>   return readl(disp->avbuf.base + reg);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 9b8b202224d9..c2c8dd4896ba 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -27,6 +27,10 @@
>  struct device;
>  struct drm_format_info;
>  struct drm_plane_state;
> +struct drm_bridge;
> +struct drm_bridge_state;
> +struct drm_connector_state;
> +struct drm_crtc_state;
>  struct platform_device;
>  struct zynqmp_disp;
>  struct zynqmp_disp_layer;
> @@ -52,6 +56,12 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp 
> *disp,
>  
>  u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
>  unsigned int *num_formats);
> +u32 *zynqmp_disp_get_input_bus_fmts(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state,
> + u32 output_fmt,
> + unsigned int *num_input_fmts);

As this is a bridge operation, I think it would be better located in
zynqmp_dp.c. You can possibly expose the avbuf_live_fmts array in
zynqmp_disp.h, but that's not really nice as you'll be missing the size.
Another option would be to split the function in two, with the part that
handles the bridge API implemented in zynqmp_dp.c, and the part that
accesses the formats array in zynqmp_disp.c.

>  void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
>  void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
>  void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>

Re: [PATCH] drm/amd/display: Add monitor patch for specific eDP

2024-02-28 Thread Hamza Mahfooz


On 2/27/24 13:18, Rodrigo Siqueira wrote:

From: Ivan Lipski 

[WHY]
Some eDP panels's ext caps don't write initial value cause the value of
dpcd_addr(0x317) is random.  It means that sometimes the eDP will
clarify it is OLED, miniLED...etc cause the backlight control interface
is incorrect.

[HOW]
Add a new panel patch to remove sink ext caps(HDR,OLED...etc)


I wonder if it would make sense to turn this into a DPCD qurik (see
drivers/gpu/drm/display/drm_dp_helper.c). Since, it is rather unsettling
that we have so many panel quirks in our driver.



Cc: sta...@vger.kernel.org # 6.5.x
Cc: Hamza Mahfooz 
Cc: Tsung-hua Lin 
Cc: Chris Chi 
Cc: Harry Wentland 
Tested-by: Daniel Wheeler 
Reviewed-by: Sun peng Li 
Acked-by: Rodrigo Siqueira 
Signed-off-by: Ivan Lipski 
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index d9a482908380..764dc3ffd91b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -63,6 +63,12 @@ static void apply_edid_quirks(struct edid *edid, struct 
dc_edid_caps *edid_caps)
DRM_DEBUG_DRIVER("Disabling FAMS on monitor with panel id 
%X\n", panel_id);
edid_caps->panel_patch.disable_fams = true;
break;
+   /* Workaround for some monitors that do not clear DPCD 0x317 if 
FreeSync is unsupported */
+   case drm_edid_encode_panel_id('A', 'U', 'O', 0xA7AB):
+   case drm_edid_encode_panel_id('A', 'U', 'O', 0xE69B):
+   DRM_DEBUG_DRIVER("Clearing DPCD 0x317 on monitor with panel id 
%X\n", panel_id);
+   edid_caps->panel_patch.remove_sink_ext_caps = true;
+   break;
default:
return;
}

--
Hamza

Re: (subset) [PATCH v7 04/36] drm/tests: Add helper to create mock crtc

On Thu, 22 Feb 2024 19:13:50 +0100, Maxime Ripard wrote:
> We're going to need a full-blown, functional, KMS device to test more
> components of the atomic modesetting infrastructure.
> 
> Let's add a new helper to create a dumb, mocked, CRTC. By default it
> will create a CRTC relying only on the default helpers, but drivers are
> free to deviate from that.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v7 05/36] drm/tests: connector: Add tests for drmm_connector_init

On Thu, 22 Feb 2024 19:13:51 +0100, Maxime Ripard wrote:
> drmm_connector_init is the preferred function to initialize a
> drm_connector structure. Let's add a bunch of unit tests for it.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v7 03/36] drm/tests: Add helper to create mock plane

On Thu, 22 Feb 2024 19:13:49 +0100, Maxime Ripard wrote:
> We're going to need a full-blown, functional, KMS device to test more
> components of the atomic modesetting infrastructure.
> 
> Let's add a new helper to create a dumb, mocked, primary plane. By
> default, it will create a linear XRGB plane, using the default
> helpers.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v7 02/36] drm/tests: helpers: Add atomic helpers

On Thu, 22 Feb 2024 19:13:48 +0100, Maxime Ripard wrote:
> The mock device we were creating was missing any of the driver-wide
> helpers. That was fine before since we weren't testing the atomic state
> path, but we're going to start, so let's use the default
> implementations.
> 
> 

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: (subset) [PATCH v7 01/36] drm/tests: helpers: Include missing drm_drv header

On Thu, 22 Feb 2024 19:13:47 +0100, Maxime Ripard wrote:
> We have a few functions declared in our kunit helpers header, some of
> them dereferencing the struct drm_driver.
> 
> However, we don't include the drm_drv.h header file defining that
> structure, leading to compilation errors if we don't include both
> headers.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime

Re: [PATCH v5 1/8] usb: misc: onboard_hub: use device supply names

Hi Javier,

Thanks for moving this patch to the front of the series!

A few more comments inline.

On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
> The current implementation uses generic names for the power supplies,
> which conflicts with proper name definitions in the device bindings.
> 
> Add a per-device property to include real supply names and keep generic
> names for existing devices to keep backward compatibility.
> 
> Signed-off-by: Javier Carrasco 
> ---
>  drivers/usb/misc/onboard_usb_hub.c | 49 
> --
>  drivers/usb/misc/onboard_usb_hub.h | 12 ++
>  2 files changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c 
> b/drivers/usb/misc/onboard_usb_hub.c
> index 0dd2b032c90b..3755f6cc1eda 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -29,17 +29,6 @@
>  
>  #include "onboard_usb_hub.h"
>  
> -/*
> - * Use generic names, as the actual names might differ between hubs. If a new
> - * hub requires more than the currently supported supplies, add a new one 
> here.
> - */
> -static const char * const supply_names[] = {
> - "vdd",
> - "vdd2",
> -};
> -
> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
> -
>  static void onboard_hub_attach_usb_driver(struct work_struct *work);
>  
>  static struct usb_device_driver onboard_hub_usbdev_driver;
> @@ -65,6 +54,30 @@ struct onboard_hub {
>   struct clk *clk;
>  };
>  
> +static int onboard_hub_get_regulator_bulk(struct device *dev,
> +   struct onboard_hub *onboard_hub)

Let's call this onboard_hub_get_regulators(), it's an implementation detail
that regulator_bulk_get() is used for getting them.

no need to pass 'dev', there is onboard_hub->dev

>  static int onboard_hub_power_on(struct onboard_hub *hub)
>  {
>   int err;
> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device *pdev)
>  {
>   struct device *dev = >dev;
>   struct onboard_hub *hub;
> - unsigned int i;
>   int err;
>  
>   hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device 
> *pdev)
>   if (!hub->pdata)
>   return -EINVAL;
>  
> - if (hub->pdata->num_supplies > MAX_SUPPLIES)
> - return dev_err_probe(dev, -EINVAL, "max %zu supplies 
> supported!\n",
> -  MAX_SUPPLIES);
> -
> - for (i = 0; i < hub->pdata->num_supplies; i++)
> - hub->supplies[i].supply = supply_names[i];
> -
> - err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, 
> hub->supplies);
> - if (err) {
> - dev_err(dev, "Failed to get regulator supplies: %pe\n", 
> ERR_PTR(err));
> + err = onboard_hub_get_regulator_bulk(dev, onboard_hub);

The local variable is called 'hub', not 'onboard_hub'.

> diff --git a/drivers/usb/misc/onboard_usb_hub.h 
> b/drivers/usb/misc/onboard_usb_hub.h
> index f360d5cf8d8a..ea24bd6790f0 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -6,54 +6,66 @@
>  #ifndef _USB_MISC_ONBOARD_USB_HUB_H
>  #define _USB_MISC_ONBOARD_USB_HUB_H
>  
> +#define MAX_SUPPLIES 2
> +
>  struct onboard_hub_pdata {
>   unsigned long reset_us; /* reset pulse width in us */
>   unsigned int num_supplies;  /* number of supplies */
> + const char * const supply_names[MAX_SUPPLIES]; /* use the real names */

The comment isn't particularly useful or accurate. Not in all cases
real names are used and outside of the context of this change the
comment is hard to understand.

I'd say just omit it, the name of the field is self-documenting enough,
there is no need to repeat the same in a comment (as for 'num_supplies'
...)

Re: [PATCH v2 1/9] media: v4l2-subdev: Add a pad variant of .query_dv_timings()

2024-02-28 Thread Paweł Anikiel

On Wed, Feb 28, 2024 at 12:25 PM Hans Verkuil  wrote:
>
> Hi Paweł,
>
> On 21/02/2024 17:02, Paweł Anikiel wrote:
> > Currently, .query_dv_timings() is defined as a video callback without
> > a pad argument. This is a problem if the subdevice can have different
> > dv timings for each pad (e.g. a DisplayPort receiver with multiple
> > virtual channels).
> >
> > To solve this, add a pad variant of this callback which includes
> > the pad number as an argument.
>
> So now we have two query_dv_timings ops: one for video ops, and one
> for pad ops. That's not very maintainable. I would suggest switching
> all current users of the video op over to the pad op.

I agree it would be better if there was only one. However, I have some concerns:
1. Isn't there a problem with backwards compatibility? For example, an
out-of-tree driver is likely to use this callback, which would break.
I'm asking because I'm not familiar with how such API changes are
handled.
2. If I do switch all current users to the pad op, I can't test those
changes. Isn't that a problem?
3. Would I need to get ACK from all the driver maintainers?

Re: [PATCH 1/3] [v2] drm/xe/kunit: fix link failure with built-in xe


On Mon, Feb 26, 2024 at 01:46:36PM +0100, Arnd Bergmann wrote:

From: Arnd Bergmann 

When the driver is built-in but the tests are in loadable modules,
the helpers don't actually get put into the driver:

ERROR: modpost: "xe_kunit_helper_alloc_xe_device" 
[drivers/gpu/drm/xe/tests/xe_test.ko] undefined!

Change the Makefile to ensure they are always part of the driver
even when the rest of the kunit tests are in loadable modules.

Fixes: 5095d13d758b ("drm/xe/kunit: Define helper functions to allocate fake xe 
device")
Signed-off-by: Arnd Bergmann 



Reviewed-by: Lucas De Marchi 

thanks
Lucas De Marchi


---
v2: don't remove KUNIT dependency
---
drivers/gpu/drm/xe/Kconfig   | 1 +
drivers/gpu/drm/xe/Kconfig.debug | 1 -
drivers/gpu/drm/xe/Makefile  | 6 --
3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index 6d4428b19a4c..c3a3b204ae5b 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -11,6 +11,7 @@ config DRM_XE
select DRM_BUDDY
select DRM_EXEC
select DRM_KMS_HELPER
+   select DRM_KUNIT_TEST_HELPERS if DRM_XE_KUNIT_TEST != n
select DRM_PANEL
select DRM_SUBALLOC_HELPER
select DRM_DISPLAY_DP_HELPER
diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
index 549065f57a78..df02e5d17d26 100644
--- a/drivers/gpu/drm/xe/Kconfig.debug
+++ b/drivers/gpu/drm/xe/Kconfig.debug
@@ -76,7 +76,6 @@ config DRM_XE_KUNIT_TEST
depends on DRM_XE && KUNIT && DEBUG_FS
default KUNIT_ALL_TESTS
select DRM_EXPORT_FOR_TESTS if m
-   select DRM_KUNIT_TEST_HELPERS
help
  Choose this option to allow the driver to perform selftests under
  the kunit framework
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 4c6ffe4b2172..b596e4482a9b 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -158,8 +158,10 @@ xe-$(CONFIG_PCI_IOV) += \
xe_lmtt_2l.o \
xe_lmtt_ml.o

-xe-$(CONFIG_DRM_XE_KUNIT_TEST) += \
-   tests/xe_kunit_helpers.o
+# include helpers for tests even when XE is built-in
+ifdef CONFIG_DRM_XE_KUNIT_TEST
+xe-y += tests/xe_kunit_helpers.o
+endif

# i915 Display compat #defines and #includes
subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
--
2.39.2

Re: [PATCH 2/3] [v2] drm/xe/mmio: fix build warning for BAR resize on 32-bit


On Mon, Feb 26, 2024 at 01:46:37PM +0100, Arnd Bergmann wrote:

From: Arnd Bergmann 

clang complains about a nonsensical test on builds with a 32-bit phys_addr_t,
which means resizing will always fail:

drivers/gpu/drm/xe/xe_mmio.c:109:23: error: result of comparison of constant 
4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is 
always false [-Werror,-Wtautological-constant-out-of-range-compare]
 109 | root_res->start > 0x1ull)
 | ~~~ ^ ~~

Previously, BAR resize was always disallowed on 32-bit kernels, but
this apparently changed recently. Since 32-bit machines can in theory
support PAE/LPAE for large address spaces, this may end up useful,
so change the driver to shut up the warning but still work when
phys_addr_t/resource_size_t is 64 bit wide.

Fixes: 9a6e6c14bfde ("drm/xe/mmio: Use non-atomic writeq/readq variant for 32b")
Fixes: 237412e45390 ("drm/xe: Enable 32bits build")
Signed-off-by: Arnd Bergmann 



Reviewed-by: Lucas De Marchi 

Lucas De Marchi


---
v2: use correct Fixes tag
---
drivers/gpu/drm/xe/xe_mmio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index e3db3a178760..7ba2477452d7 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -106,7 +106,7 @@ static void xe_resize_vram_bar(struct xe_device *xe)

pci_bus_for_each_resource(root, root_res, i) {
if (root_res && root_res->flags & (IORESOURCE_MEM | IORESOURCE_MEM_64) 
&&
-   root_res->start > 0x1ull)
+   (u64)root_res->start > 0x1ul)
break;
}

--
2.39.2

Re: [PATCH 3/3] [v2] drm/xe/xe2: fix 64-bit division in pte_update_size


On Wed, Feb 28, 2024 at 01:26:29PM +0100, Arnd Bergmann wrote:

On Mon, Feb 26, 2024, at 17:40, Lucas De Marchi wrote:

On Mon, Feb 26, 2024 at 01:46:38PM +0100, Arnd Bergmann wrote:


Fixes: 237412e45390 ("drm/xe: Enable 32bits build")
Signed-off-by: Arnd Bergmann 
---
v2: use correct Fixes tag


but what about the other comment? How are we supposed to use
DIV_ROUND_UP() but then in some places (which?) have to open code it?


The problem is not DIV_ROUND_UP() but the division but the 64-bit
division itself. There is a DIV_ROUND_UP_ULL() macro that would
address the build failure as well, but doing the shift is much
more efficient here since it can be done in a couple of instructions.


What compiler does this fail on?


I saw it with clang-19 on 32-bit arm, but I assume it happens
on others as well.


somehow it passed on x86 :-/




drivers/gpu/drm/xe/xe_migrate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
index a66fdf2d2991..ee1bb938c493 100644
--- a/drivers/gpu/drm/xe/xe_migrate.c
+++ b/drivers/gpu/drm/xe/xe_migrate.c
@@ -462,7 +462,7 @@ static u32 pte_update_size(struct xe_migrate *m,
} else {
/* Clip L0 to available size */
u64 size = min(*L0, (u64)avail_pts * SZ_2M);
-   u64 num_4k_pages = DIV_ROUND_UP(size, XE_PAGE_SIZE);
+   u32 num_4k_pages = (size + XE_PAGE_SIZE - 1) >> XE_PTE_SHIFT;


also the commit message doesn't seem to match the patch as you are only
changing one instance.


Not sure what you mean. As I wrote in the changelog, the
second instance is fixed by using a 32-bit division here,
which does not cause link failures.


I missed the type conversion to u32 and was thinking there was another
hunk missing for the second change.

all looks good to me now and I will apply later today to drm-xe-next.
Thanks.

Reviewed-by: Lucas De Marchi 

Lucas De Marchi



 Arnd

[PATCH] drm/i915/selftest_hangcheck: Check sanity with more patience

2024-02-28 Thread Janusz Krzysztofik

While trying to reproduce some other issues reported by CI for i915
hangcheck live selftest, I found them hidden behind timeout failures
reported by igt_hang_sanitycheck -- the very first hangcheck test case
executed.

Feb 22 19:49:06 DUT1394ACMR kernel: calling  mei_gsc_driver_init+0x0/0xff0 
[mei_gsc] @ 121074
Feb 22 19:49:06 DUT1394ACMR kernel: i915 :03:00.0: [drm] DRM_I915_DEBUG 
enabled
Feb 22 19:49:06 DUT1394ACMR kernel: i915 :03:00.0: [drm] Cannot find any 
crtc or sizes
Feb 22 19:49:06 DUT1394ACMR kernel: probe of i915.mei-gsc.768 returned 0 after 
1475 usecs
Feb 22 19:49:06 DUT1394ACMR kernel: probe of i915.mei-gscfi.768 returned 0 
after 1441 usecs
Feb 22 19:49:06 DUT1394ACMR kernel: initcall mei_gsc_driver_init+0x0/0xff0 
[mei_gsc] returned 0 after 3010 usecs
Feb 22 19:49:06 DUT1394ACMR kernel: i915 :03:00.0: [drm] DRM_I915_DEBUG_GEM 
enabled
Feb 22 19:49:06 DUT1394ACMR kernel: i915 :03:00.0: [drm] 
DRM_I915_DEBUG_RUNTIME_PM enabled
Feb 22 19:49:06 DUT1394ACMR kernel: i915: Performing live selftests with 
st_random_seed=0x4c26c048 st_timeout=500
Feb 22 19:49:07 DUT1394ACMR kernel: i915: Running hangcheck
Feb 22 19:49:07 DUT1394ACMR kernel: calling  mei_hdcp_driver_init+0x0/0xff0 
[mei_hdcp] @ 121074
Feb 22 19:49:07 DUT1394ACMR kernel: i915: Running 
intel_hangcheck_live_selftests/igt_hang_sanitycheck
Feb 22 19:49:07 DUT1394ACMR kernel: probe of 
:00:16.0-b638ab7e-94e2-4ea2-a552-d1c54b627f04 returned 0 after 1398 usecs
Feb 22 19:49:07 DUT1394ACMR kernel: probe of 
i915.mei-gsc.768-b638ab7e-94e2-4ea2-a552-d1c54b627f04 returned 0 after 97 usecs
Feb 22 19:49:07 DUT1394ACMR kernel: initcall mei_hdcp_driver_init+0x0/0xff0 
[mei_hdcp] returned 0 after 101960 usecs
Feb 22 19:49:07 DUT1394ACMR kernel: calling  mei_pxp_driver_init+0x0/0xff0 
[mei_pxp] @ 121094
Feb 22 19:49:07 DUT1394ACMR kernel: probe of 
:00:16.0-fbf6fcf1-96cf-4e2e-a6a6-1bab8cbe36b1 returned 0 after 435 usecs
Feb 22 19:49:07 DUT1394ACMR kernel: mei_pxp 
i915.mei-gsc.768-fbf6fcf1-96cf-4e2e-a6a6-1bab8cbe36b1: bound :03:00.0 (ops 
i915_pxp_tee_component_ops [i915])
Feb 22 19:49:07 DUT1394ACMR kernel: 100ms wait for request failed on rcs0, 
err=-62
Feb 22 19:49:07 DUT1394ACMR kernel: probe of 
i915.mei-gsc.768-fbf6fcf1-96cf-4e2e-a6a6-1bab8cbe36b1 returned 0 after 158425 
usecs
Feb 22 19:49:07 DUT1394ACMR kernel: initcall mei_pxp_driver_init+0x0/0xff0 
[mei_pxp] returned 0 after 224159 usecs
Feb 22 19:49:07 DUT1394ACMR kernel: i915/intel_hangcheck_live_selftests: 
igt_hang_sanitycheck failed with error -5
Feb 22 19:49:07 DUT1394ACMR kernel: i915: probe of :03:00.0 failed with 
error -5

Those request waits, once timed out after 100ms, have never been
confirmed to still persist over another 100ms, always being able to
complete within the originally requested wait time doubled.

Taking into account potentially significant additional concurrent workload
generated by new auxiliary drivers that didn't exist before and now are
loaded in parallel with the i915 module also when loaded in selftest mode,
relax our expectations on time consumed by the sanity check request before
it completes.

Signed-off-by: Janusz Krzysztofik 
---
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c 
b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 0dd4d00ee894e..9ce8ff1c04fe5 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -319,7 +319,7 @@ static int igt_hang_sanitycheck(void *arg)
i915_request_add(rq);
 
timeout = 0;
-   intel_wedge_on_timeout(, gt, HZ / 10 /* 100ms */)
+   intel_wedge_on_timeout(, gt, HZ / 5 /* 200ms */)
timeout = i915_request_wait(rq, 0,
MAX_SCHEDULE_TIMEOUT);
if (intel_gt_is_wedged(gt))
-- 
2.43.0

ECC memory semantics for heaps

Hi,

I'm currently working on a platform that seems to have togglable RAM ECC
support. Enabling ECC reduces the memory capacity and memory bandwidth,
so while it's a good idea to protect most of the system, it's not worth
it for things like framebuffers that won't really be affected by a
bitflip.

It's currently setup by enabling ECC on the entire memory, and then
having a region of memory where ECC is disabled and where we're supposed
to allocate from for allocations that don't need it.

My first thought to support this was to create a reserved memory region
for the !ECC memory, and to create a heap to allocate buffers from that
region. That would leave the system protected by ECC, while enabling
userspace to be nicer to the system by allocating buffers from the !ECC
region if it doesn't need it.

However, this creates basically a new combination compared to the one we
already have (ie, physically contiguous vs virtually contiguous), and we
probably would want to throw in cacheable vs non-cacheable too.

If we had to provide new heaps for each variation, we would have 8 heaps
(and 6 new ones), which could be fine I guess but would still increase
quite a lot the number of heaps we have so far.

Is it something that would be a problem? If it is, do you see another
way to support those kind of allocations (like providing hints through
the ioctl maybe?)?

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v12 0/8] Enable Adaptive Sync SDP Support for DP

2024-02-28 Thread Jani Nikula

On Wed, 28 Feb 2024, Mitul Golani  wrote:
> -v12:
> - Update cover letter

Did you just send the entire series again within 30 minutes just to
update the cover letter? You could've just replied to the cover
letter...

BR,
Jani.

-- 
Jani Nikula, Intel

Re: [PATCH v2 2/9] media: Add Chameleon v3 framebuffer driver

2024-02-28 Thread Hans Verkuil

On 28/02/2024 16:08, Paweł Anikiel wrote:
> Hi Hans, thanks for the review!
> 
> On Wed, Feb 28, 2024 at 12:24 PM Hans Verkuil  
> wrote:
>>
>> Hi Paweł,
>>
>> On 21/02/2024 17:02, Paweł Anikiel wrote:
>>> Add v4l2 driver for the Google Chameleon v3 framebuffer device.
>>
>> This is just a video capture device, right? A framebuffer device is something
>> that lives in drivers/video/fbdev.
> 
> Yes, it is just a capture device.
> 
>>
>> It is *very* confusing to see the term 'framebuffer' used in a video
>> capture context.
> 
> I agree the name is confusing. I think it started out as something
> else and unfortunately stuck around. I think it's possible to change
> it, though.

That would be very helpful.

> 
>>
>> This commit log should also give a better description of the hardware.
>> Just a single one-liner is a bit on the short side :-)
> 
> Would it be fine to just put the Kconfig help text there?

Yes.

> 
>>
>>>
>>> Signed-off-by: Paweł Anikiel 
>>> ---
>>>  drivers/media/platform/Kconfig|   1 +
>>>  drivers/media/platform/Makefile   |   1 +
>>>  drivers/media/platform/google/Kconfig |   3 +
>>>  drivers/media/platform/google/Makefile|   2 +
>>>  .../media/platform/google/chameleonv3/Kconfig |  13 +
>>>  .../platform/google/chameleonv3/Makefile  |   3 +
>>>  .../platform/google/chameleonv3/chv3-fb.c | 895 ++
>>
>> chv3-video.c would be a much better name for chv3-fb.c.
>>
>> That's a commonly used filename for video capture drivers.
> 
> I'm guessing all the instances of fb or framebuffer in the driver
> itself should be changed as well in that case?

Yes, please. I wouldn't normally ask for such major renaming, but
in this case that name is really confusing.

> 
>>
>>>  7 files changed, 918 insertions(+)
>>>  create mode 100644 drivers/media/platform/google/Kconfig
>>>  create mode 100644 drivers/media/platform/google/Makefile
>>>  create mode 100644 drivers/media/platform/google/chameleonv3/Kconfig
>>>  create mode 100644 drivers/media/platform/google/chameleonv3/Makefile
>>>  create mode 100644 drivers/media/platform/google/chameleonv3/chv3-fb.c
>>>
>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>> index 91e54215de3a..b82f7b142b85 100644
>>> --- a/drivers/media/platform/Kconfig
>>> +++ b/drivers/media/platform/Kconfig
>>> @@ -69,6 +69,7 @@ source "drivers/media/platform/aspeed/Kconfig"
>>>  source "drivers/media/platform/atmel/Kconfig"
>>>  source "drivers/media/platform/cadence/Kconfig"
>>>  source "drivers/media/platform/chips-media/Kconfig"
>>> +source "drivers/media/platform/google/Kconfig"
>>>  source "drivers/media/platform/intel/Kconfig"
>>>  source "drivers/media/platform/marvell/Kconfig"
>>>  source "drivers/media/platform/mediatek/Kconfig"
>>> diff --git a/drivers/media/platform/Makefile 
>>> b/drivers/media/platform/Makefile
>>> index 3296ec1ebe16..f7067eb05f76 100644
>>> --- a/drivers/media/platform/Makefile
>>> +++ b/drivers/media/platform/Makefile
>>> @@ -12,6 +12,7 @@ obj-y += aspeed/
>>>  obj-y += atmel/
>>>  obj-y += cadence/
>>>  obj-y += chips-media/
>>> +obj-y += google/
>>>  obj-y += intel/
>>>  obj-y += marvell/
>>>  obj-y += mediatek/
>>> diff --git a/drivers/media/platform/google/Kconfig 
>>> b/drivers/media/platform/google/Kconfig
>>> new file mode 100644
>>> index ..2a5f01034c11
>>> --- /dev/null
>>> +++ b/drivers/media/platform/google/Kconfig
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +source "drivers/media/platform/google/chameleonv3/Kconfig"
>>> diff --git a/drivers/media/platform/google/Makefile 
>>> b/drivers/media/platform/google/Makefile
>>> new file mode 100644
>>> index ..c971a09faeb4
>>> --- /dev/null
>>> +++ b/drivers/media/platform/google/Makefile
>>> @@ -0,0 +1,2 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +obj-y += chameleonv3/
>>> diff --git a/drivers/media/platform/google/chameleonv3/Kconfig 
>>> b/drivers/media/platform/google/chameleonv3/Kconfig
>>> new file mode 100644
>>> index ..76d0383a8589
>>> --- /dev/null
>>> +++ b/drivers/media/platform/google/chameleonv3/Kconfig
>>> @@ -0,0 +1,13 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +config VIDEO_CHV3_FB
>>> + tristate "Google Chameleon v3 framebuffer video driver"
>>> + depends on V4L_PLATFORM_DRIVERS
>>> + depends on VIDEO_DEV
>>> + select VIDEOBUF2_DMA_CONTIG
>>> + select V4L2_FWNODE
>>> + help
>>> +   v4l2 driver for the video interface present on the Google
>>> +   Chameleon v3. The Chameleon v3 uses the framebuffer IP core
>>> +   to take the video signal from different sources and directly
>>> +   write frames into memory.
>>
>> So it is composing different video streams into buffers? Or does it
>> capture from a single source at a time? The text is rather ambiguous.
> 
> You're right, I'll write a more precise description.
> 
>>
>>> diff --git

Re: [PATCH v2 2/9] media: Add Chameleon v3 framebuffer driver

2024-02-28 Thread Paweł Anikiel

Hi Hans, thanks for the review!

On Wed, Feb 28, 2024 at 12:24 PM Hans Verkuil  wrote:
>
> Hi Paweł,
>
> On 21/02/2024 17:02, Paweł Anikiel wrote:
> > Add v4l2 driver for the Google Chameleon v3 framebuffer device.
>
> This is just a video capture device, right? A framebuffer device is something
> that lives in drivers/video/fbdev.

Yes, it is just a capture device.

>
> It is *very* confusing to see the term 'framebuffer' used in a video
> capture context.

I agree the name is confusing. I think it started out as something
else and unfortunately stuck around. I think it's possible to change
it, though.

>
> This commit log should also give a better description of the hardware.
> Just a single one-liner is a bit on the short side :-)

Would it be fine to just put the Kconfig help text there?

>
> >
> > Signed-off-by: Paweł Anikiel 
> > ---
> >  drivers/media/platform/Kconfig|   1 +
> >  drivers/media/platform/Makefile   |   1 +
> >  drivers/media/platform/google/Kconfig |   3 +
> >  drivers/media/platform/google/Makefile|   2 +
> >  .../media/platform/google/chameleonv3/Kconfig |  13 +
> >  .../platform/google/chameleonv3/Makefile  |   3 +
> >  .../platform/google/chameleonv3/chv3-fb.c | 895 ++
>
> chv3-video.c would be a much better name for chv3-fb.c.
>
> That's a commonly used filename for video capture drivers.

I'm guessing all the instances of fb or framebuffer in the driver
itself should be changed as well in that case?

>
> >  7 files changed, 918 insertions(+)
> >  create mode 100644 drivers/media/platform/google/Kconfig
> >  create mode 100644 drivers/media/platform/google/Makefile
> >  create mode 100644 drivers/media/platform/google/chameleonv3/Kconfig
> >  create mode 100644 drivers/media/platform/google/chameleonv3/Makefile
> >  create mode 100644 drivers/media/platform/google/chameleonv3/chv3-fb.c
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 91e54215de3a..b82f7b142b85 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -69,6 +69,7 @@ source "drivers/media/platform/aspeed/Kconfig"
> >  source "drivers/media/platform/atmel/Kconfig"
> >  source "drivers/media/platform/cadence/Kconfig"
> >  source "drivers/media/platform/chips-media/Kconfig"
> > +source "drivers/media/platform/google/Kconfig"
> >  source "drivers/media/platform/intel/Kconfig"
> >  source "drivers/media/platform/marvell/Kconfig"
> >  source "drivers/media/platform/mediatek/Kconfig"
> > diff --git a/drivers/media/platform/Makefile 
> > b/drivers/media/platform/Makefile
> > index 3296ec1ebe16..f7067eb05f76 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -12,6 +12,7 @@ obj-y += aspeed/
> >  obj-y += atmel/
> >  obj-y += cadence/
> >  obj-y += chips-media/
> > +obj-y += google/
> >  obj-y += intel/
> >  obj-y += marvell/
> >  obj-y += mediatek/
> > diff --git a/drivers/media/platform/google/Kconfig 
> > b/drivers/media/platform/google/Kconfig
> > new file mode 100644
> > index ..2a5f01034c11
> > --- /dev/null
> > +++ b/drivers/media/platform/google/Kconfig
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +source "drivers/media/platform/google/chameleonv3/Kconfig"
> > diff --git a/drivers/media/platform/google/Makefile 
> > b/drivers/media/platform/google/Makefile
> > new file mode 100644
> > index ..c971a09faeb4
> > --- /dev/null
> > +++ b/drivers/media/platform/google/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +obj-y += chameleonv3/
> > diff --git a/drivers/media/platform/google/chameleonv3/Kconfig 
> > b/drivers/media/platform/google/chameleonv3/Kconfig
> > new file mode 100644
> > index ..76d0383a8589
> > --- /dev/null
> > +++ b/drivers/media/platform/google/chameleonv3/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config VIDEO_CHV3_FB
> > + tristate "Google Chameleon v3 framebuffer video driver"
> > + depends on V4L_PLATFORM_DRIVERS
> > + depends on VIDEO_DEV
> > + select VIDEOBUF2_DMA_CONTIG
> > + select V4L2_FWNODE
> > + help
> > +   v4l2 driver for the video interface present on the Google
> > +   Chameleon v3. The Chameleon v3 uses the framebuffer IP core
> > +   to take the video signal from different sources and directly
> > +   write frames into memory.
>
> So it is composing different video streams into buffers? Or does it
> capture from a single source at a time? The text is rather ambiguous.

You're right, I'll write a more precise description.

>
> > diff --git a/drivers/media/platform/google/chameleonv3/Makefile 
> > b/drivers/media/platform/google/chameleonv3/Makefile
> > new file mode 100644
> > index ..d63727148688
> > --- /dev/null
> > +++ b/drivers/media/platform/google/chameleonv3/Makefile
> > @@ -0,0 +1,3 @@
> > +#

Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Set layer mode during creation

Hi Anatoliy,

Thank you for the patch.

On Mon, Feb 26, 2024 at 08:44:42PM -0800, Anatoliy Klymenko wrote:
> Set layer mode of operation (live or dma-based) during layer creation.
> 
> Each DPSUB layer mode of operation is defined by corresponding DT node port
> connection, so it is possible to assign it during layer object creation.
> Previously it was set in layer enable functions, although it is too late
> as setting layer format depends on layer mode, and should be done before
> given layer enabled.
> 
> Signed-off-by: Anatoliy Klymenko 
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c | 20 
>  drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
>  drivers/gpu/drm/xlnx/zynqmp_dp.c   |  2 +-
>  drivers/gpu/drm/xlnx/zynqmp_kms.c  |  2 +-
>  4 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 8a39b3accce5..e6d26ef60e89 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -64,6 +64,16 @@
>  
>  #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES   3
>  
> +/**
> + * enum zynqmp_dpsub_layer_mode - Layer mode
> + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> + */
> +enum zynqmp_dpsub_layer_mode {
> + ZYNQMP_DPSUB_LAYER_NONLIVE,
> + ZYNQMP_DPSUB_LAYER_LIVE,
> +};
> +
>  /**
>   * struct zynqmp_disp_format - Display subsystem format information
>   * @drm_fmt: DRM format (4CC)
> @@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct 
> zynqmp_disp_layer *layer,
>  /**
>   * zynqmp_disp_layer_enable - Enable a layer
>   * @layer: The layer
> - * @mode: Operating mode of layer
>   *
>   * Enable the @layer in the audio/video buffer manager and the blender. DMA
>   * channels are started separately by zynqmp_disp_layer_update().
>   */
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> -   enum zynqmp_dpsub_layer_mode mode)
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>  {
> - layer->mode = mode;
>   zynqmp_disp_avbuf_enable_video(layer->disp, layer);
>   zynqmp_disp_blend_layer_enable(layer->disp, layer);
>  }
> @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct 
> zynqmp_disp *disp)
>   layer->id = i;
>   layer->disp = disp;
>   layer->info = _info[i];
> + /* For now assume dpsub works in either live or non-live mode 
> for both layers.
> +  * Hybrid mode is not supported yet.
> +  */

/*
 * For now assume dpsub works in either live or non-live mode
 * for both layers. Hybrid mode is not supported yet.
 */

Sounds like a reasonable restriction for now.

Reviewed-by: Laurent Pinchart 

> + layer->mode = disp->dpsub->dma_enabled ? 
> ZYNQMP_DPSUB_LAYER_NONLIVE
> +: 
> ZYNQMP_DPSUB_LAYER_LIVE;
>  
>   ret = zynqmp_disp_layer_request_dma(disp, layer);
>   if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h 
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 123cffac08be..9b8b202224d9 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
>   ZYNQMP_DPSUB_LAYER_GFX,
>  };
>  
> -/**
> - * enum zynqmp_dpsub_layer_mode - Layer mode
> - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> - */
> -enum zynqmp_dpsub_layer_mode {
> - ZYNQMP_DPSUB_LAYER_NONLIVE,
> - ZYNQMP_DPSUB_LAYER_LIVE,
> -};
> -
>  void zynqmp_disp_enable(struct zynqmp_disp *disp);
>  void zynqmp_disp_disable(struct zynqmp_disp *disp);
>  int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> @@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp 
> *disp,
>  
>  u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
>  unsigned int *num_formats);
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> -   enum zynqmp_dpsub_layer_mode mode);
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
>  void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
>  void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> const struct drm_format_info *info);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c 
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 1846c4971fd8..04b6bcac3b07 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
>   /* TODO: Make the format configurable. */
>   info = drm_format_info(DRM_FORMAT_YUV422);
>   zynqmp_disp_layer_set_format(layer,

Re: [PATCH] MAINTAINERS: Update email address for Tvrtko Ursulin

2024-02-28 Thread Jani Nikula

On Wed, 28 Feb 2024, Tvrtko Ursulin  wrote:
> From: Tvrtko Ursulin 
>
> I will lose access to my @.*intel.com e-mail addresses soon so let me
> adjust the maintainers entry and update the mailmap too.
>
> While at it consolidate a few other of my old emails to point to the
> main one.
>
> Signed-off-by: Tvrtko Ursulin 
> Cc: Daniel Vetter 
> Cc: Dave Airlie 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 

Acked-by: Jani Nikula 

> ---
>  .mailmap| 5 +
>  MAINTAINERS | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index b99a238ee3bd..d67e351bce8e 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -608,6 +608,11 @@ TripleX Chung  
>  TripleX Chung  
>  Tsuneo Yoshioka 
>  Tudor Ambarus  
> +Tvrtko Ursulin  
> +Tvrtko Ursulin  
> +Tvrtko Ursulin  
> +Tvrtko Ursulin  
> +Tvrtko Ursulin  
>  Tycho Andersen  
>  Tzung-Bi Shih  
>  Uwe Kleine-König 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 19f6f8014f94..b940bfe2a692 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10734,7 +10734,7 @@ INTEL DRM I915 DRIVER (Meteor Lake, DG2 and older 
> excluding Poulsbo, Moorestown
>  M:   Jani Nikula 
>  M:   Joonas Lahtinen 
>  M:   Rodrigo Vivi 
> -M:   Tvrtko Ursulin 
> +M:   Tvrtko Ursulin 
>  L:   intel-...@lists.freedesktop.org
>  S:   Supported
>  W:   https://drm.pages.freedesktop.org/intel-docs/

-- 
Jani Nikula, Intel

Re: [PATCH] drm/scheduler: Simplify the allocation of slab caches in drm_sched_fence_slab_init

2024-02-28 Thread Daniel Vetter

On Wed, Feb 21, 2024 at 04:55:58PM +0800, Kunwu Chan wrote:
> Use the new KMEM_CACHE() macro instead of direct kmem_cache_create
> to simplify the creation of SLAB caches.
> 
> Signed-off-by: Kunwu Chan 

Applied to drm-misc-next, thanks for your patch!
-Sima

> ---
>  drivers/gpu/drm/scheduler/sched_fence.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c 
> b/drivers/gpu/drm/scheduler/sched_fence.c
> index 06cedfe4b486..0f35f009b9d3 100644
> --- a/drivers/gpu/drm/scheduler/sched_fence.c
> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> @@ -33,9 +33,7 @@ static struct kmem_cache *sched_fence_slab;
>  
>  static int __init drm_sched_fence_slab_init(void)
>  {
> - sched_fence_slab = kmem_cache_create(
> - "drm_sched_fence", sizeof(struct drm_sched_fence), 0,
> - SLAB_HWCACHE_ALIGN, NULL);
> + sched_fence_slab = KMEM_CACHE(drm_sched_fence, SLAB_HWCACHE_ALIGN);
>   if (!sched_fence_slab)
>   return -ENOMEM;
>  
> -- 
> 2.39.2
> 

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

[PATCH v12 8/8] drm/i915/display: Read/Write AS sdp only when sink/source has enabled

Write/Read Adaptive sync SDP only when Sink and Source is enabled
for the same. Also along with write TRANS_VRR_VSYNC values.

Signed-off-by: Mitul Golani 
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 4 
 drivers/gpu/drm/i915/display/intel_dp.c  | 4 
 drivers/gpu/drm/i915/display/intel_vrr.c | 4 
 3 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
b/drivers/gpu/drm/i915/display/intel_ddi.c
index bea441590204..89b8d50f12c6 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3926,6 +3926,7 @@ static void intel_ddi_get_config(struct intel_encoder 
*encoder,
 {
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
+   struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
/* XXX: DSI transcoder paranoia */
if (drm_WARN_ON(_priv->drm, transcoder_is_dsi(cpu_transcoder)))
@@ -3972,6 +3973,9 @@ static void intel_ddi_get_config(struct intel_encoder 
*encoder,
intel_read_dp_sdp(encoder, pipe_config, 
HDMI_PACKET_TYPE_GAMUT_METADATA);
intel_read_dp_sdp(encoder, pipe_config, DP_SDP_VSC);
 
+   if (intel_dp_as_sdp_supported(intel_dp))
+   intel_read_dp_sdp(encoder, pipe_config, DP_SDP_ADAPTIVE_SYNC);
+
intel_audio_codec_get_config(encoder, pipe_config);
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 2ec1f923a5a0..8304ef912767 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4276,6 +4276,7 @@ void intel_dp_set_infoframes(struct intel_encoder 
*encoder,
 VIDEO_DIP_ENABLE_SPD_HSW | VIDEO_DIP_ENABLE_DRM_GLK |
 VIDEO_DIP_ENABLE_AS_ADL;
u32 val = intel_de_read(dev_priv, reg) & ~dip_enable;
+   struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
/* TODO: Sanitize DSC enabling wrt. intel_dsc_dp_pps_write(). */
if (!enable && HAS_DSC(dev_priv))
@@ -4293,6 +4294,9 @@ void intel_dp_set_infoframes(struct intel_encoder 
*encoder,
 
intel_write_dp_sdp(encoder, crtc_state, DP_SDP_VSC);
 
+   if (intel_dp_as_sdp_supported(intel_dp))
+   intel_write_dp_sdp(encoder, crtc_state, DP_SDP_ADAPTIVE_SYNC);
+
intel_write_dp_sdp(encoder, crtc_state, 
HDMI_PACKET_TYPE_GAMUT_METADATA);
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
b/drivers/gpu/drm/i915/display/intel_vrr.c
index 668927524f23..d24a42902e69 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -214,6 +214,10 @@ void intel_vrr_set_transcoder_timings(const struct 
intel_crtc_state *crtc_state)
intel_de_write(dev_priv, TRANS_VRR_VMAX(cpu_transcoder), 
crtc_state->vrr.vmax - 1);
intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder), 
trans_vrr_ctl(crtc_state));
intel_de_write(dev_priv, TRANS_VRR_FLIPLINE(cpu_transcoder), 
crtc_state->vrr.flipline - 1);
+
+   if (crtc_state->vrr.vsync_end && crtc_state->vrr.vsync_start)
+   intel_de_write(dev_priv, TRANS_VRR_VSYNC(cpu_transcoder),
+  crtc_state->vrr.vsync_end << 16 | 
crtc_state->vrr.vsync_start);
 }
 
 void intel_vrr_send_push(const struct intel_crtc_state *crtc_state)
-- 
2.25.1

[PATCH v12 6/8] drm/i915/display: Add state checker for Adaptive Sync SDP

Enable infoframe and add state checker for Adaptive Sync
SDP enablement.

Signed-off-by: Mitul Golani 
---
 drivers/gpu/drm/i915/display/intel_display.c | 46 
 drivers/gpu/drm/i915/display/intel_dp.c  |  2 +
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 00ac65a14029..be0a5fae4e58 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4781,6 +4781,17 @@ intel_compare_dp_vsc_sdp(const struct drm_dp_vsc_sdp *a,
a->content_type == b->content_type;
 }
 
+static bool
+intel_compare_dp_as_sdp(const struct drm_dp_as_sdp *a,
+   const struct drm_dp_as_sdp *b)
+{
+   return a->vtotal == b->vtotal &&
+   a->target_rr == b->target_rr &&
+   a->duration_incr_ms == b->duration_incr_ms &&
+   a->duration_decr_ms == b->duration_decr_ms &&
+   a->mode == b->mode;
+}
+
 static bool
 intel_compare_buffer(const u8 *a, const u8 *b, size_t len)
 {
@@ -4836,6 +4847,30 @@ pipe_config_dp_vsc_sdp_mismatch(struct drm_i915_private 
*i915,
drm_dp_vsc_sdp_log(, b);
 }
 
+static void
+pipe_config_dp_as_sdp_mismatch(struct drm_i915_private *i915,
+  bool fastset, const char *name,
+  const struct drm_dp_as_sdp *a,
+  const struct drm_dp_as_sdp *b)
+{
+   struct drm_printer p;
+
+   if (fastset) {
+   p = drm_dbg_printer(>drm, DRM_UT_KMS, NULL);
+
+   drm_printf(, "fastset requirement not met in %s dp sdp\n", 
name);
+   } else {
+   p = drm_err_printer(>drm, NULL);
+
+   drm_printf(, "mismatch in %s dp sdp\n", name);
+   }
+
+   drm_printf(, "expected:\n");
+   drm_dp_as_sdp_log(, a);
+   drm_printf(, "found:\n");
+   drm_dp_as_sdp_log(, b);
+}
+
 /* Returns the length up to and including the last differing byte */
 static size_t
 memcmp_diff_len(const u8 *a, const u8 *b, size_t len)
@@ -5089,6 +5124,16 @@ intel_pipe_config_compare(const struct intel_crtc_state 
*current_config,
} \
 } while (0)
 
+#define PIPE_CONF_CHECK_DP_AS_SDP(name) do { \
+   if (!intel_compare_dp_as_sdp(_config->infoframes.name, \
+ _config->infoframes.name)) { \
+   pipe_config_dp_as_sdp_mismatch(dev_priv, fastset, 
__stringify(name), \
+   
_config->infoframes.name, \
+   _config->infoframes.name); 
\
+   ret = false; \
+   } \
+} while (0)
+
 #define PIPE_CONF_CHECK_BUFFER(name, len) do { \
BUILD_BUG_ON(sizeof(current_config->name) != (len)); \
BUILD_BUG_ON(sizeof(pipe_config->name) != (len)); \
@@ -5270,6 +5315,7 @@ intel_pipe_config_compare(const struct intel_crtc_state 
*current_config,
PIPE_CONF_CHECK_INFOFRAME(hdmi);
PIPE_CONF_CHECK_INFOFRAME(drm);
PIPE_CONF_CHECK_DP_VSC_SDP(vsc);
+   PIPE_CONF_CHECK_DP_AS_SDP(as_sdp);
 
PIPE_CONF_CHECK_X(sync_mode_slaves_mask);
PIPE_CONF_CHECK_I(master_transcoder);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 1cd3cc0d0c0b..2ec1f923a5a0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2648,6 +2648,8 @@ static void intel_dp_compute_as_sdp(struct intel_dp 
*intel_dp,
as_sdp->target_rr = 0;
as_sdp->duration_incr_ms = 0;
as_sdp->duration_incr_ms = 0;
+
+   crtc_state->infoframes.enable |= 
intel_hdmi_infoframe_enable(DP_SDP_ADAPTIVE_SYNC);
 }
 
 static void intel_dp_compute_vsc_sdp(struct intel_dp *intel_dp,
-- 
2.25.1

[PATCH v12 7/8] drm/i915/display: Compute vrr_vsync params

Compute vrr_vsync_start/end, which sets the position
for hardware to send the Vsync at a fixed position
relative to the end of the Vblank.

--v2:
- Updated VSYNC_START/END macros to VRR_VSYNC_START/END. (Ankit)
- Updated bit fields of VRR_VSYNC_START/END. (Ankit)

--v3:
- Add PIPE_CONF_CHECK_I(vrr.vsync_start/end).
- Read/write vrr_vsync params only when we intend to send
adaptive_sync sdp.

Signed-off-by: Mitul Golani 
---
 drivers/gpu/drm/i915/display/intel_display.c  |  2 ++
 .../drm/i915/display/intel_display_types.h|  1 +
 drivers/gpu/drm/i915/display/intel_vrr.c  | 25 +--
 drivers/gpu/drm/i915/i915_reg.h   |  7 ++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index be0a5fae4e58..c523eec4d626 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5367,6 +5367,8 @@ intel_pipe_config_compare(const struct intel_crtc_state 
*current_config,
PIPE_CONF_CHECK_I(vrr.flipline);
PIPE_CONF_CHECK_I(vrr.pipeline_full);
PIPE_CONF_CHECK_I(vrr.guardband);
+   PIPE_CONF_CHECK_I(vrr.vsync_start);
+   PIPE_CONF_CHECK_I(vrr.vsync_end);
}
 
 #undef PIPE_CONF_CHECK_X
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h 
b/drivers/gpu/drm/i915/display/intel_display_types.h
index 1256730ea276..45b30d3ceb06 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1417,6 +1417,7 @@ struct intel_crtc_state {
bool enable, in_range;
u8 pipeline_full;
u16 flipline, vmin, vmax, guardband;
+   u32 vsync_end, vsync_start;
} vrr;
 
/* Stream Splitter for eDP MSO */
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
b/drivers/gpu/drm/i915/display/intel_vrr.c
index 5d905f932cb4..668927524f23 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -9,6 +9,7 @@
 #include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_vrr.h"
+#include "intel_dp.h"
 
 bool intel_vrr_is_capable(struct intel_connector *connector)
 {
@@ -113,6 +114,7 @@ intel_vrr_compute_config(struct intel_crtc_state 
*crtc_state,
struct drm_i915_private *i915 = to_i915(crtc->base.dev);
struct intel_connector *connector =
to_intel_connector(conn_state->connector);
+   struct intel_dp *intel_dp = intel_attached_dp(connector);
struct drm_display_mode *adjusted_mode = _state->hw.adjusted_mode;
const struct drm_display_info *info = >base.display_info;
int vmin, vmax;
@@ -165,6 +167,15 @@ intel_vrr_compute_config(struct intel_crtc_state 
*crtc_state,
if (crtc_state->uapi.vrr_enabled) {
crtc_state->vrr.enable = true;
crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
+
+   if (intel_dp_as_sdp_supported(intel_dp)) {
+   crtc_state->vrr.vsync_start =
+   (crtc_state->hw.adjusted_mode.crtc_vtotal -
+   
VRR_VSYNC_START(crtc_state->hw.adjusted_mode.vsync_start));
+   crtc_state->vrr.vsync_end =
+   (crtc_state->hw.adjusted_mode.crtc_vtotal -
+   
(VRR_VSYNC_END(crtc_state->hw.adjusted_mode.vsync_end) >> 16));
+   }
}
 }
 
@@ -263,7 +274,7 @@ void intel_vrr_get_config(struct intel_crtc_state 
*crtc_state)
 {
struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-   u32 trans_vrr_ctl;
+   u32 trans_vrr_ctl, trans_vrr_vsync;
 
trans_vrr_ctl = intel_de_read(dev_priv, TRANS_VRR_CTL(cpu_transcoder));
 
@@ -283,6 +294,16 @@ void intel_vrr_get_config(struct intel_crtc_state 
*crtc_state)
crtc_state->vrr.vmin = intel_de_read(dev_priv, 
TRANS_VRR_VMIN(cpu_transcoder)) + 1;
}
 
-   if (crtc_state->vrr.enable)
+   if (crtc_state->vrr.enable) {
crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
+
+   if (HAS_AS_SDP(dev_priv)) {
+   trans_vrr_vsync =
+   intel_de_read(dev_priv, 
TRANS_VRR_VSYNC(cpu_transcoder));
+   crtc_state->vrr.vsync_start =
+   trans_vrr_vsync & VRR_VSYNC_START_MASK;
+   crtc_state->vrr.vsync_end =
+   trans_vrr_vsync & VRR_VSYNC_START_MASK;
+   }
+   }
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dce276236707..53d8eb7ea1ea 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2007,7 +2007,9 @@
 #define

[PATCH v12 5/8] drm/i915/display: Compute AS SDP parameters.

Add necessary function definitions to compute AS SDP data.
The new intel_dp_compute_as_sdp function computes AS SDP
values based on the display configuration, ensuring proper
handling of Variable Refresh Rate (VRR).

--v2:
- Added DP_SDP_ADAPTIVE_SYNC to infoframe_type_to_idx(). [Ankit]
- Separated patch for intel_read/write_dp_sdp. [Ankit]
- _HSW_VIDEO_DIP_ASYNC_DATA_A should be from ADL onward. [Ankit]
- Fixed indentation issues. [Ankit]

--v3:
- Added VIDEO_DIP_ENABLE_AS_HSW flag to intel_dp_set_infoframes.

--v4:
- Added HAS_VRR check before writing AS SDP.

--v5:
Added missed HAS_VRR check before reading AS SDP.

--v6:
- Used Adaptive Sync sink status as a check for read/write SDP. (Ankit)

--v7:
- Remove as_sdp_enable from crtc_state.
- Add a comment mentioning current support of
  DP_AS_SDP_AVT_FIXED_VTOTAL.
- Add state checker for AS_SDP infoframe enable.

Signed-off-by: Mitul Golani 
---
 drivers/gpu/drm/i915/display/intel_dp.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 7eb83924f3fe..1cd3cc0d0c0b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2626,6 +2626,30 @@ static void intel_dp_compute_vsc_colorimetry(const 
struct intel_crtc_state *crtc
vsc->content_type = DP_CONTENT_TYPE_NOT_DEFINED;
 }
 
+static void intel_dp_compute_as_sdp(struct intel_dp *intel_dp,
+   struct intel_crtc_state *crtc_state,
+   const struct drm_connector_state 
*conn_state)
+{
+   struct drm_dp_as_sdp *as_sdp = _state->infoframes.as_sdp;
+   struct intel_connector *connector = intel_dp->attached_connector;
+   const struct drm_display_mode *adjusted_mode =
+   _state->hw.adjusted_mode;
+   int vrefresh = drm_mode_vrefresh(adjusted_mode);
+
+   if (!intel_vrr_is_in_range(connector, vrefresh) ||
+   !intel_dp_as_sdp_supported(intel_dp))
+   return;
+
+   /* Currently only DP_AS_SDP_AVT_FIXED_VTOTAL mode supported */
+   as_sdp->sdp_type = DP_SDP_ADAPTIVE_SYNC;
+   as_sdp->length = 0x9;
+   as_sdp->mode = DP_AS_SDP_AVT_FIXED_VTOTAL;
+   as_sdp->vtotal = adjusted_mode->vtotal;
+   as_sdp->target_rr = 0;
+   as_sdp->duration_incr_ms = 0;
+   as_sdp->duration_incr_ms = 0;
+}
+
 static void intel_dp_compute_vsc_sdp(struct intel_dp *intel_dp,
 struct intel_crtc_state *crtc_state,
 const struct drm_connector_state 
*conn_state)
@@ -2951,6 +2975,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
g4x_dp_set_clock(encoder, pipe_config);
 
intel_vrr_compute_config(pipe_config, conn_state);
+   intel_dp_compute_as_sdp(intel_dp, pipe_config, conn_state);
intel_psr_compute_config(intel_dp, pipe_config, conn_state);
intel_dp_drrs_compute_config(connector, pipe_config, link_bpp_x16);
intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state);
-- 
2.25.1

[PATCH v12 4/8] drm/i915/display/dp: Add wrapper function to check AS SDP

Add a wrapper function to check if both the source and
sink support Adaptive Sync SDP.

Signed-off-by: Mitul Golani 
---
 drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
 drivers/gpu/drm/i915/display/intel_dp.c | 8 
 drivers/gpu/drm/i915/display/intel_dp.h | 1 +
 3 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h 
b/drivers/gpu/drm/i915/display/intel_display_device.h
index fe4268813786..6399fbc6c738 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -68,6 +68,7 @@ struct drm_printer;
 #define HAS_TRANSCODER(i915, trans)
((DISPLAY_RUNTIME_INFO(i915)->cpu_transcoder_mask & \
  BIT(trans)) != 0)
 #define HAS_VRR(i915)  (DISPLAY_VER(i915) >= 11)
+#define HAS_AS_SDP(i915)   (DISPLAY_VER(i915) >= 13)
 #define INTEL_NUM_PIPES(i915)  
(hweight8(DISPLAY_RUNTIME_INFO(i915)->pipe_mask))
 #define I915_HAS_HOTPLUG(i915) (DISPLAY_INFO(i915)->has_hotplug)
 #define OVERLAY_NEEDS_PHYSICAL(i915)   
(DISPLAY_INFO(i915)->overlay_needs_physical)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index e5377cdc71c6..7eb83924f3fe 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -120,6 +120,14 @@ bool intel_dp_is_edp(struct intel_dp *intel_dp)
return dig_port->base.type == INTEL_OUTPUT_EDP;
 }
 
+bool intel_dp_as_sdp_supported(struct intel_dp *intel_dp)
+{
+   struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+   return HAS_AS_SDP(i915) &&
+   drm_dp_as_sdp_supported(_dp->aux, intel_dp->dpcd);
+}
+
 static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
 /* Is link rate UHBR and thus 128b/132b? */
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h 
b/drivers/gpu/drm/i915/display/intel_dp.h
index 530cc97bc42f..cc5e069091ff 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -80,6 +80,7 @@ void intel_dp_audio_compute_config(struct intel_encoder 
*encoder,
   struct drm_connector_state *conn_state);
 bool intel_dp_has_hdmi_sink(struct intel_dp *intel_dp);
 bool intel_dp_is_edp(struct intel_dp *intel_dp);
+bool intel_dp_as_sdp_supported(struct intel_dp *intel_dp);
 bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state);
 int intel_dp_link_symbol_size(int rate);
 int intel_dp_link_symbol_clock(int rate);
-- 
2.25.1

[PATCH v12 3/8] drm/i915/dp: Add Read/Write support for Adaptive Sync SDP