* [PATCH] media: dvb-usb: fix refcount leak in dvb_usb_adapter_frontend_init()
From: Wentao Liang @ 2026-06-09 4:03 UTC (permalink / raw)
To: mchehab; +Cc: kees, linux-media, linux-kernel, Wentao Liang, stable
After all frontends are registered via dvb_register_frontend(),
if dvb_create_media_graph() or dvb_usb_media_device_register()
fails, the function returns directly without calling
dvb_unregister_frontend() and dvb_frontend_detach() for each
frontend. This leaves them with a leaked reference count.
The caller's error path calls dvb_usb_adapter_dvb_exit() which
does not handle frontend cleanup. Add the missing cleanup by
calling dvb_usb_adapter_frontend_exit() on the error path.
Fixes: 9f80679511b0 ("[media] usb: check media device errors")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
index 029dad86a059..923f25ccb1e8 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
@@ -326,10 +326,16 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
ret = dvb_create_media_graph(&adap->dvb_adap, true);
if (ret)
- return ret;
+ goto err_fe_cleanup;
ret = dvb_usb_media_device_register(adap);
+ if (ret)
+ goto err_fe_cleanup;
+
+ return ret;
+err_fe_cleanup:
+ dvb_usb_adapter_frontend_exit(adap);
return ret;
}
--
2.34.1
^ permalink raw reply related
* RE: [PATCH v0 0/4] bug fixes
From: jackson.lee @ 2026-06-09 0:31 UTC (permalink / raw)
To: Brandon Brnich, mchehab@kernel.org, hverkuil-cisco@xs4all.nl,
nicolas.dufresne@collabora.com, bob.beckett@collabora.com
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
lafley.kim, hverkuil@xs4all.nl, Nas Chung
In-Reply-To: <06f8d2b9-89f3-4951-a001-9a89149ae315@ti.com>
Hi Brandon
> -----Original Message-----
> From: Brandon Brnich <b-brnich@ti.com>
> Sent: Monday, June 8, 2026 11:16 PM
> To: jackson.lee <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> hverkuil-cisco@xs4all.nl; nicolas.dufresne@collabora.com;
> bob.beckett@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; lafley.kim
> <lafley.kim@chipsnmedia.com>; hverkuil@xs4all.nl; Nas Chung
> <nas.chung@chipsnmedia.com>
> Subject: Re: [PATCH v0 0/4] bug fixes
>
> Hi Jackson,
>
> On 6/3/26 21:01, Jackson.lee wrote:
> > From: Jackson Lee <jackson.lee@chipsnmedia.com>
> >
> > A few independent fixes for the Wave5 VPU driver, covering decode
> > setup, m2m scheduling and stop_streaming robustness.
>
> This series appears to fix some issues in v4l2-compliance report, can you
> add the updated report in the next iteration of this series to make sure
> everything is now fixed with latest v4l2-compliance tag?
I'll add the test report in the next version of the patch series.
Thanks
>
> Best,
> Brandon
>
> >
> > Jackson Lee (4):
> > media: chips-media: wave5: Guard bit depth check with
> > initial_info_obtained
> > media: chips-media: wave5: Set inst->std during default format
> > initialization
> > media: chips-media: wave5: avoid skipping device_run while VPU has
> > work
> > media: chips-media: wave5: Add interrupt timeout while
> > stop_streaming
> >
> > .../chips-media/wave5/wave5-vpu-dec.c | 20 +++++++++++++++----
> > .../chips-media/wave5/wave5-vpu-enc.c | 6 ++++--
> > .../chips-media/wave5/wave5-vpuconfig.h | 2 +-
> > 3 files changed, 21 insertions(+), 7 deletions(-)
> >
^ permalink raw reply
* Re: [PATCH] iommufd: take dma_resv lock before dma_buf_unpin() in release path
From: Jason Gunthorpe @ 2026-06-09 0:17 UTC (permalink / raw)
To: Ankit Soni
Cc: Kevin Tian, Joerg Roedel, Will Deacon, Robin Murphy, Sumit Semwal,
Christian König, Leon Romanovsky, Vasant Hegde, iommu,
dri-devel, linaro-mm-sig, linux-media, linux-kernel
In-Reply-To: <20260526111034.4079-1-Ankit.Soni@amd.com>
On Tue, May 26, 2026 at 11:10:34AM +0000, Ankit Soni wrote:
> Take the dma_resv lock around dma_buf_unpin() in iopt_release_pages(),
> matching the iopt_map_dmabuf() convention. dma_buf_detach() acquires the
> reservation lock internally, so it must remain outside the locked region.
>
> Fixes: 8c5f9645c389 ("iommufd: Add dma_buf_pin()")
> Reported-by: Ankit Soni <Ankit.Soni@amd.com>
> Signed-off-by: Ankit Soni <Ankit.Soni@amd.com>
> ---
> drivers/iommu/iommufd/pages.c | 2 ++
> 1 file changed, 2 insertions(+)
Applied to for-next
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH v5 06/10] media: imx219: Fix vertical blanking and exposure for analogue binning
From: Laurent Pinchart @ 2026-06-08 21:52 UTC (permalink / raw)
To: Jai Luthra
Cc: Sakari Ailus, Jacopo Mondi, linux-media, hans, Prabhakar,
Kate Hsuan, Dave Stevenson, Tommaso Merciai, Benjamin Mugnier,
Sylvain Petinot, Christophe JAILLET, Julien Massot,
Naushir Patuck, Yan, Dongcheng, Stefan Klug, Mirela Rabulea,
André Apitzsch, Heimir Thor Sverrisson, Kieran Bingham,
Mehdi Djait, Ricardo Ribalda Delgado, Hans de Goede,
Tomi Valkeinen, David Plowman, Yu , Ong Hock, Ng, Khai Wen,
Rishikesh Donadkar
In-Reply-To: <178093582619.19620.15016359616261234139@freya>
On Mon, Jun 08, 2026 at 09:53:46PM +0530, Jai Luthra wrote:
> Quoting Sakari Ailus (2026-06-08 19:37:34)
> > On Mon, Jun 08, 2026 at 12:10:26PM +0300, Laurent Pinchart wrote:
> > > On Mon, Jun 08, 2026 at 08:58:46AM +0200, Jacopo Mondi wrote:
> > > > Hi Sakari
> > > >
> > > > On Mon, Jun 08, 2026 at 12:53:52AM +0300, Sakari Ailus wrote:
> > > > > When vertical analogue binning is in use, the minimum frame length in
> > > > > lines decreases to around half of the normal. In relation to the sensor's
> > > > > output size this means vertical blanking can be negative but that's not an
> > > > > issue as control values are signed. Remove the workaround for this
> > > >
> > > > Didn't we just discussed two weeks ago in media summit how negative
> > > > blankings are a bad idea, and of all drivers one could decide to play
> > > > with imx219 is probably the worse due it's large use base and the fact
> > > > libcamera doesn't support negative blankings ?
> > >
> > > I also think that negative blanking values are a bad idea, for this
> > > driver or any other driver. I still haven't seen any compelling
> > > argument.
> >
> > Note that the blanking controls haven't expressed blanking in other
> > configurations than those that do not use binning, either analogue or
> > digital, or cropping. The fact that negative values would result due to
> > sensor configuration does not mean the values would be somehow incorrect,
> > they simply do not reflect actual blanking configuration on the sensor.
>
> I agree.. although what is the actual blanking configuration on the sensor
> in this case?
>
> I've been banging my head for a while to figure it out (my best guess in
> the sibling thread)
>
> > In retrospect, we should have always had frame length in lines and line
> > length in pixels controls instead, or possibly besides the blanking
> > controls. But as the two blanking controls have been in use for conveying
> > frame length in lines and line length in pixels, relative to a reference
> > size,
What's the reference size ? The controls are documented as
``V4L2_CID_VBLANK (integer)``
Vertical blanking. The idle period after every frame during which no
image data is produced. The unit of vertical blanking is a line.
Every line has length of the image width plus horizontal blanking at
the pixel rate defined by ``V4L2_CID_PIXEL_RATE`` control in the
same sub-device.
``V4L2_CID_HBLANK (integer)``
Horizontal blanking. The idle period after every line of image data
during which no image data is produced. The unit of horizontal
blanking is pixels.
This is compatible with usage of the output size as a reference. And
doing so wouldn't require negative blanking, would it ?
> > we can't re-purpose them for something else anymore without breaking
> > pretty much all userspace.
> >
> > Multiplying the pixel rate and either frame length in lines or line length
> > in pixels by a constant does not make the blanking controls magically
> > correct.
The multiplication of the pixel rate may be the core of the issue. It's
getting late, I'll try to look at that tomorrow.
> Sure.. but the sensor must be reading out pixels along with non-negative
> blanking internally. If that happens to not match the register values, do
> you object to multiplying LLP or FLL with a constant?
>
> Also what do you think of sensors like IMX678/IMX283 where the HTOT (LLP)
> register is not in units of pixels at all?
>
> > I hope the above summarises my position in an understandable way.
> >
> > I believe removing the rate_factor is necesary if we want to add support
> > for the Common Raw Sensor Model to the imx219 driver. Otherwise, we'll be
> > left with a single example driver only, the ov2740, which is an entirely
> > register list based driver.
>
> Agreed.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v5 03/10] media: imx219: Account rate_factor in setting upper exposure limit
From: Laurent Pinchart @ 2026-06-08 21:38 UTC (permalink / raw)
To: Dave Stevenson
Cc: Sakari Ailus, linux-media, hans, Prabhakar, Kate Hsuan,
Tommaso Merciai, Benjamin Mugnier, Sylvain Petinot,
Christophe JAILLET, Julien Massot, Naushir Patuck, Yan, Dongcheng,
Stefan Klug, Mirela Rabulea, André Apitzsch,
Heimir Thor Sverrisson, Kieran Bingham, Mehdi Djait,
Ricardo Ribalda Delgado, Hans de Goede, Jacopo Mondi,
Tomi Valkeinen, David Plowman, Yu, Ong Hock, Ng, Khai Wen,
Jai Luthra, Rishikesh Donadkar
In-Reply-To: <CAPY8ntDPvDdj6vSVRUsTmC9tXmy8xNATrrbyqDr7DRU1NiiDQQ@mail.gmail.com>
On Mon, Jun 08, 2026 at 04:42:44PM +0100, Dave Stevenson wrote:
> On Sun, 7 Jun 2026 at 22:54, Sakari Ailus wrote:
> >
> > The rate_factor multiplier is used to multiply a few values in the
> > sensor's timing configuration and the exposure time is one of them. This
> > also needs to be taken into account in exposure time margin: multiply it
> > by rate_factor so that sensor's exposure time margin is respected.
>
> Testing the 1640x1232 mode with FRM_LENGTH_A set to 0x288 (79.07fps),
> I can write register 0x15a (COARSE_INTEGRATION_TIME_A) with values up
> to and including 0x284 without it affecting the output frame rate, and
> without image corruption.
> With IMX219_EXPOSURE_OFFSET being 4, the current code implements
> exactly those limits, so why do you believe the offset should be
> increased?
>
> To my mind section 5-5 Frame Rate Calculation Formula of the datasheet
> is fairly clear with
> [ In the case of (frame_length_lines - 4 > coarse_integration_time) ]:
> Frame_Length = frame_length_lines
> [ In the case of (frame_length_lines - 4 < coarse_integration_time) ]:
> Frame_Length = coarse_integration_time + 4
>
> The register FRM_LENGTH_A (0x160) being in units of 2 lines doesn't
> change that calculation.
Readnig Jai's and your analysis, I agree. This patch should be dropped.
> > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/i2c/imx219.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index d8fe7db18b6c..e681f80f9e3e 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -451,7 +451,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > int exposure_max, exposure_def;
> >
> > /* Update max exposure while meeting expected vblanking */
> > - exposure_max = format->height + ctrl->val - IMX219_EXPOSURE_OFFSET;
> > + exposure_max = format->height + ctrl->val -
> > + IMX219_EXPOSURE_OFFSET * rate_factor;
> > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> > exposure_max : IMX219_EXPOSURE_DEFAULT;
> > ret = __v4l2_ctrl_modify_range(imx219->exposure,
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: Adding Linux-media to Sashiko
From: Laurent Pinchart @ 2026-06-08 21:37 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Nicolas Dufresne, Ricardo Ribalda, Jai Luthra,
Linux Media Mailing List
In-Reply-To: <CAGb2v66HTeQc1UZHCXg_v1yD=_CbYUTfXRSLqK8NV8HO=oyvGA@mail.gmail.com>
On Mon, Jun 08, 2026 at 11:24:17AM +0800, Chen-Yu Tsai wrote:
> On Sat, Jun 6, 2026 at 2:24 AM Nicolas Dufresne wrote:
> > Le jeudi 28 mai 2026 à 23:44 +0200, Ricardo Ribalda a écrit :
> > > Hi Jai
> > >
> > > I thought that we agreed to experiment with cc to the list to evaluate
> > > the quality of the review and then decide how to configure it.
> > >
> > > But I might also be miss-remembering it.
My understanding is that we decided to *not* get those reviews sent to
the list, *nor* to the author. Hans confirmed this on IRC. I have
reported the issue in
https://github.com/sashiko-dev/sashiko/issues/252#issuecomment-4651247882
and Roman has reverted the addition of linux-media for the time being.
Hans is working on preparing the media summit minutes. If anyone
disagrees with Hans' and my understanding of the consensus, let's
discuss it when the minutes will be posted, and refrain from enacting
any change in the meantime.
> > > The current PR in sashiko has landed, but it is very easy (and fast)
> > > to upload a change.
> >
> > I was pushing against having the emails during the summit, so I am equally
> > surprised. We can of course let it run and see how it goes, first thing I notice
> > is the amount of "existing issue" reports it adds is quite big. My a worry is
> > that people with low knowledge might try and fix them all, which just make the
> > process stalled until sashiko gets quite about a specific code base.
>
> Unfortunately this is already happening on other lists. In the case I
> dealt with the submitter just tried to fix all the subsequent issues
> without an actual deep understanding of the problem. It didn't help that
> the submitter didn't have the hardware (or experience with the platform)
> and was just doing cleanup fixes. The end result was that the commit
> messages didn't really match reality.
>
> I suspect that the media drivers are much larger and therefore harder
> to fix by drive-by contributors like this. So as you said it could just
> stall the whole process.
>
> > > On Thu, 28 May 2026 at 18:01, Jai Luthra wrote:
> > > > Quoting Ricardo Ribalda (2026-05-28 20:41:56)
> > > > > Hi
> > > > >
> > > > > As we discussed in the media summit, I just created the Pull request
> > > > > to add linux-media to Sashiko.
> > > > >
> > > > > https://github.com/sashiko-dev/sashiko/pull/224
> > > >
> > > > Thank you. Does the cc option mean the list will get emails from Sashiko as
> > > > well?
> > > >
> > > > I thought the consensus during the summit was to keep the replies only to
> > > > the author (and maintainers can check sashiko manually) but maybe I'm
> > > > misremembering it.
> > > >
> > > > > Please let me know if something does not work as expected
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v5 02/10] media: imx219: Scale the vblank limits according to rate_factor
From: Laurent Pinchart @ 2026-06-08 21:28 UTC (permalink / raw)
To: Dave Stevenson
Cc: Sakari Ailus, linux-media, hans, Prabhakar, Kate Hsuan,
Tommaso Merciai, Benjamin Mugnier, Sylvain Petinot,
Christophe JAILLET, Julien Massot, Naushir Patuck, Yan, Dongcheng,
Stefan Klug, Mirela Rabulea, André Apitzsch,
Heimir Thor Sverrisson, Kieran Bingham, Mehdi Djait,
Ricardo Ribalda Delgado, Hans de Goede, Jacopo Mondi,
Tomi Valkeinen, David Plowman, Yu, Ong Hock, Ng, Khai Wen,
Jai Luthra, Rishikesh Donadkar
In-Reply-To: <CAPY8ntDvpP8Nuc2VFOFgp+5HSDNWmJCika5in=pjZJVry=J7RQ@mail.gmail.com>
Hi Dave,
Thanks a lot for taking the time to investigate and provide very
valuable insight. I really appreciate that.
On Mon, Jun 08, 2026 at 04:29:46PM +0100, Dave Stevenson wrote:
> On Sun, 7 Jun 2026 at 22:54, Sakari Ailus wrote:
> >
> > The limits for vertical blanking (and frame length in pixels) is related
> > to the properties of the hardware, it's not in half-line units the driver
> > uses. Multiply the vertical blanking limits by the rate_factor to satisty
> > hardware requirements.
>
> Whilst that would be a logical interpretation, it doesn't match with
> how the hardware performs, nor the docs.
>
> The datasheet for register FRM_LENGTH_A 0x160 says
> frame_length_lines
> BINNING_MODE = 0,1,2
> Unit: 1Lines
> BINNING_MODE = 3
> Units: 2Lines
>
> That's not units of 2 lines for active lines only, that is units of 2
> lines for ALL lines.
>
> I have tested it, and the sensor works fine with FRM_LENGTH_A being
> 0x278 in the 1640x1232 mode, and 0x100 in 640x480 modes with all the
> tests I've thrown at it. Drop them any lower and it does stall or give
> corrupt horizontal lines.
If I understand this correctly, it indicates that
- The frame length is expressed as a number of lines at the output of
the sensor (after binning).
- The minimum margin between the output height (after binning) and the
frame length is 32 lines.
- In special binning mode, the FRM_LENGTH_A register needs to be
programmed with frame_length_lines / 2.
The last constraint is device-specific, and as far as I understand it
can be handled directly in the driver without affecting the userspace
API by simply dividing the frame length value by 2 before writing it to
the register.
It's getting a bit late, I'll test the "regular" binning mode
(BINNING_MODE = 1) tomorrow to see how it compares (unless someone beats
me to it). Unless I get very unexpected results, it seems that the
existing implementation is correct and the patch should be dropped.
One thing that may not be implemented correctly is different binning
modes horizontally and vertically. imx219_get_rate_factor() will return
2 only when both the horizontal and vertical binning modes are "special
analog binning". When used to scale the frame length and exposure time,
I wonder if only vertical binning should be taken into account.
> (Please note that the sensor extends the frame length automatically to
> accommodate the exposure time requested, so do ensure the exposure
> time doesn't interact with the frame length if you're testing).
>
> This patch drops the maximum frame rate from 81.07 to 79.07fps in
> 1640x1232 (2.5%), and 200.1 to 188.39fps in 640x480 (6%) for no good
> reason that I can see.
> Unless anyone can produce a genuine situation where they see the
> sensor behave incorrectly with the old setup, I'll be very sad to see
> this merged.
>
> > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/i2c/imx219.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 223d3753cc93..d8fe7db18b6c 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -878,14 +878,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> >
> > if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + unsigned int rate_factor = imx219_get_rate_factor(state);
> > int exposure_max;
> > int exposure_def;
> > int llp_min;
> > int pixel_rate;
> >
> > /* Update limits and set FPS to default */
> > - ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > - IMX219_FLL_MAX - mode->height, 1,
> > + ret = __v4l2_ctrl_modify_range(imx219->vblank,
> > + IMX219_VBLANK_MIN * rate_factor,
> > + (IMX219_FLL_MAX - mode->height) *
> > + rate_factor, rate_factor,
> > mode->fll_def - mode->height);
> > if (ret)
> > return ret;
> > @@ -928,8 +931,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > return ret;
> >
> > /* Scale the pixel rate based on the mode specific factor */
> > - pixel_rate = imx219_get_pixel_rate(imx219) *
> > - imx219_get_rate_factor(state);
> > + pixel_rate = imx219_get_pixel_rate(imx219) * rate_factor;
> > ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> > pixel_rate, 1, pixel_rate);
> > if (ret)
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v5 06/10] media: imx219: Fix vertical blanking and exposure for analogue binning
From: Laurent Pinchart @ 2026-06-08 21:01 UTC (permalink / raw)
To: Jai Luthra
Cc: Jacopo Mondi, Sakari Ailus, Hans Verkuil, Dave Stevenson,
linux-media, Prabhakar, Kate Hsuan, Tommaso Merciai,
Benjamin Mugnier, Sylvain Petinot, Christophe JAILLET,
Julien Massot, Naushir Patuck, Yan, Dongcheng, Stefan Klug,
Mirela Rabulea, André Apitzsch, Heimir Thor Sverrisson,
Kieran Bingham, Mehdi Djait, Ricardo Ribalda Delgado,
Hans de Goede, Tomi Valkeinen, David Plowman, Yu, Ong Hock,
Ng, Khai Wen, Rishikesh Donadkar
In-Reply-To: <178091466607.16054.13972332068848565738@freya>
I wanted to check the valid of the static read-only registers, and ended
up dumping all registers while the sensor is running (using the
1920x1080 cropped mode, no binning). Here are the results, for the
non-zero registers. A large number of registers follow the CCS
specifications, registers marked with a (*) differ.
I will reply to the ongoing discussion separately.
0x0000 - module_model_id = 0x0219
0x0002 - ??? = 0x20
0x0004 - Lot_ID (*) = 0x686b17
0x0007 - Wafer_Num (*) = 0x0f
0x000d - Chip_Number (*) = 0x0945
0x0018 - FRM_CNT (*) = 0x56
0x0019 - PX_ORDER (*) = RGGB (0x01)
0x001b - DT_PEDESTAL (*) = 64
0x0040 - frame_format_model_type = 2-Byte Generic Frame Format (0x01)
0x0041 - frame_format_model_subtype = 1 Column, 2 Rows (0x12)
0x0042 - frame_format_descriptor_0 = Visible Pixel data: 3280 (0x5cd0)
0x0044 - frame_format_descriptor_1 = Embedded data: 2 (0x1002)
0x0046 - frame_format_descriptor_2 = Visible Pixel data: 2464 (0x59a0)
0x0086 - analog_gain_code_max = 224
0x0088 - analog_gain_code_step_size = 1
0x008e - analog_gain_c0 = 256
0x0090 - analog_gain_m1 = -1
0x0092 - analog_gain_c1 = 256
0x00c0 - data_format_model_type = 2-byte data format (0x01)
0x00c1 - data_format_model_subtype = 1 descriptor
0x00c2 - data_format_descriptor_0 = 10 bit uncompressed, 8 bit compressed (0x0a08)
0x00c4 - data_format_descriptor_1 = 10 bit uncompressed, 10 bit compressed (0x0a0a)
0x0100 - mode_select = 0x01
0x0105 - mask_corrupted_frames = 0x01
0x0114 - CSI_lane_mode (*) = 2 lanes (0x01)
0x0118 - TCLK_POST (*) = 119
0x011a - THS_PREPARE (*) = 71
0x011c - THS_ZERO_MIN (*) = 103
0x011e - THS_TRAIL (*) = 63
0x0120 - TCLK_TRAIL_MIN (*) = 55
0x0122 - TCLK_PREPARE (*) = 63
0x0124 - TCLK_ZERO (*) = 255
0x0126 - TLPX (*) = 55
0x012a - EXCK_FREQ (*) = 24
0x0142 - READOUT_V_CNT (*) = 0x5c70
0x0157 - ANA_GAIN_GLOBAL_A (*) = 232
0x0158 - DIG_GAIN_GLOBAL_A (*) = 256
0x015a - COARSE_INTEGRATION_TIME_A (*) = 1108
0x0160 - FRM_LENGTH_A (*) = 1112
0x0162 - LINE_LENGTH_A (*) = 3448
0x0164 - X_ADD_STA_A (*) = 680
0x0166 - X_ADD_END_A (*) = 2599
0x0168 - Y_ADD_STA_A (*) = 692
0x016a - Y_ADD_END_A (*) = 1771
0x016c - x_output_size (*) = 1920
0x016e - y_output_size (*) = 1080
0x0170 - X_ODD_INC_A (*) = 1
0x0171 - Y_ODD_INC_A (*) = 1
0x018a - COARSE_INTEG_TIME_SHORT_A (*) = 500
0x018c - CSI_DATA_FORMAT_A (*) = 0x0a0a
0x0192 - LSC_SELECT_TABLE_A (*) = 0x01
0x0194 - LSC_WHITE_BALANCE_RG_A (*) = 0x1000
0x0196 - ??? (*) = 0x10
0x0258 - DIG_GAIN_GLOBAL_B (*) = 256
0x025a - COARSE_INTEGRATION_TIME_B (*) = 1000
0x0260 - FRM_LENGTH_B (*) = 2728
0x0262 - LINE_LENGTH_B (*) = 3448
0x0266 - X_ADD_END_B (*) = 3279
0x026a - Y_ADD_END_B (*) = 2463
0x026c - x_output_size (*) = 3280
0x026e - y_output_size (*) = 2464
0x0270 - X_ODD_INC_B (*) = 1
0x0271 - Y_ODD_INC_B (*) = 1
0x028a - COARSE_INTEG_TIME_SHORT_B (*) = 500
0x028c - CSI_DATA_FORMAT_B (*) = 0x0a0a
0x0292 - LSC_SELECT_TABLE_B (*) = 0x01
0x0294 - LSC_WHITE_BALANCE_RG_B (*) = 0x10
0x0296 - ??? = 0x10
0x0300 - vt_pix_clk_div = 5
0x0302 - vt_sys_clk_div = 1
0x0304 - vt_pre_pll_clk_div = 3
0x0305 - op_pre_pll_clk_div = 3
0x0306 - vt_pll_multiplier = 57
0x0308 - op_pix_clk_div = 10
0x030a - op_sys_clk_div = 1
0x030d - op_pll_multiplier = 114
0x0319 - ??? = 0x03
0x031a - ??? = 0x01
0x031b - ??? = 0x4c
0x031d - ??? = 0x03
0x031e - ??? = 0x01
0x031f - ??? = 0x4c
0x0322 - FLASH_STROBE_DIV (*) = 1
0x0334 - FLASH_STROBE_HI_PERIOD_RS (*) = 1
0x0336 - FLASH_STROBE_LO_PERIOD_RS (*) = 1
0x0338 - FLASH_STROBE_COUNT_RS (*) = 1
0x0380 - x_even_inc = 0x01
0x0382 - x_odd_inc = 0x01
0x0388 - FINE_INTEG_TIME (*) = 500
0x0602 - test_data_red = 0x03ff
0x0604 - test_data_greenR = 0x03ff
0x0606 - test_data_blue = 0x03ff
0x0608 - test_data_greenB = 0x03ff
0x0624 - TP_WINDOW_WIDTH (*) = 1920
0x0626 - TP_WINDOW_HEIGHT (*) = 1080
0x1004 - coarse_integration_time_min = 1
0x1006 - coarse_integration_time_max_margin = 4
0x1081 - digital_gain_capability = 1
0x1084 - digital_gain_min = 256
0x1086 - digital_gain_max = 4095
0x1088 - digital_gain_step_size = 1
0x1100 - min_ext_clk_freq_mhz = 6.0 (0x40c00000)
0x1104 - max_ext_clk_freq_mhz = 27.0 (0x41d80000)
0x1108 - min_vt_pre_pll_clk_div = 1
0x110a - max_vt_pre_pll_clk_div = 13
0x110c - min_vt_pll_ip_freq_mhz = 6.0 (0x40c00000)
0x1110 - max_vt_pll_ip_freq_mhz = 27.0 (0x41d80000)
0x1114 - min_vt_pll_multiplier = 8
0x1116 - max_vt_pll_multiplier = 2047
0x1118 - min_pll_op_freq_mhz = 400.0 (0x43c80000)
0x111c - max_pll_op_freq_mhz = 916.0 (0x44650000)
0x1120 - min_vt_sys_clk_div = 1
0x1122 - max_vt_sys_clk_div = 2
0x1124 - min_vt_sys_clk_freq_mhz = 200.0 (0x43480000)
0x1128 - max_vt_sys_clk_freq_mhz = 700.0 (0x442f0000)
0x112c - min_vt_pix_clk_freq_mhz = 80.0 (0x42a00000)
0x1130 - max_vt_pix_clk_freq_mhz = 140.0 (0x430c0000)
0x1134 - min_vt_pix_clk_div = 5
0x1136 - max_vt_pix_clk_div = 5
0x1140 - min_frame_length_lines = 256
0x1142 - max_frame_length_lines = 65534
0x1144 - min_line_length_pck = 3448
0x1146 - max_line_length_pck = 32752
0x1148 - min_line_blanking_pck = 168
0x114a - min_frame_blanking_lines = 32
0x1160 - min_op_sys_clk_div = 1
0x1162 - max_op_sys_clk_div = 2
0x1164 - min_op_sys_clk_freq_mhz = 200.0 (0x43480000)
0x1168 - max_op_sys_clk_freq_mhz = 916.0 (0x44650000)
0x116c - min_op_pix_clk_freq_mhz (*) = 20.0 (0x41a00000)
0x1170 - max_op_pix_clk_freq_mhz (*) = 114.5 (0x42e50000)
0x1174 - min_op_pix_clk_div (*) = 8
0x1176 - max_op_pix_clk_div (*) = 10
0x1184 - x_addr_max = 3279
0x1186 - y_addr_max = 2463
0x1188 - min_x_output_size = 256
0x118a - min_y_output_size = 256
0x118c - max_x_output_size = 3280
0x118e - max_y_output_size = 2464
0x11c0 - min_even_inc = 1
0x11c2 - max_even_inc = 1
0x11c4 - min_odd_inc = 1
0x11c6 - max_odd_inc = 3
0x1211 - ??? = 1
0x1300 - compression_capability = 1
On Mon, Jun 08, 2026 at 04:01:06PM +0530, Jai Luthra wrote:
> Hi Jacopo, Sakari,
> ++ Dave, Hans and Laurent,
>
> Quoting Jacopo Mondi (2026-06-08 12:28:46)
> > Hi Sakari
> >
> > On Mon, Jun 08, 2026 at 12:53:52AM +0300, Sakari Ailus wrote:
> > > When vertical analogue binning is in use, the minimum frame length in
> > > lines decreases to around half of the normal. In relation to the sensor's
> > > output size this means vertical blanking can be negative but that's not an
> > > issue as control values are signed. Remove the workaround for this
> >
> > Didn't we just discussed two weeks ago in media summit how negative
> > blankings are a bad idea, and of all drivers one could decide to play
> > with imx219 is probably the worse due it's large use base and the fact
> > libcamera doesn't support negative blankings ?
> >
> > Have I missed something ?
> >
>
> I think it would be helpful if I write down clearly how this sensor
> operates (to the best of my knowledge) so we can decide on the correct
> fix:
>
> --------------------
>
> IMX219 sensor has an active resolution of 3280x2464.
>
> The driver currently programs the sensor VT pixel clock as fixed for a
> given lane configuration.
>
> In 2-lane mode it reads 182.4 MPixel/second:
>
> #define IMX219_PIXEL_RATE 182400000
>
> And the framerate is given by:
>
> PIXEL_RATE / (FRAME_LENGTH * LINE_LENGTH)
>
> where FRAME_LENGTH and LINE_LENGTH are registers that include the active
> height and width along with blankings.
>
> There are restrictions on the minimum of the LINE_LENGTH register and
> minimum vertical blanking (32), which cap the framerate for the full resolution
> mode.
>
> MIN_LINE_LENGTH: 0xd78 => 3448 pixels
> MIN_FRAME_LENGTH: ACTIVE_HEIGHT + MIN_VBLANK (32) = 2464 + 32
> => 2496 lines
>
> The maximum frame rate is
>
> 182400000/(2496*3448) => ~ 21.2 frames/second
>
> --------------------
>
> A user might want to stream a lower resolution with the full field-of-view,
> let's take 1640x1232 (which is exactly 1/2 of active area) as an example.
>
> The sensor hardware can achieve this using two different binning modes:
>
> 2x2-binning (regval: 0x1)
> 2x2-analog-(special)-binning (regval: 0x3)
>
> The sensor pipeline looks like:
>
> active pixel array ->
> analogue crop (none) ->
> 2x2 binning and ADC readout ->
> output to CSI-2 bus
>
> The mode names suggest that binning can happen either before or after ADC,
> but the datasheet is not very clear about the process. We can infer
> some details though. See below..
>
> --------------------
>
> With the "normal" 2x2-binning mode the sensor allows programming
> FRAME_LENGTH to a lower value. The driver still uses the min blanking of 32
> lines, but the height is now 1232, half of the 2464 before.
>
> MIN_LINE_LENGTH: 0xd78 => 3448 pixels
> MIN_FRAME_LENGTH: READOUT_HEIGHT + MIN_VBLANK = 1232 + 32
> => 1264 lines
>
> The maximum frame rate is
>
> 182400000/(1264*3448) => ~ 41.8 frames/second
>
> --------------------
>
> With the "special" 2x2-binning mode, the datasheet notes that FRAME_LENGTH
> register should be in units of 2 Lines instead of 1 Line. This means
> cutting it down by half once more:
>
> MIN_FRAME_LENGTH: (READOUT_HEIGHT + MIN_VBLANK)/2 = (1232 + 32)/2
> => 632 lines
>
> While there is a slightly higher minimum enforced for the line length:
>
> MIN_LINE_LENGTH: 0xde8 => 3560 pixels
>
> The maximum frame rate is
>
> 182400000/(632*3560) => ~ 81.0 frames/second
>
> ===================
>
> Through the minimum allowed values of the FRAME_LENGTH and LINE_LENGTH
> registers and maximum possible framerate, I think it is safe to say that:
>
> 2x2-binning => Readout half the pixels (do vertical averaging in the
> analogue domain, before ADC
> reads out the voltages)
>
> 2x2-special-binning => Readout a quarter of the pixels (???)
>
> FRAME_LENGTH being 1/4th of normal would seem to suggest that it is a 4x1
> binning (combining 4 lines instead of blocks of 2x2).. which does not make
> sense to me.
>
> My best guess is that in 2x2-special-binning mode the sensor does *both
> horizontal and vertical* averaging in the analogue domain, before the ADC
> reads out the voltages.
>
> A higher minimum LINE_LENGTH value for this mode is the best "hard"
> evidence I have for this guess unfortunately, as the datasheet is quite
> lacking on this topic.
>
> ====================
>
> The APIs before Sakari's series expose HBLANK and VBLANK controls instead
> of the actual FRAME_LENGTH/LINE_LENGTH registers to the userspace.
>
> The minimum value of FRAME_LENGTH is 632 when we are streaming 1640x1232,
> so the sensor registers would suggest that we have a *negative vertical
> blanking*. Which as Jacopo and Laurent both point out, does not make any
> conceptual sense whatsoever.
>
> What the driver does today to avoid these negative values is to double the
> PIXEL_RATE control value to 364800000 when using 2x2-special-binning mode
> (and thus userspace has no idea the FRAME_LENGTH/VBLANK is in units of
> 2xLines)
>
> As Sakari pointed out, that does not make any sense either. The sensor PLL
> values are completely unchanged, so the pixel readout must still be
> happening at the same rate. Moreover, the new raw sensor model will expose
> FRAME_LENGTH and LINE_LENGTH directly to userspace, and this hack of
> doubling the PIXEL_RATE breaks those calculations.
>
> ===================
>
> If we want to support the new raw sensor model (that mandates the new
> FRAME_LENGTH and LINE_LENGTH controls) for this sensor we have to
> fix the PIXEL_RATE for sure. I see two options going forward:
>
> OPTION 1 (as proposed by Sakari):
>
> Fix PIXEL_RATE to 182400000 and allow **negative values** for HBLANK
> and VBLANK controls when using 2x2-special-binning mode.
>
> This will break any userspace tools, many libcamera pipelines included,
> that never expected those control values to be negative (even though
> the API has always permitted those)
>
> OPTION 2 (something that struck me today morning discussing with Jacopo):
>
> Fix PIXEL_RATE to 182400000 but **adjust the HBLANK values** to go
> lower to compensate, which will diverge from the sensor registers which
> keep MIN_LINE_LENGTH fixed across both binning modes.
>
> This will make the driver quite more complicated, but userspace
> expectations of non-negative blankings will be met. And it's likely
> that the sensor is internally doing pre-ADC averaging horizontally as
> well, or so my best guess is.
>
> Of course, there are other options to just leave this highly used sensor
> alone, or support embedded data and internal pads without mandating the new
> FRAME_LENGTH and LINE_LENGTH controls. But I personally would leave that as
> a last resort.
>
> > > non-issue that doubled the pixel rate, frame length in lines and exposure
> > > time.
> > >
> > > The resulting change also fixes the minimum, the maximum and the step
> > > values for the control.
> > >
> > > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning")
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: media: qcom: Add Shikra CAMSS compatible
From: Krzysztof Kozlowski @ 2026-06-08 20:46 UTC (permalink / raw)
To: Nihal Kumar Gupta
Cc: Bryan O'Donoghue, Vladimir Zapolskiy, Loic Poulain,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Robert Foss, Andi Shyti, Bryan O'Donoghue,
Bjorn Andersson, Konrad Dybcio, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, linux-arm-msm,
linux-media, devicetree, linux-kernel, linux-i2c, imx,
linux-arm-kernel, Suresh Vankadara, Vikram Sharma
In-Reply-To: <20260608-shikra-camss-review-v2-1-ca1936bf1219@oss.qualcomm.com>
On Mon, Jun 08, 2026 at 07:36:38PM +0530, Nihal Kumar Gupta wrote:
> Shikra contains the same Camera Subsystem IP as QCM2290. Document the
> platform-specific compatible string, using qcom,qcm2290-camss as
> fallback.
>
> Unlike QCM2290, Shikra omits the CDM and OPE blocks, requiring only a
> single IOMMU context bank instead of four.
>
> Signed-off-by: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>
> ---
> .../devicetree/bindings/media/qcom,qcm2290-camss.yaml | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml
> index 391d0f6f67ef5fdfea31dd3683477561516b1556..4f39eefb4898ebc22117407f26cfb4f41deb111b 100644
> --- a/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml
> @@ -14,8 +14,11 @@ description:
>
> properties:
> compatible:
> - const: qcom,qcm2290-camss
> -
Do not remove blank lines.
> + oneOf:
> + - items:
> + - const: qcom,shikra-camss
> + - const: qcom,qcm2290-camss
> + - const: qcom,qcm2290-camss
> reg:
With this fixed:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [BUG] rkvdec-vdpu383-h264: wrong pixels at horizontal de-blocking edges y=4 and y=12
From: Nicolas Dufresne @ 2026-06-08 20:22 UTC (permalink / raw)
To: Simon Wright, Detlev Casanova, linux-media; +Cc: linux-rockchip
In-Reply-To: <66768711-6943-43f5-95fa-3d97dc638844@symple.nz>
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
Hi Simon,
Le lundi 08 juin 2026 à 17:11 +1200, Simon Wright a écrit :
> It's an un-primed hardware state after power-up, not BL31 and not a
> per-frame register. The Rockchip BSP runs a one-shot priming decode at
> every decoder power-on -- rk3576_workaround_run() in
> drivers/video/rockchip/mpp/hack/mpp_hack_rk3576.c builds a tiny
> self-contained H.264 task and runs it through the decoder at probe and
> on every pm_runtime resume. Mainline rkvdec has no equivalent, so the
> first decode(s) after each power-up run with indeterminate internal
> deblock state.
another one, in the link, it says you have compared SRAM vs DRAM for RCB, to
rule out the location of the RCB data as a problem. You haven't dumped the RCB
data before and after in a way that the state of the RCB data could not be
compared against Detlev (working) case. It if was an RCB data state, it way
simpler to just initialize it.
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [BUG] rkvdec-vdpu383-h264: wrong pixels at horizontal de-blocking edges y=4 and y=12
From: Nicolas Dufresne @ 2026-06-08 20:20 UTC (permalink / raw)
To: Simon Wright, Detlev Casanova, linux-media; +Cc: linux-rockchip
In-Reply-To: <66768711-6943-43f5-95fa-3d97dc638844@symple.nz>
[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]
Hi Simon, Detlev,
Le lundi 08 juin 2026 à 17:11 +1200, Simon Wright a écrit :
> A reference implementation -- the DMA priming buffer plus the
> link-register kick sequence, ported from the BSP -- is in the repo I
> shared (the warmup code itself is in the sibling rkvdec-vdpu383-vp9 /
> -av1 repos, src/rkvdec-link.c):
>
> https://github.com/SympleNZ/rkvdec-vdpu383-h264-bug
>
> I'm not sure of the right mainline form for a vendor-disassembly-
> derived priming sequence, so I've left it as a report + reference
> rather than a patch -- you're much better placed to decide how, or
> whether, it belongs in rkvdec. Happy to test any version on the RK3576
> boards here.
just a two cent proposal. We could save the resume state, and on the first frame
after resume, we'd prepend the workaround decode operation to the TBL. That
would be a bit cleaner, and would batch the workaround without having to
manually poll. The resume state is then cleared on the workaround decode IRQ
(IRQ needs to be enabled in the workaround). Its not a very impactful on
performance otherwise, but we know from past mpp workaround that they don't
always imply a HW bug, they often use workaround as hotfix for other issue they
haven't figure-out yet.
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH v6] dma-buf: Fix silent overflow for phys vec to sgt
From: David Hu @ 2026-06-08 19:43 UTC (permalink / raw)
To: Sumit Semwal, Christian König
Cc: Jason Gunthorpe, Nicolin Chen, Leon Romanovsky, Kevin Tian,
Ankit Agrawal, Alex Williamson, linux-media, dri-devel,
linaro-mm-sig, linux-kernel, iommu, jmoroni, praan, David Hu,
stable
In case MMIO size is bigger than 4G and peer2peer DMA goes
through host bridge, we trigger a code path that assigns the
total linked IOVA (which is greater than 4G) to mapped_len.
Previously, `mapped_len` was declared as 32-bit `unsigned int`.
When accumulating `size_t` lengths, this leads to a silent wrap-around.
This truncation causes truncated lengths to be passed to functions
like `fill_sg_entry()`.
Fix this by changing `mapped_len` to `size_t` (64-bit). While
at it, fix similar potential overflow issues in `calc_sg_nents`
by using `check_add_overflow()` for `nents` and using
`unsigned int` for the loop iterator in `fill_sg_entry` to match.
Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping routine")
Cc: stable@vger.kernel.org
Cc: iommu@lists.linux.dev
Reviewed-by: Pranjal Shrivastava <praan@google.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: David Hu <xuehaohu@google.com>
---
Changes in v6:
- Used `check_add_overflow()` in `calc_sg_nents()` for safer
accumulation (Leon).
- Dropped explicit `!nents` check and added a comment noting that
`sg_alloc_table` handles `nents == 0` (Leon).
- Collected Reviewed-by from Kevin Tian.
Changes in v5:
- Removed WARN_ON_ONCE from calc_sg_nents() to avoid log noise (Jason).
- Added explicit check for `!nents` in dma_buf_phys_vec_to_sgt() to
cleanly return -EINVAL on overflow (Jason).
Changes in v4:
- Added WARN_ON_ONCE() to the nents overflow check to prevent silent
failures (Claude Bot).
Changes in v3:
- Removed leftover sentence fragment from the commit message.
- Kept `nents = 0` initialization (previously stated as removed in the
v2 changelog) as it is strictly required for the `+=` accumulation
loop in `calc_sg_nents()`.
Changes in v2:
- Fixed 'IVOA' -> 'IOVA' typo and expanded commit message (Claude Bot).
- Added Reverse Xmas tree formatting (Pranjal).
- Folded in extra bounds checking for calc_sg_nents() (Pranjal).
- Folded in type consistency fix for fill_sg_entry() (Pranjal).
- Collected Reviewed-by from Pranjal Shrivastava.
drivers/dma-buf/dma-buf-mapping.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
index 794acff2546a..67a8ff52fb8f 100644
--- a/drivers/dma-buf/dma-buf-mapping.c
+++ b/drivers/dma-buf/dma-buf-mapping.c
@@ -5,12 +5,13 @@
*/
#include <linux/dma-buf-mapping.h>
#include <linux/dma-resv.h>
+#include <linux/overflow.h>
static struct scatterlist *fill_sg_entry(struct scatterlist *sgl, size_t length,
dma_addr_t addr)
{
unsigned int len, nents;
- int i;
+ unsigned int i;
nents = DIV_ROUND_UP(length, UINT_MAX);
for (i = 0; i < nents; i++) {
@@ -40,8 +41,11 @@ static unsigned int calc_sg_nents(struct dma_iova_state *state,
size_t i;
if (!state || !dma_use_iova(state)) {
- for (i = 0; i < nr_ranges; i++)
- nents += DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);
+ for (i = 0; i < nr_ranges; i++) {
+ unsigned int added = DIV_ROUND_UP(phys_vec[i].len, UINT_MAX);
+ if (check_add_overflow(nents, added, &nents))
+ return 0;
+ }
} else {
/*
* In IOVA case, there is only one SG entry which spans
@@ -95,9 +99,10 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
size_t nr_ranges, size_t size,
enum dma_data_direction dir)
{
- unsigned int nents, mapped_len = 0;
struct dma_buf_dma *dma;
struct scatterlist *sgl;
+ size_t mapped_len = 0;
+ unsigned int nents;
dma_addr_t addr;
size_t i;
int ret;
@@ -133,6 +138,8 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
}
nents = calc_sg_nents(dma->state, phys_vec, nr_ranges, size);
+
+ /* sg_alloc_table will cleanly fail and return -EINVAL if nents == 0 */
ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO);
if (ret)
goto err_free_state;
--
2.54.0.1064.gd145956f57-goog
^ permalink raw reply related
* Re: [PATCH v7 2/8] media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes
From: David Heidelberg @ 2026-06-08 19:32 UTC (permalink / raw)
To: Frank Li
Cc: Robert Foss, Todor Tomov, Bryan O'Donoghue,
Bryan O'Donoghue, Vladimir Zapolskiy, Mauro Carvalho Chehab,
Luca Weiss, Petr Hodina, Dr. Git, Cory Keitz, Loic Poulain,
Frank Li, Konrad Dybcio, Kieran Bingham, Sakari Ailus,
linux-media, linux-arm-msm, linux-kernel, phone-devel
In-Reply-To: <aicG5nsTy7rIuWTc@lizhi-Precision-Tower-5810>
On 08/06/2026 20:16, Frank Li wrote:
> On Fri, Jun 05, 2026 at 03:14:40PM +0200, David Heidelberg wrote:
>> So far, only D-PHY mode was supported, which uses even bits when enabling
>> or masking lanes. For C-PHY configuration, the hardware instead requires
>> using the odd bits.
>>
>> Since there can be unrecognized configuration allow returning failure.
>>
>> Acked-by: Cory Keitz <ckeitz@amazon.com>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Bryan O'Donoghue <bod@kernel.org>
>> Signed-off-by: David Heidelberg <david@ixit.cz>
>> ---
>> .../platform/qcom/camss/camss-csiphy-3ph-1-0.c | 39 +++++++++++++++++-----
>> 1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>> index dac8d2ecf7995..fa24fc9706748 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>> @@ -9,16 +9,17 @@
>> */
>>
>> #include "camss.h"
>> #include "camss-csiphy.h"
>>
>> #include <linux/delay.h>
>> #include <linux/interrupt.h>
>> #include <linux/io.h>
>> +#include <linux/media-bus-format.h>
>>
>> #define CSIPHY_3PH_LNn_CFG1(n) (0x000 + 0x100 * (n))
>> #define CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG (BIT(7) | BIT(6))
>> #define CSIPHY_3PH_LNn_CFG2(n) (0x004 + 0x100 * (n))
>> #define CSIPHY_3PH_LNn_CFG2_LP_REC_EN_INT BIT(3)
>> #define CSIPHY_3PH_LNn_CFG3(n) (0x008 + 0x100 * (n))
>> #define CSIPHY_3PH_LNn_CFG4(n) (0x00c + 0x100 * (n))
>> #define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS 0xa4
>> @@ -1108,23 +1109,32 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>> writel_relaxed(val, csiphy->base + r->reg_addr);
>> if (r->delay_us)
>> udelay(r->delay_us);
>> }
>> }
>>
>> static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
>> {
>> - u8 lane_mask;
>> - int i;
>> + u8 lane_mask = 0;
>> + u8 offset = 0;
>>
>> - lane_mask = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>> + switch (lane_cfg->phy_cfg) {
>> + case V4L2_MBUS_CSI2_CPHY:
>> + offset = 1;
>> + break;
>> + case V4L2_MBUS_CSI2_DPHY:
>> + lane_mask = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>> + break;
>> + default:
>> + break;
>> + }
>>
>> - for (i = 0; i < lane_cfg->num_data; i++)
>> - lane_mask |= 1 << lane_cfg->data[i].pos;
>> + for (int i = 0; i < lane_cfg->num_data; i++)
>> + lane_mask |= BIT(lane_cfg->data[i].pos + offset);
>>
>> return lane_mask;
>> }
>>
>> static bool csiphy_is_gen2(u32 version)
>> {
>> bool ret = false;
>>
>> @@ -1155,19 +1165,32 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>> struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>> struct csiphy_device_regs *regs = csiphy->regs;
>> u8 settle_cnt;
>> u8 val;
>> int i;
>>
>> settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>>
>> - val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>> - for (i = 0; i < c->num_data; i++)
>> - val |= BIT(c->data[i].pos * 2);
>> + val = 0;
>> +
>> + switch (c->phy_cfg) {
>> + case V4L2_MBUS_CSI2_CPHY:
>> + for (i = 0; i < c->num_data; i++)
>> + val |= BIT((c->data[i].pos * 2) + 1);
>> + break;
>> + case V4L2_MBUS_CSI2_DPHY:
>> + val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>> +
>> + for (i = 0; i < c->num_data; i++)
>> + val |= BIT(c->data[i].pos * 2);
>> + break;
>> + default:
>> + WARN_ONCE(1, "Unsupported bus type %d!\n", c->phy_cfg);
>> + }
>
> can you use similar method as csiphy_get_lane_mask()
>
Done, look better, but I guess we seriously need the refactor/split done by
Bryan as I looking at the current code.
David
> Frank
>>
>> writel_relaxed(val, csiphy->base +
>> CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
>>
>> val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;
>> writel_relaxed(val, csiphy->base +
>> CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
>>
>>
>> --
>> 2.53.0
>>
--
David Heidelberg
^ permalink raw reply
* Re: [RFC PATCH] dma-fence: Fix races of fence callbacks versus destructors by locking
From: Danilo Krummrich @ 2026-06-08 19:25 UTC (permalink / raw)
To: Christian König
Cc: phasta, Sumit Semwal, Boris Brezillon, Alice Ryhl, Daniel Almeida,
Gary Guo, Tvrtko Ursulin, linux-media, dri-devel, linux-kernel
In-Reply-To: <c8564ea0-8ff4-4049-996d-bd978c478372@amd.com>
On Mon Jun 8, 2026 at 8:47 PM CEST, Christian König wrote:
> On 6/8/26 20:39, Danilo Krummrich wrote:
>> On Mon Jun 8, 2026 at 8:32 PM CEST, Christian König wrote:
>>> On 6/8/26 19:59, Danilo Krummrich wrote:
>>>> On Mon Jun 8, 2026 at 7:34 PM CEST, Christian König wrote:
>>>>> That's why we need the RCU grace period to make sure that nobody is
>>>>> referencing the driver stuff any more.
>>>>
>>>> Right, and that's what Philipp tries to address, the requirement to wait for an
>>>> RCU grace period is perfectly fine if it is only about freeing memory, but it
>>>> can become painful if the fence private data contains data also needs to be
>>>> destructed in some way.
>>>
>>> Yeah that makes sense.
>>>
>>>> IOW, if a driver signals a fence, it is lifecycle-wise reasonable to destruct
>>>> the private data that is no longer needed (remaining users only deal with struct
>>>> dma_fence) and having to wait for a full grace period adds sublety and
>>>> complication that can be avoided with the proposed approach.
>>>
>>> Yeah, I've run into that when I tried to make the amdgpu fences independent as well.
>>>> That said, I'd like to ask the opposite question: What are the concerns with the
>>>> proposed approach over (pure) RCU?
>>>
>>> Well a) locking inversions and b) performance.
>>>
>>> For example the reason why we have the dma_fence_is_signaled() and
>>> dma_fence_is_signaled_locked() variants is because there is a measurable
>>> difference in some specific use cases for not grabbing the locks.
>>
>> I checked for this as well, but couldn't find a case where
>> dma_fence_is_signaled() is used in a way where it would be performance critical
>> to avoid the lock in any way.
>>
>> Note that the lock is only bypassed when the fence is signaled already (this
>> would be preserved) and if signaled() returns false, i.e. dma_fence_signal()
>> will take the lock anyways.
>>
>>> I personally find those micro-optimizations rather questionable, but the
>>> community agreement is that we should have them.
>>
>> I agree, it is rather questionable. So, I wouldn't make this the deciding factor
>> unless someone can present a valid case where it actually matters.
>>
>>> So my take would rather be that the dma_fence_is_signaled_locked() variant
>>> goes away and we consistently call the ops pointers without holding the
>>> dma_fence lock and the driver implementations can then optionally take it if
>>> necessary.
>>
>> How did you get to this conclusion considering that you run into what I
>> mentioned above as well and the fact that we seem to agree that the performance
>> concern is rather questionable?
>
> Quite simple, it's the cleaner approach.
I would maybe agree iff the RCU read side critical section wouldn't be needed
and we wouldn't need to deal with the consequences of having to defer
everything.
And so far it seems to me that there isn't really any other reason that the
performance concern we both don't buy into.
> Calling callbacks with locks held is rather questionable even putting the
> performance issue aside.
In general, I don't think that more flexibility for drivers is automatically
always superior.
Also, before we keep calling it a performance issue, I'd really love to know
where dma_fence_is_signaled() is called in a case where it returns false and the
spinlock causes such an overhead that it actually matters.
(As mentioned above, none of the cases where it returns true would change.)
> In detail calling the callbacks without holding locks allows all
> implementations who need it to explicitly take locks in the order they want.
I don't think this is true in this case.
1) The existence of dma_fence_is_signaled_locked() already mandates that all
such callbacks must work properly if called with the fence lock held.
2) The RCU read side critical section already mandates that driver must not
sleep within the callback.
> If you call it with the lock held you enforce the fence lock the be the
> outermost lock.
That's practically already the case, due to dma_fence_is_signaled_locked().
^ permalink raw reply
* Re: [PATCH v5] dma-buf: Fix silent overflow for phys vec to sgt
From: David Hu @ 2026-06-08 18:54 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Sumit Semwal, Christian König, Jason Gunthorpe, Nicolin Chen,
Kevin Tian, Ankit Agrawal, Alex Williamson, linux-media,
dri-devel, linaro-mm-sig, linux-kernel, iommu, jmoroni, praan,
stable
In-Reply-To: <20260607080244.GA327369@unreal>
On Sun, Jun 7, 2026 at 4:02 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Jun 04, 2026 at 03:36:48PM -0400, David Hu wrote:
> > On Thu, Jun 4, 2026 at 5:43 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Mon, Jun 01, 2026 at 08:00:12PM +0000, David Hu wrote:
> > > > @@ -133,6 +137,11 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct dma_buf_attachment *attach,
> > > > }
> > > >
> > > > nents = calc_sg_nents(dma->state, phys_vec, nr_ranges, size);
> > > > + if (!nents) {
> > > > + ret = -EINVAL;
> > > > + goto err_free_state;
> > > > + }
> > >
> > > Technically, this hunk is not necessary, since sg_alloc_table() will
> > > return -EINVAL when nents == 0. At least, that is the behavior I relied on.
> >
> > I originally added this explicit check in v5 to address Jason's
> > feedback, and to make the
> > failure explicit rather than relying on `sg_alloc_table()` failing
> > silently on `nents=0`.
>
> I prefer explicit checks, but I am not in favor of duplicating them.
> Since sg_alloc_table() already validates this condition, we do not need
> to repeat the same check in dma-buf. A comment should be sufficient to
> inform future reviewers that nents == 0 is already handled.
>
> Thanks
Hi Leon,
Thank you for clarifying this further. Removing the duplication here
sounds good to
me. I'll drop the hunk, add a comment for posterity noting that `nents
= 0` is
handled by `sg_alloc_table()`, and send out a v6 shortly.
Thanks a bunch,
David
^ permalink raw reply
* Re: [PATCH v19 0/4] Rust bindings for gem shmem
From: Danilo Krummrich @ 2026-06-08 18:50 UTC (permalink / raw)
To: Lyude Paul
Cc: dri-devel, rust-for-linux, nouveau, Alexandre Courbot, Gary Guo,
Christian König, driver-core, Miguel Ojeda,
Maarten Lankhorst, Alice Ryhl, Simona Vetter, linux-kernel,
Sumit Semwal, linux-media, Rafael J . Wysocki, Thomas Zimmermann,
Maxime Ripard, David Airlie, Benno Lossin, linaro-mm-sig,
Mukesh Kumar Chaurasiya, Asahi Lina, Daniel Almeida,
Greg Kroah-Hartman
In-Reply-To: <20260608183057.2001376-1-lyude@redhat.com>
On Mon Jun 8, 2026 at 8:29 PM CEST, Lyude Paul wrote:
> Lyude Paul (4):
> rust: drm: gem: shmem: Add DmaResvGuard helper
> rust: drm: gem: shmem: Add vmap functions
> rust: faux: Allow retrieving a bound Device
> rust: drm: gem: Introduce shmem::Object::sg_table()
Acked-by: Danilo Krummrich <dakr@kernel.org>
^ permalink raw reply
* Re: [RFC PATCH] dma-fence: Fix races of fence callbacks versus destructors by locking
From: Christian König @ 2026-06-08 18:47 UTC (permalink / raw)
To: Danilo Krummrich
Cc: phasta, Sumit Semwal, Boris Brezillon, Alice Ryhl, Daniel Almeida,
Gary Guo, Tvrtko Ursulin, linux-media, dri-devel, linux-kernel
In-Reply-To: <DJ3VYD71HDQ2.3C8GG983Z2YCM@kernel.org>
On 6/8/26 20:39, Danilo Krummrich wrote:
> On Mon Jun 8, 2026 at 8:32 PM CEST, Christian König wrote:
>> On 6/8/26 19:59, Danilo Krummrich wrote:
>>> On Mon Jun 8, 2026 at 7:34 PM CEST, Christian König wrote:
>>>> That's why we need the RCU grace period to make sure that nobody is
>>>> referencing the driver stuff any more.
>>>
>>> Right, and that's what Philipp tries to address, the requirement to wait for an
>>> RCU grace period is perfectly fine if it is only about freeing memory, but it
>>> can become painful if the fence private data contains data also needs to be
>>> destructed in some way.
>>
>> Yeah that makes sense.
>>
>>> IOW, if a driver signals a fence, it is lifecycle-wise reasonable to destruct
>>> the private data that is no longer needed (remaining users only deal with struct
>>> dma_fence) and having to wait for a full grace period adds sublety and
>>> complication that can be avoided with the proposed approach.
>>
>> Yeah, I've run into that when I tried to make the amdgpu fences independent as well.
>>> That said, I'd like to ask the opposite question: What are the concerns with the
>>> proposed approach over (pure) RCU?
>>
>> Well a) locking inversions and b) performance.
>>
>> For example the reason why we have the dma_fence_is_signaled() and
>> dma_fence_is_signaled_locked() variants is because there is a measurable
>> difference in some specific use cases for not grabbing the locks.
>
> I checked for this as well, but couldn't find a case where
> dma_fence_is_signaled() is used in a way where it would be performance critical
> to avoid the lock in any way.
>
> Note that the lock is only bypassed when the fence is signaled already (this
> would be preserved) and if signaled() returns false, i.e. dma_fence_signal()
> will take the lock anyways.
>
>> I personally find those micro-optimizations rather questionable, but the
>> community agreement is that we should have them.
>
> I agree, it is rather questionable. So, I wouldn't make this the deciding factor
> unless someone can present a valid case where it actually matters.
>
>> So my take would rather be that the dma_fence_is_signaled_locked() variant
>> goes away and we consistently call the ops pointers without holding the
>> dma_fence lock and the driver implementations can then optionally take it if
>> necessary.
>
> How did you get to this conclusion considering that you run into what I
> mentioned above as well and the fact that we seem to agree that the performance
> concern is rather questionable?
Quite simple, it's the cleaner approach.
Calling callbacks with locks held is rather questionable even putting the performance issue aside.
In detail calling the callbacks without holding locks allows all implementations who need it to explicitly take locks in the order they want.
If you call it with the lock held you enforce the fence lock the be the outermost lock.
Regards,
Christian.
^ permalink raw reply
* Re: [RFC PATCH] dma-fence: Fix races of fence callbacks versus destructors by locking
From: Danilo Krummrich @ 2026-06-08 18:39 UTC (permalink / raw)
To: Christian König
Cc: phasta, Sumit Semwal, Boris Brezillon, Alice Ryhl, Daniel Almeida,
Gary Guo, Tvrtko Ursulin, linux-media, dri-devel, linux-kernel
In-Reply-To: <ea4e0541-3702-4014-b8f6-0746a148df86@amd.com>
On Mon Jun 8, 2026 at 8:32 PM CEST, Christian König wrote:
> On 6/8/26 19:59, Danilo Krummrich wrote:
>> On Mon Jun 8, 2026 at 7:34 PM CEST, Christian König wrote:
>>> That's why we need the RCU grace period to make sure that nobody is
>>> referencing the driver stuff any more.
>>
>> Right, and that's what Philipp tries to address, the requirement to wait for an
>> RCU grace period is perfectly fine if it is only about freeing memory, but it
>> can become painful if the fence private data contains data also needs to be
>> destructed in some way.
>
> Yeah that makes sense.
>
>> IOW, if a driver signals a fence, it is lifecycle-wise reasonable to destruct
>> the private data that is no longer needed (remaining users only deal with struct
>> dma_fence) and having to wait for a full grace period adds sublety and
>> complication that can be avoided with the proposed approach.
>
> Yeah, I've run into that when I tried to make the amdgpu fences independent as well.
>> That said, I'd like to ask the opposite question: What are the concerns with the
>> proposed approach over (pure) RCU?
>
> Well a) locking inversions and b) performance.
>
> For example the reason why we have the dma_fence_is_signaled() and
> dma_fence_is_signaled_locked() variants is because there is a measurable
> difference in some specific use cases for not grabbing the locks.
I checked for this as well, but couldn't find a case where
dma_fence_is_signaled() is used in a way where it would be performance critical
to avoid the lock in any way.
Note that the lock is only bypassed when the fence is signaled already (this
would be preserved) and if signaled() returns false, i.e. dma_fence_signal()
will take the lock anyways.
> I personally find those micro-optimizations rather questionable, but the
> community agreement is that we should have them.
I agree, it is rather questionable. So, I wouldn't make this the deciding factor
unless someone can present a valid case where it actually matters.
> So my take would rather be that the dma_fence_is_signaled_locked() variant
> goes away and we consistently call the ops pointers without holding the
> dma_fence lock and the driver implementations can then optionally take it if
> necessary.
How did you get to this conclusion considering that you run into what I
mentioned above as well and the fact that we seem to agree that the performance
concern is rather questionable?
^ permalink raw reply
* Re: [RFC PATCH] dma-fence: Fix races of fence callbacks versus destructors by locking
From: Christian König @ 2026-06-08 18:32 UTC (permalink / raw)
To: Danilo Krummrich
Cc: phasta, Sumit Semwal, Boris Brezillon, Alice Ryhl, Daniel Almeida,
Gary Guo, Tvrtko Ursulin, linux-media, dri-devel, linux-kernel
In-Reply-To: <DJ3V3OCLIK2K.3CYKWYNHYU6JQ@kernel.org>
On 6/8/26 19:59, Danilo Krummrich wrote:
> On Mon Jun 8, 2026 at 7:34 PM CEST, Christian König wrote:
>> That's why we need the RCU grace period to make sure that nobody is
>> referencing the driver stuff any more.
>
> Right, and that's what Philipp tries to address, the requirement to wait for an
> RCU grace period is perfectly fine if it is only about freeing memory, but it
> can become painful if the fence private data contains data also needs to be
> destructed in some way.
Yeah that makes sense.
> IOW, if a driver signals a fence, it is lifecycle-wise reasonable to destruct
> the private data that is no longer needed (remaining users only deal with struct
> dma_fence) and having to wait for a full grace period adds sublety and
> complication that can be avoided with the proposed approach.
Yeah, I've run into that when I tried to make the amdgpu fences independent as well.
> That said, I'd like to ask the opposite question: What are the concerns with the
> proposed approach over (pure) RCU?
Well a) locking inversions and b) performance.
For example the reason why we have the dma_fence_is_signaled() and dma_fence_is_signaled_locked() variants is because there is a measurable difference in some specific use cases for not grabbing the locks.
I personally find those micro-optimizations rather questionable, but the community agreement is that we should have them.
So my take would rather be that the dma_fence_is_signaled_locked() variant goes away and we consistently call the ops pointers without holding the dma_fence lock and the driver implementations can then optionally take it if necessary.
I think for this we would just need to replace most calls to dma_fence_is_signaled_locked() with dma_fence_test_signaled().
In the long term that would also allow cleaning up the container handling and simplifying the DRM scheduler a bit.
Regards,
Christian.
^ permalink raw reply
* [PATCH v19 4/4] rust: drm: gem: Introduce shmem::Object::sg_table()
From: Lyude Paul @ 2026-06-08 18:29 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
In-Reply-To: <20260608183057.2001376-1-lyude@redhat.com>
In order to do this, we need to be careful to ensure that any interface we
expose for scatterlists ensures that any mappings created from one are
destroyed on driver-unbind. To do this, we introduce a Devres resource into
shmem::Object that we use in order to ensure that we release any SGTable
mappings on driver-unbind.
There's some other slightly unfortunate caveats of this:
* Drivers don't have explicit control at the moment over when unmapping
happens (which is exactly the same as the C side atm, so it might not be
a problem).
* We can't just return `SGTableMap` to the user through an Arc to attempt
to fix the last caveat - because that implies the gem object would need
to hold a reference count to the scatterlist mapping, which just leaves
us with the same problem.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the
SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
renaming this to shmem::SGTable makes things less confusing.
We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is
thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are
unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
+ SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to
hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and
owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to
sg_table().
* Implement Deborah's suggestion to fix double-free in
free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had
before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before)
using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a
future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid
Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment
V16:
* Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended
that is a lot cleaner.
* Fix horrid rebasing mistake
V17:
* Rebase
* Fix missing safety comment in free_callback() (we forgot to justify why
&mut is safe in `unsafe { &mut (*this).sgt_res }.reset()`)
V18:
* Use ManuallyDrop instead of SetOnce::reset()
V19:
* Explain that populate() will always return true in sg_table()
* Fix outdated comment in SGTableMap
* Fix invariant comment in SGTableMap
rust/kernel/drm/gem/shmem.rs | 174 +++++++++++++++++++++++++++++++++--
1 file changed, 164 insertions(+), 10 deletions(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index ffaa00a1af109..91441252482da 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -11,6 +11,11 @@
use crate::{
container_of,
+ device::{
+ self,
+ Bound, //
+ },
+ devres::*,
drm::{
driver,
gem,
@@ -19,14 +24,23 @@
DeviceContext,
Registered, //
},
- error::to_result,
+ error::{
+ from_err_ptr,
+ to_result, //
+ },
io::{
Io,
IoCapable,
IoKnownSize, //
},
prelude::*,
- sync::aref::ARef,
+ scatterlist,
+ sync::{
+ aref::ARef,
+ new_mutex,
+ Mutex,
+ SetOnce, //
+ },
types::{
NotThreadSafe,
Opaque, //
@@ -35,7 +49,10 @@
use core::{
ffi::c_void,
marker::PhantomData,
- mem::MaybeUninit, //
+ mem::{
+ ManuallyDrop,
+ MaybeUninit, //
+ },
ops::{
Deref,
DerefMut, //
@@ -90,6 +107,11 @@ pub struct Object<T: DriverObject, C: DeviceContext = Registered> {
obj: Opaque<bindings::drm_gem_shmem_object>,
/// Parent object that owns this object's DMA reservation object.
parent_resv_obj: Option<ARef<Object<T, C>>>,
+ /// Devres object for unmapping any SGTable on driver-unbind.
+ sgt_res: ManuallyDrop<SetOnce<Devres<SGTableMap<T, C>>>>,
+ #[pin]
+ /// Lock for protecting initialization of `sgt_res`.
+ sgt_lock: Mutex<()>,
#[pin]
inner: T,
_ctx: PhantomData<C>,
@@ -148,6 +170,8 @@ pub fn new(
try_pin_init!(Self {
obj <- Opaque::init_zeroed(),
parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
+ sgt_res: ManuallyDrop::new(SetOnce::new()),
+ sgt_lock <- new_mutex!(()),
inner <- T::new(dev, size, args),
_ctx: PhantomData::<C>,
}),
@@ -192,18 +216,26 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// - DRM always passes a valid gem object here
// - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
// `obj` is contained within a drm_gem_shmem_object
- let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
- // SAFETY:
- // - We're in free_callback - so this function is safe to call.
- // - We won't be using the gem resources on `this` after this call.
- unsafe { bindings::drm_gem_shmem_release(this) };
+ let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
// SAFETY:
// - We verified above that `obj` is valid, which makes `this` valid
// - This function is set in AllocOps, so we know that `this` is contained within a
// `Object<T, C>`
- let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+ let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
+
+ // We need to drop `sgt_res` first, since doing so requires that the GEM object is still
+ // alive.
+ // SAFETY:
+ // - We verified above that `this` is valid.
+ // - We are in free_callback, guaranteeing we have exclusive access to `this` and that
+ // `sgt_res` will not be used after dropping it here.
+ unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) };
+
+ // SAFETY:
+ // - We're in free_callback - so this function is safe to call.
+ // - We won't be using the gem resources on `this` after this call.
+ unsafe { bindings::drm_gem_shmem_release(base) };
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
@@ -279,6 +311,46 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, SIZE>> {
self.make_vmap()
}
+
+ /// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA
+ /// pages for this object.
+ ///
+ /// This will pin the object in memory. It is expected that `dev` should be a pointer to the
+ /// same [`device::Device`] which `self` belongs to, otherwise this function will return
+ /// `Err(EINVAL)`.
+ pub fn sg_table<'a>(
+ &'a self,
+ dev: &'a device::Device<Bound>,
+ ) -> Result<&'a scatterlist::SGTable> {
+ if dev.as_raw() != self.dev().as_ref().as_raw() {
+ return Err(EINVAL);
+ }
+
+ let sgt_res = 'out: {
+ // Fast path: sgt_res is already initialized
+ if let Some(sgt_res) = self.sgt_res.as_ref() {
+ break 'out sgt_res;
+ }
+
+ // Slow path: Grab the lock and see if we need to initialize sgt_res.
+ let _guard = self.sgt_lock.lock();
+
+ // If someone initialized it while we were waiting, we can exit early.
+ if let Some(sgt_res) = self.sgt_res.as_ref() {
+ break 'out sgt_res;
+ }
+
+ // If not, finish initializing and return. `populate()` cannot return false, as
+ // `sgt_res` must be unpopulated, and we must hold `sgt_lock` to reach this point.
+ self.sgt_res
+ .populate(Devres::new(dev, SGTableMap::new(self))?);
+
+ // SAFETY: We just populated sgt_res above.
+ unsafe { self.sgt_res.as_ref().unwrap_unchecked() }
+ };
+
+ Ok(sgt_res.access(dev)?)
+ }
}
impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> {
@@ -477,6 +549,64 @@ unsafe fn io_write(&self, value: $ty, address: usize) {
#[cfg(CONFIG_64BIT)]
impl_vmap_io_capable!(u64);
+/// A reference to a GEM object that is known to have a mapped [`SGTable`].
+///
+/// This is used by the Rust bindings with [`Devres`] in order to ensure that mappings for SGTables
+/// on GEM shmem objects are revoked on driver-unbind.
+///
+/// # Invariants
+///
+/// - `self.obj` always points to a valid GEM object.
+/// - This object is proof that `self.obj.owner.sgt_res` has an initialized and valid pointer to an
+/// [`SGTable`].
+///
+/// [`SGTable`]: scatterlist::SGTable
+pub struct SGTableMap<T: DriverObject, C: DeviceContext> {
+ obj: NonNull<Object<T, C>>,
+}
+
+impl<T: DriverObject, C: DeviceContext> Deref for SGTableMap<T, C> {
+ type Target = scatterlist::SGTable;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY:
+ // - The NonNull is guaranteed to be valid via our type invariants.
+ // - The sgt field is guaranteed to be initialized and valid via our type invariants.
+ unsafe { scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) }
+ }
+}
+
+impl<T: DriverObject, C: DeviceContext> Drop for SGTableMap<T, C> {
+ fn drop(&mut self) {
+ // SAFETY: `obj` is always valid via our type invariants
+ let obj = unsafe { self.obj.as_ref() };
+ let _lock = DmaResvGuard::new(obj);
+
+ // SAFETY: We acquired the lock needed for calling this function above
+ unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
+ }
+}
+
+impl<T: DriverObject, C: DeviceContext> SGTableMap<T, C> {
+ fn new(obj: &Object<T, C>) -> impl Init<Self, Error> {
+ // INVARIANT:
+ // - We call drm_gem_shmem_get_pages_sgt below and check whether or not it succeeds,
+ // fulfilling the invariant of SGTableMap that the object's `sgt` field is initialized.
+ // SAFETY:
+ // - `obj` is fully initialized, making this function safe to call.
+ from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+
+ Ok(Self { obj: obj.into() })
+ }
+}
+
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject, C: DeviceContext> Send for SGTableMap<T, C> {}
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject, C: DeviceContext> Sync for SGTableMap<T, C> {}
+
#[kunit_tests(rust_drm_gem_shmem)]
mod tests {
use super::*;
@@ -591,4 +721,28 @@ fn vmap_io() -> Result {
Ok(())
}
+
+ // TODO: I would love to actually test the success paths of sg_table(), but that would require
+ // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
+ // that for someone else.
+
+ // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
+ // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
+ // bound to it.
+ #[test]
+ fn fail_sg_table_on_wrong_dev() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+ let wrong_dev = faux::Registration::new(c"EvilKunit", None)?;
+
+ let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
+ assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+ // TODO: Someday, we should test that creating an sg_table here still succeeds.
+
+ Ok(())
+ }
}
--
2.54.0
^ permalink raw reply related
* [PATCH v19 3/4] rust: faux: Allow retrieving a bound Device
From: Lyude Paul @ 2026-06-08 18:29 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
In-Reply-To: <20260608183057.2001376-1-lyude@redhat.com>
When writing up some rust code that used faux devices for unit testing, I
noticed that we never actually added the Bound device context to
faux::Registration's AsRef<device::Device> implementation. This being said:
the Registration object itself is proof that a driver is bound to the
device - so this should be safe.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
V18:
- Add notes from Danilo to safety comment.
rust/kernel/faux.rs | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
index 43b4974f48cd2..20ab638885354 100644
--- a/rust/kernel/faux.rs
+++ b/rust/kernel/faux.rs
@@ -25,7 +25,8 @@
///
/// # Invariants
///
-/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
+/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
+/// - This object is proof that the object described by this `Registration` is bound to a device.
///
/// [`struct faux_device`]: srctree/include/linux/device/faux.h
pub struct Registration(NonNull<bindings::faux_device>);
@@ -59,10 +60,15 @@ fn as_raw(&self) -> *mut bindings::faux_device {
}
}
-impl AsRef<device::Device> for Registration {
- fn as_ref(&self) -> &device::Device {
- // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be
- // a valid initialized `device`.
+impl AsRef<device::Device<device::Bound>> for Registration {
+ fn as_ref(&self) -> &device::Device<device::Bound> {
+ // SAFETY:
+ // - The underlying `device` in `faux_device` is guaranteed by the C API to be a valid
+ // initialized `device`.
+ // - faux_match() always returns 1, and probe runs synchronously (PROBE_FORCE_SYNCHRONOUS).
+ // - suppress_bind_attrs = true on faux_driver prevents userspace-triggered unbind via sysfs
+ // - mem::forget(Registration) is not a problem; if the Registration is leaked, the faux
+ // device stays bound forever.
unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }
}
}
--
2.54.0
^ permalink raw reply related
* [PATCH v19 2/4] rust: drm: gem: shmem: Add vmap functions
From: Lyude Paul @ 2026-06-08 18:29 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
In-Reply-To: <20260608183057.2001376-1-lyude@redhat.com>
One of the more obvious use cases for gem shmem objects is the ability to
create mappings into their contents. So, let's hook this up in our rust
bindings.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
V7:
* Switch over to the new iosys map bindings that use the Io trait
V8:
* Get rid of iosys_map bindings for now, only support non-iomem types
* s/as_shmem()/as_raw_shmem()
V9:
* Get rid of some outdated comments I missed
* Add missing SIZE check to raw_vmap()
* Add a proper unit test that ensures that we actually validate SIZE at
compile-time.
Turns out it takes only 34 lines to make a boilerplate DRM driver for a
kunit test :)
* Add unit tests
* Add some missing #[inline]s
V10:
* Correct issue with iomem error path
We previously called raw_vunmap() if we got an iomem allocation, but
raw_vunmap() was written such that it assumed all allocations were sysmem
allocations. Fix this by just making raw_vunmap() accept a iosys_map.
V11:
* Use Alexandre's clever solution to remove the macros we were using for
maintaining two different VMap types.
* Change the order of items in Object<T> to ensure that sgt_res is always
dropped before obj.
* Fix typo in Object.raw_vmap()
* s/raw_vmap()/make_vmap()/
Deduplicate code a bit more as well by using more generics here
V15:
* Add these patches back
* We only have one VMap type now!
* Use ObjectConfig::default() in unit tests since we unbroke it.
V16:
* Fix huge rebase error I made and did not notice that squashed 1.5 patches
together that were definitely not supposed to be squashed
* Update old commit message
V17:
* Rebase
* Fix format of commit message title
V20:
* Drop outdated safety comment
* Move impl_vmap_io_capable! definition to right before it gets used
* Add missing `` in rustdoc for VMap type
* Add a bunch of missing `` in make_vmap()
* Remove one outdated safety comment about reading vaddr_iomem
* Add some missing periods in safety comments in make_vmap().
* Use read_volatile/write_volatile() instead of read()/write() to prevent
compiler reordering.
* Remove impl argument from impl_vmap_io_capable!()
* Check .owner() and .maxsize() in compile_time_vmap_sizes()
* Use more varied pattern in vmap_io()
rust/kernel/drm/gem/shmem.rs | 315 ++++++++++++++++++++++++++++++++++-
1 file changed, 314 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index da1acc223bad4..ffaa00a1af109 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -20,6 +20,11 @@
Registered, //
},
error::to_result,
+ io::{
+ Io,
+ IoCapable,
+ IoKnownSize, //
+ },
prelude::*,
sync::aref::ARef,
types::{
@@ -28,7 +33,9 @@
},
};
use core::{
+ ffi::c_void,
marker::PhantomData,
+ mem::MaybeUninit, //
ops::{
Deref,
DerefMut, //
@@ -39,6 +46,7 @@
},
};
use gem::{
+ BaseObject,
BaseObjectPrivate,
DriverObject,
IntoGEMObject, //
@@ -200,6 +208,77 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
// SAFETY: We're recovering the Kbox<> we created in gem_create_object()
let _ = unsafe { KBox::from_raw(this) };
}
+
+ /// Attempt to create a vmap from the gem object, and confirm the size of said vmap.
+ fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result<VMap<T, R, C, SIZE>>
+ where
+ R: Deref<Target = Self> + From<&'a Self>,
+ {
+ // INVARIANT: We check here that the gem object is at least as large as `SIZE`.
+ if self.size() < SIZE {
+ return Err(ENOSPC);
+ }
+
+ let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
+ let guard = DmaResvGuard::new(self);
+
+ // SAFETY: `drm_gem_shmem_vmap()` can be called with the DMA reservation lock held.
+ to_result(unsafe {
+ bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr())
+ })?;
+
+ // Drop the guard explicitly here, since we may need to call `raw_vunmap()` (which
+ // re-acquires the lock).
+ drop(guard);
+
+ // SAFETY: The call to `drm_gem_shmem_vmap_locked()` succeeded above, so we are guaranteed
+ // that map is properly initialized.
+ let map = unsafe { map.assume_init() };
+
+ // XXX: We don't currently support iomem allocations
+ if map.is_iomem {
+ // SAFETY: The vmap operation above succeeded, guaranteeing that `map` points to a valid
+ // memory mapping.
+ unsafe { self.raw_vunmap(map) };
+
+ Err(ENOTSUPP)
+ } else {
+ Ok(VMap {
+ // SAFETY: We checked that this is not an iomem allocation, making it safe to read
+ // vaddr.
+ addr: unsafe { map.__bindgen_anon_1.vaddr },
+ owner: self.into(),
+ })
+ }
+ }
+
+ /// Unmap a vmap from the gem object.
+ ///
+ /// # Safety
+ ///
+ /// - The caller promises that `map` is a valid vmap on this gem object.
+ /// - The caller promises that the memory pointed to by map will no longer be accesed through
+ /// this instance.
+ unsafe fn raw_vunmap(&self, mut map: bindings::iosys_map) {
+ let _guard = DmaResvGuard::new(self);
+
+ // SAFETY:
+ // - This function is safe to call with the DMA reservation lock held.
+ // - The caller promises that `map` is a valid vmap on this gem object.
+ unsafe { bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map) };
+ }
+
+ /// Creates and returns a virtual kernel memory mapping for this object.
+ #[inline]
+ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
+ self.make_vmap()
+ }
+
+ /// Creates and returns an owned reference to a virtual kernel memory mapping for this object.
+ #[inline]
+ pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, SIZE>> {
+ self.make_vmap()
+ }
}
impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> {
@@ -263,7 +342,6 @@ struct DmaResvGuard<'a, T: DriverObject, C: DeviceContext = Registered>(
impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> {
#[inline(always)]
- #[expect(unused)]
fn new(obj: &'a Object<T, C>) -> Self {
// SAFETY: This lock is initialized throughout the lifetime of `object`.
unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
@@ -279,3 +357,238 @@ fn drop(&mut self) {
unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
}
}
+
+/// A reference to a virtual mapping for an shmem-based GEM object in kernel address space.
+///
+/// # Invariants
+///
+/// - The size of `owner` is >= SIZE.
+/// - The memory pointed to by `addr` remains valid at least until this object is dropped.
+pub struct VMap<D, R, C = Registered, const SIZE: usize = 0>
+where
+ D: DriverObject,
+ C: DeviceContext,
+ R: Deref<Target = Object<D, C>>,
+{
+ addr: *mut c_void,
+ owner: R,
+}
+
+/// An alias type for a reference to a shmem-based GEM object's VMap.
+pub type VMapRef<'a, D, C, const SIZE: usize = 0> = VMap<D, &'a Object<D, C>, C, SIZE>;
+
+/// An alias type for an owned reference to a shmem-based GEM object's VMap.
+pub type VMapOwned<D, C, const SIZE: usize = 0> = VMap<D, ARef<Object<D, C>>, C, SIZE>;
+
+impl<D, R, C, const SIZE: usize> VMap<D, R, C, SIZE>
+where
+ D: DriverObject,
+ C: DeviceContext,
+ R: Deref<Target = Object<D, C>>,
+{
+ /// Borrows a reference to the object that owns this virtual mapping.
+ #[inline(always)]
+ pub fn owner(&self) -> &Object<D, C> {
+ &self.owner
+ }
+}
+
+impl<D, R, C, const SIZE: usize> Drop for VMap<D, R, C, SIZE>
+where
+ D: DriverObject,
+ C: DeviceContext,
+ R: Deref<Target = Object<D, C>>,
+{
+ #[inline(always)]
+ fn drop(&mut self) {
+ // SAFETY:
+ // - Our existence is proof that this map was previously created using self.owner.
+ // - Since we are in Drop, we are guaranteed that no one will access the memory
+ // through this mapping after calling this.
+ unsafe {
+ self.owner.raw_vunmap(bindings::iosys_map {
+ is_iomem: false,
+ __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr },
+ })
+ };
+ }
+}
+
+impl<D, R, C, const SIZE: usize> Io for VMap<D, R, C, SIZE>
+where
+ D: DriverObject,
+ C: DeviceContext,
+ R: Deref<Target = Object<D, C>>,
+{
+ #[inline(always)]
+ fn addr(&self) -> usize {
+ self.addr as usize
+ }
+
+ #[inline(always)]
+ fn maxsize(&self) -> usize {
+ self.owner.size()
+ }
+}
+
+impl<D, R, C, const SIZE: usize> IoKnownSize for VMap<D, R, C, SIZE>
+where
+ D: DriverObject,
+ C: DeviceContext,
+ R: Deref<Target = Object<D, C>>,
+{
+ const MIN_SIZE: usize = SIZE;
+}
+
+macro_rules! impl_vmap_io_capable {
+ ($ty:ty) => {
+ impl<D, R, C, const SIZE: usize> IoCapable<$ty> for VMap<D, R, C, SIZE>
+ where
+ D: DriverObject,
+ C: DeviceContext,
+ R: Deref<Target = Object<D, C>>,
+ {
+ #[inline(always)]
+ unsafe fn io_read(&self, address: usize) -> $ty {
+ let ptr = address as *mut $ty;
+
+ // SAFETY: The safety contract of `io_read` guarantees that address is a valid
+ // address within the bounds of `Self` of at least the size of $ty, and is properly
+ // aligned.
+ unsafe { ptr::read_volatile(ptr) }
+ }
+
+ #[inline(always)]
+ unsafe fn io_write(&self, value: $ty, address: usize) {
+ let ptr = address as *mut $ty;
+
+ // SAFETY: The safety contract of `io_write` guarantees that address is a valid
+ // address within the bounds of `Self` of at least the size of $ty, and is properly
+ // aligned.
+ unsafe { ptr::write_volatile(ptr, value) }
+ }
+ }
+ };
+}
+
+impl_vmap_io_capable!(u8);
+impl_vmap_io_capable!(u16);
+impl_vmap_io_capable!(u32);
+#[cfg(CONFIG_64BIT)]
+impl_vmap_io_capable!(u64);
+
+#[kunit_tests(rust_drm_gem_shmem)]
+mod tests {
+ use super::*;
+ use crate::{
+ drm::{
+ self,
+ UnregisteredDevice, //
+ },
+ faux,
+ page::PAGE_SIZE, //
+ };
+
+ // The bare minimum needed to create a fake drm driver for kunit
+
+ #[pin_data]
+ struct KunitData {}
+ struct KunitDriver;
+ struct KunitFile;
+ #[pin_data]
+ struct KunitObject {}
+
+ const INFO: drm::DriverInfo = drm::DriverInfo {
+ major: 0,
+ minor: 0,
+ patchlevel: 0,
+ name: c"kunit",
+ desc: c"Kunit",
+ };
+
+ impl drm::file::DriverFile for KunitFile {
+ type Driver = KunitDriver;
+
+ fn open(_dev: &drm::Device<KunitDriver>) -> Result<Pin<KBox<Self>>> {
+ Ok(KBox::new(Self, GFP_KERNEL)?.into())
+ }
+ }
+
+ impl gem::DriverObject for KunitObject {
+ type Driver = KunitDriver;
+ type Args = ();
+
+ fn new<C: DeviceContext>(
+ _dev: &drm::Device<KunitDriver, C>,
+ _size: usize,
+ _args: Self::Args,
+ ) -> impl PinInit<Self, Error> {
+ try_pin_init!(KunitObject {})
+ }
+ }
+
+ #[vtable]
+ impl drm::Driver for KunitDriver {
+ type Data = KunitData;
+ type File = KunitFile;
+ type Object<Ctx: DeviceContext> = Object<KunitObject, Ctx>;
+
+ const INFO: drm::DriverInfo = INFO;
+ const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[];
+ }
+
+ fn create_drm_dev() -> Result<(faux::Registration, UnregisteredDevice<KunitDriver>)> {
+ // Create a faux DRM device so we can test gem object creation.
+ let data = try_pin_init!(KunitData {});
+ let dev = faux::Registration::new(c"Kunit", None)?;
+ let drm = UnregisteredDevice::new(dev.as_ref(), data)?;
+
+ Ok((dev, drm))
+ }
+
+ #[test]
+ fn compile_time_vmap_sizes() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+
+ let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ // Try creating a normal vmap
+ obj.vmap::<PAGE_SIZE>()?;
+
+ // Try creating a vmap that's smaller then the size we specified
+ let vmap = obj.vmap::<{ PAGE_SIZE - 100 }>()?;
+
+ // Verify the owner matches
+ assert!(ptr::eq(vmap.owner(), obj.deref()));
+
+ // Verify the max size matches the actual object size
+ assert_eq!(vmap.maxsize(), PAGE_SIZE);
+
+ // Make sure creating a vmap that's too large fails
+ assert!(obj.vmap::<{ PAGE_SIZE + 200 }>().is_err());
+
+ Ok(())
+ }
+
+ #[test]
+ fn vmap_io() -> Result {
+ let (_dev, drm) = create_drm_dev()?;
+
+ let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+ let vmap = obj.vmap::<PAGE_SIZE>()?;
+
+ vmap.write8(0xDE, 0x0);
+ assert_eq!(vmap.read8(0x0), 0xDE);
+ vmap.write32(0xFEDCBA98, 0x20);
+
+ assert_eq!(vmap.read32(0x20), 0xFEDCBA98);
+
+ assert_eq!(vmap.read8(0x20), 0x98);
+ assert_eq!(vmap.read8(0x21), 0xBA);
+ assert_eq!(vmap.read8(0x22), 0xDC);
+ assert_eq!(vmap.read8(0x23), 0xFE);
+
+ Ok(())
+ }
+}
--
2.54.0
^ permalink raw reply related
* [PATCH v19 0/4] Rust bindings for gem shmem
From: Lyude Paul @ 2026-06-08 18:29 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
Most of this patch series has already been pushed upstream, this is just
the second half of the patch series that has not been pushed yet + some
additional changes which were required to implement changes requested by
the mailing list. This patch series is originally from Asahi, previously
posted by Daniel Almeida.
The previous version of the patch series can be found here:
https://patchwork.freedesktop.org/series/164580/
Branch with patches applied available here:
https://gitlab.freedesktop.org/lyudess/linux/-/commits/rust/gem-shmem
This patch series applies on top of drm-rust-next
Patch-series wide changes since V15:
* Fix some major rebasing errors I somehow didn't notice :(
* Drop the dependency on LazyInit, use the trick that Alice suggested
instead.
* Fix dependency ordering so that Tyr can get the vmap stuff first
without the other bits.
Patch-series wide changes since V16:
* Fix ordering one more time (SetOnce::reset() doesn't need to come
before adding vmap functions)
* Rebase against the latest DeviceContext changes from me that got
pushed.
Lyude Paul (4):
rust: drm: gem: shmem: Add DmaResvGuard helper
rust: drm: gem: shmem: Add vmap functions
rust: faux: Allow retrieving a bound Device
rust: drm: gem: Introduce shmem::Object::sg_table()
rust/kernel/drm/gem/shmem.rs | 524 ++++++++++++++++++++++++++++++++++-
rust/kernel/faux.rs | 16 +-
2 files changed, 524 insertions(+), 16 deletions(-)
base-commit: fea3a2dd7d3fc1936211ced5f84420e610435730
--
2.54.0
^ permalink raw reply
* [PATCH v19 1/4] rust: drm: gem: shmem: Add DmaResvGuard helper
From: Lyude Paul @ 2026-06-08 18:29 UTC (permalink / raw)
To: dri-devel, rust-for-linux, nouveau
Cc: Alexandre Courbot, Gary Guo, Christian König, driver-core,
Miguel Ojeda, Maarten Lankhorst, Alice Ryhl, Simona Vetter,
linux-kernel, Sumit Semwal, linux-media, Rafael J . Wysocki,
Thomas Zimmermann, Maxime Ripard, David Airlie, Benno Lossin,
linaro-mm-sig, Danilo Krummrich, Mukesh Kumar Chaurasiya,
Asahi Lina, Daniel Almeida, Lyude Paul, Greg Kroah-Hartman
In-Reply-To: <20260608183057.2001376-1-lyude@redhat.com>
Just a temporary holdover to make locking/unlocking the dma_resv lock much
easier.
Signed-off-by: Lyude Paul <lyude@redhat.com>
Co-authored-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
V17:
* Fix format of commit message title
V19:
* Add NotThreadSafe to DmaResvGuard
rust/kernel/drm/gem/shmem.rs | 39 ++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 084b798ce795b..da1acc223bad4 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -22,7 +22,10 @@
error::to_result,
prelude::*,
sync::aref::ARef,
- types::Opaque, //
+ types::{
+ NotThreadSafe,
+ Opaque, //
+ },
};
use core::{
marker::PhantomData,
@@ -30,7 +33,10 @@
Deref,
DerefMut, //
},
- ptr::NonNull, //
+ ptr::{
+ self,
+ NonNull, //
+ },
};
use gem::{
BaseObjectPrivate,
@@ -244,3 +250,32 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> {
dumb_map_offset: None,
};
}
+
+/// Private helper-type for holding the `dma_resv` object for a GEM shmem object.
+///
+/// When this is dropped, the `dma_resv` lock is dropped as well.
+///
+// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel.
+struct DmaResvGuard<'a, T: DriverObject, C: DeviceContext = Registered>(
+ &'a Object<T, C>,
+ NotThreadSafe,
+);
+
+impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> {
+ #[inline(always)]
+ #[expect(unused)]
+ fn new(obj: &'a Object<T, C>) -> Self {
+ // SAFETY: This lock is initialized throughout the lifetime of `object`.
+ unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
+
+ Self(obj, NotThreadSafe)
+ }
+}
+
+impl<'a, T: DriverObject, C: DeviceContext> Drop for DmaResvGuard<'a, T, C> {
+ #[inline(always)]
+ fn drop(&mut self) {
+ // SAFETY: We are releasing the lock grabbed during the creation of this object.
+ unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
+ }
+}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v7 3/8] media: qcom: camss: Prepare CSID for C-PHY support
From: Frank Li @ 2026-06-08 18:18 UTC (permalink / raw)
To: David Heidelberg
Cc: Robert Foss, Todor Tomov, Bryan O'Donoghue,
Bryan O'Donoghue, Vladimir Zapolskiy, Mauro Carvalho Chehab,
Luca Weiss, Petr Hodina, Dr. Git, Cory Keitz, Loic Poulain,
Frank Li, Konrad Dybcio, Kieran Bingham, Sakari Ailus,
linux-media, linux-arm-msm, linux-kernel, phone-devel
In-Reply-To: <20260605-qcom-cphy-v7-3-426c37e9008f@ixit.cz>
On Fri, Jun 05, 2026 at 03:14:41PM +0200, David Heidelberg wrote:
> Inherit C-PHY information from CSIPHY, so we can configure CSID
> properly.
>
> CSI2_RX_CFG0_PHY_TYPE_SEL must be set to 1, when C-PHY mode is used.
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Acked-by: Cory Keitz <ckeitz@amazon.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
> drivers/media/platform/qcom/camss/camss-csid-gen2.c | 1 +
> drivers/media/platform/qcom/camss/camss-csid.c | 5 +++++
> drivers/media/platform/qcom/camss/camss-csid.h | 6 ++++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> index eadcb2f7e3aaa..a5b406cc8ead3 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> @@ -178,16 +178,17 @@ static void __csid_configure_rx(struct csid_device *csid,
> int val;
>
> if (!lane_cnt)
> lane_cnt = 4;
>
> val = (lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
> val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
> val |= phy->csiphy_id << CSI2_RX_CFG0_PHY_NUM_SEL;
> + val |= csid->phy.phy_sel << CSI2_RX_CFG0_PHY_TYPE_SEL;
It is fine for now. Suggest change to FIELD_PREP() in future.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
>
> val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
> if (vc > 3)
> val |= 1 << CSI2_RX_CFG1_VC_MODE;
> val |= 1 << CSI2_RX_CFG1_MISR_EN;
> writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
> }
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 48459b46a981b..bcc34ac9dd212 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -1286,16 +1286,21 @@ static int csid_link_setup(struct media_entity *entity,
> /* do no allow a link from CSIPHY to CSID */
> if (!csiphy->cfg.csi2)
> return -EPERM;
>
> csid->phy.csiphy_id = csiphy->id;
>
> lane_cfg = &csiphy->cfg.csi2->lane_cfg;
> csid->phy.lane_cnt = lane_cfg->num_data;
> + if (lane_cfg->phy_cfg == V4L2_MBUS_CSI2_CPHY)
> + csid->phy.phy_sel = CSID_PHY_SEL_CPHY;
> + else
> + csid->phy.phy_sel = CSID_PHY_SEL_DPHY;
> +
> csid->phy.lane_assign = csid_get_lane_assign(lane_cfg, lane_cfg->num_data);
> csid->tpg_linked = false;
> }
> }
> /* Decide which virtual channels to enable based on which source pads are enabled */
> if (local->flags & MEDIA_PAD_FL_SOURCE) {
> struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> struct csid_device *csid = v4l2_get_subdevdata(sd);
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index 5296b10f6bac8..e65590b0df69f 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -39,16 +39,21 @@ enum csid_testgen_mode {
> CSID_PAYLOAD_MODE_USER_SPECIFIED = 6,
> CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN1 = 6, /* excluding disabled */
> CSID_PAYLOAD_MODE_COMPLEX_PATTERN = 7,
> CSID_PAYLOAD_MODE_COLOR_BOX = 8,
> CSID_PAYLOAD_MODE_COLOR_BARS = 9,
> CSID_PAYLOAD_MODE_NUM_SUPPORTED_GEN2 = 9, /* excluding disabled */
> };
>
> +enum csid_phy_sel {
> + CSID_PHY_SEL_DPHY = 0,
> + CSID_PHY_SEL_CPHY = 1
> +};
> +
> struct csid_format_info {
> u32 code;
> u8 data_type;
> u8 decode_format;
> u8 bpp;
> u8 spp; /* bus samples per pixel */
> };
>
> @@ -65,16 +70,17 @@ struct csid_testgen_config {
> };
>
> struct csid_phy_config {
> u8 csiphy_id;
> u8 lane_cnt;
> u32 lane_assign;
> u32 en_vc;
> u8 need_vc_update;
> + enum csid_phy_sel phy_sel;
> };
>
> struct csid_device;
>
> struct csid_hw_ops {
> /*
> * configure_stream - Configures and starts CSID input stream
> * @csid: CSID device
>
> --
> 2.53.0
>
^ permalink raw reply
page: next (older)
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox