Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Your subject is 87 columns long, better to squeeze it a bit.
On 05-09-22, 19:16, Clément Péron wrote:
> devm_pm_opp_set_regulators() doesn't enable regulator, which make
> regulator framework switching it off during regulator_late_cleanup().
This isn't the normal behavior as it works for everyone at the moment.
You need to explain what special you are doing here, because of which
you are reaching such a situation.
i.e. you are disabling some code that uses GPU ? Please specify exact
code so others can reproduce it as well.
> Call dev_pm_opp_set_opp() with the recommend OPP in
> panfrost_devfreq_init() to enable the regulator and avoid any switch off
> by regulator_late_cleanup().
The regulator is already enabled I think at this point by the
bootloader. What you are doing here is syncing the state of the
hardware with the software, which would disallow disabling of the
resource unnecessarily.
> Suggested-by: Viresh Kumar
> Signed-off-by: Clément Péron
> ---
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 5110cd9b2425..67b242407156 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> return PTR_ERR(opp);
>
> panfrost_devfreq_profile.initial_freq = cur_freq;
> +
> + /* Setup and enable regulator */
Similarly here, explain why this is required to be done.
> + ret = dev_pm_opp_set_opp(dev, opp);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> + return ret;
> + }
> +
> dev_pm_opp_put(opp);
Do this before checking if (ret), so the resource can be freed all the
time.
>
> /*
> --
> 2.34.1
--
viresh
Re: [PATCH] drm/bridge: anx7625: Set HPD irq detect window to 2ms
On Mon, Sep 05, 2022 at 06:48:06PM +0200, Robert Foss wrote:
> Hi Xin,
>
> On Sat, 3 Sept 2022 at 15:09, Xin Ji wrote:
> >
> > Some panels trigger HPD irq due to noise, the HPD debounce
> > may be 1.8ms, exceeding the default irq detect window, ~1.4ms.
> > This patch set HPD irq detection window to 2ms to
> > tolerate the HPD noise.
> >
> > Signed-off-by: Xin Ji
> > ---
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 14 ++
> > drivers/gpu/drm/bridge/analogix/anx7625.h | 6 ++
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index c74b5df4cade..0c323b5a1c99 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -1440,6 +1440,20 @@ static void anx7625_start_dp_work(struct
> > anx7625_data *ctx)
> >
> > static int anx7625_read_hpd_status_p0(struct anx7625_data *ctx)
> > {
> > + int ret;
> > +
> > + /* Set irq detect window to 2ms */
> > + ret = anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HPD_DET_TIMER_BIT0_7, HPD_TIME & 0xFF);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > +HPD_DET_TIMER_BIT8_15,
> > +(HPD_TIME >> 8) & 0xFF);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > +HPD_DET_TIMER_BIT16_23,
> > +(HPD_TIME >> 16) & 0xFF);
>
> Does the HPD debounce timer register need to be written for every HPD
> status read?
Hi Robert Foss, yes, it is better to set it in every HPD status check, because
the
HPD may be affected by noise, once the chip detect HPD is low, the timer
register will be automatically set to 1.4ms, so the driver better set it
in each check loop.
Thanks,
Xin
>
> > + if (ret < 0)
> > + return ret;
> > +
> > return anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, SYSTEM_STSTUS);
> > }
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index e257a84db962..14f33d6be289 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -132,6 +132,12 @@
> > #define I2S_SLAVE_MODE 0x08
> > #define AUDIO_LAYOUT 0x01
> >
> > +#define HPD_DET_TIMER_BIT0_7 0xea
> > +#define HPD_DET_TIMER_BIT8_15 0xeb
> > +#define HPD_DET_TIMER_BIT16_23 0xec
> > +/* HPD debounce time 2ms for 27M clock */
> > +#define HPD_TIME 54000
> > +
> > #define AUDIO_CONTROL_REGISTER 0xe6
> > #define TDM_TIMING_MODE 0x08
> >
> > --
> > 2.25.1
> >
Re: [PATCH 2/6] dt-bindings: reserved-memory: Support framebuffer reserved memory
On Mon, 05 Sep 2022 18:32:56 +0200, Thierry Reding wrote: > From: Thierry Reding > > Document the "framebuffer" compatible string for reserved memory nodes > to annotate reserved memory regions used for framebuffer carveouts. > > Signed-off-by: Thierry Reding > --- > .../bindings/reserved-memory/framebuffer.yaml | 46 +++ > 1 file changed, 46 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/reserved-memory/framebuffer.example.dts:18.16-23.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Re: [PATCH] drm/doc: Custom Kconfig for KUnit is no longer needed
On 9/6/22 01:47, Michał Winiarski wrote:
> References: commit 6fc3a8636a7b ("kunit: tool: Enable virtio/PCI by default
> on UML")
Use Fixes: tag for bugfix patches instead.
--
An old man doll... just what I always wanted! - Clara
[drm-misc:for-linux-next 3/9] drivers/gpu/drm/drm_atomic_helper.c:802: warning: expecting prototype for drm_atomic_helper_check_wb_connector_state(). Prototype was for drm_atomic_helper_check_wb_encod
tree: git://anongit.freedesktop.org/drm/drm-misc for-linux-next
head: 396369d6754993e40f1c84b2e22e40e92dfa4c49
commit: 254fe9c106ed69245fbe0beac582054c98a91482 [3/9] drm: drm_atomic_helper:
Add a new helper to deal with the writeback connector validation
config: i386-randconfig-a011-20220905
(https://download.01.org/0day-ci/archive/20220906/202209060828.2q6b2ehu-...@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project
f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add drm-misc git://anongit.freedesktop.org/drm/drm-misc
git fetch --no-tags drm-misc for-linux-next
git checkout 254fe9c106ed69245fbe0beac582054c98a91482
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_atomic_helper.c:802: warning: expecting prototype for
>> drm_atomic_helper_check_wb_connector_state(). Prototype was for
>> drm_atomic_helper_check_wb_encoder_state() instead
vim +802 drivers/gpu/drm/drm_atomic_helper.c
787
788 /**
789 * drm_atomic_helper_check_wb_connector_state() - Check writeback
encoder state
790 * @encoder: encoder state to check
791 * @conn_state: connector state to check
792 *
793 * Checks if the writeback connector state is valid, and returns an
error if it
794 * isn't.
795 *
796 * RETURNS:
797 * Zero for success or -errno
798 */
799 int
800 drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
801 struct drm_connector_state
*conn_state)
> 802 {
803 struct drm_writeback_job *wb_job = conn_state->writeback_job;
804 struct drm_property_blob *pixel_format_blob;
805 struct drm_framebuffer *fb;
806 size_t i, nformats;
807 u32 *formats;
808
809 if (!wb_job || !wb_job->fb)
810 return 0;
811
812 pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
813 nformats = pixel_format_blob->length / sizeof(u32);
814 formats = pixel_format_blob->data;
815 fb = wb_job->fb;
816
817 for (i = 0; i < nformats; i++)
818 if (fb->format->format == formats[i])
819 return 0;
820
821 drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n",
>format->format);
822
823 return -EINVAL;
824 }
825 EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
826
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Re: [PATCH v17 00/10] Add MT8195 DisplayPort driver
Hi, Dmitry: Dmitry Osipenko 於 2022年9月5日 週一 下午6:53寫道： > > On 9/4/22 15:59, Dmitry Osipenko wrote: > > 01.09.2022 07:41, Bo-Chen Chen пишет: > >> This patch is separated from v10 which is including dp driver, phy driver > >> and dpintf driver. This series is only contained the DisplayPort driver. > >> > >> This series can be tested using 5.19-rc2 kernel and I test it in MT8195 > >> Tomato Chromebook. Modetest these modes: > > > > Applied to drm-misc-next, thanks! > > Hello Chun-Kuang Hu, > > Angelo told me today that you wanted to pick up the MTK driver patches > and I applied them all to the drm-misc instead just of the "video/hdmi" > patch. The series was fully reviewed and tested, so I had no doubts when > applied all the patches. > > The applied patches can't be reverted, so if you have more changes > prepared for the MTK driver, then please rebase them on top of the > latest drm-misc-next. > > Apologizes for this confusion. Please let us know if we can help you. OK, if this cannot be reverted, I could only accept this. Normally, drm/mediatek patches would go though medaitek-drm-* branch. To prevent any confusion, it's better to discuss before pick up. Regards, Chun-Kuang. > > -- > Best regards, > Dmitry
[drm-misc:for-linux-next 9/9] drivers/gpu/drm/vkms/vkms_formats.c:259: undefined reference to `__divdi3'
tree: git://anongit.freedesktop.org/drm/drm-misc for-linux-next
head: 396369d6754993e40f1c84b2e22e40e92dfa4c49
commit: 396369d6754993e40f1c84b2e22e40e92dfa4c49 [9/9] drm: vkms: Add support
to the RGB565 format
config: i386-randconfig-a006-20220905
(https://download.01.org/0day-ci/archive/20220906/202209060813.wci1hzua-...@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
git remote add drm-misc git://anongit.freedesktop.org/drm/drm-misc
git fetch --no-tags drm-misc for-linux-next
git checkout 396369d6754993e40f1c84b2e22e40e92dfa4c49
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
ld: warning: arch/x86/lib/retpoline.o: missing .note.GNU-stack section
implies executable stack
ld: NOTE: This behaviour is deprecated and will be removed in a future
version of the linker
ld: drivers/gpu/drm/vkms/vkms_formats.o: in function `argb_u16_to_RGB565':
>> drivers/gpu/drm/vkms/vkms_formats.c:259: undefined reference to `__divdi3'
>> ld: drivers/gpu/drm/vkms/vkms_formats.c:260: undefined reference to
>> `__divdi3'
ld: drivers/gpu/drm/vkms/vkms_formats.c:261: undefined reference to
`__divdi3'
vim +259 drivers/gpu/drm/vkms/vkms_formats.c
241
242 static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
243 const struct line_buffer *src_buffer,
int y)
244 {
245 int x_dst = frame_info->dst.x1;
246 u16 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
247 struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
248 int x_limit = min_t(size_t, drm_rect_width(_info->dst),
249 src_buffer->n_pixels);
250
251 s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
252 s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
253
254 for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
255 s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
256 s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
257 s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
258
> 259 u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r,
> fp_rb_ratio));
> 260 u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
261 u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b,
fp_rb_ratio));
262
263 *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
264 }
265 }
266
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Re: [PATCH v1 07/11] PCI: apple: switch to using fwnode_gpiod_get_index()
On Sun, Sep 04, 2022 at 11:30:59PM -0700, Dmitry Torokhov wrote: > I would like to stop exporting OF-specific gpiod_get_from_of_node() > so that gpiolib can be cleaned a bit, so let's switch to the generic > fwnode property API. > > Signed-off-by: Dmitry Torokhov > > diff --git a/drivers/pci/controller/pcie-apple.c > b/drivers/pci/controller/pcie-apple.c > index a2c3c207a04b..d83817d3ff86 100644 > --- a/drivers/pci/controller/pcie-apple.c > +++ b/drivers/pci/controller/pcie-apple.c > @@ -516,8 +516,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie, > u32 stat, idx; > int ret, i; > > - reset = gpiod_get_from_of_node(np, "reset-gpios", 0, > -GPIOD_OUT_LOW, "PERST#"); > + reset = fwnode_gpiod_get_index(of_fwnode_handle(np), > +"reset", 0, GPIOD_OUT_LOW, "PERST#"); Hmm, I am looking at the driver and it leaks the reset gpio on unbind/unload. I guess it does not matter in practice, but still nice not to leak. Thankfully it is easy to cure by switching to devm option: devm_fwnode_gpiod_get(). I'll send and updated patch with a new justification. Thanks. -- Dmitry
Re: [PATCH v17 03/10] drm/mediatek: Add MT8195 Embedded DisplayPort driver
Hi, Bo-Chen: Please help to fix the compile warning: ../drivers/gpu/drm/mediatek/mtk_dp.c: In function ‘mtk_dp_video_mute’: ../drivers/gpu/drm/mediatek/mtk_dp.c:947:23: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘long unsigned int’ [-Wformat=] 947 | dev_dbg(mtk_dp->dev, "smc cmd: 0x%x, p1: 0x%x, ret: 0x%lx-0x%lx\n", | ^ ../include/linux/dev_printk.h:129:27: note: in definition of macro ‘dev_printk’ 129 | _dev_printk(level, dev, fmt, ##__VA_ARGS__); \ | ^~~ ../include/linux/dev_printk.h:163:31: note: in expansion of macro ‘dev_fmt’ 163 | dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ | ^~~ ../drivers/gpu/drm/mediatek/mtk_dp.c:947:2: note: in expansion of macro ‘dev_dbg’ 947 | dev_dbg(mtk_dp->dev, "smc cmd: 0x%x, p1: 0x%x, ret: 0x%lx-0x%lx\n", | ^~~ ../drivers/gpu/drm/mediatek/mtk_dp.c:947:36: note: format string is defined here 947 | dev_dbg(mtk_dp->dev, "smc cmd: 0x%x, p1: 0x%x, ret: 0x%lx-0x%lx\n", | ~^ || |unsigned int | %lx Regards, Chun-Kuang. Bo-Chen Chen 於 2022年9月1日 週四 中午12:42寫道： > > From: Markus Schneider-Pargmann > > This patch adds a embedded displayport driver for the MediaTek mt8195 SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so that > both can work with the same register range. The phy driver sets device > data that is read by the parent to get the phy device that can be used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi > > Signed-off-by: Markus Schneider-Pargmann > Signed-off-by: Guillaume Ranquet > Signed-off-by: Bo-Chen Chen > --- > drivers/gpu/drm/mediatek/Kconfig |9 + > drivers/gpu/drm/mediatek/Makefile |2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 1999 + > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 305 > 4 files changed, 2315 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > diff --git a/drivers/gpu/drm/mediatek/Kconfig > b/drivers/gpu/drm/mediatek/Kconfig > index 2976d21e9a34..e66f4a3b6be0 100644 > --- a/drivers/gpu/drm/mediatek/Kconfig > +++ b/drivers/gpu/drm/mediatek/Kconfig > @@ -21,6 +21,15 @@ config DRM_MEDIATEK > This driver provides kernel mode setting and > buffer management to userspace. > > +config DRM_MEDIATEK_DP > + tristate "DRM DPTX Support for MediaTek SoCs" > + depends on DRM_MEDIATEK > + select PHY_MTK_DP > + select DRM_DISPLAY_HELPER > + select DRM_DISPLAY_DP_HELPER > + help > + DRM/KMS Display Port driver for MediaTek SoCs. > + > config DRM_MEDIATEK_HDMI > tristate "DRM HDMI Support for Mediatek SoCs" > depends on DRM_MEDIATEK > diff --git a/drivers/gpu/drm/mediatek/Makefile > b/drivers/gpu/drm/mediatek/Makefile > index 6e604a933ed0..3517d1c65cd7 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -23,3 +23,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \ > mtk_hdmi_ddc.o > > obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o > + > +obj-$(CONFIG_DRM_MEDIATEK_DP) += mtk_dp.o > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > new file mode 100644 > index ..e2ec9b02b1aa > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -0,0 +1,1999 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019-2022 MediaTek Inc. > + * Copyright (c) 2022 BayLibre > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "mtk_dp_reg.h" > + > +#define MTK_DP_SIP_CONTROL_AARCH32 MTK_SIP_SMC_CMD(0x523) > +#define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE(BIT(0) | BIT(5)) > + > +#define MTK_DP_THREAD_CABLE_STATE_CHG BIT(0) > +#define MTK_DP_THREAD_HPD_EVENTBIT(1) > + > +#define MTK_DP_4P1T 4 > +#define MTK_DP_HDE 2 > +#define MTK_DP_PIX_PER_ADDR 2 > +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 > +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x8 > +#define MTK_DP_TRAIN_VOLTAGE_LEVEL_RETRY 5 > +#define
Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()
On Tue, Sep 06, 2022 at 01:10:10AM +0200, Pali Rohár wrote: > On Monday 05 September 2022 15:54:53 Dmitry Torokhov wrote: > > On Mon, Sep 05, 2022 at 09:00:46AM +0200, Pali Rohár wrote: > > > On Sunday 04 September 2022 23:30:58 Dmitry Torokhov wrote: > > > > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node() > > > > so that gpiolib can be cleaned a bit, so let's switch to the generic > > > > device property API. > > > > > > > > I believe that the only reason the driver, instead of the standard > > > > devm_gpiod_get_optional(), used devm_gpiod_get_from_of_node() is > > > > because it wanted to set up a pretty consumer name for the GPIO, > > > > > > IIRC consumer name is not used at all. > > > > > > The reason was to specify full name of DTS property, for easier > > > identification of the code. DTS property is "reset-gpios" but API > > > specify only "reset". > > > > I see. Do you want me to reset the patch with updated desctiption as to > > the reason devm_gpiod_get_from_of_node() was used? > > I think it is fine. So add my: > > Acked-by: Pali Rohár > > Anyway as another improvement for future I would suggest some API > function with _optional_ logic, so it could be used for more PCIe I think we need to see how many are attaching reset lines to subnodes. If there are multiple then I agree we could add _optional. So far I see: dtor@dtor-ws:~/kernel/linux-next (gpiod_get_from_of_node-remove)$ git grep '"reset"' -- drivers/pci/controller/ drivers/pci/controller/cadence/pci-j721e.c: gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pci-keystone.c: gpiod = devm_gpiod_get_optional(dev, "reset", drivers/pci/controller/dwc/pci-meson.c: mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pcie-dw-rockchip.c: rockchip->rst_gpio = devm_gpiod_get_optional(>dev, "reset", drivers/pci/controller/dwc/pcie-fu740.c:afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pcie-intel-gw.c: pcie->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/dwc/pcie-keembay.c: pcie->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); drivers/pci/controller/dwc/pcie-qcom-ep.c: pcie_ep->reset = devm_gpiod_get(dev, "reset", GPIOD_IN); drivers/pci/controller/dwc/pcie-tegra194.c: pcie->pex_rst_gpiod = devm_gpiod_get(pcie->dev, "reset", GPIOD_IN); drivers/pci/controller/pci-aardvark.c: pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); drivers/pci/controller/pci-tegra.c: "reset", drivers/pci/controller/pcie-apple.c: "reset", 0, GPIOD_OUT_LOW, "PERST#"); drivers/pci/controller/pcie-mt7621.c: port->gpio_rst = devm_gpiod_get_index_optional(dev, "reset", slot, So majority have reset lines attached to the "main" node and thus can use devm_gpiod_get_optional(). Thanks. -- Dmitry
Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()
On Monday 05 September 2022 15:54:53 Dmitry Torokhov wrote:
> On Mon, Sep 05, 2022 at 09:00:46AM +0200, Pali Rohár wrote:
> > On Sunday 04 September 2022 23:30:58 Dmitry Torokhov wrote:
> > > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> > > so that gpiolib can be cleaned a bit, so let's switch to the generic
> > > device property API.
> > >
> > > I believe that the only reason the driver, instead of the standard
> > > devm_gpiod_get_optional(), used devm_gpiod_get_from_of_node() is
> > > because it wanted to set up a pretty consumer name for the GPIO,
> >
> > IIRC consumer name is not used at all.
> >
> > The reason was to specify full name of DTS property, for easier
> > identification of the code. DTS property is "reset-gpios" but API
> > specify only "reset".
>
> I see. Do you want me to reset the patch with updated desctiption as to
> the reason devm_gpiod_get_from_of_node() was used?
I think it is fine. So add my:
Acked-by: Pali Rohár
Anyway as another improvement for future I would suggest some API
function with _optional_ logic, so it could be used for more PCIe
controller drivers (e.g. also tegra) without need to reimplement
-ENOENT handling. It is really strange if for acquiring same PERST#
line via GPIO ("reset-gpios" DT property) are used more API functions
in different PCIe drivers.
> >
> > > and we now have a special API for that.
> > >
> > > Signed-off-by: Dmitry Torokhov
> > >
> > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > b/drivers/pci/controller/pci-aardvark.c
> > > index 4834198cc86b..4a8a4a8522cb 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1856,20 +1856,19 @@ static int advk_pcie_probe(struct platform_device
> > > *pdev)
> > > return ret;
> > > }
> > >
> > > - pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> > > -"reset-gpios", 0,
> > > -GPIOD_OUT_LOW,
> > > -"pcie1-reset");
> > > + pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> > > if (ret) {
> > > - if (ret == -ENOENT) {
> > > - pcie->reset_gpio = NULL;
> > > - } else {
> > > - if (ret != -EPROBE_DEFER)
> > > - dev_err(dev, "Failed to get reset-gpio: %i\n",
> > > - ret);
> > > - return ret;
> > > - }
> > > + if (ret != -EPROBE_DEFER)
> > > + dev_err(dev, "Failed to get reset-gpio: %i\n",
> > > + ret);
> > > + return ret;
> > > + }
> > > +
> > > + ret = gpiod_set_consumer_name(pcie->reset_gpio, "pcie1-reset");
> > > + if (ret) {
> > > + dev_err(dev, "Failed to set reset gpio name: %d\n", ret);
> > > + return ret;
> > > }
> > >
> > > ret = of_pci_get_max_link_speed(dev->of_node);
> > >
> > > --
> > > b4 0.10.0-dev-fc921
>
> Thanks.
>
> --
> Dmitry
Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()
On Mon, Sep 05, 2022 at 09:00:46AM +0200, Pali Rohár wrote:
> On Sunday 04 September 2022 23:30:58 Dmitry Torokhov wrote:
> > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> > so that gpiolib can be cleaned a bit, so let's switch to the generic
> > device property API.
> >
> > I believe that the only reason the driver, instead of the standard
> > devm_gpiod_get_optional(), used devm_gpiod_get_from_of_node() is
> > because it wanted to set up a pretty consumer name for the GPIO,
>
> IIRC consumer name is not used at all.
>
> The reason was to specify full name of DTS property, for easier
> identification of the code. DTS property is "reset-gpios" but API
> specify only "reset".
I see. Do you want me to reset the patch with updated desctiption as to
the reason devm_gpiod_get_from_of_node() was used?
>
> > and we now have a special API for that.
> >
> > Signed-off-by: Dmitry Torokhov
> >
> > diff --git a/drivers/pci/controller/pci-aardvark.c
> > b/drivers/pci/controller/pci-aardvark.c
> > index 4834198cc86b..4a8a4a8522cb 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1856,20 +1856,19 @@ static int advk_pcie_probe(struct platform_device
> > *pdev)
> > return ret;
> > }
> >
> > - pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> > - "reset-gpios", 0,
> > - GPIOD_OUT_LOW,
> > - "pcie1-reset");
> > + pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> > if (ret) {
> > - if (ret == -ENOENT) {
> > - pcie->reset_gpio = NULL;
> > - } else {
> > - if (ret != -EPROBE_DEFER)
> > - dev_err(dev, "Failed to get reset-gpio: %i\n",
> > - ret);
> > - return ret;
> > - }
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(dev, "Failed to get reset-gpio: %i\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + ret = gpiod_set_consumer_name(pcie->reset_gpio, "pcie1-reset");
> > + if (ret) {
> > + dev_err(dev, "Failed to set reset gpio name: %d\n", ret);
> > + return ret;
> > }
> >
> > ret = of_pci_get_max_link_speed(dev->of_node);
> >
> > --
> > b4 0.10.0-dev-fc921
Thanks.
--
Dmitry
Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 05, 2022 at 09:19:02AM +0200, Pali Rohár wrote: > On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote: > > I would like to limit (or maybe even remove) use of > > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned > > a bit, so let's switch to the generic device property API. It may even > > help with handling secondary fwnodes when gpiolib is taught to handle > > gpios described by swnodes. > > > > Signed-off-by: Dmitry Torokhov > > > > diff --git a/drivers/pci/controller/pci-tegra.c > > b/drivers/pci/controller/pci-tegra.c > > index 8e323e93be91..929f9363e94b 100644 > > --- a/drivers/pci/controller/pci-tegra.c > > +++ b/drivers/pci/controller/pci-tegra.c > > @@ -2202,10 +2202,11 @@ static int tegra_pcie_parse_dt(struct tegra_pcie > > *pcie) > > * and in this case fall back to using AFI per port register > > * to toggle PERST# SFIO line. > > */ > > - rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, > > -"reset-gpios", 0, > > -GPIOD_OUT_LOW, > > -label); > > + rp->reset_gpio = devm_fwnode_gpiod_get(dev, > > + of_fwnode_handle(port), > > + "reset", > > + GPIOD_OUT_LOW, > > + label); > > Why in pci-aardvark.c for PERST# reset-gpio you have used > devm_gpiod_get_optional() and here in pci-tegra.c you have used > devm_fwnode_gpiod_get()? I think that PERST# logic is same in both > drivers. I believe Andy already answered that, but in this driver we can have several root ports described via subnodes of dev->of_node, and reset GPIOs are attached to those subnodes. We are forced to use devm_fwnode_gpiod_get() instead of devm_gpiod_get_optional() as we need to supply the exact fwnode we need to look up GPIO in, and can not infer it from the 'dev' parameter of devm_gpiod_get(). Thanks. -- Dmitry
[PATCH v2] drm/ssd130x: Replace simple display helpers with the atomic helpers
The simple display pipeline is a set of helpers that can be used by DRM
drivers to avoid dealing with all the needed components and just define
a few functions to operate a simple display device with one full-screen
scanout buffer feeding a single output.
But it is arguable that this provides the correct level of abstraction
for simple drivers, and recently some have been ported from using these
simple display helpers to use the regular atomic helpers instead.
The rationale for this is that the simple display pipeline helpers don't
hide that much of the DRM complexity, while adding an indirection layer
that conflates the concepts of CRTCs and planes. This makes the helpers
less flexible and harder to be reused among different graphics drivers.
Also, for simple drivers, using the full atomic helpers doesn't require
a lot of additional code. So adding a simple display pipeline layer may
not be worth it.
For these reasons, let's follow that trend and make ssd130x a plain DRM
driver that creates its own primary plane, CRTC, enconder and connector.
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
---
Changes in v2:
- Remove plane_state->fb check in struct drm_plane_helper_funcs .atomic_update
callback function (Thomas Zimmermann).
- Move ssd130x_init() call to struct drm_crtc_funcs .reset callback instead of
having in struct drm_encoder_helper_funcs .enable_atomic (Thomas Zimmermann).
drivers/gpu/drm/solomon/ssd130x.c | 260 +-
drivers/gpu/drm/solomon/ssd130x.h | 9 +-
2 files changed, 189 insertions(+), 80 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c
b/drivers/gpu/drm/solomon/ssd130x.c
index f87f5443e714..79e8e2017c68 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -18,6 +18,7 @@
#include
#include
+#include
#include
#include
#include
@@ -564,61 +565,52 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer
*fb, const struct iosys_m
return ret;
}
-static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe
*pipe,
- const struct drm_display_mode *mode)
+static int ssd130x_primary_plane_helper_atomic_check(struct drm_plane *plane,
+struct drm_atomic_state
*new_state)
{
- struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+ struct drm_plane_state *new_plane_state =
drm_atomic_get_new_plane_state(new_state, plane);
+ struct drm_crtc *new_crtc = new_plane_state->crtc;
+ struct drm_crtc_state *new_crtc_state = NULL;
- if (mode->hdisplay != ssd130x->mode.hdisplay &&
- mode->vdisplay != ssd130x->mode.vdisplay)
- return MODE_ONE_SIZE;
-
- if (mode->hdisplay != ssd130x->mode.hdisplay)
- return MODE_ONE_WIDTH;
-
- if (mode->vdisplay != ssd130x->mode.vdisplay)
- return MODE_ONE_HEIGHT;
+ if (new_crtc)
+ new_crtc_state = drm_atomic_get_new_crtc_state(new_state,
new_crtc);
- return MODE_OK;
+ return drm_atomic_helper_check_plane_state(new_plane_state,
new_crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, false);
}
-static void ssd130x_display_pipe_enable(struct drm_simple_display_pipe *pipe,
- struct drm_crtc_state *crtc_state,
- struct drm_plane_state *plane_state)
+static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_state
*old_state)
{
- struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+ struct drm_plane_state *plane_state = plane->state;
+ struct drm_plane_state *old_plane_state =
drm_atomic_get_old_plane_state(old_state, plane);
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
- struct drm_device *drm = >drm;
- int idx, ret;
+ struct drm_device *drm = plane->dev;
+ struct drm_rect src_clip, dst_clip;
+ int idx;
- ret = ssd130x_power_on(ssd130x);
- if (ret)
+ if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state,
_clip))
return;
- ret = ssd130x_init(ssd130x);
- if (ret)
- goto out_power_off;
+ dst_clip = plane_state->dst;
+ if (!drm_rect_intersect(_clip, _clip))
+ return;
if (!drm_dev_enter(drm, ))
- goto out_power_off;
-
- ssd130x_fb_blit_rect(plane_state->fb, _plane_state->data[0],
_state->dst);
-
- ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
+ return;
-
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On 9/5/22 12:47, Dmitry Torokhov wrote:
[ ... ]
We know that count is either 1 or 2 here, so strictly speaking
if (count == 1) {
} else {
}
would be sufficient. On the other side, that depends on ARRAY_SIZE() being
exactly 2, so
if (count == 1) {
} else if (count == 2) {
}
would also make sense. Either way is fine with me. I'll leave it up
to Dmitry to decide what he wants to do.
My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
do not have strong preferences either way really. It is probing code, so
performance is not critical, but I'm obviously satisfied with how the
code looks now, or I would not have sent it.
Good point.
Reviewed-by: Guenter Roeck
Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 05, 2022 at 11:03:38PM +0200, Linus Walleij wrote: > On Mon, Sep 5, 2022 at 9:37 PM Dmitry Torokhov > wrote: > > On Mon, Sep 05, 2022 at 01:57:01PM +0300, Andy Shevchenko wrote: > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > > wrote: > > > > > > > > I would like to limit (or maybe even remove) use of > > > > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned > > > > a bit, so let's switch to the generic device property API. > > > > > > > It may even > > > > help with handling secondary fwnodes when gpiolib is taught to handle > > > > gpios described by swnodes. > > > > > > I would remove this sentence from all commit messages since it's a > > > debatable thing and might even not happen, so the above is a pure > > > speculation. > > > > I have the patches. Granted, I had them since '19 ;) but I'm rebasing > > them and going to push them. I need them to convert bunch of input > > drivers away from platform data. > > That's good news! > > Are you referring to this patch set mentioned in a discussion > from 2017 thru 2020? > https://lore.kernel.org/linux-input/20200826161222.GA1665100@dtor-ws/ > > I put aside GPIO descriptor conversion for input devices (keys, buttons) > in board files anticipating a swnode mechanism. Yep, that one, exactly. It is taking a bit longer than I anticipated ;) Thanks. -- Dmitry
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On 9/5/22 12:55, Andy Shevchenko wrote: On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov wrote: On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote: On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov wrote: On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov wrote: ... - gpiod = devm_gpiod_get_from_of_node(>dev, np, - "nvidia,phy-reset-gpio", - 0, GPIOD_OUT_HIGH, - "ulpi_phy_reset_b"); + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset", + GPIOD_OUT_HIGH); err = PTR_ERR_OR_ZERO(gpiod); What does _OR_ZERO mean now? This converts a pointer to an error code if a pointer represents ERR_PTR() encoded error, or 0 to indicate success. Yes, I know that. My point is, how is it useful now (or even before)? I mean that devm_gpio_get() never returns NULL, right? What does returning NULL have to do with anything. It has to do with a dead code. If defm_gpiod_get() does not return NULL, then why do we even bother to check? PTR_ERR_OR_ZERO() converts into an error code (if the pointer is an ERR_PTR) or 0 if it is a real pointer. Its purpose is not to convert NULL into 0, its purpose is to convert a pointer either into an error code or 0. That is what is done here, and it is done all over the place in the kernel. I don't see your problem with it. Care to explain ? It converts a pointer to a "classic" return code, with negative errors and 0 on success. It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1 IS_ERR and 2 PTR_ERR, one in dev_err() and another to return). I don't see how this is relevant. You lost me. Really, please explain your problem with PTR_ERR_OR_ZERO(). Thanks, Guenter
Re: [PATCH v7 0/9] Add new formats support to vkms
On 09/05, Igor Torrente wrote: > Summary > === > This series of patches refactor some vkms components in order to introduce > new formats to the planes and writeback connector. > > Now in the blend function, the plane's pixels are converted to ARGB16161616 > and then blended together. > > The CRC is calculated based on the ARGB1616161616 buffer. And if required, > this buffer is copied/converted to the writeback buffer format. > > And to handle the pixel conversion, new functions were added to convert > from a specific format to ARGB16161616 (the reciprocal is also true). > Hi Igor, Thanks for this great work! I just applied to drm-misc-next (I did some minor code-style fixes when applying too). Best Regards, Melissa > Tests > = > This patch series was tested using the following igt tests: > -t ".*kms_plane.*" > -t ".*kms_writeback.*" > -t ".*kms_cursor_crc*" > -t ".*kms_flip.*" > > New tests passing > --- > - pipe-A-cursor-size-change > - pipe-A-cursor-alpha-transparent > > Performance > --- > It's running slightly faster than the current implementation. > > Results running the IGT[1] test > `igt@kms_cursor_crc@pipe-a-cursor-512x512-onscreen` ten times: > > | Frametime | > |::| > | Implementation | Current | This commit | > |:---:|:-:|:--:| > | frametime range | 9~22 ms | 10~22 ms | > | Average | 11.4 ms | 12.32 ms | > > Memory consumption > == > It consumes less memory than the current implementation in > the common case (more detail in the commit message). > > | Memory consumption (output dimensions) | > |:--:| > | Current | This patch| > |:--:|:-:| > | Width * Heigth | 2 * Width | > > [1] IGT commit id: bc3f6833a12221a46659535dac06ebb312490eb4 > > XRGB to ARGB behavior > = > During the development, I decided to always fill the alpha channel of > the output pixel whenever the conversion from a format without an alpha > channel to ARGB16161616 is necessary. Therefore, I ignore the value > received from the XRGB and overwrite the value with 0x. > > Primary plane and CRTC size > === > This patch series reworks the blend function to accept a primary plane with > a different size and position from CRTC. > Because now we need to fill the background, we had a loss in > performance with this change > > Alpha channel output for XRGB formats > = > There's still an open question about which value the writeback alpha channel > should be for XRGB formats. > The current igt test implementation is expecting the channel to not be change. > But it's not entirely clear if this should be the behavior followed by vkms > (or any other driver). > > Open issue: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/118 > --- > Igor Torrente (9): > drm: vkms: Replace hardcoded value of `vkms_composer.map` to > DRM_FORMAT_MAX_PLANES > drm: vkms: Rename `vkms_composer` to `vkms_frame_info` > drm: drm_atomic_helper: Add a new helper to deal with the writeback > connector validation > drm: vkms: get the reference to `drm_framebuffer` instead if coping it > drm: vkms: Add fb information to `vkms_writeback_job` > drm: vkms: Refactor the plane composer to accept new formats > drm: vkms: Supports to the case where primary plane doesn't match the > CRTC > drm: vkms: Adds XRGB_16161616 and ARGB_1616161616 formats > drm: vkms: Add support to the RGB565 format > > Documentation/gpu/vkms.rst| 7 +- > drivers/gpu/drm/drm_atomic_helper.c | 39 > drivers/gpu/drm/vkms/Makefile | 1 + > drivers/gpu/drm/vkms/vkms_composer.c | 314 -- > drivers/gpu/drm/vkms/vkms_drv.h | 33 ++- > drivers/gpu/drm/vkms/vkms_formats.c | 302 + > drivers/gpu/drm/vkms/vkms_formats.h | 12 + > drivers/gpu/drm/vkms/vkms_plane.c | 50 ++-- > drivers/gpu/drm/vkms/vkms_writeback.c | 39 +++- > include/drm/drm_atomic_helper.h | 3 + > 10 files changed, 580 insertions(+), 220 deletions(-) > create mode 100644 drivers/gpu/drm/vkms/vkms_formats.c > create mode 100644 drivers/gpu/drm/vkms/vkms_formats.h > > -- > 2.30.2 > signature.asc Description: PGP signature
Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 5, 2022 at 9:37 PM Dmitry Torokhov wrote: > On Mon, Sep 05, 2022 at 01:57:01PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > wrote: > > > > > > I would like to limit (or maybe even remove) use of > > > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned > > > a bit, so let's switch to the generic device property API. > > > > > It may even > > > help with handling secondary fwnodes when gpiolib is taught to handle > > > gpios described by swnodes. > > > > I would remove this sentence from all commit messages since it's a > > debatable thing and might even not happen, so the above is a pure > > speculation. > > I have the patches. Granted, I had them since '19 ;) but I'm rebasing > them and going to push them. I need them to convert bunch of input > drivers away from platform data. That's good news! Are you referring to this patch set mentioned in a discussion from 2017 thru 2020? https://lore.kernel.org/linux-input/20200826161222.GA1665100@dtor-ws/ I put aside GPIO descriptor conversion for input devices (keys, buttons) in board files anticipating a swnode mechanism. Yours, Linus Walleij
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov wrote: > On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov > > wrote: > > > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: > > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > > > wrote: ... > > > > > - gpiod = devm_gpiod_get_from_of_node(>dev, np, > > > > > - > > > > > "nvidia,phy-reset-gpio", > > > > > - 0, GPIOD_OUT_HIGH, > > > > > - > > > > > "ulpi_phy_reset_b"); > > > > > + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset", > > > > > + GPIOD_OUT_HIGH); > > > > > err = PTR_ERR_OR_ZERO(gpiod); > > > > > > > > What does _OR_ZERO mean now? > > > > > > This converts a pointer to an error code if a pointer represents > > > ERR_PTR() encoded error, or 0 to indicate success. > > > > Yes, I know that. My point is, how is it useful now (or even before)? > > I mean that devm_gpio_get() never returns NULL, right? > > What does returning NULL have to do with anything. It has to do with a dead code. If defm_gpiod_get() does not return NULL, then why do we even bother to check? > It converts a pointer > to a "classic" return code, with negative errors and 0 on success. > > It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1 > IS_ERR and 2 PTR_ERR, one in dev_err() and another to return). I don't see how this is relevant. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()
On Mon, Sep 05, 2022 at 01:47:41PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 10:02 AM Pali Rohár wrote: > > On Sunday 04 September 2022 23:30:58 Dmitry Torokhov wrote: > > > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node() > > > so that gpiolib can be cleaned a bit, so let's switch to the generic > > > device property API. > > > > > > I believe that the only reason the driver, instead of the standard > > > devm_gpiod_get_optional(), used devm_gpiod_get_from_of_node() is > > > because it wanted to set up a pretty consumer name for the GPIO, > > > > IIRC consumer name is not used at all. > > It's. The user space tools use it as a label. So, GPIO line can have > "name" (this is provider specific) and "label" (which is consumer > specific, i.o.w. how we use this line). > > ... > > > > + if (ret != -EPROBE_DEFER) > > > + dev_err(dev, "Failed to get reset-gpio: %i\n", > > > + ret); > > > + return ret; > > I understand that in the input subsystem maintainer's hat you don't > like dev_err_probe(), but it's a good case to have it here. The driver currently does not use this API, so I elected not to introduce it in this series. Thanks, -- Dmitry
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov > wrote: > > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > > wrote: > > ... > > > > > - gpiod = devm_gpiod_get_from_of_node(>dev, np, > > > > - > > > > "nvidia,phy-reset-gpio", > > > > - 0, GPIOD_OUT_HIGH, > > > > - "ulpi_phy_reset_b"); > > > > + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset", > > > > + GPIOD_OUT_HIGH); > > > > err = PTR_ERR_OR_ZERO(gpiod); > > > > > > What does _OR_ZERO mean now? > > > > This converts a pointer to an error code if a pointer represents > > ERR_PTR() encoded error, or 0 to indicate success. > > Yes, I know that. My point is, how is it useful now (or even before)? > I mean that devm_gpio_get() never returns NULL, right? What does returning NULL have to do with anything. It converts a pointer to a "classic" return code, with negative errors and 0 on success. It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1 IS_ERR and 2 PTR_ERR, one in dev_err() and another to return). Thanks. -- Dmitry
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 05, 2022 at 08:49:58AM -0700, Guenter Roeck wrote:
> On 9/5/22 08:21, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck wrote:
> > > On 9/5/22 04:09, Andy Shevchenko wrote:
> > > > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > > > wrote:
> >
> > ...
> >
> > > > > + count = device_property_count_u32(dev->parent,
> > > > > "rohm,hw-timeout-ms");
> > > > > + if (count < 0 && count != -EINVAL)
> > > > > + return count;
> > > > > +
> > > > > + if (count > 0) {
> > > >
> > > > > + if (count > ARRAY_SIZE(hw_margin))
> > > > > + return -EINVAL;
> > > >
> > > > Why double check? You may move it out of the (count > 0).
> >
> > > Two checks will always be needed, so I don't entirely see
> > > how that would be better.
> >
> > But not nested. That's my point:
> >
> > if (count > ARRAY_SIZE())
> >return ...
> > if (count > 0)
> >...
> >
>
> The old code has either 1 or two checks if there is no error.
> Your suggested code has always two checks. I don't see how that
> is an improvement.
>
> > > > > - if (ret == 1)
> > > > > - hw_margin_max = hw_margin[0];
> > > >
> > > > > + ret = device_property_read_u32_array(dev->parent,
> > > > > +
> > > > > "rohm,hw-timeout-ms",
> > > > > +hw_margin,
> > > > > count);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > >
> > > > So, only this needs the count > 0 check since below already has it
> > > > implicitly.
> > > >
> > > Sorry, I don't understand this comment.
> >
> > if (count > 0) {
> >ret = device_property_read_u32_array(...);
> >...
> > }
> > if (count == 1)
> > ...
> > if (count == 2)
> > ...
> >
> > But here it might be better to have the nested conditionals.
> >
>
> We know that count is either 1 or 2 here, so strictly speaking
> if (count == 1) {
> } else {
> }
> would be sufficient. On the other side, that depends on ARRAY_SIZE() being
> exactly 2, so
> if (count == 1) {
> } else if (count == 2) {
> }
> would also make sense. Either way is fine with me. I'll leave it up
> to Dmitry to decide what he wants to do.
My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
do not have strong preferences either way really. It is probing code, so
performance is not critical, but I'm obviously satisfied with how the
code looks now, or I would not have sent it.
Thanks.
--
Dmitry
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov wrote: > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > wrote: ... > > > - gpiod = devm_gpiod_get_from_of_node(>dev, np, > > > - > > > "nvidia,phy-reset-gpio", > > > - 0, GPIOD_OUT_HIGH, > > > - "ulpi_phy_reset_b"); > > > + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset", > > > + GPIOD_OUT_HIGH); > > > err = PTR_ERR_OR_ZERO(gpiod); > > > > What does _OR_ZERO mean now? > > This converts a pointer to an error code if a pointer represents > ERR_PTR() encoded error, or 0 to indicate success. Yes, I know that. My point is, how is it useful now (or even before)? I mean that devm_gpio_get() never returns NULL, right? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
> wrote:
> >
> > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> > so that gpiolib can be cleaned a bit, so let's switch to the generic
> > device property API.
> >
> > I believe that the only reason the driver, instead of the standard
> > devm_gpiod_get(), used devm_gpiod_get_from_of_node() is because it
> > wanted to set up a pretty consumer name for the GPIO, and we now have
> > a special API for that.
>
> ...
>
> > - gpiod = devm_gpiod_get_from_of_node(>dev, np,
> > - "nvidia,phy-reset-gpio",
> > - 0, GPIOD_OUT_HIGH,
> > - "ulpi_phy_reset_b");
> > + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset",
> > + GPIOD_OUT_HIGH);
> > err = PTR_ERR_OR_ZERO(gpiod);
>
> What does _OR_ZERO mean now?
This converts a pointer to an error code if a pointer represents
ERR_PTR() encoded error, or 0 to indicate success.
static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr)
{
if (IS_ERR(ptr))
return PTR_ERR(ptr);
else
return 0;
}
Thanks.
--
Dmitry
Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 05, 2022 at 01:57:01PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > wrote: > > > > I would like to limit (or maybe even remove) use of > > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned > > a bit, so let's switch to the generic device property API. > > > It may even > > help with handling secondary fwnodes when gpiolib is taught to handle > > gpios described by swnodes. > > I would remove this sentence from all commit messages since it's a > debatable thing and might even not happen, so the above is a pure > speculation. I have the patches. Granted, I had them since '19 ;) but I'm rebasing them and going to push them. I need them to convert bunch of input drivers away from platform data. Thanks. -- Dmitry
[PATCH] drm/amdgpu: cleanup coding style in amdgpu_device.c
Fix some checkpatch.pl complained about in amdgpu_device.c
Signed-off-by: Jingyu Wang
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 136 +++--
1 file changed, 69 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index afaa1056e039..05d9aa3b5131 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -149,7 +149,7 @@ static ssize_t amdgpu_device_get_pcie_replay_count(struct
device *dev,
return sysfs_emit(buf, "%llu\n", cnt);
}
-static DEVICE_ATTR(pcie_replay_count, S_IRUGO,
+static DEVICE_ATTR(pcie_replay_count, 0444,
amdgpu_device_get_pcie_replay_count, NULL);
static void amdgpu_device_get_pcie_info(struct amdgpu_device *adev);
@@ -173,7 +173,7 @@ static ssize_t amdgpu_device_get_product_name(struct device
*dev,
return sysfs_emit(buf, "%s\n", adev->product_name);
}
-static DEVICE_ATTR(product_name, S_IRUGO,
+static DEVICE_ATTR(product_name, 0444,
amdgpu_device_get_product_name, NULL);
/**
@@ -195,7 +195,7 @@ static ssize_t amdgpu_device_get_product_number(struct
device *dev,
return sysfs_emit(buf, "%s\n", adev->product_number);
}
-static DEVICE_ATTR(product_number, S_IRUGO,
+static DEVICE_ATTR(product_number, 0444,
amdgpu_device_get_product_number, NULL);
/**
@@ -217,7 +217,7 @@ static ssize_t amdgpu_device_get_serial_number(struct
device *dev,
return sysfs_emit(buf, "%s\n", adev->serial);
}
-static DEVICE_ATTR(serial_number, S_IRUGO,
+static DEVICE_ATTR(serial_number, 0444,
amdgpu_device_get_serial_number, NULL);
/**
@@ -360,11 +360,11 @@ size_t amdgpu_device_aper_access(struct amdgpu_device
*adev, loff_t pos,
if (write) {
memcpy_toio(addr, buf, count);
- mb();
+ mb(); /* make sure io happens */
amdgpu_device_flush_hdp(adev, NULL);
} else {
amdgpu_device_invalidate_hdp(adev, NULL);
- mb();
+ mb(); /* make sure io happens */
memcpy_fromio(buf, addr, count);
}
@@ -472,7 +472,7 @@ uint32_t amdgpu_device_rreg(struct amdgpu_device *adev,
* MMIO register read with bytes helper functions
* @offset:bytes offset from MMIO start
*
-*/
+ */
/**
* amdgpu_mm_rreg8 - read a memory mapped IO register
@@ -497,7 +497,7 @@ uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev,
uint32_t offset)
* @offset:bytes offset from MMIO start
* @value: the value want to be written to the register
*
-*/
+ */
/**
* amdgpu_mm_wreg8 - read a memory mapped IO register
*
@@ -615,11 +615,10 @@ void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32
index, u32 v)
if (amdgpu_device_skip_hw_access(adev))
return;
- if (index < adev->doorbell.num_doorbells) {
+ if (index < adev->doorbell.num_doorbells)
writel(v, adev->doorbell.ptr + index);
- } else {
+ else
DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index);
- }
}
/**
@@ -659,11 +658,10 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev,
u32 index, u64 v)
if (amdgpu_device_skip_hw_access(adev))
return;
- if (index < adev->doorbell.num_doorbells) {
+ if (index < adev->doorbell.num_doorbells)
atomic64_set((atomic64_t *)(adev->doorbell.ptr + index), v);
- } else {
+ else
DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index);
- }
}
/**
@@ -958,7 +956,7 @@ static void amdgpu_device_vram_scratch_fini(struct
amdgpu_device *adev)
* @registers: pointer to the register array
* @array_size: size of the register array
*
- * Programs an array or registers with and and or masks.
+ * Programs an array or registers with and or masks.
* This is a helper for setting golden registers.
*/
void amdgpu_device_program_register_sequence(struct amdgpu_device *adev,
@@ -971,7 +969,7 @@ void amdgpu_device_program_register_sequence(struct
amdgpu_device *adev,
if (array_size % 3)
return;
- for (i = 0; i < array_size; i +=3) {
+ for (i = 0; i < array_size; i += 3) {
reg = registers[i + 0];
and_mask = registers[i + 1];
or_mask = registers[i + 2];
@@ -1200,7 +1198,7 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device
*adev)
int rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
struct pci_bus *root;
struct resource *res;
- unsigned i;
+ unsigned int i;
u16 cmd;
int r;
@@ -1292,6 +1290,7 @@ bool amdgpu_device_need_post(struct amdgpu_device *adev)
if (adev->asic_type == CHIP_FIJI) {
[PATCH v7 8/9] drm: vkms: Adds XRGB_16161616 and ARGB_1616161616 formats
This will be useful to write tests that depends on these formats.
ARGB and XRGB follows the a similar implementation of the former formats.
Just adjusting for 16 bits per channel.
V3: Adapt the handlers to the new format introduced in patch 7 V3.
V5: Minor improvements
Added le16_to_cpu/cpu_to_le16 to the 16 bits color read/writes.
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/vkms/vkms_formats.c | 77 +++
drivers/gpu/drm/vkms/vkms_plane.c | 5 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 2 +
3 files changed, 83 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c
b/drivers/gpu/drm/vkms/vkms_formats.c
index 33803d3e30b7..b583da7fe0b1 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -78,6 +78,41 @@ static void XRGB_to_argb_u16(struct line_buffer
*stage_buffer,
}
}
+static void ARGB16161616_to_argb_u16(struct line_buffer *stage_buffer,
+const struct vkms_frame_info *frame_info,
+int y)
+{
+ struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
+ u16 *src_pixels = get_packed_src_addr(frame_info, y);
+ int x_limit = min_t(size_t, drm_rect_width(_info->dst),
+ stage_buffer->n_pixels);
+
+ for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
+ out_pixels[x].a = le16_to_cpu(src_pixels[3]);
+ out_pixels[x].r = le16_to_cpu(src_pixels[2]);
+ out_pixels[x].g = le16_to_cpu(src_pixels[1]);
+ out_pixels[x].b = le16_to_cpu(src_pixels[0]);
+ }
+}
+
+static void XRGB16161616_to_argb_u16(struct line_buffer *stage_buffer,
+const struct vkms_frame_info *frame_info,
+int y)
+{
+ struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
+ u16 *src_pixels = get_packed_src_addr(frame_info, y);
+ int x_limit = min_t(size_t, drm_rect_width(_info->dst),
+ stage_buffer->n_pixels);
+
+ for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
+ out_pixels[x].a = (u16)0x;
+ out_pixels[x].r = le16_to_cpu(src_pixels[2]);
+ out_pixels[x].g = le16_to_cpu(src_pixels[1]);
+ out_pixels[x].b = le16_to_cpu(src_pixels[0]);
+ }
+}
+
+
/*
* The following functions take an line of argb_u16 pixels from the
* src_buffer, convert them to a specific format, and store them in the
@@ -130,6 +165,40 @@ static void argb_u16_to_XRGB(struct vkms_frame_info
*frame_info,
}
}
+static void argb_u16_to_ARGB16161616(struct vkms_frame_info *frame_info,
+const struct line_buffer *src_buffer, int
y)
+{
+ int x_dst = frame_info->dst.x1;
+ u16 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
+ struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
+ int x_limit = min_t(size_t, drm_rect_width(_info->dst),
+ src_buffer->n_pixels);
+
+ for (size_t x = 0; x < x_limit; x++, dst_pixels += 4) {
+ dst_pixels[3] = cpu_to_le16(in_pixels[x].a);
+ dst_pixels[2] = cpu_to_le16(in_pixels[x].r);
+ dst_pixels[1] = cpu_to_le16(in_pixels[x].g);
+ dst_pixels[0] = cpu_to_le16(in_pixels[x].b);
+ }
+}
+
+static void argb_u16_to_XRGB16161616(struct vkms_frame_info *frame_info,
+const struct line_buffer *src_buffer, int
y)
+{
+ int x_dst = frame_info->dst.x1;
+ u16 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
+ struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
+ int x_limit = min_t(size_t, drm_rect_width(_info->dst),
+ src_buffer->n_pixels);
+
+ for (size_t x = 0; x < x_limit; x++, dst_pixels += 4) {
+ dst_pixels[3] = 0x;
+ dst_pixels[2] = cpu_to_le16(in_pixels[x].r);
+ dst_pixels[1] = cpu_to_le16(in_pixels[x].g);
+ dst_pixels[0] = cpu_to_le16(in_pixels[x].b);
+ }
+}
+
void *get_frame_to_line_function(u32 format)
{
switch (format) {
@@ -137,6 +206,10 @@ void *get_frame_to_line_function(u32 format)
return _to_argb_u16;
case DRM_FORMAT_XRGB:
return _to_argb_u16;
+ case DRM_FORMAT_ARGB16161616:
+ return _to_argb_u16;
+ case DRM_FORMAT_XRGB16161616:
+ return _to_argb_u16;
default:
return NULL;
}
@@ -149,6 +222,10 @@ void *get_line_to_frame_function(u32 format)
return _u16_to_ARGB;
case DRM_FORMAT_XRGB:
return _u16_to_XRGB;
+ case DRM_FORMAT_ARGB16161616:
+ return _u16_to_ARGB16161616;
+ case DRM_FORMAT_XRGB16161616:
+
[PATCH v7 9/9] drm: vkms: Add support to the RGB565 format
This commit also adds new helper macros to deal with fixed-point
arithmetic.
It was done to improve the precision of the conversion to ARGB16161616
since the "conversion ratio" is not an integer.
V3: Adapt the handlers to the new format introduced in patch 7 V3.
V5: Minor improvements
V6: Minor improvements (Pekka Paalanen)
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/vkms/vkms_formats.c | 70 +++
drivers/gpu/drm/vkms/vkms_plane.c | 6 ++-
drivers/gpu/drm/vkms/vkms_writeback.c | 3 +-
3 files changed, 76 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c
b/drivers/gpu/drm/vkms/vkms_formats.c
index b583da7fe0b1..30bf8e6660ad 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -5,6 +5,23 @@
#include "vkms_formats.h"
+/* The following macros help doing fixed point arithmetic. */
+/*
+ * With Fixed-Point scale 15 we have 17 and 15 bits of integer and fractional
+ * parts respectively.
+ * | 0.000 |
+ * 31 0
+ */
+#define SHIFT 15
+
+#define INT_TO_FIXED(a) ((a) << SHIFT)
+#define FIXED_MUL(a, b) ((s32)(((s64)(a) * (b)) >> SHIFT))
+#define FIXED_DIV(a, b) ((s32)(((s64)(a) << SHIFT) / (b)))
+/* This macro converts a fixed point number to int, and round half up it */
+#define FIXED_TO_INT_ROUND(a) (((a) + (1 << (SHIFT - 1))) >> SHIFT)
+#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
+#define INT_TO_FIXED_DIV(a, b) (FIXED_DIV(INT_TO_FIXED(a), INT_TO_FIXED(b)))
+
static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x,
int y)
{
return frame_info->offset + (y * frame_info->pitch)
@@ -112,6 +129,30 @@ static void XRGB16161616_to_argb_u16(struct line_buffer
*stage_buffer,
}
}
+static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
+ const struct vkms_frame_info *frame_info, int y)
+{
+ struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
+ u16 *src_pixels = get_packed_src_addr(frame_info, y);
+ int x_limit = min_t(size_t, drm_rect_width(_info->dst),
+ stage_buffer->n_pixels);
+
+ s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
+ s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+
+ for (size_t x = 0; x < x_limit; x++, src_pixels++) {
+ u16 rgb_565 = le16_to_cpu(*src_pixels);
+ s32 fp_r = INT_TO_FIXED((rgb_565 >> 11) & 0x1f);
+ s32 fp_g = INT_TO_FIXED((rgb_565 >> 5) & 0x3f);
+ s32 fp_b = INT_TO_FIXED(rgb_565 & 0x1f);
+
+ out_pixels[x].a = (u16)0x;
+ out_pixels[x].r = FIXED_TO_INT_ROUND(FIXED_MUL(fp_r,
fp_rb_ratio));
+ out_pixels[x].g = FIXED_TO_INT_ROUND(FIXED_MUL(fp_g,
fp_g_ratio));
+ out_pixels[x].b = FIXED_TO_INT_ROUND(FIXED_MUL(fp_b,
fp_rb_ratio));
+ }
+}
+
/*
* The following functions take an line of argb_u16 pixels from the
@@ -199,6 +240,31 @@ static void argb_u16_to_XRGB16161616(struct
vkms_frame_info *frame_info,
}
}
+static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
+ const struct line_buffer *src_buffer, int y)
+{
+ int x_dst = frame_info->dst.x1;
+ u16 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
+ struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
+ int x_limit = min_t(size_t, drm_rect_width(_info->dst),
+ src_buffer->n_pixels);
+
+ s32 fp_rb_ratio = INT_TO_FIXED_DIV(65535, 31);
+ s32 fp_g_ratio = INT_TO_FIXED_DIV(65535, 63);
+
+ for (size_t x = 0; x < x_limit; x++, dst_pixels++) {
+ s32 fp_r = INT_TO_FIXED(in_pixels[x].r);
+ s32 fp_g = INT_TO_FIXED(in_pixels[x].g);
+ s32 fp_b = INT_TO_FIXED(in_pixels[x].b);
+
+ u16 r = FIXED_TO_INT_ROUND(FIXED_DIV(fp_r, fp_rb_ratio));
+ u16 g = FIXED_TO_INT_ROUND(FIXED_DIV(fp_g, fp_g_ratio));
+ u16 b = FIXED_TO_INT_ROUND(FIXED_DIV(fp_b, fp_rb_ratio));
+
+ *dst_pixels = cpu_to_le16(r << 11 | g << 5 | b);
+ }
+}
+
void *get_frame_to_line_function(u32 format)
{
switch (format) {
@@ -210,6 +276,8 @@ void *get_frame_to_line_function(u32 format)
return _to_argb_u16;
case DRM_FORMAT_XRGB16161616:
return _to_argb_u16;
+ case DRM_FORMAT_RGB565:
+ return _to_argb_u16;
default:
return NULL;
}
@@ -226,6 +294,8 @@ void *get_line_to_frame_function(u32 format)
return _u16_to_ARGB16161616;
case DRM_FORMAT_XRGB16161616:
return _u16_to_XRGB16161616;
+ case DRM_FORMAT_RGB565:
+ return _u16_to_RGB565;
default:
return NULL;
}
diff --git
[PATCH v7 7/9] drm: vkms: Supports to the case where primary plane doesn't match the CRTC
We will remove the current assumption that the primary plane has the
same size and position as CRTC and that the primary plane is the
bottom-most in zpos order, or is even enabled. At least as far
as the blending machinery is concerned.
For that we will add CRTC dimension information to `vkms_crtc_state`
and add a opaque black backgound color.
Because now we need to fill the background, we had a loss in
performance with this change. Results running the IGT[1] test
`igt@kms_cursor_crc@pipe-a-cursor-512x512-onscreen` ten times:
| Frametime |
|::|
| Implementation | Previous | This commit |
|:---:|:-:|:--:|
| frametime range | 5~18 ms | 10~22 ms |
| Average | 8.47 ms | 12.32 ms |
[1] IGT commit id: bc3f6833a12221a46659535dac06ebb312490eb4
V6: Improve the commit description (Pekka Paalanen).
Update some comments (Pekka Paalanen).
Remove some fields from `vkms_crtc_state` and move where
some variables are set (Pekka Paalanen).
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
Documentation/gpu/vkms.rst| 3 +-
drivers/gpu/drm/vkms/vkms_composer.c | 59 +--
drivers/gpu/drm/vkms/vkms_writeback.c | 4 ++
3 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index a49e4ae92653..49db221c0f52 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -121,8 +121,7 @@ There's lots of plane features we could add support for:
- ARGB format on primary plane: blend the primary plane into background with
translucent alpha.
-- Support when the primary plane isn't exactly matching the output size: blend
- the primary plane into the black background.
+- Add background color KMS property[Good to get started].
- Full alpha blending on all planes.
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index 5b1a8bdd8268..8e53fa80742b 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -61,6 +61,13 @@ static bool check_y_limit(struct vkms_frame_info
*frame_info, int y)
return false;
}
+static void fill_background(const struct pixel_argb_u16 *background_color,
+ struct line_buffer *output_buffer)
+{
+ for (size_t i = 0; i < output_buffer->n_pixels; i++)
+ output_buffer->pixels[i] = *background_color;
+}
+
/**
* @wb_frame_info: The writeback frame buffer metadata
* @crtc_state: The crtc state
@@ -78,21 +85,17 @@ static void blend(struct vkms_writeback_job *wb,
struct line_buffer *output_buffer, size_t row_size)
{
struct vkms_plane_state **plane = crtc_state->active_planes;
- struct vkms_frame_info *primary_plane_info = plane[0]->frame_info;
u32 n_active_planes = crtc_state->num_active_planes;
- int y_dst = primary_plane_info->dst.y1;
- int h_dst = drm_rect_height(_plane_info->dst);
- int y_limit = y_dst + h_dst;
+ const struct pixel_argb_u16 background_color = { .a = 0x };
- for (size_t y = y_dst; y < y_limit; y++) {
- plane[0]->plane_read(output_buffer, primary_plane_info, y);
+ size_t crtc_y_limit = crtc_state->base.crtc->mode.vdisplay;
- /* If there are other planes besides primary, we consider the
active
-* planes should be in z-order and compose them associatively:
-* ((primary <- overlay) <- cursor)
-*/
- for (size_t i = 1; i < n_active_planes; i++) {
+ for (size_t y = 0; y < crtc_y_limit; y++) {
+ fill_background(_color, output_buffer);
+
+ /* The active planes are composed associatively in z-order. */
+ for (size_t i = 0; i < n_active_planes; i++) {
if (!check_y_limit(plane[i]->frame_info, y))
continue;
@@ -124,14 +127,24 @@ static int check_format_funcs(struct vkms_crtc_state
*crtc_state,
return 0;
}
+static int check_iosys_map(struct vkms_crtc_state *crtc_state)
+{
+ struct vkms_plane_state **plane_state = crtc_state->active_planes;
+ u32 n_active_planes = crtc_state->num_active_planes;
+
+ for (size_t i = 0; i < n_active_planes; i++)
+ if (iosys_map_is_null(_state[i]->frame_info->map[0]))
+ return -1;
+
+ return 0;
+}
+
static int compose_active_planes(struct vkms_writeback_job *active_wb,
struct vkms_crtc_state *crtc_state,
u32 *crc32)
{
size_t line_width, pixel_size = sizeof(struct pixel_argb_u16);
- struct vkms_frame_info *primary_plane_info = NULL;
struct line_buffer output_buffer, stage_buffer;
- struct vkms_plane_state *act_plane =
[PATCH v7 6/9] drm: vkms: Refactor the plane composer to accept new formats
Currently the blend function only accepts XRGB_ and ARGB_
as a color input.
This patch refactors all the functions related to the plane composition
to overcome this limitation.
The pixels blend is done using the new internal format. And new handlers
are being added to convert a specific format to/from this internal format.
So the blend operation depends on these handlers to convert to this common
format. The blended result, if necessary, is converted to the writeback
buffer format.
This patch introduces three major differences to the blend function.
1 - All the planes are blended at once.
2 - The blend calculus is done as per line instead of per pixel.
3 - It is responsible to calculates the CRC and writing the writeback
buffer(if necessary).
These changes allow us to allocate way less memory in the intermediate
buffer to compute these operations. Because now we don't need to
have the entire intermediate image lines at once, just one line is
enough.
| Memory consumption (output dimensions) |
|:--:|
| Current | This patch|
|:--:|:-:|
| Width * Heigth | 2 * Width |
Beyond memory, we also have a minor performance benefit from all
these changes. Results running the IGT[1] test
`igt@kms_cursor_crc@pipe-a-cursor-512x512-onscreen` ten times:
| Frametime |
|:--:|
| Implementation | Current | This commit |
|:---:|:-:|::|
| frametime range | 9~22 ms |5~17 ms |
| Average | 11.4 ms |7.8 ms|
[1] IGT commit id: bc3f6833a12221a46659535dac06ebb312490eb4
V2: Improves the performance drastically, by performing the operations
per-line and not per-pixel(Pekka Paalanen).
Minor improvements(Pekka Paalanen).
V3: Changes the code to blend the planes all at once. This improves
performance, memory consumption, and removes much of the weirdness
of the V2(Pekka Paalanen and me).
Minor improvements(Pekka Paalanen and me).
V4: Rebase the code and adapt it to the new NUM_OVERLAY_PLANES constant.
V5: Minor checkpatch fixes and the removal of TO-DO item(Melissa Wen).
Several security/robustness improvents(Pekka Paalanen).
Removes check_planes_x_bounds function and allows partial
partly off-screen(Pekka Paalanen).
V6: Fix a mismatch of some variable sizes (Pekka Paalanen).
Several minor improvements (Pekka Paalanen).
Reviewed-by: Melissa Wen
Reported-by: kernel test robot
Signed-off-by: Igor Torrente
---
Documentation/gpu/vkms.rst| 4 -
drivers/gpu/drm/vkms/Makefile | 1 +
drivers/gpu/drm/vkms/vkms_composer.c | 322 --
drivers/gpu/drm/vkms/vkms_formats.c | 155 +
drivers/gpu/drm/vkms/vkms_formats.h | 12 +
drivers/gpu/drm/vkms/vkms_plane.c | 3 +
drivers/gpu/drm/vkms/vkms_writeback.c | 3 +
7 files changed, 318 insertions(+), 182 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_formats.c
create mode 100644 drivers/gpu/drm/vkms/vkms_formats.h
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 973e2d43108b..a49e4ae92653 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -118,10 +118,6 @@ Add Plane Features
There's lots of plane features we could add support for:
-- Clearing primary plane: clear primary plane before plane composition (at the
- start) for correctness of pixel blend ops. It also guarantees alpha channel
- is cleared in the target buffer for stable crc. [Good to get started]
-
- ARGB format on primary plane: blend the primary plane into background with
translucent alpha.
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 72f779cbfedd..1b28a6a32948 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -3,6 +3,7 @@ vkms-y := \
vkms_drv.o \
vkms_plane.o \
vkms_output.o \
+ vkms_formats.o \
vkms_crtc.o \
vkms_composer.o \
vkms_writeback.o
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index bca049d879e1..5b1a8bdd8268 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -7,204 +7,188 @@
#include
#include
#include
+#include
#include "vkms_drv.h"
-static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
-const struct vkms_frame_info *frame_info)
+static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
{
- u32 pixel;
- int src_offset = frame_info->offset + (y * frame_info->pitch)
- + (x * frame_info->cpp);
+ u32 new_color;
- pixel = *(u32 *)[src_offset];
+ new_color = (src * 0x + dst * (0x - alpha));
- return pixel;
+ return DIV_ROUND_CLOSEST(new_color,
[PATCH v7 5/9] drm: vkms: Add fb information to `vkms_writeback_job`
This commit is the groundwork to introduce new formats to the planes and
writeback buffer. As part of it, a new buffer metadata field is added to
`vkms_writeback_job`, this metadata is represented by the `vkms_frame_info`
struct.
Also adds two new function pointers (`line_to_frame_func` and
`frame_to_line_func`) are defined to handle format conversion
from/to internal format.
A new internal format(`struct pixel_argb_u16`) is introduced to deal with
all possible inputs. It consists of 16 bits fields that represent each of
the channels.
These things will allow us, in the future, to have different compositing
and wb format types.
V2: Change the code to get the drm_framebuffer reference and not copy its
contents (Thomas Zimmermann).
V3: Drop the refcount in the wb code (Thomas Zimmermann).
V5: Add {wb,plane}_format_transform_func to vkms_writeback_job
and vkms_plane_state (Pekka Paalanen)
V6: Improvements to some struct/struct members names (Pekka Paalanen).
Splits this patch in two (Pekka Paalanen).
V7: Replace line_to_frame_func and frame_to_line_func typedefs
with the function signature and void* (Melissa Wen).
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/vkms/vkms_drv.h | 23 ++-
drivers/gpu/drm/vkms/vkms_writeback.c | 20 +---
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 38c44943d915..0a67b8073f7e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -23,11 +23,6 @@
#define NUM_OVERLAY_PLANES 8
-struct vkms_writeback_job {
- struct iosys_map map[DRM_FORMAT_MAX_PLANES];
- struct iosys_map data[DRM_FORMAT_MAX_PLANES];
-};
-
struct vkms_frame_info {
struct drm_framebuffer *fb;
struct drm_rect src, dst;
@@ -37,6 +32,22 @@ struct vkms_frame_info {
unsigned int cpp;
};
+struct pixel_argb_u16 {
+ u16 a, r, g, b;
+};
+
+struct line_buffer {
+ size_t n_pixels;
+ struct pixel_argb_u16 *pixels;
+};
+
+struct vkms_writeback_job {
+ struct iosys_map data[DRM_FORMAT_MAX_PLANES];
+ struct vkms_frame_info wb_frame_info;
+ void (*wb_write)(struct vkms_frame_info *frame_info,
+const struct line_buffer *buffer, int y);
+};
+
/**
* vkms_plane_state - Driver specific plane state
* @base: base plane state
@@ -45,6 +56,8 @@ struct vkms_frame_info {
struct vkms_plane_state {
struct drm_shadow_plane_state base;
struct vkms_frame_info *frame_info;
+ void (*plane_read)(struct line_buffer *buffer,
+ const struct vkms_frame_info *frame_info, int y);
};
struct vkms_plane {
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
b/drivers/gpu/drm/vkms/vkms_writeback.c
index d427b6c52d03..e0a1ba378fc9 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -75,12 +75,15 @@ static int vkms_wb_prepare_job(struct
drm_writeback_connector *wb_connector,
if (!vkmsjob)
return -ENOMEM;
- ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data);
+ ret = drm_gem_fb_vmap(job->fb, vkmsjob->wb_frame_info.map,
vkmsjob->data);
if (ret) {
DRM_ERROR("vmap failed: %d\n", ret);
goto err_kfree;
}
+ vkmsjob->wb_frame_info.fb = job->fb;
+ drm_framebuffer_get(vkmsjob->wb_frame_info.fb);
+
job->priv = vkmsjob;
return 0;
@@ -99,7 +102,9 @@ static void vkms_wb_cleanup_job(struct
drm_writeback_connector *connector,
if (!job->fb)
return;
- drm_gem_fb_vunmap(job->fb, vkmsjob->map);
+ drm_gem_fb_vunmap(job->fb, vkmsjob->wb_frame_info.map);
+
+ drm_framebuffer_put(vkmsjob->wb_frame_info.fb);
vkmsdev = drm_device_to_vkms_device(job->fb->dev);
vkms_set_composer(>output, false);
@@ -116,14 +121,23 @@ static void vkms_wb_atomic_commit(struct drm_connector
*conn,
struct drm_writeback_connector *wb_conn = >wb_connector;
struct drm_connector_state *conn_state = wb_conn->base.state;
struct vkms_crtc_state *crtc_state = output->composer_state;
+ struct drm_framebuffer *fb = connector_state->writeback_job->fb;
+ struct vkms_writeback_job *active_wb;
+ struct vkms_frame_info *wb_frame_info;
if (!conn_state)
return;
vkms_set_composer(>output, true);
+ active_wb = conn_state->writeback_job->priv;
+ wb_frame_info = _wb->wb_frame_info;
+
spin_lock_irq(>composer_lock);
- crtc_state->active_writeback = conn_state->writeback_job->priv;
+ crtc_state->active_writeback = active_wb;
+ wb_frame_info->offset = fb->offsets[0];
+ wb_frame_info->pitch = fb->pitches[0];
+ wb_frame_info->cpp = fb->format->cpp[0];
crtc_state->wb_pending = true;
[PATCH v7 4/9] drm: vkms: get the reference to `drm_framebuffer` instead if coping it
Instead of coping `drm_framebuffer` - which can cause problems -
we just get the reference and add the ref count.
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/vkms/vkms_composer.c | 4 ++--
drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
drivers/gpu/drm/vkms/vkms_plane.c| 10 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index 7c62c5741430..bca049d879e1 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -153,7 +153,7 @@ static void compose_plane(struct vkms_frame_info
*primary_plane_info,
struct vkms_frame_info *plane_frame_info,
void *vaddr_out)
{
- struct drm_framebuffer *fb = _frame_info->fb;
+ struct drm_framebuffer *fb = plane_frame_info->fb;
void *vaddr;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
@@ -175,7 +175,7 @@ static int compose_active_planes(void **vaddr_out,
struct vkms_frame_info *primary_plane_info,
struct vkms_crtc_state *crtc_state)
{
- struct drm_framebuffer *fb = _plane_info->fb;
+ struct drm_framebuffer *fb = primary_plane_info->fb;
struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
const void *vaddr;
int i;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index cde7d9ac70ad..38c44943d915 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -29,7 +29,7 @@ struct vkms_writeback_job {
};
struct vkms_frame_info {
- struct drm_framebuffer fb;
+ struct drm_framebuffer *fb;
struct drm_rect src, dst;
struct iosys_map map[DRM_FORMAT_MAX_PLANES];
unsigned int offset;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c
b/drivers/gpu/drm/vkms/vkms_plane.c
index 52ec5a691002..41301d383017 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -49,12 +49,12 @@ static void vkms_plane_destroy_state(struct drm_plane
*plane,
struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
struct drm_crtc *crtc = vkms_state->base.base.crtc;
- if (crtc) {
+ if (crtc && vkms_state->frame_info->fb) {
/* dropping the reference we acquired in
* vkms_primary_plane_update()
*/
- if (drm_framebuffer_read_refcount(_state->frame_info->fb))
- drm_framebuffer_put(_state->frame_info->fb);
+ if (drm_framebuffer_read_refcount(vkms_state->frame_info->fb))
+ drm_framebuffer_put(vkms_state->frame_info->fb);
}
kfree(vkms_state->frame_info);
@@ -109,9 +109,9 @@ static void vkms_plane_atomic_update(struct drm_plane
*plane,
frame_info = vkms_plane_state->frame_info;
memcpy(_info->src, _state->src, sizeof(struct drm_rect));
memcpy(_info->dst, _state->dst, sizeof(struct drm_rect));
- memcpy(_info->fb, fb, sizeof(struct drm_framebuffer));
+ frame_info->fb = fb;
memcpy(_info->map, _plane_state->data,
sizeof(frame_info->map));
- drm_framebuffer_get(_info->fb);
+ drm_framebuffer_get(frame_info->fb);
frame_info->offset = fb->offsets[0];
frame_info->pitch = fb->pitches[0];
frame_info->cpp = fb->format->cpp[0];
--
2.30.2
[PATCH v7 3/9] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation
Add a helper function to validate the connector configuration received in
the encoder atomic_check by the drivers.
So the drivers don't need to do these common validations themselves.
V2: Move the format verification to a new helper at the drm_atomic_helper.c
(Thomas Zimmermann).
V3: Format check improvements (Leandro Ribeiro).
Minor improvements(Thomas Zimmermann).
V5: Fix some grammar issues in the commit message (André Almeida).
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/drm_atomic_helper.c | 39 +++
drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++
include/drm/drm_atomic_helper.h | 3 +++
3 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index d36720f419f7..ee5fea48b5cb 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -785,6 +785,45 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
+/**
+ * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder state
+ * @encoder: encoder state to check
+ * @conn_state: connector state to check
+ *
+ * Checks if the writeback connector state is valid, and returns an error if it
+ * isn't.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int
+drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
+struct drm_connector_state *conn_state)
+{
+ struct drm_writeback_job *wb_job = conn_state->writeback_job;
+ struct drm_property_blob *pixel_format_blob;
+ struct drm_framebuffer *fb;
+ size_t i, nformats;
+ u32 *formats;
+
+ if (!wb_job || !wb_job->fb)
+ return 0;
+
+ pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
+ nformats = pixel_format_blob->length / sizeof(u32);
+ formats = pixel_format_blob->data;
+ fb = wb_job->fb;
+
+ for (i = 0; i < nformats; i++)
+ if (fb->format->format == formats[i])
+ return 0;
+
+ drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n",
>format->format);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
+
/**
* drm_atomic_helper_check_plane_state() - Check plane state for validity
* @plane_state: plane state to check
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
b/drivers/gpu/drm/vkms/vkms_writeback.c
index 3b3c1e757ab4..d427b6c52d03 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -31,6 +31,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder
*encoder,
{
struct drm_framebuffer *fb;
const struct drm_display_mode *mode = _state->mode;
+ int ret;
if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
return 0;
@@ -42,11 +43,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder
*encoder,
return -EINVAL;
}
- if (fb->format->format != vkms_wb_formats[0]) {
- DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
- >format->format);
- return -EINVAL;
- }
+ ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
+ if (ret < 0)
+ return ret;
return 0;
}
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 54b321f20d53..06d8902a8097 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -49,6 +49,9 @@ struct drm_private_state;
int drm_atomic_helper_check_modeset(struct drm_device *dev,
struct drm_atomic_state *state);
+int
+drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
+struct drm_connector_state
*conn_state);
int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
const struct drm_crtc_state *crtc_state,
int min_scale,
--
2.30.2
[PATCH v7 2/9] drm: vkms: Rename `vkms_composer` to `vkms_frame_info`
Changes the name of this struct to a more meaningful name.
A name that represents better what this struct is about.
Composer is the code that do the compositing of the planes.
This struct contains information on the frame used in the output
composition. Thus, vkms_frame_info is a better name to represent
this.
V5: Fix a commit message typo(Melissa Wen).
V6: Fix wrong iosys_map_is_null verification at compose_plane
(Melissa Wen).
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/vkms/vkms_composer.c | 87 ++--
drivers/gpu/drm/vkms/vkms_drv.h | 6 +-
drivers/gpu/drm/vkms/vkms_plane.c| 38 ++--
3 files changed, 66 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index 775b97766e08..7c62c5741430 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -11,11 +11,11 @@
#include "vkms_drv.h"
static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
-const struct vkms_composer *composer)
+const struct vkms_frame_info *frame_info)
{
u32 pixel;
- int src_offset = composer->offset + (y * composer->pitch)
- + (x * composer->cpp);
+ int src_offset = frame_info->offset + (y * frame_info->pitch)
+ + (x * frame_info->cpp);
pixel = *(u32 *)[src_offset];
@@ -26,24 +26,24 @@ static u32 get_pixel_from_buffer(int x, int y, const u8
*buffer,
* compute_crc - Compute CRC value on output frame
*
* @vaddr: address to final framebuffer
- * @composer: framebuffer's metadata
+ * @frame_info: framebuffer's metadata
*
* returns CRC value computed using crc32 on the visible portion of
* the final framebuffer at vaddr_out
*/
static uint32_t compute_crc(const u8 *vaddr,
- const struct vkms_composer *composer)
+ const struct vkms_frame_info *frame_info)
{
int x, y;
u32 crc = 0, pixel = 0;
- int x_src = composer->src.x1 >> 16;
- int y_src = composer->src.y1 >> 16;
- int h_src = drm_rect_height(>src) >> 16;
- int w_src = drm_rect_width(>src) >> 16;
+ int x_src = frame_info->src.x1 >> 16;
+ int y_src = frame_info->src.y1 >> 16;
+ int h_src = drm_rect_height(_info->src) >> 16;
+ int w_src = drm_rect_width(_info->src) >> 16;
for (y = y_src; y < y_src + h_src; ++y) {
for (x = x_src; x < x_src + w_src; ++x) {
- pixel = get_pixel_from_buffer(x, y, vaddr, composer);
+ pixel = get_pixel_from_buffer(x, y, vaddr, frame_info);
crc = crc32_le(crc, (void *), sizeof(u32));
}
}
@@ -98,8 +98,8 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
* blend - blend value at vaddr_src with value at vaddr_dst
* @vaddr_dst: destination address
* @vaddr_src: source address
- * @dst_composer: destination framebuffer's metadata
- * @src_composer: source framebuffer's metadata
+ * @dst_frame_info: destination framebuffer's metadata
+ * @src_frame_info: source framebuffer's metadata
* @pixel_blend: blending equation based on plane format
*
* Blend the vaddr_src value with the vaddr_dst value using a pixel blend
@@ -111,33 +111,33 @@ static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
* pixel color values
*/
static void blend(void *vaddr_dst, void *vaddr_src,
- struct vkms_composer *dst_composer,
- struct vkms_composer *src_composer,
+ struct vkms_frame_info *dst_frame_info,
+ struct vkms_frame_info *src_frame_info,
void (*pixel_blend)(const u8 *, u8 *))
{
int i, j, j_dst, i_dst;
int offset_src, offset_dst;
u8 *pixel_dst, *pixel_src;
- int x_src = src_composer->src.x1 >> 16;
- int y_src = src_composer->src.y1 >> 16;
+ int x_src = src_frame_info->src.x1 >> 16;
+ int y_src = src_frame_info->src.y1 >> 16;
- int x_dst = src_composer->dst.x1;
- int y_dst = src_composer->dst.y1;
- int h_dst = drm_rect_height(_composer->dst);
- int w_dst = drm_rect_width(_composer->dst);
+ int x_dst = src_frame_info->dst.x1;
+ int y_dst = src_frame_info->dst.y1;
+ int h_dst = drm_rect_height(_frame_info->dst);
+ int w_dst = drm_rect_width(_frame_info->dst);
int y_limit = y_src + h_dst;
int x_limit = x_src + w_dst;
for (i = y_src, i_dst = y_dst; i < y_limit; ++i) {
for (j = x_src, j_dst = x_dst; j < x_limit; ++j) {
- offset_dst = dst_composer->offset
-+ (i_dst * dst_composer->pitch)
-+ (j_dst++ * dst_composer->cpp);
-
[PATCH v7 1/9] drm: vkms: Replace hardcoded value of `vkms_composer.map` to DRM_FORMAT_MAX_PLANES
The `map` vector at `vkms_composer` uses a hardcoded value to define its
size.
If someday the maximum number of planes increases, this hardcoded value
can be a problem.
This value is being replaced with the DRM_FORMAT_MAX_PLANES macro.
Acked-by: Thomas Zimmermann
Reviewed-by: Melissa Wen
Signed-off-by: Igor Torrente
---
drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 1d60654b553b..ae6c5a3d356c 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -31,7 +31,7 @@ struct vkms_writeback_job {
struct vkms_composer {
struct drm_framebuffer fb;
struct drm_rect src, dst;
- struct iosys_map map[4];
+ struct iosys_map map[DRM_FORMAT_MAX_PLANES];
unsigned int offset;
unsigned int pitch;
unsigned int cpp;
--
2.30.2
[PATCH v7 0/9] Add new formats support to vkms
Summary === This series of patches refactor some vkms components in order to introduce new formats to the planes and writeback connector. Now in the blend function, the plane's pixels are converted to ARGB16161616 and then blended together. The CRC is calculated based on the ARGB1616161616 buffer. And if required, this buffer is copied/converted to the writeback buffer format. And to handle the pixel conversion, new functions were added to convert from a specific format to ARGB16161616 (the reciprocal is also true). Tests = This patch series was tested using the following igt tests: -t ".*kms_plane.*" -t ".*kms_writeback.*" -t ".*kms_cursor_crc*" -t ".*kms_flip.*" New tests passing --- - pipe-A-cursor-size-change - pipe-A-cursor-alpha-transparent Performance --- It's running slightly faster than the current implementation. Results running the IGT[1] test `igt@kms_cursor_crc@pipe-a-cursor-512x512-onscreen` ten times: | Frametime | |::| | Implementation | Current | This commit | |:---:|:-:|:--:| | frametime range | 9~22 ms | 10~22 ms | | Average | 11.4 ms | 12.32 ms | Memory consumption == It consumes less memory than the current implementation in the common case (more detail in the commit message). | Memory consumption (output dimensions) | |:--:| | Current | This patch| |:--:|:-:| | Width * Heigth | 2 * Width | [1] IGT commit id: bc3f6833a12221a46659535dac06ebb312490eb4 XRGB to ARGB behavior = During the development, I decided to always fill the alpha channel of the output pixel whenever the conversion from a format without an alpha channel to ARGB16161616 is necessary. Therefore, I ignore the value received from the XRGB and overwrite the value with 0x. Primary plane and CRTC size === This patch series reworks the blend function to accept a primary plane with a different size and position from CRTC. Because now we need to fill the background, we had a loss in performance with this change Alpha channel output for XRGB formats = There's still an open question about which value the writeback alpha channel should be for XRGB formats. The current igt test implementation is expecting the channel to not be change. But it's not entirely clear if this should be the behavior followed by vkms (or any other driver). Open issue: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/118 --- Igor Torrente (9): drm: vkms: Replace hardcoded value of `vkms_composer.map` to DRM_FORMAT_MAX_PLANES drm: vkms: Rename `vkms_composer` to `vkms_frame_info` drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation drm: vkms: get the reference to `drm_framebuffer` instead if coping it drm: vkms: Add fb information to `vkms_writeback_job` drm: vkms: Refactor the plane composer to accept new formats drm: vkms: Supports to the case where primary plane doesn't match the CRTC drm: vkms: Adds XRGB_16161616 and ARGB_1616161616 formats drm: vkms: Add support to the RGB565 format Documentation/gpu/vkms.rst| 7 +- drivers/gpu/drm/drm_atomic_helper.c | 39 drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_composer.c | 314 -- drivers/gpu/drm/vkms/vkms_drv.h | 33 ++- drivers/gpu/drm/vkms/vkms_formats.c | 302 + drivers/gpu/drm/vkms/vkms_formats.h | 12 + drivers/gpu/drm/vkms/vkms_plane.c | 50 ++-- drivers/gpu/drm/vkms/vkms_writeback.c | 39 +++- include/drm/drm_atomic_helper.h | 3 + 10 files changed, 580 insertions(+), 220 deletions(-) create mode 100644 drivers/gpu/drm/vkms/vkms_formats.c create mode 100644 drivers/gpu/drm/vkms/vkms_formats.h -- 2.30.2
[PATCH] drm/doc: Custom Kconfig for KUnit is no longer needed
When built for UML, KUnit provides virtio/PCI, which means that the
DMA/IOMEM UML emulation needed by DRM is already present and does not
need to be manually added with --kconfig_add.
References: commit 6fc3a8636a7b ("kunit: tool: Enable virtio/PCI by default on
UML")
Signed-off-by: Michał Winiarski
---
Documentation/gpu/drm-internals.rst | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/Documentation/gpu/drm-internals.rst
b/Documentation/gpu/drm-internals.rst
index 5fd20a306718..c264a9587d21 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -228,16 +228,11 @@ follows:
.. code-block:: bash
- $ ./tools/testing/kunit/kunit.py run
--kunitconfig=drivers/gpu/drm/tests \
- --kconfig_add CONFIG_VIRTIO_UML=y \
- --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
+ $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests
.. note::
The configuration included in ``.kunitconfig`` should be as generic as
possible.
- ``CONFIG_VIRTIO_UML`` and ``CONFIG_UML_PCI_OVER_VIRTIO`` are not
- included in it because they are only required for User Mode Linux.
-
Legacy Support Code
===
--
2.37.3
[PATCH] drm/amdgpu: cleanup coding style in amdgpu_kms.c
Fix some checkpatch.pl complained about in amdgpu_kms.c
Signed-off-by: Jingyu Wang
---
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 77668c3dae5b..1f90a096232d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -532,6 +532,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data,
struct drm_file *filp)
crtc = (struct drm_crtc *)minfo->crtcs[i];
if (crtc && crtc->base.id == info->mode_crtc.id) {
struct amdgpu_crtc *amdgpu_crtc =
to_amdgpu_crtc(crtc);
+
ui32 = amdgpu_crtc->crtc_id;
found = 1;
break;
@@ -550,7 +551,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data,
struct drm_file *filp)
if (ret)
return ret;
- ret = copy_to_user(out, , min((size_t)size, sizeof(ip)));
+ ret = copy_to_user(out, , min_t((size_t)size, sizeof(ip)));
return ret ? -EFAULT : 0;
}
case AMDGPU_INFO_HW_IP_COUNT: {
@@ -696,17 +697,18 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data,
struct drm_file *filp)
? -EFAULT : 0;
}
case AMDGPU_INFO_READ_MMR_REG: {
- unsigned n, alloc_size;
+ unsigned int n, alloc_size;
uint32_t *regs;
- unsigned se_num = (info->read_mmr_reg.instance >>
+ unsigned int se_num = (info->read_mmr_reg.instance >>
AMDGPU_INFO_MMR_SE_INDEX_SHIFT) &
AMDGPU_INFO_MMR_SE_INDEX_MASK;
- unsigned sh_num = (info->read_mmr_reg.instance >>
+ unsigned int sh_num = (info->read_mmr_reg.instance >>
AMDGPU_INFO_MMR_SH_INDEX_SHIFT) &
AMDGPU_INFO_MMR_SH_INDEX_MASK;
/* set full masks if the userspace set all bits
-* in the bitfields */
+* in the bitfields
+*/
if (se_num == AMDGPU_INFO_MMR_SE_INDEX_MASK)
se_num = 0x;
else if (se_num >= AMDGPU_GFX_MAX_SE)
@@ -830,7 +832,7 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data,
struct drm_file *filp)
return ret;
}
case AMDGPU_INFO_VCE_CLOCK_TABLE: {
- unsigned i;
+ unsigned int i;
struct drm_amdgpu_info_vce_clock_table vce_clk_table = {};
struct amd_vce_state *vce_state;
@@ -1379,7 +1381,7 @@ static int amdgpu_debugfs_firmware_info_show(struct
seq_file *m, void *unused)
int ret, i;
static const char *ta_fw_name[TA_FW_TYPE_MAX_INDEX] = {
-#define TA_FW_NAME(type) [TA_FW_TYPE_PSP_##type] = #type
+#define TA_FW_NAME(type) ([TA_FW_TYPE_PSP_##type] = #type)
TA_FW_NAME(XGMI),
TA_FW_NAME(RAS),
TA_FW_NAME(HDCP),
base-commit: e47eb90a0a9ae20b82635b9b99a8d0979b757ad8
--
2.34.1
[PATCH v3 2/2] drm/plane_helper: Split into parameterized test cases
The test was constructed as a single function (test case) which checks
multiple conditions, calling the function that is tested multiple times
with different arguments.
This usually means that it can be easily converted into multiple test
cases.
Split igt_check_plane_state into two parameterized test cases,
drm_check_plane_state and drm_check_invalid_plane_state.
Passing output:
== drm_plane_helper (2 subtests) ===
== drm_check_plane_state ===
[PASSED] clipping_simple
[PASSED] clipping_rotate_reflect
[PASSED] positioning_simple
[PASSED] upscaling
[PASSED] downscaling
[PASSED] rounding1
[PASSED] rounding2
[PASSED] rounding3
[PASSED] rounding4
== [PASSED] drm_check_plane_state ==
== drm_check_invalid_plane_state ===
[PASSED] positioning_invalid
[PASSED] upscaling_invalid
[PASSED] downscaling_invalid
== [PASSED] drm_check_invalid_plane_state ==
[PASSED] drm_plane_helper =
Testing complete. Ran 12 tests: passed: 12
v2: Add missing EXPECT/ASSERT (Maíra)
v3: Use single EXPECT insted of condition + KUNIT_FAILURE (Maíra)
Signed-off-by: Michał Winiarski
---
drivers/gpu/drm/tests/drm_plane_helper_test.c | 466 ++
1 file changed, 268 insertions(+), 198 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_plane_helper_test.c
b/drivers/gpu/drm/tests/drm_plane_helper_test.c
index 0bbd42d2d37b..01b76f1d93f4 100644
--- a/drivers/gpu/drm/tests/drm_plane_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_plane_helper_test.c
@@ -12,233 +12,303 @@
#include
#include
-static void set_src(struct drm_plane_state *plane_state,
- unsigned int src_x, unsigned int src_y,
- unsigned int src_w, unsigned int src_h)
+static const struct drm_crtc_state crtc_state = {
+ .crtc = ZERO_SIZE_PTR,
+ .enable = true,
+ .active = true,
+ .mode = {
+ DRM_MODE("1024x768", 0, 65000, 1024, 1048,
+1184, 1344, 0, 768, 771, 777, 806, 0,
+DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+ },
+};
+
+struct drm_check_plane_state_test {
+ const char *name;
+ const char *msg;
+ struct {
+ unsigned int x;
+ unsigned int y;
+ unsigned int w;
+ unsigned int h;
+ } src, src_expected;
+ struct {
+ int x;
+ int y;
+ unsigned int w;
+ unsigned int h;
+ } crtc, crtc_expected;
+ unsigned int rotation;
+ int min_scale;
+ int max_scale;
+ bool can_position;
+};
+
+static int drm_plane_helper_init(struct kunit *test)
{
- plane_state->src_x = src_x;
- plane_state->src_y = src_y;
- plane_state->src_w = src_w;
- plane_state->src_h = src_h;
+ const struct drm_check_plane_state_test *params = test->param_value;
+ struct drm_plane *plane;
+ struct drm_framebuffer *fb;
+ struct drm_plane_state *mock;
+
+ plane = kunit_kzalloc(test, sizeof(*plane), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, plane);
+
+ fb = kunit_kzalloc(test, sizeof(*fb), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, fb);
+ fb->width = 2048;
+ fb->height = 2048;
+
+ mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, mock);
+ mock->plane = plane;
+ mock->crtc = ZERO_SIZE_PTR;
+ mock->fb = fb;
+ mock->rotation = params->rotation;
+ mock->src_x = params->src.x;
+ mock->src_y = params->src.y;
+ mock->src_w = params->src.w;
+ mock->src_h = params->src.h;
+ mock->crtc_x = params->crtc.x;
+ mock->crtc_y = params->crtc.y;
+ mock->crtc_w = params->crtc.w;
+ mock->crtc_h = params->crtc.h;
+
+ test->priv = mock;
+
+ return 0;
}
-static bool check_src_eq(struct kunit *test, struct drm_plane_state
*plane_state,
+static void check_src_eq(struct kunit *test, struct drm_plane_state
*plane_state,
unsigned int src_x, unsigned int src_y,
unsigned int src_w, unsigned int src_h)
{
struct drm_rect expected = DRM_RECT_INIT(src_x, src_y, src_w, src_h);
- if (plane_state->src.x1 < 0) {
- kunit_err(test,
- "src x coordinate %x should never be below 0, src: "
DRM_RECT_FP_FMT,
- plane_state->src.x1,
DRM_RECT_FP_ARG(_state->src));
- return false;
- }
- if (plane_state->src.y1 < 0) {
- kunit_err(test,
- "src y coordinate %x should never be below 0, src: "
DRM_RECT_FP_FMT,
- plane_state->src.y1,
DRM_RECT_FP_ARG(_state->src));
-
[PATCH v3 1/2] drm/plane_helper: Print actual/expected values on failure
Currently the values are printed with debug log level.
Adjust the log level and link the output with the test by using kunit_err.
Example output:
foo: dst: 20x20+10+10, expected: 10x10+0+0
foo: EXPECTATION FAILED at drivers/gpu/drm/tests/drm_plane_helper_test.c:85
Signed-off-by: Michał Winiarski
---
drivers/gpu/drm/tests/drm_plane_helper_test.c | 78 +++
1 file changed, 44 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_plane_helper_test.c
b/drivers/gpu/drm/tests/drm_plane_helper_test.c
index be6cff0020ed..0bbd42d2d37b 100644
--- a/drivers/gpu/drm/tests/drm_plane_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_plane_helper_test.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
static void set_src(struct drm_plane_state *plane_state,
unsigned int src_x, unsigned int src_y,
@@ -21,26 +22,32 @@ static void set_src(struct drm_plane_state *plane_state,
plane_state->src_h = src_h;
}
-static bool check_src_eq(struct drm_plane_state *plane_state,
+static bool check_src_eq(struct kunit *test, struct drm_plane_state
*plane_state,
unsigned int src_x, unsigned int src_y,
unsigned int src_w, unsigned int src_h)
{
+ struct drm_rect expected = DRM_RECT_INIT(src_x, src_y, src_w, src_h);
+
if (plane_state->src.x1 < 0) {
- pr_err("src x coordinate %x should never be below 0.\n",
plane_state->src.x1);
- drm_rect_debug_print("src: ", _state->src, true);
+ kunit_err(test,
+ "src x coordinate %x should never be below 0, src: "
DRM_RECT_FP_FMT,
+ plane_state->src.x1,
DRM_RECT_FP_ARG(_state->src));
return false;
}
if (plane_state->src.y1 < 0) {
- pr_err("src y coordinate %x should never be below 0.\n",
plane_state->src.y1);
- drm_rect_debug_print("src: ", _state->src, true);
+ kunit_err(test,
+ "src y coordinate %x should never be below 0, src: "
DRM_RECT_FP_FMT,
+ plane_state->src.y1,
DRM_RECT_FP_ARG(_state->src));
return false;
}
- if (plane_state->src.x1 != src_x ||
- plane_state->src.y1 != src_y ||
- drm_rect_width(_state->src) != src_w ||
- drm_rect_height(_state->src) != src_h) {
- drm_rect_debug_print("src: ", _state->src, true);
+ if (plane_state->src.x1 != expected.x1 ||
+ plane_state->src.y1 != expected.y1 ||
+ drm_rect_width(_state->src) != drm_rect_width() ||
+ drm_rect_height(_state->src) != drm_rect_height()) {
+ kunit_err(test, "src: " DRM_RECT_FP_FMT ", expected: "
DRM_RECT_FP_FMT,
+ DRM_RECT_FP_ARG(_state->src),
DRM_RECT_FP_ARG());
+
return false;
}
@@ -57,15 +64,18 @@ static void set_crtc(struct drm_plane_state *plane_state,
plane_state->crtc_h = crtc_h;
}
-static bool check_crtc_eq(struct drm_plane_state *plane_state,
+static bool check_crtc_eq(struct kunit *test, struct drm_plane_state
*plane_state,
int crtc_x, int crtc_y,
unsigned int crtc_w, unsigned int crtc_h)
{
- if (plane_state->dst.x1 != crtc_x ||
- plane_state->dst.y1 != crtc_y ||
- drm_rect_width(_state->dst) != crtc_w ||
- drm_rect_height(_state->dst) != crtc_h) {
- drm_rect_debug_print("dst: ", _state->dst, false);
+ struct drm_rect expected = DRM_RECT_INIT(crtc_x, crtc_y, crtc_w,
crtc_h);
+
+ if (plane_state->dst.x1 != expected.x1 ||
+ plane_state->dst.y1 != expected.y1 ||
+ drm_rect_width(_state->dst) != drm_rect_width() ||
+ drm_rect_height(_state->dst) != drm_rect_height()) {
+ kunit_err(test, "dst: " DRM_RECT_FMT ", expected: "
DRM_RECT_FMT,
+ DRM_RECT_ARG(_state->dst),
DRM_RECT_ARG());
return false;
}
@@ -109,8 +119,8 @@ static void igt_check_plane_state(struct kunit *test)
false, false);
KUNIT_EXPECT_FALSE_MSG(test, ret, 0, "Simple clipping check should
pass\n");
KUNIT_EXPECT_TRUE(test, plane_state.visible);
- KUNIT_EXPECT_TRUE(test, check_src_eq(_state, 0, 0, 1024 << 16,
768 << 16));
- KUNIT_EXPECT_TRUE(test, check_crtc_eq(_state, 0, 0, 1024, 768));
+ KUNIT_EXPECT_TRUE(test, check_src_eq(test, _state, 0, 0, 1024 <<
16, 768 << 16));
+ KUNIT_EXPECT_TRUE(test, check_crtc_eq(test, _state, 0, 0, 1024,
768));
/* Rotated clipping + reflection, no scaling. */
plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
@@ -120,8 +130,8 @@ static void igt_check_plane_state(struct kunit *test)
false, false);
Re: [PATCH 1/4] dma-buf: Check status of enable-signaling bit on debug
Am 05.09.22 um 18:39 schrieb Tvrtko Ursulin:
On 05/09/2022 12:21, Christian König wrote:
Am 05.09.22 um 12:56 schrieb Arvind Yadav:
The core DMA-buf framework needs to enable signaling
before the fence is signaled. The core DMA-buf framework
can forget to enable signaling before the fence is signaled.
To avoid this scenario on the debug kernel, check the
DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT status bit before checking
the signaling bit status to confirm that enable_signaling
is enabled.
You might want to put this patch at the end of the series to avoid
breaking the kernel in between.
Signed-off-by: Arvind Yadav
---
include/linux/dma-fence.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 775cdc0b4f24..60c0e935c0b5 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -428,6 +428,11 @@ dma_fence_is_signaled_locked(struct dma_fence
*fence)
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+#ifdef CONFIG_DEBUG_FS
CONFIG_DEBUG_FS is certainly wrong, probably better to check for
CONFIG_DEBUG_WW_MUTEX_SLOWPATH here.
Apart from that looks good to me,
What's the full story in this series - I'm afraid the cover letter
does not make it clear to a casual reader like myself? Where does the
difference between debug and non debug kernel come from?
We have a bug that the drm_sync file doesn't properly enable signaling
leading to an igt test failure.
And how do the proposed changes relate to the following kerneldoc
excerpt:
* Since many implementations can call dma_fence_signal() even
when before
* @enable_signaling has been called there's a race window, where the
* dma_fence_signal() might result in the final fence reference being
* released and its memory freed. To avoid this, implementations
of this
* callback should grab their own reference using dma_fence_get(),
to be
* released when the fence is signalled (through e.g. the interrupt
* handler).
*
* This callback is optional. If this callback is not present,
then the
* driver must always have signaling enabled.
Is it now an error, or should be impossible condition, for "is
signaled" to return true _unless_ signaling has been enabled?
That's neither an error nor impossible. For debugging we just never
return signaled from the dma_fence_is_signaled() function when signaling
was not enabled before.
I also plan to remove the return value from the enable_signaling
callback. That was just not very well designed.
If the statement (in a later patch) is signalling should always be
explicitly enabled by the callers of dma_fence_add_callback, then what
about the existing call to __dma_fence_enable_signaling from
dma_fence_add_callback?
Oh, good point. That sounds like we have some bug in the core dma_fence
code as well.
Calls to dma_fence_add_callback() and dma_fence_wait() should enable
signaling implicitly and don't need an extra call for that.
Only dma_fence_is_signaled() needs this explicit enabling of signaling
through dma_fence_enable_sw_signaling().
Or if the rules are changing shouldn't kerneldoc be updated as part of
the series?
I think the kerneldoc is just a bit misleading. The point is that when
you need to call dma_fence_enable_sw_signaling() you must hold a
reference to the fence object.
But that's true for all the dma_fence_* functions. The race described in
the comment is just nonsense because you need to hold that reference anyway.
Regards,
Christian.
Regards,
Tvrtko
Christian.
+ if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >flags))
+ return false;
+#endif
+
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags))
return true;
Re: [PATCH RESEND drm-misc-next 0/8] drm/arm/malidp: use drm managed resources
On Mon, Sep 05, 2022 at 05:19:02PM +0200, Danilo Krummrich wrote: > Hi, Hi Danilo, > > This patch series converts the driver to use drm managed resources to prevent > potential use-after-free issues on driver unbind/rebind and to get rid of the > usage of deprecated APIs. Appologies for the extended silence, I was on holiday for 3 weeks and stayed away from mailing lists. Will review the two series this week. Best regards, Liviu > > Danilo Krummrich (8): > drm/arm/malidp: use drmm_* to allocate driver structures > drm/arm/malidp: replace drm->dev_private with drm_to_malidp() > drm/arm/malidp: crtc: use drmm_crtc_init_with_planes() > drm/arm/malidp: plane: use drm managed resources > drm/arm/malidp: use drm_dev_unplug() > drm/arm/malidp: plane: protect device resources after removal > drm/arm/malidp: crtc: protect device resources after removal > drm/arm/malidp: drv: protect device resources after removal > > drivers/gpu/drm/arm/malidp_crtc.c | 48 +--- > drivers/gpu/drm/arm/malidp_drv.c| 58 ++--- > drivers/gpu/drm/arm/malidp_drv.h| 2 + > drivers/gpu/drm/arm/malidp_hw.c | 10 ++--- > drivers/gpu/drm/arm/malidp_mw.c | 6 +-- > drivers/gpu/drm/arm/malidp_planes.c | 45 +++--- > 6 files changed, 100 insertions(+), 69 deletions(-) > > > base-commit: 8fe444eb326869823f3788a4b4da5dca03339d10 > -- > 2.37.2 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯
RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Hi,
Thanks for the patch.
> Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended
> one to configure and enable regulator
>
> devm_pm_opp_set_regulators() doesn't enable regulator, which make
> regulator framework switching it off during regulator_late_cleanup().
In that case, why not regulator_get()for
Dynamic regulator(non fixed regulator)??
>
> Call dev_pm_opp_set_opp() with the recommend OPP in
> panfrost_devfreq_init() to enable the regulator and avoid any switch off
> by regulator_late_cleanup().
>
> Suggested-by: Viresh Kumar
> Signed-off-by: Clément Péron
> ---
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 5110cd9b2425..67b242407156 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device
> *pfdev)
> return PTR_ERR(opp);
>
> panfrost_devfreq_profile.initial_freq = cur_freq;
> +
> + /* Setup and enable regulator */
> + ret = dev_pm_opp_set_opp(dev, opp);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> + return ret;
> + }
FYI,
On RZ/G2L mali gpu, we have fixed regulator and
I was able to do GPU OPP transition without any issues previously.
root@smarc-rzg2l:~# cat /sys/class/devfreq/1184.gpu/trans_stat
From : To
: 5000 6250 1 12500 2 25000
4 5 time(ms)
* 5000: 0 0 0 0 0 0
0 1 144
6250: 0 0 0 0 0 0
0 0 0
1: 0 0 0 0 0 0
0 9 524
12500: 0 0 9 0 0 0
0 3 2544
2: 0 0 011 0 0
046 3304
25000: 1 0 0 033 0
0 0 7496
4: 0 0 0 01619
0 0 2024
5: 1 0 0 1 815
35 0 4032
Total transition : 208
Cheers,
Biju
Re: [PATCH v4 4/8] drm/vc4: hdmi: Simplify the hotplug handling
On Mon, Aug 29, 2022 at 03:47:27PM +0200, Maxime Ripard wrote:
> Our detect callback has a bunch of operations to perform depending on
> the current and last status of the connector, such a setting the CEC
> physical address or enabling the scrambling again.
>
> This is currently dealt with a bunch of if / else statetements that make
> it fairly difficult to read and extend.
>
> Let's move all that logic to a function of its own.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 66 ++
> 1 file changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b786645eaeb7..9fad57ebdd11 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -273,17 +273,53 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi
> *vc4_hdmi) {}
>
> static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
>
> +static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
> + enum drm_connector_status status)
> +{
> + struct drm_connector *connector = _hdmi->connector;
> + struct edid *edid;
> +
> + /*
> + * NOTE: This function should really be called with
> + * vc4_hdmi->mutex held, but doing so results in reentrancy
> + * issues since cec_s_phys_addr_from_edid might call
> + * .adap_enable, which leads to that funtion being called with
> + * our mutex held.
> + *
> + * Concurrency isn't an issue at the moment since we don't share
> + * any state with any of the other frameworks so we can ignore
> + * the lock for now.
> + */
> +
> + if (status == connector->status)
> + return;
This looks to have a change in behaviour to not call
vc4_hdmi_enable_scrambling() unless a change in the
connector status was detected. The previous code called
it regarless.
When I was doing the i915 stuff I did have a sink that
left hpd asserted while turned off, and when powering
back up it instead generated a pulse on the hpd line.
Not sure if such a pulse is always slow enough that
you can reasonably guarantee a detection of both edges...
Apart from that (and the cec locking mess that I dared
not even look at) the rest of the series looks OK to me.
> +
> + if (status == connector_status_disconnected) {
> + cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> + return;
> + }
> +
> + edid = drm_get_edid(connector, vc4_hdmi->ddc);
> + if (!edid)
> + return;
> +
> + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
> + kfree(edid);
> +
> + vc4_hdmi_enable_scrambling(_hdmi->encoder.base.base);
> +}
> +
> static enum drm_connector_status
> vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
> {
> struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> - bool connected = false;
> + enum drm_connector_status status = connector_status_disconnected;
>
> /*
>* NOTE: This function should really take vc4_hdmi->mutex, but
>* doing so results in reentrancy issues since
> - * cec_s_phys_addr_from_edid might call .adap_enable, which
> - * leads to that funtion being called with our mutex held.
> + * vc4_hdmi_handle_hotplug() can call into other functions that
> + * would take the mutex while it's held here.
>*
>* Concurrency isn't an issue at the moment since we don't share
>* any state with any of the other frameworks so we can ignore
> @@ -294,31 +330,17 @@ vc4_hdmi_connector_detect(struct drm_connector
> *connector, bool force)
>
> if (vc4_hdmi->hpd_gpio) {
> if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> - connected = true;
> + status = connector_status_connected;
> } else {
> if (vc4_hdmi->variant->hp_detect &&
> vc4_hdmi->variant->hp_detect(vc4_hdmi))
> - connected = true;
> + status = connector_status_connected;
> }
>
> - if (connected) {
> - if (connector->status != connector_status_connected) {
> - struct edid *edid = drm_get_edid(connector,
> vc4_hdmi->ddc);
> -
> - if (edid) {
> - cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap,
> edid);
> - kfree(edid);
> - }
> - }
> -
> - vc4_hdmi_enable_scrambling(_hdmi->encoder.base);
> - pm_runtime_put(_hdmi->pdev->dev);
> - return connector_status_connected;
> - }
> -
> - cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> + vc4_hdmi_handle_hotplug(vc4_hdmi, status);
> pm_runtime_put(_hdmi->pdev->dev);
> - return connector_status_disconnected;
> +
> + return status;
> }
>
> static int
Re: [PATCH v3 2/5] arm64: dts: allwinner: h6: Add cooling map for GPU
Dne ponedeljek, 05. september 2022 ob 19:15:58 CEST je Clément Péron
napisal(a):
> Add a simple cooling map for the GPU.
>
> This cooling map come from the vendor kernel 4.9 with a
> 2°C hysteresis added.
>
> Signed-off-by: Clément Péron
> ---
> arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 51 +++-
> 1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi index
> 5a28303d3d4c..1259ab0c3956 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -186,6 +186,7 @@ gpu: gpu@180 {
> clocks = < CLK_GPU>, < CLK_BUS_GPU>;
> clock-names = "core", "bus";
> resets = < RST_BUS_GPU>;
> + #cooling-cells = <2>;
> status = "disabled";
> };
>
> @@ -1072,9 +1073,55 @@ map0 {
> };
>
> gpu-thermal {
> - polling-delay-passive = <0>;
> - polling-delay = <0>;
> + polling-delay-passive = <1000>;
> + polling-delay = <2000>;
> thermal-sensors = < 1>;
> +
> + trips {
> + gpu_alert0: gpu-alert-0 {
> + temperature = <95000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + gpu_alert1: gpu-alert-1 {
> + temperature =
> <10>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + gpu_alert2: gpu-alert-2 {
> + temperature =
> <105000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + gpu-crit {
> + temperature =
> <115000>;
> + hysteresis = <0>;
> + type = "critical";
> + };
> + };
> +
> + cooling-maps {
> + // Fordid the GPU to go over
756MHz
Typo: Fordid -> Forbid
Also next below.
Best regards,
Jernej
> + map0 {
> + trip = <_alert0>;
> + cooling-device = <
> 1 THERMAL_NO_LIMIT>;
> + };
> +
> + // Fordid the GPU to go over
> 624MHz
> + map1 {
> + trip = <_alert1>;
> + cooling-device = <
> 2 THERMAL_NO_LIMIT>;
> + };
> +
> + // Fordid the GPU to go over
> 576MHz
> + map2 {
> + trip = <_alert2>;
> + cooling-device = <
> 3 THERMAL_NO_LIMIT>;
> + };
> + };
> };
> };
> };
> --
> 2.34.1
Re: [PATCH v3 0/3] Drivers: hv: Avoid allocating MMIO from framebuffer region for other passed through PCI devices
On Sat, Aug 27, 2022 at 03:03:42PM +0200, Vitaly Kuznetsov wrote: [...] > Changes since v2re (PATCH3). > > Vitaly Kuznetsov (3): > PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO > definitions to pci_ids.h > Drivers: hv: Always reserve framebuffer region for Gen1 VMs > Drivers: hv: Never allocate anything besides framebuffer from > framebuffer memory region Series applied to hyperv-fixes. Thanks. >
Re: [PATCH v3 1/3] PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO definitions to pci_ids.h
On Tue, Aug 30, 2022 at 09:31:54AM +0200, Vitaly Kuznetsov wrote: > Bjorn Helgaas writes: > > > On Sat, Aug 27, 2022 at 03:03:43PM +0200, Vitaly Kuznetsov wrote: > >> There are already three places in kernel which define > >> PCI_VENDOR_ID_MICROSOFT > >> and two for PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these > >> from core Vmbus code. Move the defines where they belong. > > > > It's a minor annoyance that the above is 81 characters long when "git > > log" adds its 4-character indent, so it wraps in a default terminal. > > > > It'd be nice if we could settle on a conventional spelling of "Vmbus", > > too. "Vmbus" looks to be in the minority: > > > > $ git grep Vmbus | wc -l; git grep VMbus | wc -l; git grep VMBus | wc -l > > 4 > > 82 > > 62 > > > > FWIW, one published microsoft.com doc uses "VMBus": > > https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/hyper-v-architecture > > Makes sense, > > Wei, > > assuming there are no other concerns about these patches, would you be > able to tweak the commit message here when queueing or would you like me > to send v4 instead? I can do the tweaking. Don't bother sending v4 if there is no other concern. Thanks, Wei. > > Thanks! > > -- > Vitaly >
Re: [PATCH] drm/bridge: anx7625: Set HPD irq detect window to 2ms
Hi Xin,
On Sat, 3 Sept 2022 at 15:09, Xin Ji wrote:
>
> Some panels trigger HPD irq due to noise, the HPD debounce
> may be 1.8ms, exceeding the default irq detect window, ~1.4ms.
> This patch set HPD irq detection window to 2ms to
> tolerate the HPD noise.
>
> Signed-off-by: Xin Ji
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 14 ++
> drivers/gpu/drm/bridge/analogix/anx7625.h | 6 ++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index c74b5df4cade..0c323b5a1c99 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1440,6 +1440,20 @@ static void anx7625_start_dp_work(struct anx7625_data
> *ctx)
>
> static int anx7625_read_hpd_status_p0(struct anx7625_data *ctx)
> {
> + int ret;
> +
> + /* Set irq detect window to 2ms */
> + ret = anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> + HPD_DET_TIMER_BIT0_7, HPD_TIME & 0xFF);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> +HPD_DET_TIMER_BIT8_15,
> +(HPD_TIME >> 8) & 0xFF);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> +HPD_DET_TIMER_BIT16_23,
> +(HPD_TIME >> 16) & 0xFF);
Does the HPD debounce timer register need to be written for every HPD
status read?
> + if (ret < 0)
> + return ret;
> +
> return anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, SYSTEM_STSTUS);
> }
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h
> b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index e257a84db962..14f33d6be289 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -132,6 +132,12 @@
> #define I2S_SLAVE_MODE 0x08
> #define AUDIO_LAYOUT 0x01
>
> +#define HPD_DET_TIMER_BIT0_7 0xea
> +#define HPD_DET_TIMER_BIT8_15 0xeb
> +#define HPD_DET_TIMER_BIT16_23 0xec
> +/* HPD debounce time 2ms for 27M clock */
> +#define HPD_TIME 54000
> +
> #define AUDIO_CONTROL_REGISTER 0xe6
> #define TDM_TIMING_MODE 0x08
>
> --
> 2.25.1
>
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
Hi Maxime, W dniu 5.09.2022 o 15:37, Maxime Ripard pisze: >>> + vfp = vfp_min + (porches_rem / 2); >>> + vbp = porches - vfp; >> >> Relative position of the vertical sync within the VBI effectively moves the >> image up and down. Adding that (porches_rem / 2) moves the image up off >> center >> by that many pixels. I'd keep the VFP always at minimum to keep the image >> centered. > > And you would increase the back porch only then? Well, increasing vbp only gives a centered image with the default 480i/576i resolutions. However, only ever changing vbp will cause the image to be always at the bottom of the screen when the active line count is decreased (e.g. setting the resolution to 720x480 but for 50Hz "PAL" - like many game consoles did back in the day). I believe that the perfect solution would: - Use the canonical / standard-defined blanking line counts for the standard vertical resolutions (480/486/576) - Increase vfp and vbp from there by the same number if a smaller number of active lines is specified, so that the resulting image is centered - Likewise, decrease vfp and vbp by the same number if the active line number is larger and there is still leeway (this should allow for seamless handling of 480i vs. 486i for 60 Hz "NTSC") - If even more active lines are specified, once the limit for vfp is hit, then decrease vbp only - the resulting image will definitely be off-center, but there's no other way I hope this makes sense for you as well. Best regards, Mateusz Kwiatkowski
Re: [PATCH 1/4] dma-buf: Check status of enable-signaling bit on debug
On 05/09/2022 12:21, Christian König wrote:
Am 05.09.22 um 12:56 schrieb Arvind Yadav:
The core DMA-buf framework needs to enable signaling
before the fence is signaled. The core DMA-buf framework
can forget to enable signaling before the fence is signaled.
To avoid this scenario on the debug kernel, check the
DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT status bit before checking
the signaling bit status to confirm that enable_signaling
is enabled.
You might want to put this patch at the end of the series to avoid
breaking the kernel in between.
Signed-off-by: Arvind Yadav
---
include/linux/dma-fence.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 775cdc0b4f24..60c0e935c0b5 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -428,6 +428,11 @@ dma_fence_is_signaled_locked(struct dma_fence
*fence)
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+#ifdef CONFIG_DEBUG_FS
CONFIG_DEBUG_FS is certainly wrong, probably better to check for
CONFIG_DEBUG_WW_MUTEX_SLOWPATH here.
Apart from that looks good to me,
What's the full story in this series - I'm afraid the cover letter does not
make it clear to a casual reader like myself? Where does the difference between
debug and non debug kernel come from?
And how do the proposed changes relate to the following kerneldoc excerpt:
* Since many implementations can call dma_fence_signal() even when
before
* @enable_signaling has been called there's a race window, where the
* dma_fence_signal() might result in the final fence reference being
* released and its memory freed. To avoid this, implementations of this
* callback should grab their own reference using dma_fence_get(), to be
* released when the fence is signalled (through e.g. the interrupt
* handler).
*
* This callback is optional. If this callback is not present, then the
* driver must always have signaling enabled.
Is it now an error, or should be impossible condition, for "is signaled" to
return true _unless_ signaling has been enabled?
If the statement (in a later patch) is signalling should always be explicitly
enabled by the callers of dma_fence_add_callback, then what about the existing
call to __dma_fence_enable_signaling from dma_fence_add_callback?
Or if the rules are changing shouldn't kerneldoc be updated as part of the
series?
Regards,
Tvrtko
Christian.
+ if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >flags))
+ return false;
+#endif
+
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags))
return true;
[PATCH v2 4/4] dma-buf: Check status of enable-signaling bit on debug
The core DMA-buf framework needs to enable signaling
before the fence is signaled. The core DMA-buf framework
can forget to enable signaling before the fence is signaled.
To avoid this scenario on the debug kernel, check the
DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT status bit before checking
the signaling bit status to confirm that enable_signaling
is enabled.
Signed-off-by: Arvind Yadav
---
Changes in v1 :
1- Addressing Christian's comment to replace
CONFIG_DEBUG_WW_MUTEX_SLOWPATH instead of CONFIG_DEBUG_FS.
2- As per Christian's comment moving this patch at last so
The version of this patch is also changed and previously
it was [PATCH 1/4]
---
include/linux/dma-fence.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 775cdc0b4f24..ba1ddc14c5d4 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -428,6 +428,11 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >flags))
+ return false;
+#endif
+
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >flags))
return true;
--
2.25.1
[PATCH v2 3/4] dma-buf: enable signaling for selftest fence on debug
Here's on debug enabling software signaling for selftest.
Signed-off-by: Arvind Yadav
---
Changes in v1 :
1- Addressing Christian's comment to remove unnecessary callback.
2- Replacing CONFIG_DEBUG_WW_MUTEX_SLOWPATH instead of CONFIG_DEBUG_FS.
3- The version of this patch is also changed and previously
it was [PATCH 4/4]
---
drivers/dma-buf/st-dma-fence-chain.c | 8 +
drivers/dma-buf/st-dma-fence-unwrap.c | 44 +++
drivers/dma-buf/st-dma-fence.c| 25 ++-
drivers/dma-buf/st-dma-resv.c | 20
4 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/st-dma-fence-chain.c
b/drivers/dma-buf/st-dma-fence-chain.c
index 8ce1ea59d31b..d3070f8a393c 100644
--- a/drivers/dma-buf/st-dma-fence-chain.c
+++ b/drivers/dma-buf/st-dma-fence-chain.c
@@ -87,6 +87,10 @@ static int sanitycheck(void *arg)
if (!chain)
err = -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(chain);
+#endif
+
dma_fence_signal(f);
dma_fence_put(f);
@@ -143,6 +147,10 @@ static int fence_chains_init(struct fence_chains *fc,
unsigned int count,
}
fc->tail = fc->chains[i];
+
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(fc->chains[i]);
+#endif
}
fc->chain_length = i;
diff --git a/drivers/dma-buf/st-dma-fence-unwrap.c
b/drivers/dma-buf/st-dma-fence-unwrap.c
index 4105d5ea8dde..b76cdd9ee0c7 100644
--- a/drivers/dma-buf/st-dma-fence-unwrap.c
+++ b/drivers/dma-buf/st-dma-fence-unwrap.c
@@ -102,6 +102,10 @@ static int sanitycheck(void *arg)
if (!f)
return -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f);
+#endif
+
array = mock_array(1, f);
if (!array)
return -ENOMEM;
@@ -124,12 +128,20 @@ static int unwrap_array(void *arg)
if (!f1)
return -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f1);
+#endif
+
f2 = mock_fence();
if (!f2) {
dma_fence_put(f1);
return -ENOMEM;
}
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f2);
+#endif
+
array = mock_array(2, f1, f2);
if (!array)
return -ENOMEM;
@@ -164,12 +176,20 @@ static int unwrap_chain(void *arg)
if (!f1)
return -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f1);
+#endif
+
f2 = mock_fence();
if (!f2) {
dma_fence_put(f1);
return -ENOMEM;
}
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f2);
+#endif
+
chain = mock_chain(f1, f2);
if (!chain)
return -ENOMEM;
@@ -204,12 +224,20 @@ static int unwrap_chain_array(void *arg)
if (!f1)
return -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f1);
+#endif
+
f2 = mock_fence();
if (!f2) {
dma_fence_put(f1);
return -ENOMEM;
}
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f2);
+#endif
+
array = mock_array(2, f1, f2);
if (!array)
return -ENOMEM;
@@ -248,12 +276,20 @@ static int unwrap_merge(void *arg)
if (!f1)
return -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f1);
+#endif
+
f2 = mock_fence();
if (!f2) {
err = -ENOMEM;
goto error_put_f1;
}
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f2);
+#endif
+
f3 = dma_fence_unwrap_merge(f1, f2);
if (!f3) {
err = -ENOMEM;
@@ -296,10 +332,18 @@ static int unwrap_merge_complex(void *arg)
if (!f1)
return -ENOMEM;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f1);
+#endif
+
f2 = mock_fence();
if (!f2)
goto error_put_f1;
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f2);
+#endif
+
f3 = dma_fence_unwrap_merge(f1, f2);
if (!f3)
goto error_put_f2;
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
index c8a12d7ad71a..b7880d8374db 100644
--- a/drivers/dma-buf/st-dma-fence.c
+++ b/drivers/dma-buf/st-dma-fence.c
@@ -101,7 +101,9 @@ static int sanitycheck(void *arg)
f = mock_fence();
if (!f)
return -ENOMEM;
-
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ dma_fence_enable_sw_signaling(f);
+#endif
dma_fence_signal(f);
dma_fence_put(f);
@@ -117,6 +119,9 @@ static int test_signaling(void *arg)
if (!f)
return -ENOMEM;
[PATCH v2 2/4] dma-buf: enable signaling for the stub fence on debug
Here's on debug enabling software signaling for the stub fence which is always signaled. This fence should enable software signaling otherwise the AMD GPU scheduler will cause a GPU reset due to a GPU scheduler cleanup activity timeout. Signed-off-by: Arvind Yadav --- Changes in v1 : 1- Addressing Christian's comment to remove unnecessary callback. 2- Replacing CONFIG_DEBUG_WW_MUTEX_SLOWPATH instead of CONFIG_DEBUG_FS. 3- The version of this patch is also changed and previously it was [PATCH 3/4] --- drivers/dma-buf/dma-fence.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 066400ed8841..2378b12538c4 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -27,6 +27,10 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); static DEFINE_SPINLOCK(dma_fence_stub_lock); static struct dma_fence dma_fence_stub; +#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH +static bool __dma_fence_enable_signaling(struct dma_fence *fence); +#endif + /* * fence context counter: each execution context should have its own * fence context, this allows checking if fences belong to the same @@ -136,6 +140,9 @@ struct dma_fence *dma_fence_get_stub(void) _fence_stub_ops, _fence_stub_lock, 0, 0); +#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH + __dma_fence_enable_signaling(_fence_stub); +#endif dma_fence_signal_locked(_fence_stub); } spin_unlock(_fence_stub_lock); -- 2.25.1
[PATCH v2 1/4] drm/sched: Enable signaling for finished fence
Here's enabling software signaling for finished fence. Signed-off-by: Arvind Yadav --- Changes in v1 : 1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from this patch. 2- The version of this patch is also changed and previously it was [PATCH 2/4] --- drivers/gpu/drm/scheduler/sched_main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index e0ab14e0fb6b..fe72de0e2911 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -962,6 +962,8 @@ static int drm_sched_main(void *param) /* Drop for original kref_init of the fence */ dma_fence_put(fence); + dma_fence_enable_sw_signaling(_fence->finished); + r = dma_fence_add_callback(fence, _job->cb, drm_sched_job_done_cb); if (r == -ENOENT) -- 2.25.1
[PATCH v2 0/4] dma-buf: To check enable signaling before signaled
TTM, GEM, DRM or the core DMA-buf framework are needs to enable software signaling before the fence is signaled. The core DMA-buf framework software can forget to call enable_signaling before the fence is signaled. It means framework code can forget to call dma_fence_enable_sw_signaling() before calling dma_fence_is_signaled(). To avoid this scenario on debug kernel, check the DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT bit status before checking the MA_FENCE_FLAG_SIGNALED_BIT bit status to confirm that software signaling is enabled. Arvind Yadav (4): [PATCH v2 1/4] drm/sched: Enable signaling for finished fence [PATCH v2 2/4] dma-buf: enable signaling for the stub fence on debug [PATCH v2 3/4] dma-buf: enable signaling for selftest fence on debug [PATCH v2 4/4] dma-buf: Check status of enable-signaling bit on debug drivers/dma-buf/dma-fence.c| 7 drivers/dma-buf/st-dma-fence-chain.c | 8 + drivers/dma-buf/st-dma-fence-unwrap.c | 44 ++ drivers/dma-buf/st-dma-fence.c | 25 ++- drivers/dma-buf/st-dma-resv.c | 20 drivers/gpu/drm/scheduler/sched_main.c | 2 ++ include/linux/dma-fence.h | 5 +++ 7 files changed, 110 insertions(+), 1 deletion(-) -- 2.25.1
[PATCH 6/6] arm64: tegra: Add simple framebuffer on Jetson Xavier NX
From: Thierry Reding
Add the framebuffer carveout reserved memory node as well as a simple-
framebuffer node that is used to bind to the framebuffer that the
bootloader has set up.
Signed-off-by: Thierry Reding
---
.../nvidia/tegra194-p3509-+p3668-0001.dts | 32 +++
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 2 +-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p3509-+p3668-0001.dts
b/arch/arm64/boot/dts/nvidia/tegra194-p3509-+p3668-0001.dts
index 238fd98e8e45..163950321c38 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p3509-+p3668-0001.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p3509-+p3668-0001.dts
@@ -7,4 +7,36 @@
/ {
model = "NVIDIA Jetson Xavier NX Developer Kit (eMMC)";
compatible = "nvidia,p3509-+p3668-0001", "nvidia,tegra194";
+
+ chosen {
+ framebuffer {
+ compatible = "simple-framebuffer";
+ memory-region = <>;
+ power-domains = < TEGRA194_POWER_DOMAIN_DISP>;
+ clocks = < TEGRA194_CLK_SOR1_REF>,
+< TEGRA194_CLK_SOR1_OUT>,
+< TEGRA194_CLK_SOR1_PAD_CLKOUT>,
+< TEGRA194_CLK_PLLD2>,
+< TEGRA194_CLK_PLLDP>,
+< TEGRA194_CLK_NVDISPLAY_DISP>,
+< TEGRA194_CLK_NVDISPLAYHUB>,
+< TEGRA194_CLK_NVDISPLAY_P0>;
+ width = <1920>;
+ height = <1080>;
+ stride = <7680>;
+ format = "a8b8g8r8";
+ };
+ };
+
+ reserved-memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ fb: framebuffer@2,573a {
+ compatible = "framebuffer";
+ reg = <0x2 0x573a 0x0 0x007e9000>;
+ iommu-addresses = < 0x2 0x573a 0x0 0x007e9000>;
+ };
+ };
};
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 3e6ac20ace3d..5c5343cf8dc9 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1929,7 +1929,7 @@ display-hub@1520 {
ranges = <0x1520 0x1520 0x4>;
- display@1520 {
+ dc0: display@1520 {
compatible = "nvidia,tegra194-dc";
reg = <0x1520 0x1>;
interrupts = ;
--
2.37.2
[PATCH 4/6] drm/format-helper: Support the AB24 format
From: Thierry Reding
Add a conversion helper for the AB24 format to use in drm_fb_blit().
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_format_helper.c | 35 +
1 file changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index 56642816fdff..d564412a816b 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -503,6 +503,36 @@ static void drm_fb_rgb888_to_xrgb(struct iosys_map
*dst, const unsigned int
drm_fb_rgb888_to_xrgb_line);
}
+static void drm_fb_xrgb_to_abgr_line(void *dbuf, const void *sbuf,
unsigned int pixels)
+{
+ __le32 *dbuf32 = dbuf;
+ const __le32 *sbuf32 = sbuf;
+ unsigned int x;
+ u32 pix;
+
+ for (x = 0; x < pixels; x++) {
+ pix = le32_to_cpu(sbuf32[x]);
+ pix = ((pix & 0xff00) >> 24) << 24 |
+ ((pix & 0x00ff) >> 16) << 0 |
+ ((pix & 0xff00) >> 8) << 8 |
+ ((pix & 0x00ff) >> 0) << 16;
+ *dbuf32++ = cpu_to_le32(pix);
+ }
+}
+
+static void drm_fb_xrgb_to_abgr(struct iosys_map *dst, const unsigned
int *dst_pitch,
+ const struct iosys_map *src,
+ const struct drm_framebuffer *fb,
+ const struct drm_rect *clip)
+{
+ static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+ 4,
+ };
+
+ drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+ drm_fb_xrgb_to_abgr_line);
+}
+
static void drm_fb_xrgb_to_xrgb2101010_line(void *dbuf, const void *sbuf,
unsigned int pixels)
{
__le32 *dbuf32 = dbuf;
@@ -672,6 +702,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int
*dst_pitch, uint32_t d
drm_fb_rgb565_to_xrgb(dst, dst_pitch, src, fb,
clip);
return 0;
}
+ } else if (dst_format == DRM_FORMAT_ABGR) {
+ if (fb_format == DRM_FORMAT_XRGB) {
+ drm_fb_xrgb_to_abgr(dst, dst_pitch, src, fb,
clip);
+ return 0;
+ }
} else if (dst_format == DRM_FORMAT_XRGB2101010) {
if (fb_format == DRM_FORMAT_XRGB) {
drm_fb_xrgb_to_xrgb2101010(dst, dst_pitch, src, fb,
clip);
--
2.37.2
[PATCH 5/6] drm/simpledrm: Support the AB24 format
From: Thierry Reding
Add AB24 to the list of supported formats. The format helpers support
conversion to that format and it is documented in the simple-framebuffer
device tree bindings.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tiny/simpledrm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 16377b39f012..7829a12ba3dd 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -563,6 +563,7 @@ static int simpledrm_device_init_mm(struct simpledrm_device
*sdev)
static const uint32_t simpledrm_primary_plane_formats[] = {
DRM_FORMAT_XRGB,
DRM_FORMAT_ARGB,
+ DRM_FORMAT_ABGR,
DRM_FORMAT_RGB565,
//DRM_FORMAT_XRGB1555,
//DRM_FORMAT_ARGB1555,
--
2.37.2
[PATCH 3/6] drm/simpledrm: Add support for system memory framebuffers
From: Thierry Reding
Simple framebuffers can be set up in system memory, which cannot be
requested and/or I/O remapped using the I/O resource helpers. Add a
separate code path that obtains system memory framebuffers from the
reserved memory region referenced in the memory-region property.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/tiny/simpledrm.c | 166 +--
1 file changed, 135 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index a81f91814595..16377b39f012 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -2,6 +2,7 @@
#include
#include
+#include
#include
#include
#include
@@ -218,6 +219,8 @@ struct simpledrm_device {
/* memory management */
void __iomem *screen_base;
+ phys_addr_t sysmem_start;
+ size_t sysmem_size;
/* modesetting */
uint32_t formats[8];
@@ -451,6 +454,100 @@ static int simpledrm_device_init_regulators(struct
simpledrm_device *sdev)
}
#endif
+/*
+ * Memory management
+ */
+
+static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev,
phys_addr_t start,
+ size_t size)
+{
+ struct drm_device *dev = >dev;
+ phys_addr_t end = start + size - 1;
+
+ drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", ,
);
+
+ sdev->screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
+ if (!sdev->screen_base)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev,
phys_addr_t start,
+ size_t size)
+{
+ struct drm_device *dev = >dev;
+ phys_addr_t end = start + size - 1;
+ struct resource *mem;
+
+ drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", ,
);
+
+ mem = devm_request_mem_region(dev->dev, start, size,
sdev->dev.driver->name);
+ if (!mem) {
+ /*
+* We cannot make this fatal. Sometimes this comes from magic
+* spaces our resource handlers simply don't know about. Use
+* the I/O-memory resource as-is and try to map that instead.
+*/
+ drm_warn(dev, "could not acquire memory region [%pa-%pa]\n",
, );
+ } else {
+ size = resource_size(mem);
+ start = mem->start;
+ }
+
+ sdev->screen_base = devm_ioremap_wc(dev->dev, start, size);
+ if (!sdev->screen_base)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void simpledrm_device_exit_mm(void *data)
+{
+ struct simpledrm_device *sdev = data;
+ struct drm_device *dev = >dev;
+
+ of_reserved_mem_device_release(dev->dev);
+}
+
+static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
+{
+ int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t
size);
+ struct drm_device *dev = >dev;
+ struct platform_device *pdev = to_platform_device(dev->dev);
+ phys_addr_t start, end;
+ size_t size;
+ int ret;
+
+ ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node,
0);
+ if (ret) {
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+
+ init = simpledrm_device_init_mm_io;
+ size = resource_size(res);
+ start = res->start;
+ } else {
+ devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm,
sdev);
+ init = simpledrm_device_init_mm_sysmem;
+ start = sdev->sysmem_start;
+ size = sdev->sysmem_size;
+ }
+
+ end = start + size - 1;
+
+ ret = devm_aperture_acquire_from_firmware(dev, start, size);
+ if (ret) {
+ drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n",
, , ret);
+ return ret;
+ }
+
+ return init(sdev, start, size);
+}
+
/*
* Modesetting
*/
@@ -511,13 +608,18 @@ static void
simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
struct drm_framebuffer *fb = plane_state->fb;
struct drm_device *dev = plane->dev;
struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
- struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
struct drm_rect src_clip, dst_clip;
+ struct iosys_map dst;
int idx;
if (!fb)
return;
+ if (sdev->sysmem_size == 0)
+ iosys_map_set_vaddr_iomem(, sdev->screen_base);
+ else
+ iosys_map_set_vaddr(, sdev->screen_base);
+
if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state,
_clip))
return;
@@ -721,8 +823,6 @@ static struct
[PATCH 1/6] dt-bindings: display: simple-framebuffer: Support system memory framebuffers
From: Thierry Reding In order to support framebuffers residing in system memory, allow the memory-region property to override the framebuffer memory specification in the "reg" property. Signed-off-by: Thierry Reding --- .../devicetree/bindings/display/simple-framebuffer.yaml | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml index dd64f70b5014..3e9857eb002e 100644 --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml @@ -63,6 +63,11 @@ properties: reg: description: Location and size of the framebuffer memory + memory-region: +maxItems: 1 +description: Phandle to a node describing the memory to be used for the + framebuffer. If present, overrides the "reg" property (if one exists). + clocks: description: List of clocks used by the framebuffer. -- 2.37.2
[PATCH 2/6] dt-bindings: reserved-memory: Support framebuffer reserved memory
From: Thierry Reding
Document the "framebuffer" compatible string for reserved memory nodes
to annotate reserved memory regions used for framebuffer carveouts.
Signed-off-by: Thierry Reding
---
.../bindings/reserved-memory/framebuffer.yaml | 46 +++
1 file changed, 46 insertions(+)
create mode 100644
Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
diff --git a/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
b/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
new file mode 100644
index ..80574854025d
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/framebuffer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: /reserved-memory framebuffer node bindings
+
+maintainers:
+ - devicetree-s...@vger.kernel.org
+
+allOf:
+ - $ref: "reserved-memory.yaml"
+
+properties:
+ compatible:
+const: framebuffer
+description: >
+ This indicates a region of memory meant to be used as a framebuffer for
+ a set of display devices. It can be used by an operating system to keep
+ the framebuffer from being overwritten and use it as the backing memory
+ for a display device (such as simple-framebuffer).
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ chosen {
+framebuffer {
+ compatible = "simple-framebuffer";
+ memory-region = <>;
+};
+ };
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ fb: framebuffer@8000 {
+ compatible = "framebuffer";
+ reg = <0x8000 0x007e9000>;
+ };
+ };
+
+...
--
2.37.2
[PATCH 0/6] drm/simpledrm: Support system memory framebuffers
From: Thierry Reding Hi, this series of patches adds support for framebuffers residing in system memory to the simple-framebuffer DRM driver. To do this, the DT bindings are extended do accept the memory-region property in addition to the reg property for specifying the framebuffer memory. This is done because the framebuffer memory will typically also need to be marked as reserved so that the operating system will not reuse it and the memory-region property is the standard property to reference reserved memory regions. A new compatible string is documented to annotate the framebuffer memory regions and the simpledrm driver has code added to bind such annotated regions to the simple-framebuffer device. The second half of the series then adds support for the AB24 format and ties it all together to provide a simple-framebuffer on Jetson Xavier NX. It should be noted, though, that the Jetson Xavier NX device tree nodes are an example only and ultimately these will be generated (or at least filled in) at runtime because of the variable nature of the values that they contain. This example also uses (but doesn't depend on) the iommu-addresses property that has been proposed and which will hopefully be merged soon. Thierry Thierry Reding (6): dt-bindings: display: simple-framebuffer: Support system memory framebuffers dt-bindings: reserved-memory: Support framebuffer reserved memory drm/simpledrm: Add support for system memory framebuffers drm/format-helper: Support the AB24 format drm/simpledrm: Support the AB24 format arm64: tegra: Add simple framebuffer on Jetson Xavier NX .../bindings/display/simple-framebuffer.yaml | 5 + .../bindings/reserved-memory/framebuffer.yaml | 46 + .../nvidia/tegra194-p3509-+p3668-0001.dts | 32 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 2 +- drivers/gpu/drm/drm_format_helper.c | 35 drivers/gpu/drm/tiny/simpledrm.c | 167 ++ 6 files changed, 255 insertions(+), 32 deletions(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml -- 2.37.2
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
Hi Maxime,
W dniu 5.09.2022 o 15:32, Maxime Ripard pisze:
> Hi,
>
> On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven wrote:
+enum drm_mode_analog {
+ DRM_MODE_ANALOG_NTSC,
+ DRM_MODE_ANALOG_PAL,
+};
>>>
>>> Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is
>>> common,
>>> but strictly speaking a misnomer. Those are color encoding systems, and your
>>> patchset fully supports lesser used, but standard encodings for those (e.g.
>>> PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more
>>> neutral
>>> naming scheme. Some ideas:
>>>
>>> - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh
>>> rate)
>>> - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line
>>> count)
>>
>> IMHO these are bad names, as e.g. VGA640x480@60 is also analog, using
>> 60 Hz and 525 lines. Add "TV" to the name?
>>
>>> - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU
>>> System
>>> Letter Designations)
>>
>> Or DRM_MODE_ITU_*?
>> But given the long list of letters, this looks fragile to me.
>
> Does it matter at all? It's an internal API that isn't exposed at all.
> I'd rather have a common name that everyone can understand in this case
> rather than a *perfect* name where most will scratch their head
> wondering what it's about.
You may have a point. But in that case, maybe it'd make sense to at least add
a short comment explaining what do you mean by "NTSC" and "PAL" in this context?
Best regards,
Mateusz Kwiatkowski
[PATCH] drm/aperture: Fix some kerneldoc comments
From: Thierry Reding Reword some kerneldoc comments for the DRM aperture handling code. Signed-off-by: Thierry Reding --- drivers/gpu/drm/drm_aperture.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_aperture.c b/drivers/gpu/drm/drm_aperture.c index fdb7d5c17ba1..3b8fdeeafd53 100644 --- a/drivers/gpu/drm/drm_aperture.c +++ b/drivers/gpu/drm/drm_aperture.c @@ -74,7 +74,7 @@ * given framebuffer memory. Ownership of the framebuffer memory is achieved * by calling devm_aperture_acquire_from_firmware(). On success, the driver * is the owner of the framebuffer range. The function fails if the - * framebuffer is already by another driver. See below for an example. + * framebuffer is already owned by another driver. See below for an example. * * .. code-block:: c * @@ -112,7 +112,7 @@ * * The generic driver is now subject to forced removal by other drivers. This * only works for platform drivers that support hot unplug. - * When a driver calls drm_aperture_remove_conflicting_framebuffers() et al + * When a driver calls drm_aperture_remove_conflicting_framebuffers() et al. * for the registered framebuffer range, the aperture helpers call * platform_device_unregister() and the generic driver unloads itself. It * may not access the device's registers, framebuffer memory, ROM, etc @@ -164,7 +164,7 @@ EXPORT_SYMBOL(devm_aperture_acquire_from_firmware); * @primary: also kick vga16fb if present * @req_driver: requesting DRM driver * - * This function removes graphics device drivers which use memory range described by + * This function removes graphics device drivers which use the memory range described by * @base and @size. * * Returns: @@ -182,8 +182,8 @@ EXPORT_SYMBOL(drm_aperture_remove_conflicting_framebuffers); * @pdev: PCI device * @req_driver: requesting DRM driver * - * This function removes graphics device drivers using memory range configured - * for any of @pdev's memory bars. The function assumes that PCI device with + * This function removes graphics device drivers using the memory range configured + * for any of @pdev's memory bars. The function assumes that a PCI device with * shadowed ROM drives a primary display and so kicks out vga16fb. * * Returns: -- 2.37.2
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On 9/5/22 08:21, Andy Shevchenko wrote:
On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck wrote:
On 9/5/22 04:09, Andy Shevchenko wrote:
On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
wrote:
...
+ count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
+ if (count < 0 && count != -EINVAL)
+ return count;
+
+ if (count > 0) {
+ if (count > ARRAY_SIZE(hw_margin))
+ return -EINVAL;
Why double check? You may move it out of the (count > 0).
Two checks will always be needed, so I don't entirely see
how that would be better.
But not nested. That's my point:
if (count > ARRAY_SIZE())
return ...
if (count > 0)
...
The old code has either 1 or two checks if there is no error.
Your suggested code has always two checks. I don't see how that
is an improvement.
- if (ret == 1)
- hw_margin_max = hw_margin[0];
+ ret = device_property_read_u32_array(dev->parent,
+"rohm,hw-timeout-ms",
+hw_margin, count);
+ if (ret < 0)
+ return ret;
So, only this needs the count > 0 check since below already has it implicitly.
Sorry, I don't understand this comment.
if (count > 0) {
ret = device_property_read_u32_array(...);
...
}
if (count == 1)
...
if (count == 2)
...
But here it might be better to have the nested conditionals.
We know that count is either 1 or 2 here, so strictly speaking
if (count == 1) {
} else {
}
would be sufficient. On the other side, that depends on ARRAY_SIZE() being
exactly 2, so
if (count == 1) {
} else if (count == 2) {
}
would also make sense. Either way is fine with me. I'll leave it up
to Dmitry to decide what he wants to do.
Thanks,
Guenter
- if (ret == 2) {
- hw_margin_max = hw_margin[1];
- hw_margin_min = hw_margin[0];
+ if (count == 1)
+ hw_margin_max = hw_margin[0];
+
+ if (count == 2) {
+ hw_margin_max = hw_margin[1];
+ hw_margin_min = hw_margin[0];
+ }
}
Re: [PATCH 2/4] drm/sched: Add callback and enable signaling on debug
Am 05.09.22 um 15:46 schrieb Yadav, Arvind: On 9/5/2022 4:55 PM, Christian König wrote: [SNIP] Am 05.09.22 um 12:56 schrieb Arvind Yadav: .release = drm_sched_fence_release_finished, }; diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index e0ab14e0fb6b..140e3d8646e2 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -961,7 +961,9 @@ static int drm_sched_main(void *param) s_fence->parent = dma_fence_get(fence); /* Drop for original kref_init of the fence */ dma_fence_put(fence); Uff, not related to your patch but that looks wrong to me. The reference can only be dropped after the call to dma_fence_add_callback(). Shall I take care with this patch or I will submit separate one ? Separate one. It's probably no big deal since we grab another reference right before, but still quite some broken coding. Thanks, Christian. - +#ifdef CONFIG_DEBUG_FS + dma_fence_enable_sw_signaling(_fence->finished); +#endif This should always be called, independent of the config options set. Christian. sure, I will remove the Config check. ~arvind r = dma_fence_add_callback(fence, _job->cb, drm_sched_job_done_cb); if (r == -ENOENT)
[PATCH RESEND drm-misc-next 7/7] drm/arm/hdlcd: debugfs: protect device resources after removal
(Hardware) resources which are bound to the driver and device lifecycle must not be accessed after the device and driver are unbound. However, the DRM device isn't freed as long as the last user didn't close it, hence userspace can still call into the driver. Therefore protect the critical sections which are accessing those resources with drm_dev_enter() and drm_dev_exit(). Signed-off-by: Danilo Krummrich --- drivers/gpu/drm/arm/hdlcd_drv.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index e41def6d47cc..020c7d0c70a5 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -204,11 +204,19 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg) struct drm_info_node *node = (struct drm_info_node *)m->private; struct drm_device *drm = node->minor->dev; struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm); - unsigned long clkrate = clk_get_rate(hdlcd->clk); - unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000; + unsigned long clkrate, mode_clock; + int idx; + + if (!drm_dev_enter(drm, )) + return -ENODEV; + + clkrate = clk_get_rate(hdlcd->clk); + mode_clock = hdlcd->crtc.mode.crtc_clock * 1000; seq_printf(m, "hw : %lu\n", clkrate); seq_printf(m, "mode: %lu\n", mode_clock); + + drm_dev_exit(idx); return 0; } -- 2.37.2
[PATCH RESEND drm-misc-next 6/7] drm/arm/hdlcd: crtc: protect device resources after removal
(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.
However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.
Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/hdlcd_crtc.c | 49 ++--
1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 17d3ccf12245..bfc42d4a53c2 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -39,27 +40,47 @@
static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, ))
+ return;
/* stop the controller on cleanup */
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
+
+ drm_dev_exit(idx);
}
static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
- unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+ unsigned int mask;
+ int idx;
+ if (!drm_dev_enter(crtc->dev, ))
+ return -ENODEV;
+
+ mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
+ drm_dev_exit(idx);
+
return 0;
}
static void hdlcd_crtc_disable_vblank(struct drm_crtc *crtc)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
- unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+ unsigned int mask;
+ int idx;
+ if (!drm_dev_enter(crtc->dev, ))
+ return;
+
+ mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
+
+ drm_dev_exit(idx);
}
static const struct drm_crtc_funcs hdlcd_crtc_funcs = {
@@ -170,21 +191,33 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc
*crtc,
struct drm_atomic_state *state)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, ))
+ return;
clk_prepare_enable(hdlcd->clk);
hdlcd_crtc_mode_set_nofb(crtc);
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
drm_crtc_vblank_on(crtc);
+
+ drm_dev_exit(idx);
}
static void hdlcd_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, ))
+ return;
drm_crtc_vblank_off(crtc);
hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
clk_disable_unprepare(hdlcd->clk);
+
+ drm_dev_exit(idx);
}
static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
@@ -192,6 +225,10 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct
drm_crtc *crtc,
{
struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
long rate, clk_rate = mode->clock * 1000;
+ int idx;
+
+ if (!drm_dev_enter(crtc->dev, ))
+ return MODE_NOCLOCK;
rate = clk_round_rate(hdlcd->clk, clk_rate);
/* 0.1% seems a close enough tolerance for the TDA19988 on Juno */
@@ -200,6 +237,8 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct
drm_crtc *crtc,
return MODE_NOCLOCK;
}
+ drm_dev_exit(idx);
+
return MODE_OK;
}
@@ -267,6 +306,10 @@ static void hdlcd_plane_atomic_update(struct drm_plane
*plane,
struct hdlcd_drm_private *hdlcd;
u32 dest_h;
dma_addr_t scanout_start;
+ int idx;
+
+ if (!drm_dev_enter(plane->dev, ))
+ return;
if (!fb)
return;
@@ -279,6 +322,8 @@ static void hdlcd_plane_atomic_update(struct drm_plane
*plane,
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
+
+ drm_dev_exit(idx);
}
static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
--
2.37.2
[PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources
Use drm managed resource allocation (drmm_universal_plane_alloc()) in
order to get rid of the explicit destroy hook in struct drm_plane_funcs.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index c0a5ca7f578a..17d3ccf12245 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs
hdlcd_plane_helper_funcs = {
static const struct drm_plane_funcs hdlcd_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
- .destroy= drm_plane_cleanup,
.reset = drm_atomic_helper_plane_reset,
.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
@@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
{
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
struct drm_plane *plane = NULL;
u32 formats[ARRAY_SIZE(supported_formats)], i;
- int ret;
-
- plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
- if (!plane)
- return ERR_PTR(-ENOMEM);
for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
formats[i] = supported_formats[i].fourcc;
- ret = drm_universal_plane_init(drm, plane, 0xff, _plane_funcs,
- formats, ARRAY_SIZE(formats),
- NULL,
- DRM_PLANE_TYPE_PRIMARY, NULL);
- if (ret)
- return ERR_PTR(ret);
+ plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
+ _plane_funcs,
+ formats, ARRAY_SIZE(formats),
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (IS_ERR(plane))
+ return plane;
drm_plane_helper_add(plane, _plane_helper_funcs);
hdlcd->plane = plane;
--
2.37.2
[PATCH RESEND drm-misc-next 5/7] drm/arm/hdlcd: use drm_dev_unplug()
When the driver is unbound, there might still be users in userspace having an open fd and are calling into the driver. While this is fine for drm managed resources, it is not for resources bound to the device/driver lifecycle, e.g. clocks or MMIO mappings. To prevent use-after-free issues we need to protect those resources with drm_dev_enter() and drm_dev_exit(). This does only work if we indicate that the drm device was unplugged, hence use drm_dev_unplug() instead of drm_dev_unregister(). Protecting the particular resources with drm_dev_enter()/drm_dev_exit() is handled by subsequent patches. Signed-off-by: Danilo Krummrich --- drivers/gpu/drm/arm/hdlcd_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index 120c87934a91..e41def6d47cc 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -325,7 +325,7 @@ static void hdlcd_drm_unbind(struct device *dev) struct drm_device *drm = dev_get_drvdata(dev); struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm); - drm_dev_unregister(drm); + drm_dev_unplug(drm); drm_kms_helper_poll_fini(drm); component_unbind_all(dev, drm); of_node_put(hdlcd->crtc.port); -- 2.37.2
[PATCH RESEND drm-misc-next 1/7] drm/arm/hdlcd: use drmm_* to allocate driver structures
Use drm managed resources to allocate driver structures and get rid of
the deprecated drm_dev_alloc() call and replace it with
devm_drm_dev_alloc().
This also serves as preparation to get rid of drm_device->dev_private
and to fix use-after-free issues on driver unload.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/hdlcd_drv.c | 12
drivers/gpu/drm/arm/hdlcd_drv.h | 1 +
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index a032003c340c..463381d11cff 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -247,13 +247,11 @@ static int hdlcd_drm_bind(struct device *dev)
struct hdlcd_drm_private *hdlcd;
int ret;
- hdlcd = devm_kzalloc(dev, sizeof(*hdlcd), GFP_KERNEL);
- if (!hdlcd)
- return -ENOMEM;
+ hdlcd = devm_drm_dev_alloc(dev, _driver, typeof(*hdlcd), base);
+ if (IS_ERR(hdlcd))
+ return PTR_ERR(hdlcd);
- drm = drm_dev_alloc(_driver, dev);
- if (IS_ERR(drm))
- return PTR_ERR(drm);
+ drm = >base;
drm->dev_private = hdlcd;
dev_set_drvdata(dev, drm);
@@ -319,7 +317,6 @@ static int hdlcd_drm_bind(struct device *dev)
err_free:
drm_mode_config_cleanup(drm);
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
return ret;
}
@@ -344,7 +341,6 @@ static void hdlcd_drm_unbind(struct device *dev)
drm_mode_config_cleanup(drm);
drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
}
static const struct component_master_ops hdlcd_master_ops = {
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 909c39c28487..3892b36767ac 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -7,6 +7,7 @@
#define __HDLCD_DRV_H__
struct hdlcd_drm_private {
+ struct drm_device base;
void __iomem*mmio;
struct clk *clk;
struct drm_crtc crtc;
--
2.37.2
[PATCH RESEND drm-misc-next 3/7] drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes()
Use drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes() to get rid of the explicit drm_crtc_cleanup() call. Signed-off-by: Danilo Krummrich --- drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c index 4a8959d0b2a6..c0a5ca7f578a 100644 --- a/drivers/gpu/drm/arm/hdlcd_crtc.c +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c @@ -42,7 +42,6 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc) /* stop the controller on cleanup */ hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0); - drm_crtc_cleanup(crtc); } static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc) @@ -333,8 +332,8 @@ int hdlcd_setup_crtc(struct drm_device *drm) if (IS_ERR(primary)) return PTR_ERR(primary); - ret = drm_crtc_init_with_planes(drm, >crtc, primary, NULL, - _crtc_funcs, NULL); + ret = drmm_crtc_init_with_planes(drm, >crtc, primary, NULL, +_crtc_funcs, NULL); if (ret) return ret; -- 2.37.2
[PATCH RESEND drm-misc-next 2/7] drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv()
Using drm_device->dev_private is deprecated. Since we've switched to
devm_drm_dev_alloc(), struct drm_device is now embedded in struct
hdlcd_drm_private, hence we can use container_of() to get the struct
drm_device instance instead.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/hdlcd_crtc.c | 4 ++--
drivers/gpu/drm/arm/hdlcd_drv.c | 10 --
drivers/gpu/drm/arm/hdlcd_drv.h | 1 +
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 7030339fa232..4a8959d0b2a6 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -275,7 +275,7 @@ static void hdlcd_plane_atomic_update(struct drm_plane
*plane,
dest_h = drm_rect_height(_plane_state->dst);
scanout_start = drm_fb_dma_get_gem_addr(fb, new_plane_state, 0);
- hdlcd = plane->dev->dev_private;
+ hdlcd = drm_to_hdlcd_priv(plane->dev);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, fb->pitches[0]);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
@@ -325,7 +325,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device
*drm)
int hdlcd_setup_crtc(struct drm_device *drm)
{
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
struct drm_plane *primary;
int ret;
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 463381d11cff..120c87934a91 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -98,7 +98,7 @@ static void hdlcd_irq_uninstall(struct hdlcd_drm_private
*hdlcd)
static int hdlcd_load(struct drm_device *drm, unsigned long flags)
{
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
struct platform_device *pdev = to_platform_device(drm->dev);
struct resource *res;
u32 version;
@@ -190,7 +190,7 @@ static int hdlcd_show_underrun_count(struct seq_file *m,
void *arg)
{
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct drm_device *drm = node->minor->dev;
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
seq_printf(m, "underrun : %d\n",
atomic_read(>buffer_underrun_count));
seq_printf(m, "dma_end : %d\n", atomic_read(>dma_end_count));
@@ -203,7 +203,7 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void
*arg)
{
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct drm_device *drm = node->minor->dev;
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
unsigned long clkrate = clk_get_rate(hdlcd->clk);
unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
@@ -253,7 +253,6 @@ static int hdlcd_drm_bind(struct device *dev)
drm = >base;
- drm->dev_private = hdlcd;
dev_set_drvdata(dev, drm);
hdlcd_setup_mode_config(drm);
@@ -324,7 +323,7 @@ static int hdlcd_drm_bind(struct device *dev)
static void hdlcd_drm_unbind(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
- struct hdlcd_drm_private *hdlcd = drm->dev_private;
+ struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
drm_dev_unregister(drm);
drm_kms_helper_poll_fini(drm);
@@ -339,7 +338,6 @@ static void hdlcd_drm_unbind(struct device *dev)
pm_runtime_disable(dev);
of_reserved_mem_device_release(dev);
drm_mode_config_cleanup(drm);
- drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
}
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 3892b36767ac..f1c1da2ac2db 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -21,6 +21,7 @@ struct hdlcd_drm_private {
#endif
};
+#define drm_to_hdlcd_priv(x) container_of(x, struct hdlcd_drm_private, base)
#define crtc_to_hdlcd_priv(x) container_of(x, struct hdlcd_drm_private, crtc)
static inline void hdlcd_write(struct hdlcd_drm_private *hdlcd,
--
2.37.2
[PATCH RESEND drm-misc-next 0/7] drm/arm/hdlcd: use drm managed resources
Hi, This patch series converts the driver to use drm managed resources to prevent potential use-after-free issues on driver unbind/rebind and to get rid of the usage of deprecated APIs. Danilo Krummrich (7): drm/arm/hdlcd: use drmm_* to allocate driver structures drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv() drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes() drm/arm/hdlcd: plane: use drm managed resources drm/arm/hdlcd: use drm_dev_unplug() drm/arm/hdlcd: crtc: protect device resources after removal drm/arm/hdlcd: debugfs: protect device resources after removal drivers/gpu/drm/arm/hdlcd_crtc.c | 78 drivers/gpu/drm/arm/hdlcd_drv.c | 36 --- drivers/gpu/drm/arm/hdlcd_drv.h | 2 + 3 files changed, 79 insertions(+), 37 deletions(-) base-commit: 8fe444eb326869823f3788a4b4da5dca03339d10 -- 2.37.2
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck wrote:
> On 9/5/22 04:09, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > wrote:
...
> >> + count = device_property_count_u32(dev->parent,
> >> "rohm,hw-timeout-ms");
> >> + if (count < 0 && count != -EINVAL)
> >> + return count;
> >> +
> >> + if (count > 0) {
> >
> >> + if (count > ARRAY_SIZE(hw_margin))
> >> + return -EINVAL;
> >
> > Why double check? You may move it out of the (count > 0).
>
> Two checks will always be needed, so I don't entirely see
> how that would be better.
But not nested. That's my point:
if (count > ARRAY_SIZE())
return ...
if (count > 0)
...
> >> - if (ret == 1)
> >> - hw_margin_max = hw_margin[0];
> >
> >> + ret = device_property_read_u32_array(dev->parent,
> >> +"rohm,hw-timeout-ms",
> >> +hw_margin, count);
> >> + if (ret < 0)
> >> + return ret;
> >
> > So, only this needs the count > 0 check since below already has it
> > implicitly.
> >
> Sorry, I don't understand this comment.
if (count > 0) {
ret = device_property_read_u32_array(...);
...
}
if (count == 1)
...
if (count == 2)
...
But here it might be better to have the nested conditionals.
> >> - if (ret == 2) {
> >> - hw_margin_max = hw_margin[1];
> >> - hw_margin_min = hw_margin[0];
> >> + if (count == 1)
> >> + hw_margin_max = hw_margin[0];
> >> +
> >> + if (count == 2) {
> >> + hw_margin_max = hw_margin[1];
> >> + hw_margin_min = hw_margin[0];
> >> + }
> >> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH V2 1/2] dt-bindings: Add byteswap order to chrontel ch7033
Thanks Laurent, On Sat, 3 Sept 2022 at 02:17, Laurent Pinchart wrote: > > Hi Chris, > > Thank you for the patch. > > On Fri, Sep 02, 2022 at 10:39:05AM -0500, Chris Morgan wrote: > > From: Chris Morgan > > > > Update dt-binding documentation to add support for setting byteswap of > > chrontel ch7033. > > > > New property name of chrontel,byteswap added to set the byteswap order. > > This property is optional. > > > > Signed-off-by: Chris Morgan > > Reviewed-by: Robert Foss > > --- > > .../bindings/display/bridge/chrontel,ch7033.yaml| 13 + > > 1 file changed, 13 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml > > b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml > > index bb6289c7d375..984b90893583 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml > > @@ -14,6 +14,19 @@ properties: > >compatible: > > const: chrontel,ch7033 > > > > + chrontel,byteswap: > > +$ref: /schemas/types.yaml#/definitions/uint8 > > +enum: > > + - 0 # BYTE_SWAP_RGB > > + - 1 # BYTE_SWAP_RBG > > + - 2 # BYTE_SWAP_GRB > > + - 3 # BYTE_SWAP_GBR > > + - 4 # BYTE_SWAP_BRG > > + - 5 # BYTE_SWAP_BGR > > +description: | > > + Set the byteswap value of the bridge. This is optional and if not > > + set value of BYTE_SWAP_BGR is used. > > I don't think this belongs to the device tree. The source of data > connected to the CH7033 input could use different formats. This > shouldn't be hardcoded, but queried at runtime, using the input and > output media bus formats infrastructure that the DRM bridge framework > includes. Chris, will you have a look at submitting a fix for this during the coming days? If not, we can revert this series and apply a fixed version later. > > > + > >reg: > > maxItems: 1 > > description: I2C address of the device > > -- > Regards, > > Laurent Pinchart
[PATCH RESEND drm-misc-next 8/8] drm/arm/malidp: drv: protect device resources after removal
(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.
However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.
Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_drv.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index aedd30f5f451..8bb8e8d14461 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -234,9 +234,12 @@ static void malidp_atomic_commit_tail(struct
drm_atomic_state *state)
struct malidp_drm *malidp = drm_to_malidp(drm);
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state;
- int i;
+ int i, idx;
bool fence_cookie = dma_fence_begin_signalling();
+ if (!drm_dev_enter(drm, ))
+ return;
+
pm_runtime_get_sync(drm->dev);
/*
@@ -267,6 +270,8 @@ static void malidp_atomic_commit_tail(struct
drm_atomic_state *state)
pm_runtime_put(drm->dev);
drm_atomic_helper_cleanup_planes(drm, state);
+
+ drm_dev_exit(idx);
}
static const struct drm_mode_config_helper_funcs malidp_mode_config_helpers = {
--
2.37.2
[PATCH RESEND drm-misc-next 7/8] drm/arm/malidp: crtc: protect device resources after removal
(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.
However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.
Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_crtc.c | 41 +--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
b/drivers/gpu/drm/arm/malidp_crtc.c
index dc01c43f6193..fa95278abae6 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -27,6 +28,7 @@ static enum drm_mode_status malidp_crtc_mode_valid(struct
drm_crtc *crtc,
{
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
+ int idx;
/*
* check that the hardware can drive the required clock rate,
@@ -34,6 +36,9 @@ static enum drm_mode_status malidp_crtc_mode_valid(struct
drm_crtc *crtc,
*/
long rate, req_rate = mode->crtc_clock * 1000;
+ if (!drm_dev_enter(>base, ))
+ return MODE_NOCLOCK;
+
if (req_rate) {
rate = clk_round_rate(hwdev->pxlclk, req_rate);
if (rate != req_rate) {
@@ -43,6 +48,8 @@ static enum drm_mode_status malidp_crtc_mode_valid(struct
drm_crtc *crtc,
}
}
+ drm_dev_exit(idx);
+
return MODE_OK;
}
@@ -53,6 +60,10 @@ static void malidp_crtc_atomic_enable(struct drm_crtc *crtc,
struct malidp_hw_device *hwdev = malidp->dev;
struct videomode vm;
int err = pm_runtime_get_sync(crtc->dev->dev);
+ int idx;
+
+ if (!drm_dev_enter(>base, ))
+ return;
if (err < 0) {
DRM_DEBUG_DRIVER("Failed to enable runtime power management:
%d\n", err);
@@ -68,6 +79,8 @@ static void malidp_crtc_atomic_enable(struct drm_crtc *crtc,
hwdev->hw->modeset(hwdev, );
hwdev->hw->leave_config_mode(hwdev);
drm_crtc_vblank_on(crtc);
+
+ drm_dev_exit(idx);
}
static void malidp_crtc_atomic_disable(struct drm_crtc *crtc,
@@ -77,7 +90,10 @@ static void malidp_crtc_atomic_disable(struct drm_crtc *crtc,
crtc);
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
- int err;
+ int err, idx;
+
+ if (!drm_dev_enter(>base, ))
+ return;
/* always disable planes on the CRTC that is being turned off */
drm_atomic_helper_disable_planes_on_crtc(old_state, false);
@@ -91,6 +107,8 @@ static void malidp_crtc_atomic_disable(struct drm_crtc *crtc,
if (err < 0) {
DRM_DEBUG_DRIVER("Failed to disable runtime power management:
%d\n", err);
}
+
+ drm_dev_exit(idx);
}
static const struct gamma_curve_segment {
@@ -260,7 +278,10 @@ static int malidp_crtc_atomic_check_scaling(struct
drm_crtc *crtc,
u32 h_upscale_factor = 0; /* U16.16 */
u32 v_upscale_factor = 0; /* U16.16 */
u8 scaling = cs->scaled_planes_mask;
- int ret;
+ int idx, ret;
+
+ if (!drm_dev_enter(>base, ))
+ return -ENODEV;
if (!scaling) {
s->scale_enable = false;
@@ -334,6 +355,9 @@ static int malidp_crtc_atomic_check_scaling(struct drm_crtc
*crtc,
ret = hwdev->hw->se_calc_mclk(hwdev, s, );
if (ret < 0)
return -EINVAL;
+
+ drm_dev_exit(idx);
+
return 0;
}
@@ -498,9 +522,16 @@ static int malidp_crtc_enable_vblank(struct drm_crtc *crtc)
{
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
+ int idx;
+
+ if (!drm_dev_enter(>base, ))
+ return -ENODEV;
malidp_hw_enable_irq(hwdev, MALIDP_DE_BLOCK,
hwdev->hw->map.de_irq_map.vsync_irq);
+
+ drm_dev_exit(idx);
+
return 0;
}
@@ -508,9 +539,15 @@ static void malidp_crtc_disable_vblank(struct drm_crtc
*crtc)
{
struct malidp_drm *malidp = crtc_to_malidp_device(crtc);
struct malidp_hw_device *hwdev = malidp->dev;
+ int idx;
+
+ if (!drm_dev_enter(>base, ))
+ return;
malidp_hw_disable_irq(hwdev, MALIDP_DE_BLOCK,
hwdev->hw->map.de_irq_map.vsync_irq);
+
+ drm_dev_exit(idx);
}
static const struct drm_crtc_funcs malidp_crtc_funcs = {
--
2.37.2
[PATCH RESEND drm-misc-next 6/8] drm/arm/malidp: plane: protect device resources after removal
(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.
However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.
Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_planes.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c
b/drivers/gpu/drm/arm/malidp_planes.c
index 34547edf1ee3..d2ea60549454 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -790,9 +790,12 @@ static void malidp_de_plane_update(struct drm_plane *plane,
u16 pixel_alpha = new_state->pixel_blend_mode;
u8 plane_alpha = new_state->alpha >> 8;
u32 src_w, src_h, dest_w, dest_h, val;
- int i;
+ int i, idx;
struct drm_framebuffer *fb = plane->state->fb;
+ if (!drm_dev_enter(plane->dev, ))
+ return;
+
mp = to_malidp_plane(plane);
/*
@@ -897,16 +900,24 @@ static void malidp_de_plane_update(struct drm_plane
*plane,
malidp_hw_write(mp->hwdev, val,
mp->layer->base + MALIDP_LAYER_CONTROL);
+
+ drm_dev_exit(idx);
}
static void malidp_de_plane_disable(struct drm_plane *plane,
struct drm_atomic_state *state)
{
struct malidp_plane *mp = to_malidp_plane(plane);
+ int idx;
+
+ if (!drm_dev_enter(plane->dev, ))
+ return;
malidp_hw_clearbits(mp->hwdev,
LAYER_ENABLE | LAYER_FLOWCFG(LAYER_FLOWCFG_MASK),
mp->layer->base + MALIDP_LAYER_CONTROL);
+
+ drm_dev_exit(idx);
}
static const struct drm_plane_helper_funcs malidp_de_plane_helper_funcs = {
--
2.37.2
[PATCH RESEND drm-misc-next 5/8] drm/arm/malidp: use drm_dev_unplug()
When the driver is unbound, there might still be users in userspace having an open fd and are calling into the driver. While this is fine for drm managed resources, it is not for resources bound to the device/driver lifecycle, e.g. clocks or MMIO mappings. To prevent use-after-free issues we need to protect those resources with drm_dev_enter() and drm_dev_exit(). This does only work if we indicate that the drm device was unplugged, hence use drm_dev_unplug() instead of drm_dev_unregister(). Protecting the particular resources with drm_dev_enter()/drm_dev_exit() is handled by subsequent patches. Signed-off-by: Danilo Krummrich --- drivers/gpu/drm/arm/malidp_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 678c5b0d8014..aedd30f5f451 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -893,7 +893,7 @@ static void malidp_unbind(struct device *dev) struct malidp_drm *malidp = drm_to_malidp(drm); struct malidp_hw_device *hwdev = malidp->dev; - drm_dev_unregister(drm); + drm_dev_unplug(drm); drm_kms_helper_poll_fini(drm); pm_runtime_get_sync(dev); drm_atomic_helper_shutdown(drm); -- 2.37.2
[PATCH RESEND drm-misc-next 4/8] drm/arm/malidp: plane: use drm managed resources
Use drm managed resource allocation (drmm_universal_plane_alloc()) in
order to get rid of the explicit destroy hook in struct drm_plane_funcs.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_planes.c | 28 +++-
1 file changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c
b/drivers/gpu/drm/arm/malidp_planes.c
index 815d9199752f..34547edf1ee3 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -68,14 +68,6 @@
/* readahead for partial-frame prefetch */
#define MALIDP_MMU_PREFETCH_READAHEAD 8
-static void malidp_de_plane_destroy(struct drm_plane *plane)
-{
- struct malidp_plane *mp = to_malidp_plane(plane);
-
- drm_plane_cleanup(plane);
- kfree(mp);
-}
-
/*
* Replicate what the default ->reset hook does: free the state pointer and
* allocate a new empty object. We just need enough space to store
@@ -260,7 +252,6 @@ static bool malidp_format_mod_supported_per_plane(struct
drm_plane *plane,
static const struct drm_plane_funcs malidp_de_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
- .destroy = malidp_de_plane_destroy,
.reset = malidp_plane_reset,
.atomic_duplicate_state = malidp_duplicate_plane_state,
.atomic_destroy_state = malidp_destroy_plane_state,
@@ -972,12 +963,6 @@ int malidp_de_planes_init(struct drm_device *drm)
for (i = 0; i < map->n_layers; i++) {
u8 id = map->layers[i].id;
- plane = kzalloc(sizeof(*plane), GFP_KERNEL);
- if (!plane) {
- ret = -ENOMEM;
- goto cleanup;
- }
-
/* build the list of DRM supported formats based on the map */
for (n = 0, j = 0; j < map->n_pixel_formats; j++) {
if ((map->pixel_formats[j].layer & id) == id)
@@ -990,13 +975,14 @@ int malidp_de_planes_init(struct drm_device *drm)
/*
* All the layers except smart layer supports AFBC modifiers.
*/
- ret = drm_universal_plane_init(drm, >base, crtcs,
- _de_plane_funcs, formats, n,
- (id == DE_SMART) ? linear_only_modifiers :
modifiers,
- plane_type, NULL);
-
- if (ret < 0)
+ plane = drmm_universal_plane_alloc(drm, struct malidp_plane,
base,
+ crtcs,
_de_plane_funcs, formats, n,
+ (id == DE_SMART) ?
linear_only_modifiers :
+ modifiers, plane_type, NULL);
+ if (IS_ERR(plane)) {
+ ret = PTR_ERR(plane);
goto cleanup;
+ }
drm_plane_helper_add(>base,
_de_plane_helper_funcs);
--
2.37.2
[PATCH RESEND drm-misc-next 3/8] drm/arm/malidp: crtc: use drmm_crtc_init_with_planes()
Use drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes()
to get rid of the explicit destroy hook in struct drm_plane_funcs.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_crtc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
b/drivers/gpu/drm/arm/malidp_crtc.c
index 34ad7e1cd2b8..dc01c43f6193 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -514,7 +514,6 @@ static void malidp_crtc_disable_vblank(struct drm_crtc
*crtc)
}
static const struct drm_crtc_funcs malidp_crtc_funcs = {
- .destroy = drm_crtc_cleanup,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
.reset = malidp_crtc_reset,
@@ -548,8 +547,8 @@ int malidp_crtc_init(struct drm_device *drm)
return -EINVAL;
}
- ret = drm_crtc_init_with_planes(drm, >crtc, primary, NULL,
- _crtc_funcs, NULL);
+ ret = drmm_crtc_init_with_planes(drm, >crtc, primary, NULL,
+_crtc_funcs, NULL);
if (ret)
return ret;
--
2.37.2
[PATCH RESEND drm-misc-next 2/8] drm/arm/malidp: replace drm->dev_private with drm_to_malidp()
Using drm_device->dev_private is deprecated. Since we've switched to
devm_drm_dev_alloc(), struct drm_device is now embedded in struct
malidp_drm, hence we can use container_of() to get the struct drm_device
instance instead.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_crtc.c | 2 +-
drivers/gpu/drm/arm/malidp_drv.c| 29 +
drivers/gpu/drm/arm/malidp_drv.h| 1 +
drivers/gpu/drm/arm/malidp_hw.c | 10 +-
drivers/gpu/drm/arm/malidp_mw.c | 6 +++---
drivers/gpu/drm/arm/malidp_planes.c | 4 ++--
6 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
b/drivers/gpu/drm/arm/malidp_crtc.c
index 962730772b2f..34ad7e1cd2b8 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -526,7 +526,7 @@ static const struct drm_crtc_funcs malidp_crtc_funcs = {
int malidp_crtc_init(struct drm_device *drm)
{
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
struct drm_plane *primary = NULL, *plane;
int ret;
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 41c80e905991..678c5b0d8014 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -169,7 +169,7 @@ static void malidp_atomic_commit_se_config(struct drm_crtc
*crtc,
*/
static int malidp_set_and_wait_config_valid(struct drm_device *drm)
{
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
struct malidp_hw_device *hwdev = malidp->dev;
int ret;
@@ -190,7 +190,7 @@ static int malidp_set_and_wait_config_valid(struct
drm_device *drm)
static void malidp_atomic_commit_hw_done(struct drm_atomic_state *state)
{
struct drm_device *drm = state->dev;
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
int loop = 5;
malidp->event = malidp->crtc.state->event;
@@ -231,7 +231,7 @@ static void malidp_atomic_commit_hw_done(struct
drm_atomic_state *state)
static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
{
struct drm_device *drm = state->dev;
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state;
int i;
@@ -393,7 +393,7 @@ static const struct drm_mode_config_funcs
malidp_mode_config_funcs = {
static int malidp_init(struct drm_device *drm)
{
int ret;
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
struct malidp_hw_device *hwdev = malidp->dev;
drm_mode_config_init(drm);
@@ -429,7 +429,7 @@ static int malidp_irq_init(struct platform_device *pdev)
{
int irq_de, irq_se, ret = 0;
struct drm_device *drm = dev_get_drvdata(>dev);
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
struct malidp_hw_device *hwdev = malidp->dev;
/* fetch the interrupts from DT */
@@ -463,7 +463,7 @@ static int malidp_dumb_create(struct drm_file *file_priv,
struct drm_device *drm,
struct drm_mode_create_dumb *args)
{
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
/* allocate for the worst case scenario, i.e. rotated buffers */
u8 alignment = malidp_hw_get_pitch_align(malidp->dev, 1);
@@ -509,7 +509,7 @@ static void malidp_error_stats_dump(const char *prefix,
static int malidp_show_stats(struct seq_file *m, void *arg)
{
struct drm_device *drm = m->private;
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
unsigned long irqflags;
struct malidp_error_stats de_errors, se_errors;
@@ -532,7 +532,7 @@ static ssize_t malidp_debugfs_write(struct file *file,
const char __user *ubuf,
{
struct seq_file *m = file->private_data;
struct drm_device *drm = m->private;
- struct malidp_drm *malidp = drm->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(drm);
unsigned long irqflags;
spin_lock_irqsave(>errors_lock, irqflags);
@@ -553,7 +553,7 @@ static const struct file_operations malidp_debugfs_fops = {
static void malidp_debugfs_init(struct drm_minor *minor)
{
- struct malidp_drm *malidp = minor->dev->dev_private;
+ struct malidp_drm *malidp = drm_to_malidp(minor->dev);
malidp_error_stats_init(>de_errors);
malidp_error_stats_init(>se_errors);
@@ -653,7 +653,7 @@ static ssize_t core_id_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
struct drm_device
[PATCH RESEND drm-misc-next 0/8] drm/arm/malidp: use drm managed resources
Hi, This patch series converts the driver to use drm managed resources to prevent potential use-after-free issues on driver unbind/rebind and to get rid of the usage of deprecated APIs. Danilo Krummrich (8): drm/arm/malidp: use drmm_* to allocate driver structures drm/arm/malidp: replace drm->dev_private with drm_to_malidp() drm/arm/malidp: crtc: use drmm_crtc_init_with_planes() drm/arm/malidp: plane: use drm managed resources drm/arm/malidp: use drm_dev_unplug() drm/arm/malidp: plane: protect device resources after removal drm/arm/malidp: crtc: protect device resources after removal drm/arm/malidp: drv: protect device resources after removal drivers/gpu/drm/arm/malidp_crtc.c | 48 +--- drivers/gpu/drm/arm/malidp_drv.c| 58 ++--- drivers/gpu/drm/arm/malidp_drv.h| 2 + drivers/gpu/drm/arm/malidp_hw.c | 10 ++--- drivers/gpu/drm/arm/malidp_mw.c | 6 +-- drivers/gpu/drm/arm/malidp_planes.c | 45 +++--- 6 files changed, 100 insertions(+), 69 deletions(-) base-commit: 8fe444eb326869823f3788a4b4da5dca03339d10 -- 2.37.2
[PATCH RESEND drm-misc-next 1/8] drm/arm/malidp: use drmm_* to allocate driver structures
Use drm managed resources to allocate driver structures and get rid of
the deprecated drm_dev_alloc() call and replace it with
devm_drm_dev_alloc().
This also serves as preparation to get rid of drm_device->dev_private
and to fix use-after-free issues on driver unload.
Signed-off-by: Danilo Krummrich
---
drivers/gpu/drm/arm/malidp_drv.c | 20 +++-
drivers/gpu/drm/arm/malidp_drv.h | 1 +
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 1d0b0c54ccc7..41c80e905991 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -716,11 +717,13 @@ static int malidp_bind(struct device *dev)
int ret = 0, i;
u32 version, out_depth = 0;
- malidp = devm_kzalloc(dev, sizeof(*malidp), GFP_KERNEL);
- if (!malidp)
- return -ENOMEM;
+ malidp = devm_drm_dev_alloc(dev, _driver, typeof(*malidp), base);
+ if (IS_ERR(malidp))
+ return PTR_ERR(malidp);
+
+ drm = >base;
- hwdev = devm_kzalloc(dev, sizeof(*hwdev), GFP_KERNEL);
+ hwdev = drmm_kzalloc(drm, sizeof(*hwdev), GFP_KERNEL);
if (!hwdev)
return -ENOMEM;
@@ -753,12 +756,6 @@ static int malidp_bind(struct device *dev)
if (ret && ret != -ENODEV)
return ret;
- drm = drm_dev_alloc(_driver, dev);
- if (IS_ERR(drm)) {
- ret = PTR_ERR(drm);
- goto alloc_fail;
- }
-
drm->dev_private = malidp;
dev_set_drvdata(dev, drm);
@@ -887,8 +884,6 @@ static int malidp_bind(struct device *dev)
malidp_runtime_pm_suspend(dev);
drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
-alloc_fail:
of_reserved_mem_device_release(dev);
return ret;
@@ -917,7 +912,6 @@ static void malidp_unbind(struct device *dev)
malidp_runtime_pm_suspend(dev);
drm->dev_private = NULL;
dev_set_drvdata(dev, NULL);
- drm_dev_put(drm);
of_reserved_mem_device_release(dev);
}
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index cdfddfabf2d1..00be369b28f1 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -29,6 +29,7 @@ struct malidp_error_stats {
};
struct malidp_drm {
+ struct drm_device base;
struct malidp_hw_device *dev;
struct drm_crtc crtc;
struct drm_writeback_connector mw_connector;
--
2.37.2
Re: [PATCH v9 04/10] dt-bindings: backlight: Add MediaTek MT6370 backlight
On Tue, 30 Aug 2022, ChiaEn Wu wrote: > From: ChiYuan Huang > > Add MT6370 backlight binding documentation. > > Reviewed-by: Rob Herring > Reviewed-by: Daniel Thompson > Signed-off-by: ChiYuan Huang > Signed-off-by: ChiaEn Wu > --- > .../leds/backlight/mediatek,mt6370-backlight.yaml | 121 > + > 1 file changed, 121 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml Applied, thanks. -- Lee Jones [李琼斯]
Re: [PATCH v9 10/10] video: backlight: mt6370: Add MediaTek MT6370 support
On Tue, 30 Aug 2022, ChiaEn Wu wrote: > From: ChiaEn Wu > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > Add support for the MediaTek MT6370 backlight driver. > It controls 4 channels of 8 series WLEDs in > 2048 (only for MT6370/MT6371) / 16384 (only for MT6372) > current steps (30 mA) in exponential or linear mapping curves. > > Reviewed-by: Daniel Thompson > Signed-off-by: ChiaEn Wu > --- > > v9 > - Revise the format of the comments. > --- > drivers/video/backlight/Kconfig| 13 ++ > drivers/video/backlight/Makefile | 1 + > drivers/video/backlight/mt6370-backlight.c | 351 > + > 3 files changed, 365 insertions(+) > create mode 100644 drivers/video/backlight/mt6370-backlight.c Applied, thanks. -- Lee Jones [李琼斯]
Re: [PATCH v2 00/41] drm: Analog TV Improvements
Den 05.09.2022 16.57, skrev Maxime Ripard: > On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote: >> >> >> Den 01.09.2022 21.35, skrev Noralf Trønnes: >>> >>> >>> I have finally found a workaround for my kernel hangs. >>> >>> Dom had a look at my kernel and found that the VideoCore was fine, and >>> he said this: >>> That suggests cause of lockup was on arm side rather than VC side. But it's hard to diagnose further. Once you've had a peripheral not respond, the AXI bus locks up and no further operations are possible. Usual causes of this are required clocks being stopped or domains disabled and then trying to access the hardware. >>> >>> So when I got this on my 64-bit build: >>> >>> [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- SError >>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: GW >>> 5.19.0-rc6-00096-gba7973977976-dirty #1 >>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) >>> [ 166.702206] Workqueue: events_freezable_power_ thermal_zone_device_check >>> [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS >>> BTYPE=--) >>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 >>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70 >>> ... >>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] >>> >>> I wondered if that reg read was stalled due to a clock being stopped. >>> >>> Lo and behold, disabling runtime pm and keeping the vec clock running >>> all the time fixed it[1]. >>> >>> I don't know what the problem is, but at least I can now test this patchset. >>> >>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 >>> >> >> It turns out I didn't have to disable runtime pm: >> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2 > > If the bcm2711_thermal IP needs that clock to be enabled, it should grab > a reference itself, but it looks like even the device tree binding > doesn't ask for one. > The first thing I tried was to unload the bcm2711_thermal module before running modeset and it still hung, so I don't think that's the problem. Noralf.
Re: [PATCH v9 10/10] video: backlight: mt6370: Add MediaTek MT6370 support
On Tue, 30 Aug 2022, ChiaEn Wu wrote: > From: ChiaEn Wu > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > Add support for the MediaTek MT6370 backlight driver. > It controls 4 channels of 8 series WLEDs in > 2048 (only for MT6370/MT6371) / 16384 (only for MT6372) > current steps (30 mA) in exponential or linear mapping curves. > > Reviewed-by: Daniel Thompson > Signed-off-by: ChiaEn Wu > --- > > v9 > - Revise the format of the comments. > --- > drivers/video/backlight/Kconfig| 13 ++ > drivers/video/backlight/Makefile | 1 + > drivers/video/backlight/mt6370-backlight.c | 351 > + > 3 files changed, 365 insertions(+) > create mode 100644 drivers/video/backlight/mt6370-backlight.c Applied, thanks. -- Lee Jones [李琼斯]
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On 9/5/22 04:09, Andy Shevchenko wrote:
On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
wrote:
I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
so that gpiolib can be cleaned a bit, so let's switch to the generic
fwnode property API.
While at it switch the rest of the calls to read properties in
it, switch
bd9576_wdt_probe() to the generic device property API as well.
...
struct device *dev = >dev;
struct device *parent = dev->parent;
can make your code slightly neater.
...
+ count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
+ if (count < 0 && count != -EINVAL)
+ return count;
+
+ if (count > 0) {
+ if (count > ARRAY_SIZE(hw_margin))
+ return -EINVAL;
Why double check? You may move it out of the (count > 0).
Two checks will always be needed, so I don't entirely see
how that would be better.
...
- if (ret == 1)
- hw_margin_max = hw_margin[0];
+ ret = device_property_read_u32_array(dev->parent,
+"rohm,hw-timeout-ms",
+hw_margin, count);
+ if (ret < 0)
+ return ret;
So, only this needs the count > 0 check since below already has it implicitly.
Sorry, I don't understand this comment.
Guenter
- if (ret == 2) {
- hw_margin_max = hw_margin[1];
- hw_margin_min = hw_margin[0];
+ if (count == 1)
+ hw_margin_max = hw_margin[0];
+
+ if (count == 2) {
+ hw_margin_max = hw_margin[1];
+ hw_margin_min = hw_margin[0];
+ }
}
Re: [Intel-gfx] [RFC PATCH v3 10/17] drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl
On 02/09/2022 06:41, Niranjana Vishwanathapura wrote:
On Thu, Sep 01, 2022 at 08:58:57AM +0100, Tvrtko Ursulin wrote:
On 01/09/2022 06:09, Niranjana Vishwanathapura wrote:
On Wed, Aug 31, 2022 at 08:38:48AM +0100, Tvrtko Ursulin wrote:
On 27/08/2022 20:43, Andi Shyti wrote:
From: Niranjana Vishwanathapura
Implement new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only
works in vm_bind mode. The vm_bind mode only works with
this new execbuf3 ioctl.
The new execbuf3 ioctl will not have any list of objects to validate
bind as all required objects binding would have been requested by the
userspace before submitting the execbuf3.
And the legacy support like relocations etc are removed.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Ramalingam C
Signed-off-by: Andi Shyti
---
[snip]
+static void signal_fence_array(const struct i915_execbuffer *eb,
+ struct dma_fence * const fence)
+{
+ unsigned int n;
+
+ for (n = 0; n < eb->num_fences; n++) {
+ struct drm_syncobj *syncobj;
+ unsigned int flags;
+
+ syncobj = ptr_unpack_bits(eb->fences[n].syncobj, , 2);
+ if (!(flags & I915_TIMELINE_FENCE_SIGNAL))
+ continue;
+
+ if (eb->fences[n].chain_fence) {
+ drm_syncobj_add_point(syncobj,
+ eb->fences[n].chain_fence,
+ fence,
+ eb->fences[n].value);
+ /*
+ * The chain's ownership is transferred to the
+ * timeline.
+ */
+ eb->fences[n].chain_fence = NULL;
+ } else {
+ drm_syncobj_replace_fence(syncobj, fence);
+ }
+ }
+}
Semi-random place to ask - how many of the code here is direct copy
of existing functions from i915_gem_execbuffer.c? There seems to be
some 100% copies at least. And then some more with small tweaks.
Spend some time and try to figure out some code sharing?
During VM_BIND design review, maintainers expressed thought on keeping
execbuf3 completely separate and not touch the legacy execbuf path.
Got a link so this maintainer can see what exactly was said? Just to
make sure there isn't any misunderstanding on what "completely
separate" means to different people.
Here is one (search for copypaste/copy-paste)
https://patchwork.freedesktop.org/patch/486608/?series=93447=3
It is hard to search for old discussion threads. May be maintainers
can provide feedback here directly. Dave, Daniel? :)
Thanks. I had a read and don't see a fundamental conflict with what I
said. Conclusion seemed to be to go with a new ioctl and implement code
sharing where it makes sense. Which is what TODO in the cover letter
acknowledges so there should be no disagreement really.
I also think, execbuf3 should be fully separate. We can do some code
sharing where is a close 100% copy (there is a TODO in cover letter).
There are some changes like the timeline fence array handling here
which looks similar, but the uapi is not exactly the same. Probably,
we should keep them separate and not try to force code sharing at
least at this point.
Okay did not spot that TODO in the cover. But fair since it is RFC to
be unfinished.
I do however think it should be improved before considering the merge.
Because looking at the patch, 100% copies are:
for_each_batch_create_order
for_each_batch_add_order
eb_throttle
eb_pin_timeline
eb_pin_engine
eb_put_engine
__free_fence_array
put_fence_array
await_fence_array
signal_fence_array
retire_requests
eb_request_add
eb_requests_get
eb_requests_put
eb_find_context
Quite a lot.
Then there is a bunch of almost same functions which could be shared
if there weren't two incompatible local struct i915_execbuffer's.
Especially given when the out fence TODO item gets handled a chunk
more will also become a 100% copy.
There are difinitely a few which is 100% copies hence should have a
shared code.
But some are not. Like, fence_array stuff though looks very similar,
the uapi structures are different between execbuf3 and legacy execbuf.
The internal flags are also different (eg., __EXEC3_ENGINE_PINNED vs
__EXEC_ENGINE_PINNED) which causes minor differences hence not a
100% copy.
So, I am not convinced if it is worth carrying legacy stuff into
execbuf3 code. I think we need to look at these on a case by case
basis and see if abstracting common functionality to a separate
shared code makes sense or it is better to keep the code separate.
No one is suggesting to carry any legacy stuff into eb3. What I'd
suggest is to start something like i915_gem_eb_common.h|c and stuff the
100% copies from the above list in there.
Common struct eb with struct eb2 and eb3 inheriting from it should do
the trick. Similarly eb->flags shouldn't be a hard problem to solve.
Then you see what remains and whether it makes sense to consolidate further.
Regards,
Tvrtko
This could be done by having a common struct i915_execbuffer and
Re: [PATCH 2/4] drm/sched: Add callback and enable signaling on debug
On 9/5/2022 7:16 PM, Yadav, Arvind wrote:
On 9/5/2022 4:55 PM, Christian König wrote:
Am 05.09.22 um 12:56 schrieb Arvind Yadav:
Here's on debug adding an enable_signaling callback for finished
fences and enabling software signaling for finished fence.
Signed-off-by: Arvind Yadav
---
drivers/gpu/drm/scheduler/sched_fence.c | 12
drivers/gpu/drm/scheduler/sched_main.c | 4 +++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c
b/drivers/gpu/drm/scheduler/sched_fence.c
index 7fd869520ef2..ebd26cdb79a0 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -122,16 +122,28 @@ static void
drm_sched_fence_release_finished(struct dma_fence *f)
dma_fence_put(>scheduled);
}
+#ifdef CONFIG_DEBUG_FS
+static bool drm_sched_enable_signaling(struct dma_fence *f)
+{
+ return true;
+}
+#endif
static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
.get_driver_name = drm_sched_fence_get_driver_name,
.get_timeline_name = drm_sched_fence_get_timeline_name,
+#ifdef CONFIG_DEBUG_FS
+ .enable_signaling = drm_sched_enable_signaling,
+#endif
.release = drm_sched_fence_release_scheduled,
};
static const struct dma_fence_ops drm_sched_fence_ops_finished = {
.get_driver_name = drm_sched_fence_get_driver_name,
.get_timeline_name = drm_sched_fence_get_timeline_name,
+#ifdef CONFIG_DEBUG_FS
+ .enable_signaling = drm_sched_enable_signaling,
+#endif
Adding the callback should not be necessary.
sure, I will remove this change.
.release = drm_sched_fence_release_finished,
};
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index e0ab14e0fb6b..140e3d8646e2 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -961,7 +961,9 @@ static int drm_sched_main(void *param)
s_fence->parent = dma_fence_get(fence);
/* Drop for original kref_init of the fence */
dma_fence_put(fence);
Uff, not related to your patch but that looks wrong to me. The
reference can only be dropped after the call to
dma_fence_add_callback().
Shall I take care with this patch or I will submit separate one ?
This changes was recently added by Andrey Grodzovsky (commit :
45ecaea73) to fix the negative refcount.
-
+#ifdef CONFIG_DEBUG_FS
+ dma_fence_enable_sw_signaling(_fence->finished);
+#endif
This should always be called, independent of the config options set.
Christian.
sure, I will remove the Config check.
~arvind
r = dma_fence_add_callback(fence, _job->cb,
drm_sched_job_done_cb);
if (r == -ENOENT)
Re: [PATCH v2 00/41] drm: Analog TV Improvements
On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote: > > > Den 01.09.2022 21.35, skrev Noralf Trønnes: > > > > > > I have finally found a workaround for my kernel hangs. > > > > Dom had a look at my kernel and found that the VideoCore was fine, and > > he said this: > > > >> That suggests cause of lockup was on arm side rather than VC side. > >> > >> But it's hard to diagnose further. Once you've had a peripheral not > >> respond, the AXI bus locks up and no further operations are possible. > >> Usual causes of this are required clocks being stopped or domains > >> disabled and then trying to access the hardware. > >> > > > > So when I got this on my 64-bit build: > > > > [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- SError > > [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: GW > > 5.19.0-rc6-00096-gba7973977976-dirty #1 > > [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) > > [ 166.702206] Workqueue: events_freezable_power_ thermal_zone_device_check > > [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS > > BTYPE=--) > > [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 > > [ 166.702261] lr : regmap_mmio_read+0x44/0x70 > > ... > > [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] > > > > I wondered if that reg read was stalled due to a clock being stopped. > > > > Lo and behold, disabling runtime pm and keeping the vec clock running > > all the time fixed it[1]. > > > > I don't know what the problem is, but at least I can now test this patchset. > > > > [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 > > > > It turns out I didn't have to disable runtime pm: > https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2 If the bcm2711_thermal IP needs that clock to be enabled, it should grab a reference itself, but it looks like even the device tree binding doesn't ask for one. Maxime signature.asc Description: PGP signature
Re: (subset) [PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs
On Sun, 4 Sep 2022 23:30:52 -0700, Dmitry Torokhov wrote: > I would like to stop exporting OF-specific [devm_]gpiod_get_from_of_node() > so that gpiolib can be cleaned a bit. We can do that by switching drivers > to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open > the door to augmenting device tree and ACPI information through secondary > software properties (once we teach gpiolib how to handle those). > > I hope that relevant maintainers will take patches through their trees and > then we could merge the last one some time after -rc1. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [08/11] regulator: bd71815: switch to using devm_fwnode_gpiod_get() commit: 97c9278ec624a0d5d7c56aa20e16afc8aaa96557 [09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get() commit: 587bfe3f7a270f0a4076e624d318292324bdead8 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
On Fri, Sep 02, 2022 at 12:46:29AM +0200, Mateusz Kwiatkowski wrote:
> > @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> > unsigned int xres;
> > unsigned int yres;
> > unsigned int flags;
> > + unsigned int tv_mode;
> > };
>
> Are _all_ named modes supposed to be about analog TV?
>
> If so, then probably this structure should be renamed drm_named_analog_tv_mode
> or something.
I don't think they need to, but it's the only use case we've had so far.
We could also imagine using UHD for 3840x2160 for example, so I wouldn't
say it's limited to analog tv.
> If not, then including tv_mode in all of them sounds almost dangrous. 0 is a
> valid value for enum drm_connector_tv_mode, corresponding to
> DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be
> the one that has a numeric value of 0?) and if there ever is a named mode that
> is not related to analog TV, it looks that it will refer to NTSC-443.
>
> Not sure where could that actually propagate, and maybe what I'm saying can't
> happen, but I'm imagining weird scenarios where a GPU that has both a
> VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the
> composite output by default because a named mode for the modern output is
> selected.
So, named modes are per-connector so the fact that there's another
output doesn't really matter. Then, the answer is quite simple actually,
the HDMI driver wouldn't register and use the TV mode property at all,
so it would completely ignore it, no matter what value it has.
So it's not really a concern.
> Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense?
But I guess we can add it still.
Maxime
signature.asc
Description: PGP signature
[PATCH v3 4/4] drm/format-helper: Add drm_fb_build_fourcc_list() helper
Add drm_fb_build_fourcc_list() function that builds a list of supported
formats from native and emulated ones. Helpful for all drivers that do
format conversion as part of their plane updates. Update current caller.
v3:
* improve warnings on ignored formats (Sam)
v2:
* use u32 instead of uint32_t (Sam)
* print a warning if output array is too small (Sam)
* comment fixes (Sam)
Signed-off-by: Thomas Zimmermann
Reviewed-by: Sam Ravnborg
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_format_helper.c | 108
drivers/gpu/drm/tiny/simpledrm.c| 47 ++--
include/drm/drm_format_helper.h | 11 ++-
3 files changed, 123 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index 56642816fdff..4afc4ac27342 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -793,3 +793,111 @@ void drm_fb_xrgb_to_mono(struct iosys_map *dst, const
unsigned int *dst_pitc
kfree(src32);
}
EXPORT_SYMBOL(drm_fb_xrgb_to_mono);
+
+static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs,
uint32_t fourcc)
+{
+ const uint32_t *fourccs_end = fourccs + nfourccs;
+
+ while (fourccs < fourccs_end) {
+ if (*fourccs == fourcc)
+ return true;
+ ++fourccs;
+ }
+ return false;
+}
+
+/**
+ * drm_fb_build_fourcc_list - Filters a list of supported color formats against
+ *the device's native formats
+ * @dev: DRM device
+ * @native_fourccs: 4CC codes of natively supported color formats
+ * @native_nfourccs: The number of entries in @native_fourccs
+ * @driver_fourccs: 4CC codes of all driver-supported color formats
+ * @driver_nfourccs: The number of entries in @driver_fourccs
+ * @fourccs_out: Returns 4CC codes of supported color formats
+ * @nfourccs_out: The number of available entries in @fourccs_out
+ *
+ * This function create a list of supported color format from natively
+ * supported formats and the emulated formats.
+ * At a minimum, most userspace programs expect at least support for
+ * XRGB on the primary plane. Devices that have to emulate the
+ * format, and possibly others, can use drm_fb_build_fourcc_list() to
+ * create a list of supported color formats. The returned list can
+ * be handed over to drm_universal_plane_init() et al. Native formats
+ * will go before emulated formats. Other heuristics might be applied
+ * to optimize the order. Formats near the beginning of the list are
+ * usually preferred over formats near the end of the list.
+ *
+ * Returns:
+ * The number of color-formats 4CC codes returned in @fourccs_out.
+ */
+size_t drm_fb_build_fourcc_list(struct drm_device *dev,
+ const u32 *native_fourccs, size_t
native_nfourccs,
+ const u32 *driver_fourccs, size_t
driver_nfourccs,
+ u32 *fourccs_out, size_t nfourccs_out)
+{
+ u32 *fourccs = fourccs_out;
+ const u32 *fourccs_end = fourccs_out + nfourccs_out;
+ bool found_native = false;
+ size_t i;
+
+ /*
+* The device's native formats go first.
+*/
+
+ for (i = 0; i < native_nfourccs; ++i) {
+ u32 fourcc = native_fourccs[i];
+
+ if (is_listed_fourcc(fourccs_out, fourccs - fourccs_out,
fourcc)) {
+ continue; /* skip duplicate entries */
+ } else if (fourccs == fourccs_end) {
+ drm_warn(dev, "Ignoring native format %p4cc\n",
);
+ continue; /* end of available output buffer */
+ }
+
+ drm_dbg_kms(dev, "adding native format %p4cc\n", );
+
+ if (!found_native)
+ found_native = is_listed_fourcc(driver_fourccs,
driver_nfourccs, fourcc);
+ *fourccs = fourcc;
+ ++fourccs;
+ }
+
+ /*
+* The plane's atomic_update helper converts the framebuffer's color
format
+* to a native format when copying to device memory.
+*
+* If there is not a single format supported by both, device and
+* driver, the native formats are likely not supported by the conversion
+* helpers. Therefore *only* support the native formats and add a
+* conversion helper ASAP.
+*/
+ if (!found_native) {
+ drm_warn(dev, "Format conversion helpers required to add extra
formats.\n");
+ goto out;
+ }
+
+ /*
+* The extra formats, emulated by the driver, go second.
+*/
+
+ for (i = 0; (i < driver_nfourccs) && (fourccs < fourccs_end); ++i) {
+ u32 fourcc = driver_fourccs[i];
+
+ if (is_listed_fourcc(fourccs_out, fourccs - fourccs_out,
fourcc)) {
+ continue; /* skip
[PATCH v3 2/4] drm/probe-helper: Add drm_crtc_helper_mode_valid_fixed()
Add drm_crtc_helper_mode_valid_fixed(), which validates a given mode
against a display hardware's mode. Convert simpledrm and use it in a
few other drivers with static modes.
v2:
* rename 'static' and 'hw' to 'fixed' everywhere
Signed-off-by: Thomas Zimmermann
Reviewed-by: Sam Ravnborg
---
drivers/gpu/drm/drm_mipi_dbi.c | 18 ++
drivers/gpu/drm/drm_probe_helper.c | 25
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 1 +
drivers/gpu/drm/tiny/hx8357d.c | 1 +
drivers/gpu/drm/tiny/ili9163.c | 1 +
drivers/gpu/drm/tiny/ili9341.c | 1 +
drivers/gpu/drm/tiny/ili9486.c | 1 +
drivers/gpu/drm/tiny/mi0283qt.c | 1 +
drivers/gpu/drm/tiny/panel-mipi-dbi.c| 1 +
drivers/gpu/drm/tiny/repaper.c | 10
drivers/gpu/drm/tiny/simpledrm.c | 10 +---
drivers/gpu/drm/tiny/st7735r.c | 1 +
include/drm/drm_mipi_dbi.h | 2 ++
include/drm/drm_probe_helper.h | 8 +--
14 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index de2a5be67415..a6ac56580876 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -309,6 +309,24 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb,
struct drm_rect *rect)
drm_dev_exit(idx);
}
+/**
+ * mipi_dbi_pipe_mode_valid - MIPI DBI mode-valid helper
+ * @pipe: Simple display pipe
+ * @mode: The mode to test
+ *
+ * This function validates a given display mode against the MIPI DBI's hardware
+ * display. Drivers can use this as their
_simple_display_pipe_funcs->mode_valid
+ * callback.
+ */
+enum drm_mode_status mipi_dbi_pipe_mode_valid(struct drm_simple_display_pipe
*pipe,
+ const struct drm_display_mode
*mode)
+{
+ struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+
+ return drm_crtc_helper_mode_valid_fixed(>crtc, mode,
>mode);
+}
+EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
+
/**
* mipi_dbi_pipe_update - Display pipe update helper
* @pipe: Simple display pipe
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 818150a1b3b0..8956b367e12f 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1014,6 +1014,31 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_helper_hpd_irq_event);
+/**
+ * drm_crtc_helper_mode_valid_fixed - Validates a display mode
+ * @crtc: the crtc
+ * @mode: the mode to validate
+ * @fixed_mode: the display hardware's mode
+ *
+ * Returns:
+ * MODE_OK on success, or another mode-status code otherwise.
+ */
+enum drm_mode_status drm_crtc_helper_mode_valid_fixed(struct drm_crtc *crtc,
+ const struct
drm_display_mode *mode,
+ const struct
drm_display_mode *fixed_mode)
+{
+
+ if (mode->hdisplay != fixed_mode->hdisplay && mode->vdisplay !=
fixed_mode->vdisplay)
+ return MODE_ONE_SIZE;
+ else if (mode->hdisplay != fixed_mode->hdisplay)
+ return MODE_ONE_WIDTH;
+ else if (mode->vdisplay != fixed_mode->vdisplay)
+ return MODE_ONE_HEIGHT;
+
+ return MODE_OK;
+}
+EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed);
+
/**
* drm_connector_helper_get_modes_from_ddc - Updates the connector's EDID
* property from the connector's
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 7da09e34385d..39dc40cf681f 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -576,6 +576,7 @@ static void ili9341_dbi_enable(struct
drm_simple_display_pipe *pipe,
}
static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
+ .mode_valid = mipi_dbi_pipe_mode_valid,
.enable = ili9341_dbi_enable,
.disable = mipi_dbi_pipe_disable,
.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c
index 57f229a785bf..48c24aa8c28a 100644
--- a/drivers/gpu/drm/tiny/hx8357d.c
+++ b/drivers/gpu/drm/tiny/hx8357d.c
@@ -181,6 +181,7 @@ static void yx240qv29_enable(struct drm_simple_display_pipe
*pipe,
}
static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
+ .mode_valid = mipi_dbi_pipe_mode_valid,
.enable = yx240qv29_enable,
.disable = mipi_dbi_pipe_disable,
.update = mipi_dbi_pipe_update,
diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
index 86439e50e304..9a1a5943bee0 100644
--- a/drivers/gpu/drm/tiny/ili9163.c
+++ b/drivers/gpu/drm/tiny/ili9163.c
@@ -100,6 +100,7 @@ static void
[PATCH v3 3/4] drm/modes: Add initializer macro DRM_MODE_INIT()
The macro DRM_MODE_INIT() initializes an instance of
struct drm_display_mode with typical parameters. Convert simpledrm
and also update the macro DRM_SIMPLE_MODE().
v3:
* fix DRM_MODE_INIT() docs (kernel test robot)
Signed-off-by: Thomas Zimmermann
Reviewed-by: Sam Ravnborg
---
drivers/gpu/drm/tiny/simpledrm.c | 23 -
include/drm/drm_modes.h | 35 +++-
2 files changed, 39 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 5bed4d564104..404290760c60 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -30,16 +30,6 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
-/*
- * Assume a monitor resolution of 96 dpi to
- * get a somewhat reasonable screen size.
- */
-#define RES_MM(d) \
- (((d) * 254ul) / (96ul * 10ul))
-
-#define SIMPLEDRM_MODE(hd, vd) \
- DRM_SIMPLE_MODE(hd, vd, RES_MM(hd), RES_MM(vd))
-
/*
* Helpers for simplefb
*/
@@ -641,10 +631,15 @@ static const struct drm_mode_config_funcs
simpledrm_mode_config_funcs = {
static struct drm_display_mode simpledrm_mode(unsigned int width,
unsigned int height)
{
- struct drm_display_mode mode = { SIMPLEDRM_MODE(width, height) };
-
- mode.clock = mode.hdisplay * mode.vdisplay * 60 / 1000 /* kHz */;
- drm_mode_set_name();
+ /*
+* Assume a monitor resolution of 96 dpi to
+* get a somewhat reasonable screen size.
+*/
+ const struct drm_display_mode mode = {
+ DRM_MODE_INIT(60, width, height,
+ DRM_MODE_RES_MM(width, 96ul),
+ DRM_MODE_RES_MM(height, 96ul))
+ };
return mode;
}
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index a80ae9639e96..b0c680e6f670 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -138,6 +138,35 @@ enum drm_mode_status {
.vsync_start = (vss), .vsync_end = (vse), .vtotal = (vt), \
.vscan = (vs), .flags = (f)
+/**
+ * DRM_MODE_RES_MM - Calculates the display size from resolution and DPI
+ * @res: The resolution in pixel
+ * @dpi: The number of dots per inch
+ */
+#define DRM_MODE_RES_MM(res, dpi) \
+ (((res) * 254ul) / ((dpi) * 10ul))
+
+#define __DRM_MODE_INIT(pix, hd, vd, hd_mm, vd_mm) \
+ .type = DRM_MODE_TYPE_DRIVER, .clock = (pix), \
+ .hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
+ .htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
+ .vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
+ .height_mm = (vd_mm)
+
+/**
+ * DRM_MODE_INIT - Initialize display mode
+ * @hz: Vertical refresh rate in Hertz
+ * @hd: Horizontal resolution, width
+ * @vd: Vertical resolution, height
+ * @hd_mm: Display width in millimeters
+ * @vd_mm: Display height in millimeters
+ *
+ * This macro initializes a _display_mode that contains information about
+ * refresh rate, resolution and physical size.
+ */
+#define DRM_MODE_INIT(hz, hd, vd, hd_mm, vd_mm) \
+ __DRM_MODE_INIT((hd) * (vd) * (hz) / 1000 /* kHz */, hd, vd, hd_mm,
vd_mm)
+
/**
* DRM_SIMPLE_MODE - Simple display mode
* @hd: Horizontal resolution, width
@@ -149,11 +178,7 @@ enum drm_mode_status {
* resolution and physical size.
*/
#define DRM_SIMPLE_MODE(hd, vd, hd_mm, vd_mm) \
- .type = DRM_MODE_TYPE_DRIVER, .clock = 1 /* pass validation */, \
- .hdisplay = (hd), .hsync_start = (hd), .hsync_end = (hd), \
- .htotal = (hd), .vdisplay = (vd), .vsync_start = (vd), \
- .vsync_end = (vd), .vtotal = (vd), .width_mm = (hd_mm), \
- .height_mm = (vd_mm)
+ __DRM_MODE_INIT(1 /* pass validation */, hd, vd, hd_mm, vd_mm)
#define CRTC_INTERLACE_HALVE_V (1 << 0) /* halve V values for interlacing */
#define CRTC_STEREO_DOUBLE (1 << 1) /* adjust timings for stereo modes */
--
2.37.2
[PATCH v3 0/4] drm/probe-helper, modes: Helpers for single-mode displays
This patchset moves code from simpledrm to probe and display-mode helpers. The new functions will be useful for the upcoming driver for PowerPC displays. [1] Where possible, existing drivers are being converted to use them. v3: * better warnings in drm_fb_build_fourcc_list() (Sam) * fix docs (kernel test robot) v2: * replace 'static' and 'hw' naming with 'fixed' * use u32 for 4CC codes (Sam) * print a warning if not all formats can be used (Sam) * comment fixes (Sam) [1] https://patchwork.freedesktop.org/series/106538/ Thomas Zimmermann (4): drm/probe-helper: Add drm_connector_helper_get_modes_fixed() drm/probe-helper: Add drm_crtc_helper_mode_valid_fixed() drm/modes: Add initializer macro DRM_MODE_INIT() drm/format-helper: Add drm_fb_build_fourcc_list() helper drivers/gpu/drm/drm_format_helper.c | 108 +++ drivers/gpu/drm/drm_mipi_dbi.c | 38 +++ drivers/gpu/drm/drm_probe_helper.c | 65 +++ drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 1 + drivers/gpu/drm/tiny/hx8357d.c | 1 + drivers/gpu/drm/tiny/ili9163.c | 1 + drivers/gpu/drm/tiny/ili9341.c | 1 + drivers/gpu/drm/tiny/ili9486.c | 1 + drivers/gpu/drm/tiny/mi0283qt.c | 1 + drivers/gpu/drm/tiny/panel-mipi-dbi.c| 1 + drivers/gpu/drm/tiny/repaper.c | 26 ++--- drivers/gpu/drm/tiny/simpledrm.c | 96 +++-- drivers/gpu/drm/tiny/st7735r.c | 1 + include/drm/drm_format_helper.h | 11 +- include/drm/drm_mipi_dbi.h | 2 + include/drm/drm_modes.h | 35 +- include/drm/drm_probe_helper.h | 9 +- 17 files changed, 276 insertions(+), 122 deletions(-) base-commit: f2c3a05d33693ad51996fa7d12d3b2d4b0f372eb -- 2.37.2
