hi Ville:
very glad to receive your feedback
Some of them are very good ideas.
Some are not very clear and hope to get your further reply!
On 2023/10/26 3:49, Ville Syrjälä wrote:
> On Wed, Oct 25, 2023 at 10:28:56PM +0300, Dmitry Baryshkov wrote:
>> On 25/10/2023 13:39, Keith Zhao wrote:
>> > add 2 crtcs and 8 planes in vs-drm
>> >
>> > Signed-off-by: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/verisilicon/Makefile | 8 +-
>> > drivers/gpu/drm/verisilicon/vs_crtc.c | 257 ++++
>> > drivers/gpu/drm/verisilicon/vs_crtc.h | 43 +
>> > drivers/gpu/drm/verisilicon/vs_dc.c | 1002 ++++++++++++
>> > drivers/gpu/drm/verisilicon/vs_dc.h | 80 +
>> > drivers/gpu/drm/verisilicon/vs_dc_hw.c | 1959 ++++++++++++++++++++++++
>> > drivers/gpu/drm/verisilicon/vs_dc_hw.h | 492 ++++++
>> > drivers/gpu/drm/verisilicon/vs_drv.c | 2 +
>> > drivers/gpu/drm/verisilicon/vs_plane.c | 526 +++++++
>> > drivers/gpu/drm/verisilicon/vs_plane.h | 58 +
>> > drivers/gpu/drm/verisilicon/vs_type.h | 69 +
>> > 11 files changed, 4494 insertions(+), 2 deletions(-)
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.c
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.h
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h
>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h
>> >
>> > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile
>> > index 7d3be305b..1d48016ca 100644
>> > --- a/drivers/gpu/drm/verisilicon/Makefile
>> > +++ b/drivers/gpu/drm/verisilicon/Makefile
>> > @@ -1,7 +1,11 @@
>> > # SPDX-License-Identifier: GPL-2.0
>> >
>> > -vs_drm-objs := vs_drv.o \
>> > - vs_modeset.o
>> > +vs_drm-objs := vs_dc_hw.o \
>> > + vs_dc.o \
>> > + vs_crtc.o \
>> > + vs_drv.o \
>> > + vs_modeset.o \
>> > + vs_plane.o
>> >
>> > obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o
>> >
>> > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
>> > new file mode 100644
>> > index 000000000..8a658ea77
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
>> > @@ -0,0 +1,257 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/*
>> > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
>> > + *
>> > + */
>> > +
>> > +#include <linux/clk.h>
>> > +#include <linux/debugfs.h>
>> > +#include <linux/media-bus-format.h>
>> > +
>> > +#include <drm/drm_atomic_helper.h>
>> > +#include <drm/drm_atomic.h>
>> > +#include <drm/drm_crtc.h>
>> > +#include <drm/drm_gem_atomic_helper.h>
>> > +#include <drm/drm_vblank.h>
>> > +#include <drm/vs_drm.h>
>> > +
>> > +#include "vs_crtc.h"
>> > +#include "vs_dc.h"
>> > +#include "vs_drv.h"
>> > +
>> > +static void vs_crtc_reset(struct drm_crtc *crtc)
>> > +{
>> > + struct vs_crtc_state *state;
>> > +
>> > + if (crtc->state) {
>> > + __drm_atomic_helper_crtc_destroy_state(crtc->state);
>> > +
>> > + state = to_vs_crtc_state(crtc->state);
>> > + kfree(state);
>> > + crtc->state = NULL;
>> > + }
>>
>> You can call your crtc_destroy_state function directly here.
ok got it !
>>
>> > +
>> > + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> > + if (!state)
>> > + return;
>> > +
>> > + __drm_atomic_helper_crtc_reset(crtc, &state->base);
>> > +}
>> > +
>> > +static struct drm_crtc_state *
>> > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
>> > +{
>> > + struct vs_crtc_state *ori_state;
>>
>> It might be a matter of taste, but it is usually old_state.
>>
>> > + struct vs_crtc_state *state;
>> > +
>> > + if (!crtc->state)
>> > + return NULL;
>> > +
>> > + ori_state = to_vs_crtc_state(crtc->state);
>> > + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> > + if (!state)
>> > + return NULL;
>> > +
>> > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base);
>> > +
>> > + state->output_fmt = ori_state->output_fmt;
>> > + state->encoder_type = ori_state->encoder_type;
>> > + state->bpp = ori_state->bpp;
>> > + state->underflow = ori_state->underflow;
>>
>> Can you use kmemdup instead?
ok
>>
>> > +
>> > + return &state->base;
>> > +}
>> > +
>> > +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc,
>> > + struct drm_crtc_state *state)
>> > +{
>> > + __drm_atomic_helper_crtc_destroy_state(state);
>> > + kfree(to_vs_crtc_state(state));
>> > +}
>> > +
>> > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc)
>> > +{
>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>> > +
>> > + vs_dc_enable_vblank(dc, true);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc)
>> > +{
>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>> > +
>> > + vs_dc_enable_vblank(dc, false);
>> > +}
>> > +
>> > +static const struct drm_crtc_funcs vs_crtc_funcs = {
>> > + .set_config = drm_atomic_helper_set_config,
>> > + .page_flip = drm_atomic_helper_page_flip,
>>
>> destroy is required, see drm_mode_config_cleanup()
will add destory
>>
>> > + .reset = vs_crtc_reset,
>> > + .atomic_duplicate_state = vs_crtc_atomic_duplicate_state,
>> > + .atomic_destroy_state = vs_crtc_atomic_destroy_state,
>>
>> please consider adding atomic_print_state to output driver-specific bits.
>>
will add atomic_print_state to print my private definitions
>> > + .enable_vblank = vs_crtc_enable_vblank,
>> > + .disable_vblank = vs_crtc_disable_vblank,
>> > +};
>> > +
>> > +static u8 cal_pixel_bits(u32 bus_format)
>>
>> This looks like a generic helper code, which can go to a common place.
>>
>> > +{
>> > + u8 bpp;
>> > +
>> > + switch (bus_format) {
>> > + case MEDIA_BUS_FMT_RGB565_1X16:
>> > + case MEDIA_BUS_FMT_UYVY8_1X16:
>> > + bpp = 16;
>> > + break;
>> > + case MEDIA_BUS_FMT_RGB666_1X18:
>> > + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
>> > + bpp = 18;
>> > + break;
>> > + case MEDIA_BUS_FMT_UYVY10_1X20:
>> > + bpp = 20;
>> > + break;
>> > + case MEDIA_BUS_FMT_BGR888_1X24:
>> > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
>> > + case MEDIA_BUS_FMT_YUV8_1X24:
>> > + bpp = 24;
>> > + break;
>> > + case MEDIA_BUS_FMT_RGB101010_1X30:
>> > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
>> > + case MEDIA_BUS_FMT_YUV10_1X30:
>> > + bpp = 30;
>> > + break;
>> > + default:
>> > + bpp = 24;
>> > + break;
>> > + }
>> > +
>> > + return bpp;
>> > +}
>> > +
>> > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc,
>> > + struct drm_atomic_state *state)
>> > +{
>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>> > + struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state);
>> > +
>> > + vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt);
>> > +
>> > + vs_dc_enable(dc, crtc);
>> > + drm_crtc_vblank_on(crtc);
>> > +}
>> > +
>> > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc,
>> > + struct drm_atomic_state *state)
>> > +{
>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev);
>> > +
>> > + drm_crtc_vblank_off(crtc);
>> > +
>> > + vs_dc_disable(dc, crtc);
>> > +
>> > + if (crtc->state->event && !crtc->state->active) {
>> > + spin_lock_irq(&crtc->dev->event_lock);
>> > + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> > + spin_unlock_irq(&crtc->dev->event_lock);
>> > +
>> > + crtc->state->event = NULL;
>>
>> I think even should be cleared within the lock.
>
> event_lock doesn't protect anything in the crtc state.
how about match like this:
spin_lock_irq(&crtc->dev->event_lock);
if (crtc->state->event) {
drm_crtc_send_vblank_event(crtc, crtc->state->event);
crtc->state->event = NULL;
}
spin_unlock_irq(&crtc->dev->event_lock);
>
> But the bigger problem in this code is the prevalent crtc->state
> usage. That should pretty much never be done, especially in anything
> that can get called from the actual commit phase where you no longer
> have the locks held. Instead one should almost always use the
> get_{new,old}_state() stuff, or the old/new/oldnew state iterators.
the prevalent crtc->state usage :
crtc->state should not be used directly?
need replace it by get_{new,old}_state()
for example:
drm_crtc_send_vblank_event(crtc, crtc->state->event);
should do like this :
struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
...
drm_crtc_send_vblank_event(crtc, crtc_state->event);
...
If there is a problem, help further correct
wish give a example
Thanks
>
>>
>> > + }
>> > +}
>> > +
>> > +static void vs_crtc_atomic_begin(struct drm_crtc *crtc,
>> > + struct drm_atomic_state *state)
>> > +{
>> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
>> > + crtc);
>> > +
>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc);
>> > + struct device *dev = vs_crtc->dev;
>> > + struct drm_property_blob *blob = crtc->state->gamma_lut;
>
> Eg. here you are using drm_atomic_get_new_crtc_state() correctly, but
> then proceed to directly access crtc->state anyway.
>
struct drm_property_blob *blob = crtc->state->gamma_lut;
change to
struct drm_property_blob *blob = crtc_state ->gamma_lut;
>> > + struct drm_color_lut *lut;
>> > + struct vs_dc *dc = dev_get_drvdata(dev);
>> > +
>> > + if (crtc_state->color_mgmt_changed) {
>> > + if (blob && blob->length) {
>> > + lut = blob->data;
>> > + vs_dc_set_gamma(dc, crtc, lut,
>> > + blob->length / sizeof(*lut));
>> > + vs_dc_enable_gamma(dc, crtc, true);
>> > + } else {
>> > + vs_dc_enable_gamma(dc, crtc, false);
>> > + }
>> > + }
>> > +}
>
