sorry
Dmitry ,accidentally wrote the wrong name
Take no offense
On 2023/10/26 17:42, Keith Zhao wrote:
> 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);
>>> > + }
>>> > + }
>>> > +}
>>
